-
Notifications
You must be signed in to change notification settings - Fork 365
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 dedicated doc on details of allowing access #729
Conversation
# when combined with other allow options | ||
OAuthenticator.class_traits()[ | ||
"allowed_users" | ||
].help = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base Authenticator.allowed_users docstring gives the wrong impression of how it relates to other allow
config, since the base Authenticator doesn't have other allow
config. Patching this way allows us to modify the docstring, which in turn shows up in our docs, without redeclaring the trait.
It's unclear to me a bit whether things like allowed_idps/hosted_domain expanations belong here vs the provider-specific doc pages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions, looks good to me!
Those two are odd ducks, they shouldn't be classified as allow config. They are requirements to authenticate currently, and hosted_domain could move to be part of the blocked check if the implementation is refactored. See #683 |
Co-authored-by: Simon Li <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great to have!
Co-authored-by: Erik Sundell <[email protected]>
I'll reflect further on #729 (comment), but i think there isn't an action point remaining in this PR about it - this LGTM and can be merged. @minrk - ok to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @minrk!!!
Thanks for the thoughtful reviews! |
Rendered doc: https://oauthenticator--729.org.readthedocs.build/en/729/topic/allowing.html
closes #727