hussein-awala commented on code in PR #71920:
URL: https://github.com/apache/airflow/pull/71920#discussion_r3836805517
##########
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 endpoint loop takes `path_parts[0]` from the first URL matching the host
and then exits. However, the tenant-agnostic filter is applied only after the
loop.
As a result, a deployment with:
`api_base_url = "https://login.microsoftonline.com/common/oauth2/v2.0/"`
alongside a tenant-specific `access_token_url` resolves to `None` and cannot
log in, even though the configuration contains a usable tenant.
The loop should continue to the next endpoint when the segment is
tenant-agnostic instead of breaking.
There is currently no test covering this type of mixed configuration.
Separately, and not caused by this loop's early exit: sovereign clouds
don't match the accepted host, `login.microsoftonline.us`,
`*.partner.microsoftonline.cn`, and B2C's `b2clogin.com` resolve to None, so
those deployments cannot authenticate at all.
Is that in scope for this PR, or would it be better tracked separately?
It’s worth settling before `3.8.1` is released, since this affects a broader
class of deployments than the one this PR fixes.
--
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]
