You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pierrejeambrun (via GitHub)" <gi...@apache.org> on 2024/09/05 08:28:38 UTC
[PR] UI Rest API authentication [airflow]
pierrejeambrun opened a new pull request, #42019:
URL: https://github.com/apache/airflow/pull/42019
## 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:
- Allow the endpoint migrations to be ported with the appropriate permissions and tested, to not loose them in transit
- Use this PR as a preliminary work to discuss and see how FastAPI can integrate our permission/auth code, make a POC with the `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 importantly `FabAuthManager` etc. is tightly coupled to flask. For instance the way we read webserver config file from python with `from_pyfile`, check app config with `app.config`, access global flask object from within an app context `g`, `request`, `get_current_user`, `flask_login`, `current_app` etc...
So this PR does not rewrite all that code to be `Flask` independant 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746942510
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745343507
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
(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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746652987
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
```
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not depend on FAB
```
Can you elaborate how this could work outside of a `flask request context` and `flask app context` like here in a FastAPI app ? In its current implementation, the `SimpleAuthManager` is using the flask `session` object to check if the user [is_logged_in](https://github.com/apache/airflow/pull/42004/files#diff-c8acbf9922a035c99d5347625cc8c008a0a2a2c1e1e451699ee8145b7fa85265R77) or to retrieve the current user via [get_user](https://github.com/apache/airflow/pull/42004/files#diff-c8acbf9922a035c99d5347625cc8c008a0a2a2c1e1e451699ee8145b7fa85265R85). Accessing the flask `session` object is not possible in FastAPI, we do not have such context available at runtime. So basically i'm not sure how I can use that in our new `api_ui` code ? Calling those method will yield a runtime Error like this one `RuntimeError: Working outside of application context`.
Maybe the session accessor should be a parameter. (But once again FastAPI requires dependency injection, there is no equivalent to a global `flask.session`, we need a parameter with `request: Request`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747222312
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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.
I can take a look at the session, but maybe the front need to use a JWT auth base with FastAPI that would be easier I think. Instead of a cookie based sessions. Not sure yet what is best.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746997791
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
But I guess this is an interesting discussion for later. I feel like this is out of scope for this particular PR, I don't want to make adaptations to the permissions yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747022327
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
Review Comment:
cf this comment for a more detailed example with the `session` backend for instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #42019:
URL: https://github.com/apache/airflow/pull/42019#issuecomment-2330921873
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745879131
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
> Taking a look at the code it appears that a flask app context is still required
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not rely on FAB
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746933916
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
> I have another proposal. Why do you want to have basic authentication for this API?
Because the basic auth is the easier to modify and make compatible with FastAPI. Also FastAPI session implementation has some security implications + requires extra packages and configuration. So I thought at first I would make the basic auth work and these can be follow up / improvements. Cf this FastAPI issue:
https://github.com/fastapi/fastapi/issues/754
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745341605
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745202196
##########
hatch_build.py:
##########
@@ -428,6 +428,9 @@
"cryptography>=41.0.0",
"deprecated>=1.2.13",
"dill>=0.2.2",
+ # Required for python 3.8 and 3.9 to work with new annotations styles. Check package
+ # description on PyPI for more details: https://pypi.org/project/eval-type-backport/
+ "eval-type-backport>=0.2.0",
Review Comment:
This basically fixes `pydantic` + `__future__ import annotations` for older python versions. I suspect this was why we have the exclusion in `pyproject.toml` to not upgrade to new style annotations in:
```
# Pydantic also require models to be imported during execution
"airflow/serialization/pydantic/*.py" = ["I002", "UP007", "TCH001"]
```
Might be worth to try after this PR and see if this can be upgraded now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746652987
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
```
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not depend on FAB
```
Can you elaborate how this could work outside of a `flask request context` and `flask app context` like here in a FastAPI app ? In its current implementation, the `SimpleAuthManager` is using the flask `session` object to check if the user `is_logged_in` or to retrieve the current user via `get_user`. Accessing the flask `session` object is not possible in FastAPI, we do not have such context available at runtime. So basically i'm not sure how I can use that in our new `api_ui` code ? Calling those method will yield a runtime like this one `RuntimeError: Working outside of application context`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745464971
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 flask `g` global object to retrieve the user. Uses `flask_login.LoginManager`, `flask.flash` or the `session` global object.
- The `AnonymousUser` is using the flask.current_app() and a `flask_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 re-write another SecurityManager / AuthManager agnostic of flask things will be ok.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747020436
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Similar issues will arrive with kerberos backend. And there was similar issues with the basic auth backend but I was able to work around it for the POC. (and by modifying a little bit the backend)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745822102
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Thanks to both of you for your comments that is really helpful.
I am opened to both solutions just let me know what you prefer:
- We leave it like this and rely on the modified `basic_auth` backend with some flask_app init thingy to make basic auth work for the time being (development purpose until we can move fowrard with with AIP-79)
- We wait for the SimpleAuthManager PR to be merged and use it, also make it the default. But in this case are we able to load users from db and to a real auth workflow as we have now with basic_auth ? Before for development purposes, tests, etc I don't want to have to provide a fake list of users hardcoded somewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745822102
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Thanks to both of you for your comments that is really helpful.
I am opened to both solutions just let me know what you prefer:
- We leave it like this and rely on the modified `basic_auth` backend with some flask_app init thingy to make basic auth work for the time being (development purpose until we can move fowrard with with AIP-79)
- We wait for the SimpleAuthManager PR to be merged and use it, also make it the default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745202196
##########
hatch_build.py:
##########
@@ -428,6 +428,9 @@
"cryptography>=41.0.0",
"deprecated>=1.2.13",
"dill>=0.2.2",
+ # Required for python 3.8 and 3.9 to work with new annotations styles. Check package
+ # description on PyPI for more details: https://pypi.org/project/eval-type-backport/
+ "eval-type-backport>=0.2.0",
Review Comment:
This basically fixes `pydantic` + `__future__ import annotations` for older python versions. I suspect this was why we have the exclusion in `pyproject.toml` to not upgrade to new style annotations in:
```
# Pydantic also require models to be imported during execution
"airflow/serialization/pydantic/*.py" = ["I002", "UP007", "TCH001"]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745840124
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
2) SimpleAuthManager would be my preference - but I am not sure if it's easy :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747199970
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
`session` is a good callout! I missed that. I might need to find another solution for simple auth manager then. But in your case, I am really wondering if the basic auth is the good mechanism. What is your plan to pass credentials from the back-end to the front-end? Ideally, the front-end should call the API with no auth and the API retrieves the session for the current user in the back-end. I think this is something we should sort out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746938557
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
Review Comment:
Ok then i'll remove the comment and you guys implement it in the FAB auth manager ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745822102
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Thanks to both of you for your comments that is really helpful.
I am opened to both solutions just let me know what you prefer:
- We leave it like this and rely on the modified `basic_auth` backend with some flask_app init thingy to make basic auth work for the time being (development purpose until we can move fowrard with with AIP-79)
- We wait for the SimpleAuthManager PR to be merged and use it, also make it the default. But in this case are we able to load users from db and to a real auth workflow as we have now with basic_auth ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1771090415
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
Review Comment:
PR as been merged. Just removing the `# Handle AUTH_ROLE_PUBLIC` todo is enough. Backend is now taking care of that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745040928
##########
airflow/providers/fab/auth_manager/api/auth/backend/basic_auth.py:
##########
@@ -21,14 +21,17 @@
from functools import wraps
from typing import TYPE_CHECKING, Any, Callable, TypeVar, cast
-from flask import Response, request
+from flask import Response, has_app_context, request
from flask_appbuilder.const import AUTH_LDAP
from flask_login import login_user
from airflow.providers.fab.auth_manager.security_manager.override import FabAirflowSecurityManagerOverride
from airflow.www.extensions.init_auth_manager import get_auth_manager
if TYPE_CHECKING:
+ from fastapi.security import HTTPBasicCredentials
+ from werkzeug.datastructures import Authorization
Review Comment:
Yes this adds changes to the provider and core. Should I split the PR ?
Also this adds a new FastAPI dependency to the provider for type checking should I update requirements ? (I assume no because it has no effect at runtime but I prefer to double check)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747222312
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 do we ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747222312
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746652987
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
```
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not depend on FAB
```
Can you elaborate how this could work outside of a `flask request context` and `flask app context` like here in a FastAPI app ? In its current implementation, the `SimpleAuthManager` is using the flask `session` object to check if the user [is_logged_in](https://github.com/apache/airflow/pull/42004/files#diff-c8acbf9922a035c99d5347625cc8c008a0a2a2c1e1e451699ee8145b7fa85265R77) or to retrieve the current user via [get_user](https://github.com/apache/airflow/pull/42004/files#diff-c8acbf9922a035c99d5347625cc8c008a0a2a2c1e1e451699ee8145b7fa85265R85). Accessing the flask `session` object is not possible in FastAPI, we do not have such context available at runtime. So basically i'm not sure how I can use that in our new `api_ui` code ? Calling those method will yield a runtime Error like this one `RuntimeError: Working outside of application context`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746994315
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
I understand your point and this makes sense. I do not have any strong opinion on that because I feel like both approach can be justified depending on how we want to build our permission model.
If a user request ressources 1, 2 and 3. If he is missing permissions on one of those ressources I can understand that the request is denied because he is basically forbidden from making that request.
Failing silently could have some weird user implications, for instance we return only the thing that you actually have access to i.e 1 and 2. Then the request can still be successful, but only contains partially the information that the user need. You have no other way than checking the entire content of the request to see if you are missing some informations or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745822102
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Thanks to both of you for your comments that is really helpful.
I am opened to both solutions just let me know what you prefer:
- We leave it like this and rely on the modified `basic_auth` backend with some flask_app init thingy to make basic auth work for the time being (development purpose until we can move fowrard with with AIP-79)
- We wait for the SimpleAuthManager PR to be merged and use it, also make it the default. Taking a look at the code it appears that a `flask app context` is still required. I do not have that in here at the moment. Also are we able to load users from the db and have a real auth workflow as we have now with basic_auth (permissions granted etc...) For developing airflow 3 front-end if we have something closer to the real auth workflow it might be easier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747222312
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 backend yet do we ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745464971
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 flask `g` global object to retrieve the user. Uses `flask_login.LoginManager`, `flask.flash` or the `session` global object.
- The `AnonymousUser` is using the flask.current_app() and a `flask_login` mixin.
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 re-write another SecurityManager / AuthManager agnostic of flask things will be ok.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746652987
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
```
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not depend on FAB
```
Can you elaborate how this could work outside `flask request context` and `flask app context`? In its current implementation, the `SimpleAuthManager` is using the flask `session` object to check if the user `is_logged_in` or to retrieve the current user via `get_user`. Accessing the flask `session` object is not possible in FastAPI, we do not have such context available at runtime. So basically i'm not sure how I can use that in our new `api_ui` code ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745929596
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
All the "list resources" scenarios (list DAGs, list connections, list variables ...) is not properly handled to me. Example: when listing connections, we are checking if the user has "GET" permissions on all connections. But the user might have "GET" permissions on one connection only, though in that case they would still have access denied when listing the connections. This is because in FAB you cannot give permissions on specific resources (besides DAGs) and we inherited from that. But we might want to fix that. Should we introduce a "LIST" permissions in auth managers?
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
Review Comment:
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
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
Review Comment:
`AUTH_ROLE_PUBLIC` is part of FAB auth manager. You should not handle it directly. I just realized that it is not handled properly today in Airflow. We should move it to FAB auth manager
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
I remember when I moved this code I hated it and still hate it. I think we should probably simplify it with Airflow 3. We might want to return `True` in that case? That case is, the user is trying to access a list of DAGs and the user does not have permissions to see all DAGs. Here we are checking if the user has access to at least one, if not we return `AccessDenied`. To me, we should just return `True`, the function that retrieves the DAGs already filter in the DAGs that the user has access to. The only change would be, if a user does not has permissions to see DAGs, instead of having an access denied error, they would see an empty list of DAGs.
We dont need to solve it in this PR though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747021667
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1763776144
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
Review Comment:
#42280
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745202196
##########
hatch_build.py:
##########
@@ -428,6 +428,9 @@
"cryptography>=41.0.0",
"deprecated>=1.2.13",
"dill>=0.2.2",
+ # Required for python 3.8 and 3.9 to work with new annotations styles. Check package
+ # description on PyPI for more details: https://pypi.org/project/eval-type-backport/
+ "eval-type-backport>=0.2.0",
Review Comment:
This basically fixes `pydantic` + `__future__ import annotations` for older python versions. I suspect this was why we have the exclusion in `pyproject.toml` to not upgrade to new style annotations in:
```
# Pydantic also require models to be imported during execution
"airflow/serialization/pydantic/*.py" = ["I002", "UP007", "TCH001"]
```
Might be worth to try after this PR and see if this can be upgraded now. (UP007)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745210736
##########
hatch_build.py:
##########
@@ -428,6 +428,9 @@
"cryptography>=41.0.0",
"deprecated>=1.2.13",
"dill>=0.2.2",
+ # Required for python 3.8 and 3.9 to work with new annotations styles. Check package
+ # description on PyPI for more details: https://pypi.org/project/eval-type-backport/
+ "eval-type-backport>=0.2.0",
Review Comment:
Nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #42019:
URL: https://github.com/apache/airflow/pull/42019#issuecomment-2331771663
> Maybe we can leverage that new `SimpleAuthManager` indeed #42004
Yep. That would be strongly preferred I think and main idea behind implementing it by @vincbeck
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746652987
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
```
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not depend on FAB
```
Can you elaborate how this could work outside `flask request context` and `flask app context` like here in a FastAPI app ? In its current implementation, the `SimpleAuthManager` is using the flask `session` object to check if the user `is_logged_in` or to retrieve the current user via `get_user`. Accessing the flask `session` object is not possible in FastAPI, we do not have such context available at runtime. So basically i'm not sure how I can use that in our new `api_ui` code ? Calling those method will yield a runtime like this one `RuntimeError: Working outside of application context`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746652987
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
```
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not depend on FAB
```
Can you elaborate how this could work outside `flask request context` and `flask app context`? In its current implementation, the `SimpleAuthManager` is using the flask `session` object to check if the user `is_logged_in` or to retrieve the current user via `get_user`. Accessing the flask `session` object is not possible in FastAPI, we do not have such context available at runtime. So basically i'm not sure how I can use that in our new `api_ui` code ? Calling those method will yield a runtime like this one `RuntimeError: Working outside of application context`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746994315
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
I understand your point and this makes sense. I do not have any strong opinion on that because I feel like both approach can be justified depending on how we want to build our permission model.
In its original form, if a user request ressources 1, 2 and 3. If he is missing permissions on one of those ressources I can understand that the request is denied because he is basically forbidden from making that request.
Failing silently could have some weird user implications, for instance we return only the thing that you actually have access to i.e 1 and 2. Then the request is successful (200) from a user point of viwe, but actually only contains partially the information that the user needs. The user has no other way than checking the entire content of the request to see if he is missing some informations or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1761428092
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745689679
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
There is definitely going to have to be some decoupling/refactoring anyways. For example, [auth managers expect an appbuilder today](https://github.com/apache/airflow/blob/12bb8b35241f0915e82a322c7905c8602df95a7f/airflow/auth/managers/base_auth_manager.py#L72), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745894596
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
I have another proposal. Why do you want to have basic authentication for this API? Should not it be session? This API will be used by the UI, hence a session will be there. Basic authentication makes only sense to me for testing purposes because it makes it easier to hit endpoint directly. Using the [session auth](airflow/api/auth/backend/session.py) uses the auth manager directly and simply calls `is_logged_in` method which would solve a lot of your problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745707778
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747222312
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 token might seem more appropriate. But we do not have that kind of backend yet do we ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746933916
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
> I have another proposal. Why do you want to have basic authentication for this API?
Because the basic auth is the easier to modify and make compatible with FastAPI. Also FastAPI session implementation has some security implications + requires extra packages and configuration. So I thought at first I would make the basic auth work and these can be follow up / improvements. Cf this FastAPI issue:
https://github.com/fastapi/fastapi/issues/754
The UI can work with basic auth for development time.
`is_logged_in` calls `get_user`that uses `flask_login.current_user` I tried that doesn't work in a fastapi context. I might try again to be sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746942510
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
Review Comment:
This is what I did at first, exact same code, the problem is in the current state, 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 bakend 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` and limited fastapi to it for the moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746994315
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
I understand your point and this makes sense. I do not have any strong opinion on that because I feel like both approach can be justified depending on how we want to build our permission model.
In its original form, if a user request ressources 1, 2 and 3. If he is missing permissions on one of those ressources I can understand that the request is denied because he is basically forbidden from making that request.
Failing silently could have some weird user implications, for instance we return only the thing that you actually have access to i.e 1 and 2. Then the request can still be successful, but only contains partially the information that the user need. You have no other way than checking the entire content of the request to see if you are missing some informations or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747222312
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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. 🤔. But there are solutions for 'basic auth' workflow on the front end.
I can take a look at the session, but maybe the front need to use a JWT auth base or something like this in a more modern scenario instead of cookie based sessions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747016229
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
Just for the sake of it, an example with the `session` backend:
`self.get_user()` will never return anything else than None because the `get_user` calls `flask_login.current_user`, here is the actual implementation:
```python
def _get_user():
if has_request_context():
if "_login_user" not in g:
current_app.login_manager._load_user()
return g._login_user
return None
```
Once again we need a flask request context that we do not have in our FastAPI app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1761368047
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745464971
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 (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 app I believe. 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 will be a bit easier if both are sync, basically that's close to a copy past (i am exagerating a bit but you get the idea). More code will need to be adapted / rewritted 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, 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 flask `g` global object to retrieve the user. Uses flask_login.LoginManager, flask flash or session global object.
- The `AnonymousUser` is using the flask.current_app() and a `flask_login` mixin.
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 re-write another SecurityManager / AuthManager agnostic of flask things will be ok.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745464971
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 flask `g` global object to retrieve the user. Uses `flask_login.LoginManager`, `flask.flash` or the `session` global object.
- The `AnonymousUser` is using the flask.current_app() and a `flask_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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745464971
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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 flask `g` global object to retrieve the user. Uses flask_login.LoginManager, flask flash or session global object.
- The `AnonymousUser` is using the flask.current_app() and a `flask_login` mixin.
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 re-write another SecurityManager / AuthManager agnostic of flask things will be ok.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #42019:
URL: https://github.com/apache/airflow/pull/42019#issuecomment-2331715207
Maybe we can leverage that new `SimpleAuthManager` indeed https://github.com/apache/airflow/pull/42004
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745840124
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
2) would be my preference - but I am not sure if it's easy :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745879131
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
> Taking a look at the code it appears that a flask app context is still required
The only dependency on FAB that the auth managers have now is the way to register custom views. It is currently using appbuilder, this is something we want to get rid of. This will go away when AIP-68 is complete and there will be a way for plugins to extend new Airflow UI. But besides that, simple auth manager do not depend on FAB
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747227805
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #42019:
URL: https://github.com/apache/airflow/pull/42019#issuecomment-2331149944
Well the CI is not happy, working on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745464971
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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, 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 flask `g` global object to retrieve the user. Uses flask_login.LoginManager, flask flash or session global object.
- The `AnonymousUser` is using the flask.current_app() and a `flask_login` mixin.
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 re-write another SecurityManager / AuthManager agnostic of flask things will be ok.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745874973
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
> Also are we able to load users from the db and have a real auth workflow as we have now with basic_auth (permissions granted etc...) For developing airflow 3 front-end if we have something closer to the real auth workflow it might be easier.
No. Simple auth manager does not use the DB, everything is stored in the config. Besides that, there is a real workflow etc, you would be able to use different users (as long as they are defined in the config)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1745822102
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Thanks to both of you for your comments that is really helpful.
I am opened to both solutions just let me know what you prefer:
- We leave it like this and rely on the modified `basic_auth` backend with some flask_app init thingy to make basic auth work for the time being
- We wait for the SimpleAuthManager PR to be merged and use it, also make it the default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1746933916
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
> I have another proposal. Why do you want to have basic authentication for this API?
Because the basic auth is the easier to modify and make compatible with FastAPI. Also FastAPI session implementation has some security implications + requires extra packages and configuration. So I thought at first I would make the basic auth work and these can be follow up / improvements. Cf this FastAPI issue:
https://github.com/fastapi/fastapi/issues/754
The UI can work with basic auth for development time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747016907
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Just for the sake of it, an example with the `session` backend:
`self.get_user()` will never return anything else than None because the `get_user` calls `flask_login.current_user`, here is the actual implementation:
```python
def _get_user():
if has_request_context():
if "_login_user" not in g:
current_app.login_manager._load_user()
return g._login_user
return None
```
Once again we need a flask request context that we do not have in our FastAPI app (and we will most likely never have that because I don't even know how that would work, FastAPI has its own request lifecycle, and we don't want to init an extra flask context for each fastapi request)
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
+ user = auth_current_user(credentials)
+ if user is not None:
+ return user
+
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise HTTPException(401, headers={"WWW-Authenticate": "Basic"})
+
+
+def _requires_access(
+ *,
+ is_authorized_callback: Callable[[], bool],
+) -> None:
+ if not is_authorized_callback():
+ raise HTTPException(403, "Forbidden")
+
+
+def requires_access_dataset(
+ method: Annotated[ResourceMethod, Depends(method)],
+ uri: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: get_auth_manager().is_authorized_dataset(
+ user=user,
+ method=method,
+ details=DatasetDetails(uri=uri),
+ )
+ )
+
+
+def requires_access_dag(access_entity: DagAccessEntity | None = None) -> Callable:
+ def inner(
+ method: Annotated[ResourceMethod, Depends(method)],
+ dag_id: str | None = None,
+ user: Annotated[BaseUser | None, Depends(check_authentication)] = None,
+ ) -> None:
+ def callback():
+ access = get_auth_manager().is_authorized_dag(
+ method=method, access_entity=access_entity, details=DagDetails(id=dag_id), user=user
+ )
+
+ # ``access`` means here:
+ # - if a DAG id is provided (``dag_id`` not None): is the user authorized to access this DAG
+ # - if no DAG id is provided: is the user authorized to access all DAGs
+ if dag_id or access or access_entity:
+ return access
+
+ # No DAG id is provided, the user is not authorized to access all DAGs and authorization is done
+ # on DAG level
+ # If method is "GET", return whether the user has read access to any DAGs
+ # If method is "PUT", return whether the user has edit access to any DAGs
+ return (method == "GET" and any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
+ method == "PUT" and any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ )
Review Comment:
Just for the sake of it, an example with the `session` backend:
`self.get_user()` will never return anything else than None because the `get_user` calls `flask_login.current_user`, here is the actual implementation:
```python
def _get_user():
if has_request_context():
if "_login_user" not in g:
current_app.login_manager._load_user()
return g._login_user
return None
```
Once again we need a flask request context that we do not have in our FastAPI app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747016907
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
Just for the sake of it, an example with the `session` backend:
`self.get_user()` will never return anything else than None because the `get_user` calls `flask_login.current_user`, here is the actual implementation:
```python
def _get_user():
if has_request_context():
if "_login_user" not in g:
current_app.login_manager._load_user()
return g._login_user
return None
```
Once again we need a flask request context that we do not have in our FastAPI app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747200768
##########
airflow/api_ui/app.py:
##########
@@ -32,17 +38,35 @@ def init_dag_bag(app: FastAPI) -> None:
app.state.dag_bag = get_dag_bag()
-def create_app() -> FastAPI:
+def init_flask_app(app: FastAPI, testing: bool = False) -> None:
+ """
+ Auth providers and permission logic are tightly coupled to Flask.
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] UI Rest API authentication [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #42019:
URL: https://github.com/apache/airflow/pull/42019#discussion_r1747188730
##########
airflow/api_ui/security.py:
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from typing import Callable, cast
+
+from fastapi import Depends, HTTPException, Request
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
+from typing_extensions import Annotated
+
+from airflow.auth.managers.base_auth_manager import ResourceMethod
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_details import DagAccessEntity, DagDetails, DatasetDetails
+from airflow.providers.fab.auth_manager.api.auth.backend.basic_auth import auth_current_user
+from airflow.providers.fab.auth_manager.models import User
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+
+security = HTTPBasic()
+
+
+def method(request: Request) -> ResourceMethod:
+ return cast(ResourceMethod, request.method)
+
+
+def check_authentication(
+ credentials: Annotated[HTTPBasicCredentials, Depends(security)],
+) -> User | None:
+ """Check that the request has valid authorization information."""
+ # TODO:
+ # - Handle other auth backends
+ # - Handle AUTH_ROLE_PUBLIC
Review Comment:
Yep, I'll create a PR for it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org