onlyarnav commented on code in PR #68951: URL: https://github.com/apache/airflow/pull/68951#discussion_r3476411826
########## providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py: ########## @@ -78,6 +78,13 @@ log = logging.getLogger(__name__) RESOURCE_ID_ATTRIBUTE_NAME = "resource_id" +KEYCLOAK_RESOURCE_NOT_FOUND_ERROR = "resource not found:" + + +def _is_missing_keycloak_resource_response(status_code: int, text: Any) -> bool: + return status_code == 500 and isinstance(text, str) and KEYCLOAK_RESOURCE_NOT_FOUND_ERROR in text.lower() Review Comment: 1. Helper function is introduced for the specific problem mentioned in #68943 and not for all the error 500 (i forgot to link the issue in PR 🥲) 2. Regarding the second query, it was added on the basis of behavior reported in the same issue, its not documented it is rather the fix of the problem in that specific issue ## Below is detailed Claude response: The helper was introduced to make the intent of the check explicit. We're not handling arbitrary 500 responses here—we're specifically detecting the Keycloak case where a referenced authorization resource does not exist and converting that into a denied authorization decision rather than breaking the `/dags` page. That said, the helper and constant are currently only used once, so I can inline them if you think that improves readability. Regarding the `500 + "resource not found:"` match, this was added based on the behavior reported in an issue. When Airflow checks authorization for a DAG whose team resource is not defined in Keycloak, Keycloak returns a 500 response containing `resource not found: <resource>`, which currently causes the entire `/dags` page to fail. The goal of this change is to treat that specific scenario as a missing authorization resource and deny access instead of propagating the error. I don't believe the response text is part of a documented Keycloak API contract, so the check is based on observed behavior rather than a guaranteed response format. If there's a more structured error field or status code we can rely on, I'd be happy to switch to that instead. -- 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]
