#12274 org.matrix.login.jwt login succeeded even if user is deactivated

[laurent-treeb]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[dklimpel]

Is it useful to create a test to avoid it in feature?

[clokep]

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?

[anoadragon453]

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

	# If the password hash is None, the account has likely been deactivated
	if not password_hash:
	deactivated = await self.store.get_user_deactivated_status(user_id)
	if deactivated:
	raise UserDeactivatedError("This account has been deactivated")

.

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.

[clokep]

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).

[anoadragon453]

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.

[clokep]

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.