-
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
UI Rest API authentication #42019
UI Rest API authentication #42019
Conversation
cc: @vincbeck @jedcunningham I know you both are working on the FAB / auth / permissions part, might use your input. (maybe there's an easier way of making that work) |
Well the CI is not happy, working on it. |
977a039
to
35f93fe
Compare
def create_app() -> FastAPI: | ||
def init_flask_app(app: FastAPI, testing: bool = False) -> None: | ||
""" | ||
Auth providers and permission logic are tightly coupled to Flask. |
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.
Question. Is it entirely true for Airflow 3? Do we want to base all our authentication on Flask and have to create a Flask App (cc: @vincbeck ?). I do not think there is (maybe due to compatibility but can be removed) any requirement to use flask in our Auth Manager?
Not that I have another proposal there - as I have a little knowledge about flask/asgi/wsgi etc - but I am not sure if we really need to use Flask. Flask is essentially synchronous and writing APIs is not really the main goal of Flask (producing HTML output is). But if we get rid of Flask we could be free to use sync/async requests as we want and some of the problems where joining Starlette, ASGI, gunicorn, uvicorn and all the complexity resulting from that could be avoided.
If we are going fast-api-first and async built-in, getting rid of flask should only bring benefits IMHO
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.
(Maybe I do not understand the tight-coupling here) - but I don't think we are in any way tightly coupled with flask (other than compatibility requirements for the old plugins).
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.
Yes indeed we really do not want to have Flask around anymore I believe when we release airflow 3. There is no reason to if all of our APIs are on FastAPI.
Yes FastAPI has async support natively, but we cannot have that 'freely' now because most of the db utility code of airflow, for instance how to create sessions and provide them via a decorator, how to create the db engine, context managers etc are not yet written in an async way, so we would need to write that. (I think that would be part of AIP-70). It is definitely the endgoal to be fully async for a FastAPI. Maybe we don't need AIP-70 for that but it's a significant effort to rewrite all of those and not critical for the POC to work. Also regarding the migration of endpionts from the old api to the new api it will be a bit easier if both are sync, basically that's close to a copy and past (i am exaggerating a bit but you get the idea). More code will need to be adapted if the new endpoints are async while the old ones are not. Maybe it is better to do that in a 'second' step, when everything is working in fastapi synchronously.
do not think there is (maybe due to compatibility but can be removed) any requirement to use flask in our Auth Manager?
(Maybe I do not understand the tight-coupling here) - but I don't think we are in any way tightly coupled with flask (other than compatibility requirements for the old plugins).
Those are the problem at the moment I believe. At multiple places in our backend_auth, permission code, etc.. etc..., we use the flask app context to retrieve global things such as the connected user, the sessions, the current request. Those are not available when running FastAPI, to give some exemples:
FABAuthManager
uses flask_login.current_user, connexion.FlaskAPI,- Almost all the backend_auth accessing the flask app or app context
FabAirflowSecurityManagerOverride
has calls to flask libraries to handle jwt or openid or access directly the flaskg
global object to retrieve the user. Usesflask_login.LoginManager
,flask.flash
or thesession
global object.- The
AnonymousUser
is using the flask.current_app() and aflask_login
mixin. - Most of the places where an AirflowApp, AppBuilder is required, we need a flask app underneath
Those are just some exemples, there a plenty more in the code, maybe I am misunderstanding something and Vincent/Jed will have more information on that and we can simply write another SecurityManager / AuthManager agnostic of flask but I am not sure yet what is the best approach.
Also just to mention that FastAPI and Flask work in two different paradigm. Flask uses global object and application context accessor basically via importing g
or current_user
and using them where you need them in the views / provider / utility / managers. In opposition FastAPI uses dependency injection, where function signature declares its dependency and FastAPI will inject them at runtime. There might be some refactoring involved due to this shift.
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.
There is definitely going to have to be some decoupling/refactoring anyways. For example, auth managers expect an appbuilder today, and it's already on my "must change" list for AIP-79 :)
But yes, FAB / our security manager / auth managers, it's all pretty intertwined and messy today. I believe we can solve it in 3, but in the meantime I think initing like this is okay.
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.
I have quickly looked at the code, I will look deeper and provide inline comments to help but overall:
- We must implement this new endpoint in an auth manager agnostic way. In other words, we should not assume the environment is going to use FAB auth manager or any other auth manager.
FABAuthManager
,FabAirflowSecurityManagerOverride
,AnonymousUser
are all specific to FAB auth manager. - Even though we want to remove FAB from Airflow 3, we want FAB auth manager to be usable with Airflow 3. I know it might sound contradictory, I dont have real solution here, this will need to be done in AIP-79. The way I see it (please correct me if I am wrong @jedcunningham) is, for the moment we might need to initialize some resources (such as Flask app) just for FAB auth manager to work properly. Another solution would be to make the UI Rest API only compatible with simple auth manager and once AIP-79 is complete, we can get rid of this limitation. But that would mean we need to make the simple auth manager the default one before we can merge this PR
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.
I dont want to block your PR though, this is not blocking and can be solved later but this is something we definitely need to figure out
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.
If we do not rely on a session auth, we need the front end to pass a Basic auth header. Basically the base 64 encoded username and password on each request. There are utilities in the front end to do that. And maybe store the username/password in cookies in the frontend. 🤔. There are definitely solutions for 'basic auth' workflow on the front end but this is just for development indeed.
For production I am not even sure that we want session cookie based auth for a modern FastAPI app. JWT Bearer might seem more appropriate. But we do not have that kind of auth backend available yet ?
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.
Yes I think we need to align on the authentication storage. I assumed we were going to use a session but we might want to take a different approach.
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.
After a discussion with @bbovenzi at the Airflow summit, it seems we are aiming toward an identification using JWT token and session as storage for the 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.
Great, thanks for the heads up, keep me posted if there is some work done in that direction that will allow me to move this PR forward. (but no hurry for now it can stay here and we move on without auth for development)
17423a3
to
d5eba9b
Compare
Maybe we can leverage that new |
# TODO: | ||
# - Handle other auth backends | ||
# - Handle AUTH_ROLE_PUBLIC | ||
user = auth_current_user(credentials) |
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.
Instead of relying on basic auth (that is part of FAB auth manager), I would do it the same way as in https://github.com/apache/airflow/blob/main/airflow/api_connexion/security.py#L44
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.
This is not possible at the moment.
This is what I did at first, exact same code, the problem is in the implementation, none of the backends are working with fastapi for the same reason, they all need a flask app context to access flask.g
, flask.request
. So I modified what is necessary on the basic_auth
to make it work, but then limited the backend to basic auth because others won't be working. (kerberos and session backend mostly), and it is too much effort to make it work cleanly so I 'hacked' my way around the basic_auth
.
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.
cf this comment for a more detailed example with the session
backend for instance.
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.
PR as been merged. Just removing the # Handle AUTH_ROLE_PUBLIC
todo is enough. Backend is now taking care of that.
I'll close this one in favor of #42634 |
What does this PR do
Add support for
basic_auth
backend to the new UI Rest API. Add relevant permissions helper and tests. The goal for this is first to show how permissions and auth can be handled in FastAPI and also have a small working POC for us to be able to then work on the front end. (And be able to migrate existing endpoints with appropriate permission decorators and write test against that as well).Goal:
basic_auth
backend.What this PR does not do
After some digging in the last two days, I find that our
auth_backends
implementations,auth_manager
init code,BaseAuthManager
and more importantlyFabAuthManager
etc. are tightly coupled to flask. For instance the way we read webserver config file from python withfrom_pyfile
, check app config withapp.config
, access global flask object from within an app contextg
,request
,get_current_user
,flask_login
,current_app
, all of that does not work without flask.This PR does not rewrite all that code to be
Flask
/ webserver framework agnostic and usable with FastAPI because the effort is too high at the moment and this is not what I am trying to demonstrate with this PR.We basically use the
flask_app
init code to initialize correctly all the different helper classes, and I added a few checks here and there to make the basic auth work outside of a Flask app context.