-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Update Simple auth manager to define its UI (login form) using fastapi #45696
base: main
Are you sure you want to change the base?
Conversation
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.
Tested locally. It works!
I'd like us to match the main UI's setup a bit more. Use pnpm
instead of npm
and I want to see if we can share the same linting/prettier rules.
export const Login = () => { | ||
const onSuccess = (data: LoginResponse) => { | ||
// Redirect to index page with the token | ||
globalThis.location.replace(`/webapp/?token=${data.jwt_token}`); |
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.
We'll need to remember to change this whenever we drop the /webapp
. Or better, have one source of truth on the UI base route
I agree that would be great. I made multiple tries but could not come up with something working. Typescript could not recognize types defined in the simple auth manager UI directory. We could give it another try later. I dont think this is a must have for now |
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.
That's a little unclear for me where we're at and what we want to do for auth managers.
For other auth managers, especially FAB one, do we plan to use the flask middleware to be able to render all of those flask views into fastapi ? (avoiding the flask <-> fastapi convertion, I think this is what was suggested by Jed.)
Then we only do this work for the simple auth manager because we do not need backward compat and because it's more convenient to have that in native FastAPI ?
Yes to everything :) Edit: more details. Mainly because it does not make sense to define the UI in Flask in core Airflow while the future of Airflow is clearly fastapi. |
tldr core + simple auth manager should not require flask to be installed at all. That ship has already sailed if you are using the fab auth manager though, so might as well use the middleware and not rewrite stuff. |
Yep, this is what I did in #45765 for FAB :) |
Any other concerns/questions? |
Question for the audience :) I am looking into updating the basic tests to include tests from the simple auth manager UI as part of the tests-ui. Some caching is done that I am not familiar with. Do I need to do the same or just
|
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.
Any other concerns/questions?
No, thanks for answering my questions.
Overall code looks good and I agree with the general approach.
I think Brent is already working on a detailed review and functional testing.
Fantastic! Thank you :) |
I am not super familiar with this workflow but by the look of it I would say that caching the node_modules is always helpful. I think we can just follow the same logic and apply that to the |
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.
Re-approved a second time by mistake.
9abb97f
to
d8f6975
Compare
856b391
to
fa2a841
Compare
0cf5935
to
44e8cde
Compare
I dont understand what is going on ... The command However when I run the script |
SimpleAuthManager
is a minimalist auth manager in core Airflow. It defines its own login form in order for users to log in. Example below:Simple auth manager has its UI defined using Flask, you can then use it with Airflow 2 UI. But the goal is to use simple auth manager in Airflow 3. In order to use simple auth manager with Airflow 3 UI, we need to implement its views in the fastapi application. This is the goal of this PR.
After this PR is merged, I'll remove the UI/views definitions of simple auth manager in Flask. We do not need the simple auth manager to be compatible with Airflow 2 UI (although that was necessary for testing it).
As a result of this PR, multiple endpoints are added:
(Sorry for the massive PR).
Testing
To test this PR and play around with the simple auth manager login form:
AIRFLOW__CORE__AUTH_MANAGER="airflow.auth.managers.simple.simple_auth_manager.SimpleAuthManager"
airflow/auth/managers/simple/ui/
, run npm run build. I need to update the pre-commit script to do that automatically^ 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.