New, Dynamic user registration role #1410
Merged
Conversation
This file contains 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
dpgaspar
requested changes
Jun 22, 2020
requirements.txt
Outdated
| @@ -22,6 +22,7 @@ flask==1.1.1 | |||
| idna==2.9 # via email-validator | |||
| itsdangerous==1.1.0 # via flask | |||
| jinja2==2.10.1 # via flask, flask-babel | |||
| jmespath==0.9.5 | |||
There was a problem hiding this comment.
If it's not on setup.py it can't be here, this file is auto generated by pip-compile. We could add it to requirements-dev.txt if we decide it's not a root dependency
There was a problem hiding this comment.
Thanks! I think it's not a root dependency. Added to extras.
flask_appbuilder/security/manager.py
Outdated
| @@ -347,6 +348,10 @@ def auth_user_registration(self): | |||
| def auth_user_registration_role(self): | |||
| return self.appbuilder.get_app.config["AUTH_USER_REGISTRATION_ROLE"] | |||
|
|
|||
| @property | |||
| def auth_user_registration_role_jmespath(self): | |||
There was a problem hiding this comment.
nit: I'm starting to add type annotations, at least on every new PR.
def auth_user_registration_role_jmespath(self) -> str:
|
@dpgaspar Thanks for the review! I've moved the dependency to setup's extras and added a few lines of documentation with examples |
dpgaspar
approved these changes
Jun 23, 2020
This was referenced Jul 3, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR brings dynamic user role on registration based on OAuth userinfo.
The current approach sets static
AUTH_USER_REGISTRATION_ROLEfor all users if self-registration is enabled. In our case, with OAuth authentication on GSuite domain (with FAB being auth engine for Airflow), we'd like to set team members as Admins and every other employee as read-only user ("Viewer").This PR implements method that is used in Grafana (docs), it uses JMESPath to resolve the role from userinfo retrieved from OAuth. It's quite flexible, as some OAuth providers return only username (like Google does), but others, like Okta, can return groups, so they can be utilized by the dynamic expression.
If needed, I'm more than happy to extend the docs by adding more examples of JMESPath.