Fixed #357 - Password hashing when changed manually in admin by Alien501 · Pull Request #360 · djangoindia/djangoindia.org

Alien501 · 2025-04-13T08:13:52Z

Closes #357

Now password will be hashed when modified in admin

Changes

Added hash function in users model

Type of change

Bug fix
Feature update
Breaking change
Documentation update

How has this been tested?

After changing password, a login attempt had been done.

Author Checklist

Code has been commented, particularly in hard-to-understand areas
Changes generate no new warnings
Vital changes have been captured in unit and/or integration tests
New and existing unit tests pass locally with my changes
Documentation has been extended, if necessary
Merging to main from fork:branchname

Summary by CodeRabbit

Bug Fixes
- Passwords set or updated through the admin are now always securely hashed, both on user creation and when changing the password field. This prevents accidental plain-text storage and ensures consistent security across admin actions. No interface changes; existing users are unaffected. Future creates/edits via the admin work as before with improved safety.

PankajVishw50 · 2025-04-19T08:34:20Z

backend/djangoindia/db/models/user.py

    def save(self, *args, **kwargs):
        self.email = self.email.lower().strip()

+        if not self.password.startswith('pbkdf2_sha256'):


This would only work if the default hashing algorithm is used. If other algorithms are used, it would cause issues.

Alien501 · 2025-04-20T04:10:36Z

@PankajVishw50 made it to be generic, could you check my latest push

PankajVishw50

Looks good to me. Just a small typo in comment has should be written hash

PankajVishw50 · 2025-04-20T04:34:00Z

backend/djangoindia/db/models/user.py

    def save(self, *args, **kwargs):
        self.email = self.email.lower().strip()
+        try:
+            # Trying to find the has used on the password, if it couldn't find a one, exception will be thrown, ie, password isn't hashed :-)


Fix Typo. has -> hash

sumit4613 · 2025-05-05T15:01:15Z

backend/djangoindia/db/models/user.py

+        try:
+            # Trying to find the hash used on the password, if it couldn't find a one, exception will be thrown, ie, password isn't hashed :-)
+            identify_hasher(self.password)
+        except:


It's not ideal to use bare except. You can use except Exception: if you're not sure what errors will be thrown.

ankanchanda

Please address the comment by sumit4613, and close this.

ankanchanda · 2025-04-13T17:00:42Z

backend/djangoindia/db/models/user.py

+        if not self.password.startswith('pbkdf2_sha256'):
+            self.password = make_password(self.password)
+


Don't we need to change anything on the login part?

… into issue_357

DevilsAutumn · 2025-05-31T05:25:10Z

backend/djangoindia/db/models/user.py

+        try:
+            # Trying to find the hash used on the password, if it couldn't find a one, exception will be thrown, ie, password isn't hashed :-)
+            identify_hasher(self.password)
+        except ValueError:
+            # Hashing password
+            self.password = make_password(self.password)


@Alien501 I think this is not the best approach. This will try to hash the password everytime a user instance is saved. This is a django admin specific issue, and IMHO it would be better to override the save_model method of UserAdmin to something like:

def save_model(self, request, obj, form, change): if "password" in form.changed_data or not change: obj.set_password(form.cleaned_data["password"]) return super().save_model(request, obj, form, change)

Now, whenever a user password is changed or a new user is created from django admin, It will hash its password. Also, by default django uses PBKDF2PasswordHasher which is pretty safe. Since we won't be changing the password hasher ever, using set_password(which uses make_password under the hood) looks like a good choice to me.

Let me know what you think.

cc: @ankanchanda @sumit4613

Agreed. I'll move the password hashing logic to UserAdmin instead. Thanks for the suggestion

yanurag-dev · 2025-09-28T16:18:58Z

@CodeRabbit fullreview

coderabbitai · 2025-09-28T16:19:27Z

@Yadavanurag13 I'll conduct a full review of the PR to ensure the password hashing implementation is properly done and follows best practices.

✅ Actions performed

Full review triggered.

coderabbitai · 2025-09-28T16:19:30Z

Walkthrough

Introduces a custom save_model in UserAdmin to hash passwords on creation or when the password field changes, calling set_password before delegating to the superclass. Also adds hashers imports in the user model file without changing public APIs.

Changes

Cohort / File(s)	Summary
Admin password hashing flow `backend/djangoindia/db/admin.py`	Adds UserAdmin.save_model override to hash passwords on create or when "password" is modified, using obj.set_password, then calls super().save_model.
Auth hashers import `backend/djangoindia/db/models/user.py`	Adds imports for make_password and identify_hasher; no functional changes in methods. Minor formatting adjustment.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Admin as Admin User
    participant UI as Django Admin UI
    participant UA as UserAdmin.save_model
    participant User as User instance
    participant DB as Database

    Admin->>UI: Edit user password and save
    UI->>UA: save_model(request, obj, form, change)
    alt New object or "password" in changed_data
        UA->>User: set_password(raw_password)
        note right of UA: Hashes the provided password
    else No password change
        UA->>User: Leave password as-is
    end
    UA->>DB: super().save_model(...)\n(persist user)
    DB-->>UA: Saved
    UA-->>UI: Return
    UI-->>Admin: Save complete

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A bunny taps the admin screen—click, hop, hash!
No plaintext carrots left in the stash.
With salted twists and peppered might,
The password burrows out of sight.
Bugs beware my thumping tread—
Now only hashes get to bed. 🥕🔐

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes Check	⚠️ Warning	The added imports of make_password and identify_hasher in the user model file are not used by the new admin save_model logic and do not relate to the specified bug fix, indicating an out-of-scope change beyond the linked issue’s objectives.	Remove the unused imports from backend/djangoindia/db/models/user.py or clarify their intended use in the admin hashing workflow.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly and succinctly describes the core change—ensuring passwords are hashed when modified manually via the admin—and references the linked issue for context without extraneous detail.
Linked Issues Check	✅ Passed	The implementation of a custom save_model in UserAdmin now hashes passwords on creation and when the password field is modified via the admin, directly fulfilling the primary requirement of issue #357 without introducing unintended behavior.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eabde9f and eb1f093.

📒 Files selected for processing (2)

backend/djangoindia/db/admin.py (1 hunks)
backend/djangoindia/db/models/user.py (1 hunks)

coderabbitai · 2025-09-28T16:24:47Z

backend/djangoindia/db/admin.py

+        if "password" in form.changed_data or not change:
+            obj.set_password(form.cleaned_data["password"])
+        return super().save_model(request, obj, form, change)


⚠️ Potential issue | 🟠 Major

Avoid double-hashing already encoded passwords

Right now any value that looks like a Django hash (e.g. pbkdf2_sha256$...) gets passed back through set_password, so if an admin pastes an existing hash when migrating data the user’s credentials become unusable. Use the identify_hasher helper you already imported to detect pre-hashed inputs and only call set_password when we truly have raw text.

- if "password" in form.changed_data or not change: - obj.set_password(form.cleaned_data["password"]) + if "password" in form.changed_data or not change: + password = form.cleaned_data.get("password") + if password: + try: + identify_hasher(password) + except ValueError: + obj.set_password(password) + else: + obj.password = password return super().save_model(request, obj, form, change)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

if "password" in form.changed_data or not change:

obj.set_password(form.cleaned_data["password"])

return super().save_model(request, obj, form, change)

if "password" in form.changed_data or not change:

password = form.cleaned_data.get("password")

if password:

try:

identify_hasher(password)

except ValueError:

obj.set_password(password)

else:

obj.password = password

return super().save_model(request, obj, form, change)

🤖 Prompt for AI Agents

In backend/djangoindia/db/admin.py around lines 404-406, the code always calls obj.set_password(...) which will double-hash values that are already Django password hashes; use identify_hasher to detect pre-hashed inputs and only call set_password for raw passwords. Modify the conditional so that when "password" in form.changed_data or not change, you get pw = form.cleaned_data["password"], attempt to identify_hasher(pw) in a try/except: if identification succeeds treat it as an existing hash and assign obj.password = pw directly, otherwise call obj.set_password(pw); keep the existing return super().save_model(...) unchanged.

Fixed djangoindia#357 - Password hashing when changed manually in admin

8a1fa50

Alien501 requested a review from ankanchanda as a code owner April 13, 2025 08:13

PankajVishw50 suggested changes Apr 19, 2025

View reviewed changes

Alien501 and others added 2 commits April 20, 2025 09:37

Made it generic

e82d6ec

Merge branch 'main' into issue_357

c074f6e

PankajVishw50 approved these changes Apr 20, 2025

View reviewed changes

Fixed typo in comment

9e31abe

PankajVishw50 approved these changes Apr 21, 2025

View reviewed changes

sumit4613 reviewed May 5, 2025

View reviewed changes

ankanchanda previously approved these changes May 10, 2025

View reviewed changes

Alien501 added 3 commits May 11, 2025 08:52

Added specific exception

f305027

Added specific exception

3be7aad

Merge branch 'issue_357' of https://github.com/Alien501/djangoindia.org…

5154261

… into issue_357

Alien501 dismissed ankanchanda’s stale review via 5154261 May 11, 2025 03:23

DevilsAutumn requested changes May 31, 2025

View reviewed changes

Removed hash-checker from Model to admin

eb1f093

coderabbitai bot reviewed Sep 28, 2025

View reviewed changes

		if not self.password.startswith('pbkdf2_sha256'):
		self.password = make_password(self.password)

Conversation

Alien501 commented Apr 13, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #357

Changes

Type of change

How has this been tested?

Author Checklist

Summary by CodeRabbit

Uh oh!

PankajVishw50 Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

Alien501 commented Apr 20, 2025

Uh oh!

PankajVishw50 left a comment

Choose a reason for hiding this comment

Uh oh!

PankajVishw50 Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

sumit4613 May 5, 2025

Choose a reason for hiding this comment

Uh oh!

ankanchanda left a comment

Choose a reason for hiding this comment

Uh oh!

ankanchanda Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

Alien501 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

DevilsAutumn May 31, 2025

Choose a reason for hiding this comment

Uh oh!

Alien501 Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

yanurag-dev commented Sep 28, 2025

Uh oh!

coderabbitai bot commented Sep 28, 2025

Uh oh!

coderabbitai bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Alien501 commented Apr 13, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 28, 2025 •

edited

Loading