Copilot commented on code in PR #71920:
URL: https://github.com/apache/airflow/pull/71920#discussion_r3836831898
##########
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
Review Comment:
The loop stops at the first `login.microsoftonline.com` endpoint even when
its tenant segment is `common`, `organizations`, or `consumers`. If, for
example, `api_base_url` uses `common` but `access_token_url` contains the
deployment tenant, this returns `None` without checking the later endpoint and
rejects an otherwise resolvable configuration. Skip tenant-agnostic candidates
and continue searching the remaining endpoints, as the previous implementation
did.
---
Drafted-by: GitHub Copilot (no human review before posting)
##########
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"):
+ return None
+ return tenant_identifier
+
+ def _resolve_azure_tenant_guid(self, tenant_identifier: str) -> str:
+ """
+ Resolve an Azure tenant identifier (GUID or domain) to a canonical
tenant GUID.
+
+ If the identifier is a valid UUID, it is normalized to lowercase
hyphenated format
+ without calling any network endpoint. If the identifier is a domain,
its OpenID
+ discovery metadata is queried from login.microsoftonline.com to
extract the canonical
+ tenant GUID from the issuer claim. Successful resolutions are cached.
+ """
+ try:
+ return str(uuid.UUID(tenant_identifier))
+ except (ValueError, AttributeError):
+ pass
+
+ if tenant_identifier in self._azure_tenant_guid_cache:
+ return self._azure_tenant_guid_cache[tenant_identifier]
+
+ import requests
Review Comment:
This adds a new import inside a method, but this authentication path is not
a worker-isolation lazy-loading boundary and the project requires imports at
module scope. Move the `requests` import to the module imports (and reuse it in
the related request helpers), or document a valid exception for keeping it
local.
---
Drafted-by: GitHub Copilot (no human review before posting)
--
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]
