Unify password changing and invalidate auth tokens

[KN4CK3R]

• Unify the password changing code
• Invalidate existing auth tokens when changing passwords

[KN4CK3R]

Have to fight an import cycle in the auth service first...

[delvh]

There's an import cycle?

[KN4CK3R]

Yes, after fixing the CI error

[zeripath]

Wait a sec... Deleting the auth tokens when changing passwords may be a major gotcha.

Is this normal behaviour for other services?

[KN4CK3R]

It's recommended by the original authors (https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies)

Important: If the user should ever change their password, you should invalidate all existing long-term authentication tokens for that user.

I don't know if others do that but I see the security benefit. A password change requests a re-login on other devices.
See the comments to this answer for example: https://security.stackexchange.com/a/112857

[zeripath]

It's recommended by the original authors (https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies)

Important: If the user should ever change their password, you should invalidate all existing long-term authentication tokens for that user.

I don't know if others do that but I see the security benefit. A password change requests a re-login on other devices. See the comments to this answer for example: https://security.stackexchange.com/a/112857

I agree that that it might make sense but I think we need to update the UI to make this clear to the user.

[wxiaoguang]

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.

[delvh]

@KN4CK3R why blocked?

[KN4CK3R]

Want to wait for #27625 (comment)

[wxiaoguang]

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)

[KN4CK3R]

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.

[KN4CK3R]

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.

[GiteaBot]

@KN4CK3R please fix the merge conflicts. 🍵

[KN4CK3R]

First positive effect of #28733: Changed the changed lines in this PR from +44 -42 to +20 -1 🎉