Skip to content

Conversation

@vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Sep 4, 2024

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
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

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).

@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
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

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
Copy link
Contributor

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

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 :)

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

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
joaopamaral pushed a commit to joaopamaral/airflow that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants