Skip to content

Implement SimpleAuthManager#42004

Merged
vincbeck merged 7 commits intoapache:mainfrom
aws-mwaa:vincbeck/simple_auth_manager
Sep 17, 2024
Merged

Implement SimpleAuthManager#42004
vincbeck merged 7 commits intoapache:mainfrom
aws-mwaa:vincbeck/simple_auth_manager

Conversation

@vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Sep 4, 2024 •
edited
Loading

Implement the simple auth manager intended to be used in Airflow 3. For more information about the simple auth manager, see #41683.

Login form
Screenshot 2024-09-04 at 3 44 47 PM

Login form (dark mode)
Screenshot 2024-09-04 at 11 55 19 AM

Error message when wrong credentials
Screenshot 2024-09-04 at 11 58 37 AM

What is not part of this PR and will be done in follow-up PRs:

  • The feature in simple auth manager disabling the authentication and allowing everyone with all permissions
  • Documentation
  • Setting the simple auth manager as default in Airflow
  • Update tests to rely on simple auth manager and no longer on FAB auth manager (besides FAB auth manager tests of course)
  • Add warning message in the UI letting the user knows this is for development purposes only

This simple auth manager is required for completion of AIP-79.

Other note:

  • The simple auth manager uses Flask app builder to register a view to Airflow environment. This is how it is done today in auth managers to register custom views. This will change in the future and will likely leverage the mechanism provided by AIP-68 to create new views in the Airflow environment through plugins

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Sep 4, 2024
@vincbeck vincbeck requested review from bbovenzi and removed request for bbovenzi September 4, 2024 21:12
ferruzzi
ferruzzi reviewed Sep 4, 2024
View reviewed changes
Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

I'm not great with front-end yet, but it all appears to be in order. +0 (binding) :P

potiuk
potiuk approved these changes Sep 4, 2024
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM (but also no UI / View experience enough to make constructive comments - so maybe others (@bbovenzi @pierrejeambrun might comment how this will fit in the future as well).

I like how indeed "simple" it is now. Probably we should figure out the way how you could set (and override) the user/role mappoing. I am not 100% convinced if webserver config is the right way - IMHO having a toml-based structured configuration might be better (but it will be future work as you mentioned).

jedcunningham
jedcunningham reviewed Sep 4, 2024
View reviewed changes
@pierrejeambrun pierrejeambrun mentioned this pull request Sep 5, 2024
Closed
@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 5, 2024

I am not 100% convinced if webserver config is the right way - IMHO having a toml-based structured configuration might be better (but it will be future work as you mentioned).

100% agree with you. I dont like it either but it is more a temporary solution. Whenever we switch the Airflow config to TOML, my intention is to leverage it

@vincbeck vincbeck requested a review from jscheffl as a code owner September 16, 2024 16:23
jscheffl
jscheffl approved these changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Pure-reading the code looks good - as integration in breeze will come later - what is the esiest way to test (locally)? Would like to see it pre.merge on my laptop once :-D

@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 16, 2024

Pure-reading the code looks good - as integration in breeze will come later - what is the esiest way to test (locally)? Would like to see it pre.merge on my laptop once :-D

export AIRFLOW__CORE__AUTH_MANAGER='airflow.auth.managers.simple.simple_auth_manager.SimpleAuthManager' :)

jscheffl
jscheffl reviewed Sep 16, 2024
View reviewed changes
@jscheffl
Copy link
Contributor

jscheffl commented Sep 16, 2024

Pure-reading the code looks good - as integration in breeze will come later - what is the esiest way to test (locally)? Would like to see it pre.merge on my laptop once :-D

export AIRFLOW__CORE__AUTH_MANAGER='airflow.auth.managers.simple.simple_auth_manager.SimpleAuthManager' :)

RTFM :-D Besides one small nit in the example is working for the legacy UI.
NOT working for the new UI though. But would rather blame the new UI and not the simple login manager?

@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 16, 2024

Pure-reading the code looks good - as integration in breeze will come later - what is the esiest way to test (locally)? Would like to see it pre.merge on my laptop once :-D

export AIRFLOW__CORE__AUTH_MANAGER='airflow.auth.managers.simple.simple_auth_manager.SimpleAuthManager' :)

RTFM :-D Besides one small nit in the example is working for the legacy UI. NOT working for the new UI though. But would rather blame the new UI and not the simple login manager?

Yep, waiting for AIP-68 :)

pierrejeambrun
pierrejeambrun reviewed Sep 17, 2024
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Frontend code looks good. Just the form could be improved for better error handling (we have a useErrorToast) and use ReactQuery with an onSubmit, instead of the oldschool action interface. But this will get updated when we move to the new UI anyway, so I don't think it's worth spending more time on that.

@vincbeck vincbeck merged commit b599d81 into apache:main Sep 17, 2024
@vincbeck vincbeck deleted the vincbeck/simple_auth_manager branch September 17, 2024 16:24
gopidesupavan pushed a commit to gopidesupavan/airflow that referenced this pull request Sep 17, 2024
@vincbeck @gopidesupavan
Implement SimpleAuthManager (apache#42004)
38355b8
@vincbeck vincbeck mentioned this pull request Sep 17, 2024
Merged
@vincbeck vincbeck mentioned this pull request Oct 10, 2024
Merged
joaopamaral pushed a commit to joaopamaral/airflow that referenced this pull request Oct 21, 2024
@vincbeck @joaopamaral
Implement SimpleAuthManager (apache#42004)
3ab4723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@ferruzzi ferruzzi ferruzzi left review comments

@pierrejeambrun pierrejeambrun pierrejeambrun left review comments

@jedcunningham jedcunningham jedcunningham left review comments

@jscheffl jscheffl jscheffl approved these changes

@ephraimbuddy ephraimbuddy ephraimbuddy approved these changes

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

@ashb ashb Awaiting requested review from ashb

@bbovenzi bbovenzi Awaiting requested review from bbovenzi

Assignees

No one assigned

Labels

area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vincbeck @jscheffl @potiuk @ferruzzi @ephraimbuddy @pierrejeambrun @jedcunningham