-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unify password changing and invalidate auth tokens #27625
Conversation
KN4CK3R
commented
Oct 14, 2023
- Unify the password changing code
- Invalidate existing auth tokens when changing passwords
Have to fight an import cycle in the auth service first... |
There's an import cycle? |
Yes, after fixing the CI error
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait a sec... Deleting the auth tokens when changing passwords may be a major gotcha.
Is this normal behaviour for other services?
It's recommended by the original authors (https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies)
I don't know if others do that but I see the security benefit. A password change requests a re-login on other devices. |
I agree that that it might make sense but I think we need to update the UI to make this clear to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think it is right to remove auth tokens when changing password, to make users safe.
absent for long time, feel free to provide more ideas if you have time.
@KN4CK3R why blocked? |
Want to wait for #27625 (comment) |
One more question: if the current login uses an auth token, after deleting all auth tokens, should the current login gets a new auth token ? (just a question, regenerating it or not, either looks good to me, and keeping things simple is also good) |
Good question. Currently when you change your password in the account settings, you stay logged in because your session still exists but the "remember me" cookie is removed. So when your session times out, you have to login again. In that situation it may be valid to generate a new "remember me" session if it was present before changing the password. I will have a look if there are some best practices out there. |
I could not really find any info how to handle a password change combined with what to do with remember me infos for the local remember me session. It's consens to destroy all other sessions to lock out a possible attacker. This PR does that to the remember me tokens but I think we can't destroy other (middleware) sessions? There is no consens what should be done with the local (normal) session. Some say the user should re-login, others say otherwise. OWASP tells to regenerate the local session after a password change but I have enough knowledge if we have a potential problem here. https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change So at the moment, I think it's ok to destroy the local remember me session and require a login after the normal session timed out. It's no big deal to change this behaviour later. |
@KN4CK3R please fix the merge conflicts. 🍵 |
…itea into enhancement-auth-token
First positive effect of #28733: Changed the changed lines in this PR from |
* giteaofficial/main: [skip ci] Updated licenses and gitignores Show whether a PR is WIP inside popups (go-gitea#28975) Unify password changing and invalidate auth tokens (go-gitea#27625) Unify user update methods (go-gitea#28733) Do not render empty comments (go-gitea#29039)
- Unify the password changing code - Invalidate existing auth tokens when changing passwords