Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implements admin API to lock an user (MSC3939) #15870

Merged
merged 19 commits into from
Aug 10, 2023
Merged

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Jul 3, 2023

This PR implements a way to lock an user without deactivating them.

It is based on MSC3939.

The locked user is also hidden from the user directory. This behavior can be tweaked with an option.

Pull Request Checklist

@MatMaul MatMaul changed the title Implements admin API to lock an user Implements admin API to lock an user (MSC3939) Jul 5, 2023
@MatMaul
Copy link
Contributor Author

MatMaul commented Jul 5, 2023

This comment suggests that this MSC was also written to deal with the expiring accounts feature.

I am hence inclined to:

  • ditch allow_expired parameter from get_user_by_req and use allow_locked for both
  • return the locked error code in the expired account case

I think it also mean that we want to deprecate is_user_expired module API and introduce is_user_locked instead.

I'll not do that in this PR and push a new one later on.

@MatMaul MatMaul marked this pull request as ready for review July 5, 2023 13:00
@MatMaul MatMaul requested a review from a team as a code owner July 5, 2023 13:00
@@ -205,6 +207,16 @@ async def get_user_by_req(
# so that we don't provision the user if they don't have enough permission:
requester = await self.get_user_by_access_token(access_token, allow_expired)

# Deny the request if the user account is locked.
if not allow_locked and await self.store.get_user_locked_status(
Copy link
Contributor Author

@MatMaul MatMaul Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandhose @hughns do you have an insight about if we should do that in the MSC3861 case too ?

I feel that it is perhaps something that should be dealt with by the OIDC provider, but I also think that it doesn't hurt to have the synapse layer provides an extra mechanism on top.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we keep the synapse layer here, if a mechanism exists to signal a locked user in OIDC (does it ?), we should probably convert that to the proper synapse API error code (introduced by MSC3939).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently implemented account locking in MAS. This should be implemented when Synapse introspects the token ; there is no standard on the introspection API to signal that, but we can do something somewhat custom. It's already on my radar

@MatMaul MatMaul added the Z-Time-Tracked Element employees should track their time spent on this issue/PR. label Jul 5, 2023
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo!

synapse/storage/databases/main/registration.py Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z-Time-Tracked Element employees should track their time spent on this issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants