#12274 org.matrix.login.jwt login succeeded even if user is deactivated#12276
#12274 org.matrix.login.jwt login succeeded even if user is deactivated#12276laurent-treeb wants to merge 1 commit intomatrix-org:developfrom
Conversation
|
Is it useful to create a test to avoid it in feature? |
| else: # check if user deactivated | ||
| deactivated = await self.hs.get_datastore().get_user_deactivated_status(canonical_uid) | ||
| if deactivated: | ||
| raise UserDeactivatedError("This account has been deactivated") |
There was a problem hiding this comment.
This doesn't look quite right since it is in the create_non_existent_users block.
I'd be curious where we check for deactivation for password users / SSO users and why that isn't being applied properly. Can we consolidate that logic into a single spot?
There was a problem hiding this comment.
Annoyingly the place where we currently check for user deactivation is while authenticating against the local password database:
synapse/synapse/handlers/auth.py
Lines 1379 to 1383 in e24ff8e
Thankfully we can check if a user is deactivated via the deactivated column on the users table (rather than using an empty password hash as a heuristic).
So I'd recommend pulling the user deactivated check out of _check_local_password and instead moving it into _validate_userid_login, such that it can be applied to all user logins; not just those against the local password database.
There was a problem hiding this comment.
Thanks for digging into that @anoadragon453! That sounds sensible to me... I think we did something like this for SSO too at some point.
Looks like we did something specific for SSO too (see #7240).
There was a problem hiding this comment.
Ah, it makes sense that you'd want to show an error page for SSO flows, rather than raising an UserDeactivatedError.
Still, the local password and JWT flows could be consolidated.
|
Closing due to lack of response. If you're still interested in working on this please take a look at the review comments above and let us know. |
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)