stegololz commented on PR #61351: URL: https://github.com/apache/airflow/pull/61351#issuecomment-3847758556
Hello, Latest commit fixes many comments from the review done on branch https://github.com/apache/airflow/pull/61256 before the split. - Team scoped authorization is now aligned with model available on [resource_details.py](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py) - About the “weird LIST if clause”: I first removed it completely on my local environment to get back the 403 errors and understand them. The 403 actually comes from the API auth flow. e.g. for dags, when the UI calls GET /api/v2/dags/~/dagRuns, flow goes like this: API goes through [requires_access_dag](https://github.com/apache/airflow/blob/b906080fe82cd72c9024dd62cf1b64c15f403ed8/airflow-core/src/airflow/api_fastapi/core_api/security.py#L148C1-L168C17) function ``` dag_id = dag_id if dag_id != "~" else None team_name = DagModel.get_team_name(dag_id) if dag_id else None ``` It normalizes `~` to `None`, so `team_name=None`, and then calls `is_authorized_dag` , that goes into [_is_authorized]( https://github.com/apache/airflow/blob/1530da7879136913dbc40760c29330fd353c0f67/providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py#L341C1-L380C86). ``` if resource_id: context_attributes[RESOURCE_ID_ATTRIBUTE_NAME] = resource_id elif method == "GET": method = "LIST" ``` Since there’s no `resource_id`, `_is_authorized` converts GET -> LIST and checks `Dag#LIST`. With my first multi‑team implementation, only team‑scoped resources existed (e.g. Dag:team), so a global Dag#LIST check was denied -> 403. First implementation did not solve this cleanly, so I refactored: for teamless LIST, I now check a global LIST permission on the global resource (e.g. Dag#LIST, no :team). This keeps team_name required for non‑LIST while letting list endpoints proceed. There is a theoretical caveat as it assumes every list endpoint applies permission filters later. I verified the code locally and it seems true for dags/dagRuns/pools/connections/variables/tags/dashboard stats, but I'd like your validation on this point to know if my assumption is correct. I’ll keep the PR as draft for now because I still have unresolved 403 errors around assets. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
