Update authorization logic on list APIs in FabAuthManager

[vincbeck]

Resolves #53936.

Follow-up of this comment.

Description

For every list Dag or sub Dag entity (e.g. task instance, dag run, ...), the current authorization is a bit messy in FabAuthManager. Since a user can have access to specifics Dags (and not all of them), when the user lists Dags, Airflow uses get_authorized_dag_ids from auth manager to figure whether the user has access to at least one Dag. If the user has access to at least one Dag then the access is granted to the API, if not the access is denied. If the access is granted, then the implementation of the API itself uses again get_authorized_dag_ids to retrieve the list of Dags to return to the user.

I do think we should not check whether the user is authorized to list dags because the API returns only the dags the user has access to anyway. The consequence would is, instead of having a 403, the user would get an empty list of DAGs. In a fined grained access context, it makes more sense. Let's say I am user who has access to the Dag test only but this Dag does not exist (or has been removed) in the Airflow environment. In the current implementation, if I list Dags, I'll get a 403, but does it make sense? I have permissions to access Dags, I just happen to not have access to Dags existing in the environment. @pierrejeambrun also brought it up in his comment that with the introduction of filters, having this logic on the API level does not make sense.

Testing

I tried to test it as much as I could but please test it on your end as well since this PR is quite impactful.

Future

Dags is the only resource type having fine grained access (authorizing a user having access to one specific Dag) in Airflow because it has been historically like this in FabAuthManager. But technically, nothing prevent us today, through KeycloakAuthManager for instance, to give permissions to users to say, one specific connection. Everything would work but the list API, you would get a 403. I am planning to add such capability across auth managers to other resources in the near future.

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[pierrejeambrun]

Interesting, a few points to mention:

• New core providing an extra unknown argument to old fab provider.
• For other list endpoint using AccesView.Something for permission we will keep returning 403 and not an empty list when permission is missing. For instance get_plugins, that might feel inconsistent to sometimes end up with 403, and sometimes with 404 on listing endpoints.

Many a simple approach would be to keep the change located to the fab provider, and change _is_authorized_dag so instead of granting access if len(authorized_dags) > 0, it will grand access if:

• user has access to all dags: self._is_authorized(method=method, resource_type=RESOURCE_DAG, user=user) or one permission with permissions.RESOURCE_DAG
• or user has at least one dag permission with RESOURCE_DAG_PREFIX. (The intend is to grant access to dags list to this user so it won't 403 in this case)

This way we don't change any of the core code and we could possibly get what we want:

• no permissions at all -> 403, makes sense
• at least one permission set for dag but no dag matching -> 200 empty list
• match endpoint filtering behavior, filters are set, but no resulting dag match the permissions set -> 200 empty list

And we don't get to modify core, add comments here and there, make it consistant with 'providers' or 'access view' endpoints and write compat code.

[pierrejeambrun]

Airflow uses get_authorized_dag_ids from auth manager to figure whether the user has access to at least one Dag

That's true only for the fab auth manager. The interface we call is is_authorized_dag from the core API, which could or could not call that depending on the auth manager implementation. This is why we might want to keep it, or we could be breaking workflow for people with their own auth manager implemented doing some extra stuff there.

[pierrejeambrun]

And for the related issue, it won't solve it in case the user does not have any dag access at all (full, or at least 1 prefix permission), but to handle this we can simply catch the 403 on the front-end and return an empty list, that's what we plan to do for the get_providers call in the homepage that crashes the UI for user that do not have access to the plugin view.

[vincbeck]

My concern with this approach is each auth manager needs to implement their own logic: "is the user is authorized to access at least one DAG?". Depending on the auth manager underlying service/tool it is using, it can be complex to check that. Very often, I think, it will end up using get_authorized_dag_ids like FabAuthManager is doing today.

The solution you are proposing is a valid solution but only for FabAuthManager, ideally I would like to solve this issue across all auth managers.

Another possible solution is to add a new method LIST as part of ResourceMethod = Literal["GET", "POST", "PUT", "DELETE"]. This way, will be super simple to check whether the user has permissions to list resources: auth_manager.is_authorized_dag(method="LIST"). Then each auth manager needs to handle this new method. On FabAuthManager side, we could use the technique you described.

[vincbeck]

After discussing offline with @pierrejeambrun , I actually I agree with him now, I am closing this PR and create a new one to update FabAuthManager