This repository was archived by the owner on Apr 26, 2024. It is now read-only.
Do not check for internal account lock for MSC3861 delegated auth#16215
Open
sandhose wants to merge 3 commits intorelease-v1.91from
Open
Do not check for internal account lock for MSC3861 delegated auth#16215sandhose wants to merge 3 commits intorelease-v1.91from
sandhose wants to merge 3 commits intorelease-v1.91from
Conversation
sandhose
force-pushed
the
quenting/hotfix-delegated-auth-admin
branch
from
e9fc985 to
842b43c
Compare
MatMaul
approved these changes
Aug 31, 2023
Contributor
MatMaul
left a comment
MatMaul
left a comment
There was a problem hiding this comment.
LGTM. If you know any RFC that would define locked account in OAuth2 spec, I am interested to have a look so we can convert the token to a proper Matrix response.
I couldn't find anything after a quick search.
hughns
force-pushed
the
quenting/hotfix-delegated-auth-admin
branch
2 times, most recently
from
1b9bd10 to
b54983d
Compare
hughns
force-pushed
the
quenting/hotfix-delegated-auth-admin
branch
from
01597b8 to
10b4e12
Compare
Member
|
@sandhose I've added some tests. Please can you take a look? |
DMRobertson
reviewed
Sep 4, 2023
Comment on lines
-285
to
-295
| # Deny the request if the user account is locked. | ||
| if not allow_locked and await self.store.get_user_locked_status( | ||
| requester.user.to_string() | ||
| ): | ||
| raise AuthError( | ||
| 401, | ||
| "User account has been locked", | ||
| errcode=Codes.USER_LOCKED, | ||
| additional_fields={"soft_logout": True}, | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Note that the same logic remains in auth/internal.py
Contributor
There was a problem hiding this comment.
Before this was removed, I think self.store.get_user_locked_status would raise a 404 error because the dummy user doesn't exist?
DMRobertson
reviewed
Sep 4, 2023
| @@ -0,0 +1 @@ | |||
| Fix a bug where admin tokens stopped working with MSC3861 auth delegation was enabled. No newline at end of file | |||
Contributor
There was a problem hiding this comment.
Is this all admin tokens, or just the admin token reserved for the identity provider?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Turns out #15870 broke the admin token for MSC3861, because MAS is doing the request with a fake user (and it's not synapse's responsibility to know if an account is locked or not)