Skip to content
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 possibility to create user in the Remote User mode Auth. #19963

Merged
merged 1 commit into from Jan 28, 2022

Conversation

lwyszomi
Copy link
Contributor

@lwyszomi lwyszomi commented Dec 2, 2021

This fix gives a possibility to create user in the Remote User Auth. I know that we have open discussion about the same issue in the LDAP mode (#18545), but in the Remote User mode I think this option should be available.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Dec 2, 2021
@potiuk potiuk requested a review from jhtimmins December 6, 2021 00:08
@potiuk
Copy link
Member

potiuk commented Dec 6, 2021

I think some common approach here should be worked out. I am for adding it, but I am not as "deeply" in the rules and configuration - I believe for the new UI we are porting FAB permission model to Airlfow. so maybe that's a good time to discuss and agree something there @ashb @jithimmins @bbovenzi @ryanahamilton ?

@kosteev
Copy link
Contributor

kosteev commented Jan 17, 2022

@potiuk @jhtimmins Can you, please, review this fix/change one more time.

@jhtimmins
Copy link
Contributor

This is a sensible change, and looks like it was supported in the original version of UserRemoteUserModelView in Flask Appbuilder.

https://github.com/dpgaspar/Flask-AppBuilder/blob/985b1b7c1d77d38e667199573455a4a3b6f40ac9/flask_appbuilder/security/views.py#L286

It shouldn't be necessary to modify _class_permission_name or class_permission_name_mapping though, since those values remain unchanged from the base class.

It may be tricky to test though. @kosteev or @lwyszomi can one of y'all verify that the change works as intended and that you can actually add a new user and then log in with that user?

Copy link
Contributor

@jhtimmins jhtimmins left a comment

Choose a reason for hiding this comment

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

This change makes sense. Please see my comment for requested changes, in addition to the merge conflicts.

@lwyszomi
Copy link
Contributor Author

@jhtimmins I duplicated both values from base Class to easly show that this is needed in the UserRemoteUserModelView. We have similar implementation in the CustomUserDBModelView one of the parameters are copy pasted for Mixin class, second is changed.

Change is already tested and all works fine.

@jhtimmins jhtimmins merged commit cdd9ea6 into apache:main Jan 28, 2022
@potiuk potiuk added this to the Airflow 2.2.4 milestone Feb 6, 2022
@potiuk
Copy link
Member

potiuk commented Feb 6, 2022

Hey @jedcunningham - I think that one should be easy to cherry-pick to 2.1.4 and it is a bit of regression (not entirely - this was a change in FAB rather than in Airflow). I marked it as 2.1.4 but maybe you can decide whether to cherry-pick it to 2.2.4 or not.

jedcunningham pushed a commit that referenced this pull request Feb 8, 2022
jedcunningham pushed a commit that referenced this pull request Feb 10, 2022
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Feb 16, 2022
jedcunningham pushed a commit that referenced this pull request Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants