-
Notifications
You must be signed in to change notification settings - Fork 111
Auto login after a password change #30
Comments
I like this. We should make sure to inform the user that their password was successfully reset before they are taken somewhere else. |
I agree. What do you think about a confirmation page like the verification one that would redirect (with a meta tag) the user automatically to a specified page after a small pause to allow him to understand what is going on (behavior that could be overridden by the developper by changing the template)? This route would only be available if the auto login feature is on. |
I like this -- I'd definitely merge that. Having it pause for like 10 seconds in the meta tag should be fine. |
It's almost done. I just need to test one thing and I'll submit the PR
|
We haven't talked about this yet @rdegges - any security / password related stuff should be verified before it is released. For example, depending on security requirements, it is rarely a good idea to allow auto-login without first asserting the old password that is to be changed. |
@lhazlewood I fully agree with you and this is why this flow is opt-in. In our app, we decided this was an acceptable flow security-wise. In this case, what would verifying the old password add to the security? Auto-login or not, if someone had access to your reset link, your account is now compromised. The attacker would be able to reset the password and log in anyway. The advantage here is flexibility. In my directory, I can set a password requirement with a minimum of 1 character. I would expect to have the same flexibility here to decide what's my acceptable level of security. On a different subject, we discussed the fact that after a password reset/change, all sessions (or at least, all other sessions) should be nuked to prevent access to the account on other computers. How would it be possible to do this client-sessions if the sessions are client-side? In my case, I can do it because I swapped the session middleware for express-session, but still... the session nuking should happen in express-stormpath in my opinion. |
For 'forgot password' scenario, this is true and auto-login makes sense here. When I posted i was thinking of the 'I know my existing password and just want to change it to a new one' scenario. For this latter scenario, the existing password should be asserted. It wasn't clear to me exactly which scenario was being discussed here :) |
I don't like static announcement pages that meta refresh to another location - it feels very 'Web 1.0' ish. Instead, in the Java SDK, if a success message or status is supposed to be displayed, I've removed the intermediate status pages and a This acts as a message source for 'flash' (one time use) messages: the destination page/controller knows that if that query param is present, it can take the query param value and use that as an i18n key and show a status message anywhere it wants in the resulting page. For example, consider the registration flow:
For a Bootstrap theme, this basically just shows a colored message box (http://getbootstrap.com/css/#helper-classes-backgrounds). |
:)
I agree with you on this. Using flash messages is fairly common practice. The other way around would be to store the messages in the session instead of having a querystring. Personnally, I'm not fond of having state in the querystring, just because the user can mess with it and have your application behave in a way you didn't expect it to (even if it's showing a harmless message on the login page), but that's a personnal preference. I'd like to think that my app is like a state machine, if you do not come from the place that generates that message, it should not be possible to show it. |
Like the auto-login feature post-registration when the verification flow is disabled, we would like to have a similir functionnality post-password change. From a UX perspective, it makes little sense to have a auto-login at the registration but not at after the password reset.
I would like very much to add this (fully optional) feature to the lib.
What do you think? I would probably have a PR tomorrow.
The text was updated successfully, but these errors were encountered: