aaron-y-chen commented on code in PR #71920:
URL: https://github.com/apache/airflow/pull/71920#discussion_r3837440787
##########
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:
As Hussein mentioned, it's a real issue. I'll add a regression test and fix
it properly instead of just committing copilot's suggestion.
--
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]
