-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add command auth-provider for finding users by their external id on auth provider #79
Conversation
Hi, thanks a lot for the submission. Great addition to synadm! First a high level suggestion / design question: Since both arguments (user, provider) are mandatory, they could be implemented as regular positional arguments or was there a special reason why you designed provider as an optional --option? In any case (positional argument or --option), the handling of a missing argument can be handled by the Click library already. Pos Args handle it by default, --option you have to set the required flag: https://click.palletsprojects.com/en/7.x/options/#basic-value-options It's on my todo list to finally write some contribution guidelines and even automate some auto-linting in PR's, so you couldn't possibly have known that: Please try to stay below 79 chars line length and use PEP 8 line continuation methods described in the PEP 8 styleguides. Here you could shorten / rewrite the first sentence of your docstring so it is obvious to the user what this command is supposed to do. Just shorten it as good as possible so it makes sense and is not cut. A more detailed description can be added after a newline in the docstring.
Suggestion: Find a user based on their auth-provider ID. Also, but I'm not quite sure about that one: Do you think it would make sense to reveal some detail about what I see you added added parts of this information from the api docs which I think is a good start:
Maybe point to another synadm command that reveals what m.login.sso tells. I think |
Hi @JOJ0 Thanks for the feedback! My responses are below:
Mainly to be explicit about it. Invoking
Great! I have made that change.
Absolutely! I understand. I am pretty new to Python so still learning the ropes here. I have changed this manually by running
Excellent! Done!
I couldn't find an existing command to add to the description here, so improved the description slightly with an example. |
…h is caught early and displayed
It's ok, I was not sure what's the best way either. We have mandatory One new idea though, not sure if it makes sense: Could You could rename EXTERNAL_ID to EXTERNAL_USER_ID, it would clarify even more what the argument is about:
Thanks for the changes. Great. To make sure and even test the new automatic linting you could just
Please add an empty line after the very first sentence in the docstring and keep writing of ID/id consistent throughout. I suggest ID everywhere.
Yeah you're right, the user details command does not reveal that, I was mistaken, so yes: Your change is great. Having some examples what a provider could be helps a lot to a novice user like me that hadn't much to do with auth-providers and Matrix yet ;-) |
Ok, lets keep it as is then.
I don't believe so. EMS does call their OIDC config as 'oidc' and they may have 'github' and 'google' configurations for "Login via Github/Google", but I don't believe its safe to assume a default here. Its really what string you define it as in your Synapse config. Mapping providers are a different thing. That's more like how to map out the response from these auth providers. They have a default one, but you can define a custom one as well.
Done!
Done!
Ok, will rebase to trigger that.
Sure! So, lets say you configured SSO with a custom auth provider (this could be an established service like Google or Github as well), but now you have an identifier for them in the external system (your own or Github's username) but you don't know what username they chose to create or was created for them on Synapse. So this lets you find that user by specifying that external ID. Like show the user who is "ashfame" on Github. Maybe "ashfame" was already taken and I got "@ashfame2:server.host" as my identity. Hope that's helpful :) |
Ok, looks like I don't need to rebase, it can already be triggered but require an approval from your side. |
ah oh, shouldn't it run automatically once approved? Atleast on the same PR? Please hit the button again. |
I think it works as designed by github, once your first pr was merged, it will be automatic. |
@JOJ0 Is this ready to be merged? :) |
Yes it is! Thanks a lot for another great synadm feature and speak soon I guess. :-) |
This PR adds support for admin endpoint for finding users by their external id on auth provider, which was added in Synapse v1.68.0
Unlike other endpoints, it requires 2 values:
Correct usage
Incorrect usage
Looking forward to your feedback!
Subsequent PR to follow up for similar lookup for 3PID that's merged but not released (Coming in Synapse v1.72.0) and then we can evaluate if we should add shortcuts for them under search or something else.