Page MenuHomePhabricator

Delete OAuth 2.0 access tokens on password change
Open, Needs TriagePublic

Description

"As a User, I want to delete all access tokens when I change my password, so that applications need to re-authorize."

This is a best practice in the OAuth 2.0 world, based on the assumption that the user needed to change their password because they're concerned about a security breach.

Event Timeline

Aklapper added a subscriber: EvanProdromou.

Adding missing MediaWiki-REST-API code project tag as Core Platform Team Initiatives (MW REST API in PHP) team tag is archived and its parent Platform Engineering team does not exist anymore

Tgr added subscribers: sbassett, Tgr.

We'd have to delete accepted consumers in the OAuth session provider's invalidateSessionsForUser() callback, much like we already do for preventSessionsForUser(). That would be run every time the user is force-logged out (e.g. password change, 2FA change, user rename, steward lock, invalidateUserSessions.php). Code-wise a trivial change, not 100% sure of the implications but seems reasonable. (Maybe a bit disruptive for owner-only consumers, where the user would have to go to each such app's Special:OAuthComsumerRegistration subpage and do a token reset. Not much security value if we don't disable owner-only consumers, though.)
@sbassett maybe the Security team has an opinion on this?

@sbassett maybe the Security team has an opinion on this?

In the UI, could we have this as a default-checked option for the user? That way, someone who's rotating a password but doesn't have strong suspicions of malice towards their account, wouldn't have to re-auth their affected accounts, if I'm understanding this correctly.

Possible although a bit ugly. There are two core mechanisms for removing authentication methods, invalidateSessionsForUser() which we are already calling for credential changes, and preventSessionsForUser() which is intended to be permanent (used for account usurpation by a system account). AuthManager::changeAuthenticationData() which handles credential changes does not handle multiple form fragments like login/signup does. This doesn't fit any of those generic mechanisms, so it would have to be a one-off - modify the form generation logic in SpecialChangeCredentials to add the checkbox, add a new hook to SpecialChangeCredentials::success() (and I suppose changePassword.php) that gets called when the option is checked, and add a hook handler to the OAuth extension that removes the access tokens. And maybe add some way for hook handlers to return a message, so e.g. OAuth can advise the user on how to reset owner-only keys.

FWIW we already reset bot passwords on password change. I guess the discrepancy between that and OAuth tokens doesn't really make sense.