Show a lockout banner and clear the lockout on password change - #38116
Show a lockout banner and clear the lockout on password change#38116AmitPhulera wants to merge 5 commits into
Conversation
Too many failed password attempts lock an account for password-based auth (login form, Basic-auth APIs such as OData feeds). The lock was cleared by the login form and the emailed reset link, but not by the in-session Change My Password form, so a locked user who changed their password stayed locked for API access.
Follow the post/redirect/get pattern so the re-rendered page reflects the updated user state, and refresh the session auth hash the way Django's own PasswordChangeView does so the user is not logged out of the session they just used to change their password. Other sessions are still invalidated.
The lockout after too many failed password attempts only affects password-based auth, so a user with an existing web session never learns about it and cannot tell why their OData feed or other API integration stopped authenticating. SSO sessions cannot change a password, so the flag also says whether to offer that as the fix. SAAS-19943
dimagimon
added
the
Risk: High
fd2f2a9 to
65e7a86
Compare
Explains that the lock blocks OData feeds and other integrations that sign in with the user's password, and links to Change My Password, which now clears it. SSO sessions are told to log out and back in instead. The banner reuses the maintenance/trial banner rule so it sits flush against the navbar and is layered above fixed elements such as the App Manager preview toggler. SAAS-19943
base_navigation.html gained the lockout banner include and the alert stylesheets gained the alert-lockout selector in both Bootstrap 3 and Bootstrap 5 versions.
65e7a86 to
87da013
Compare
| clean_password(request.POST['new_password1']) | ||
| self.password_change_form.save() | ||
| # keep the current session valid; other sessions are invalidated | ||
| update_session_auth_hash(request, self.password_change_form.user) |
There was a problem hiding this comment.
This slightly changes existing behaviour, we used to log out user when they have changed the password. Now it would keep the current session logged in after the password is change.
In my opinion, this is a better User Experience.
There was a problem hiding this comment.
I'm slightly skeptical about this change security-wise but I agree it's nicer UX. I'd guess that by updating the session auth hash we're invalidating the prior session token and issuing a new one? Would be interested to hear other opinions on it before approving.
There was a problem hiding this comment.
I'd guess that by updating the session auth hash we're invalidating the prior session token and issuing a new one?
Yes. That is true.
Would be interested to hear other opinions on it before approving.
Makes sense. I also wanted inputs on it before I merge it. I will open it up to the wider team to get the feedback.
| user = super(HQPasswordChangeForm, self).save(commit) | ||
| couch_user = CouchUser.from_django_user(user) | ||
| couch_user.last_password_set = datetime.utcnow() | ||
| # a new password also lifts the lockout from too many failed attempts |
There was a problem hiding this comment.
"also" without context of something else is odd here. I don't think I find this comment necessary
There was a problem hiding this comment.
True. I thought the commit message paired with this docstring would be helpful. Fine to remove if it was not helpful.
Sharing the reasoning - Earlier while resetting password from HQ directly failed attempts was not reset. It used to be only reset with forget password workflow.
That is what "also" signifies here.
| {% include "hqwebapp/partials/trial_banner.html" %} | ||
| {% endif %} | ||
| @@ -17,38 +17,62 @@ | ||
| @@ -20,38 +20,62 @@ |
There was a problem hiding this comment.
Would be nice to see this commit with its default "Bootstrap 5 Migration - Rebuilt diffs" message, but that's a minor nitpick.
There was a problem hiding this comment.
Ohh! I see. I asked Claude to redo the commit messages, I think I messed up the original message in that. I will keep that in mind. Thanks for flagging.
| clean_password(request.POST['new_password1']) | ||
| self.password_change_form.save() | ||
| # keep the current session valid; other sessions are invalidated | ||
| update_session_auth_hash(request, self.password_change_form.user) |
There was a problem hiding this comment.
I'm slightly skeptical about this change security-wise but I agree it's nicer UX. I'd guess that by updating the session auth hash we're invalidating the prior session token and issuing a new one? Would be interested to hear other opinions on it before approving.
AmitPhulera
changed the title
3 participants
Product Description
Ticket: https://dimagi.atlassian.net/browse/SAAS-19943
After five failed password attempts an account is locked for password-based auth. Because the lock is only checked by the login form and by Basic-auth API requests, a web user with an existing session keeps browsing HQ normally and has no way of knowing that their OData feed (or any other integration that signs in with their password) has stopped authenticating. They only find out after logging out and trying to log back in.
This PR:
Review by commit 🐡
Dashboard :
Feature Flag
None.
Safety Assurance
Safety story
{}in every other case.login_attemptson password change mirrors what the login form and the password-reset-by-email flow already do.maximum password attempts exceeded), banner appears on Bootstrap 3 and Bootstrap 5 pages, changing the password in-session removes the banner and the API accepts the new password.Automated test coverage
Added some tests for the changes made.
QA Plan
N/A
Rollback instructions
Labels & Review