aaron-y-chen commented on code in PR #71920:
URL: https://github.com/apache/airflow/pull/71920#discussion_r3838512834
##########
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -2427,44 +2432,126 @@ def _get_microsoft_jwks(self) -> list[dict[str, Any]]:
return requests.get(MICROSOFT_KEY_SET_URL, timeout=30).json()
- def _get_azure_tenant_id(self) -> str | None:
+ def _get_azure_tenant_identifier(self) -> str | None:
"""
- Resolve the Azure AD tenant the deployment is configured against.
+ Extract the configured Azure AD tenant identifier.
Prefers an explicit ``tenant_id`` in ``client_kwargs``; otherwise
derives it from
- the tenant segment of the configured Azure endpoints, which is where
the documented
- configuration puts it
(``https://login.microsoftonline.com/<tenant-id>/...``).
+ the tenant path segment of configured Azure HTTPS endpoints on
``login.microsoftonline.com``.
- Returns ``None`` when the configuration is tenant-agnostic (the
``common`` or
- ``organizations`` endpoints), because there is then no single issuer
to pin to.
+ Returns ``None`` when no tenant can be determined or when the
configuration uses
+ tenant-agnostic endpoints (``common``, ``organizations``,
``consumers``).
"""
azure = self.oauth_remotes["azure"]
tenant_id = azure.client_kwargs.get("tenant_id")
- if tenant_id:
- return tenant_id
-
- for url in (
- getattr(azure, "api_base_url", None),
- getattr(azure, "access_token_url", None),
- getattr(azure, "authorize_url", None),
- ):
- if not isinstance(url, str):
- continue
- match = re.search(r"login\.microsoftonline\.com/([^/]+)/", url)
- if match and match.group(1) not in ("common", "organizations",
"consumers"):
- return match.group(1)
+ if tenant_id and isinstance(tenant_id, str) and tenant_id.strip():
+ tenant_identifier = tenant_id.strip()
+ else:
+ tenant_identifier = None
+ for url in (
+ getattr(azure, "api_base_url", None),
+ getattr(azure, "access_token_url", None),
+ getattr(azure, "authorize_url", None),
+ ):
+ if not isinstance(url, str):
+ continue
+ parsed = urllib.parse.urlsplit(url)
+ if parsed.scheme.lower() != "https":
+ continue
+ if (parsed.hostname or "").lower() !=
"login.microsoftonline.com":
+ continue
+ path_parts = [segment for segment in parsed.path.split("/") if
segment]
+ if path_parts:
+ tenant_identifier = path_parts[0]
+ break
- return None
+ if not tenant_identifier or tenant_identifier.lower() in ("common",
"organizations", "consumers"):
Review Comment:
The first point was indeed a bug, and I've fixed it. Thanks for catching it!
😄
The second point is worth addressing, but I think it is outside the scope of
this PR. I'd prefer to handle it separately in a follow-up 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
