-
Notifications
You must be signed in to change notification settings - Fork 45
Allow openid for integration tests #1156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| from fastapi import Depends | ||
| from fastapi.security.base import SecurityBase | ||
| from fastapi_azure_auth import SingleTenantAzureAuthorizationCodeBearer | ||
| from fastapi_azure_auth.auth import AzureAuthorizationCodeBearerBase | ||
| from fastapi_azure_auth.exceptions import InvalidAuthHttp | ||
| from fastapi_azure_auth.user import User | ||
| from pydantic import BaseModel | ||
|
|
@@ -21,13 +22,47 @@ def __init__(self) -> None: | |
| self.scheme_name = "No Security" | ||
|
|
||
|
|
||
| azure_scheme = SingleTenantAzureAuthorizationCodeBearer( | ||
| app_client_id=settings.AZURE_CLIENT_ID, | ||
| tenant_id=settings.AZURE_TENANT_ID, | ||
| scopes={ | ||
| def build_azure_scheme() -> AzureAuthorizationCodeBearerBase: | ||
| """ | ||
| Build the security scheme used to validate access tokens. | ||
|
|
||
| By default this is a single tenant Azure Entra ID scheme. If | ||
| ``settings.OPENID_CONFIG_URL`` is set, the OpenID Connect discovery document is | ||
| read from that URL instead, which allows ISAR to be pointed at a different | ||
| OpenID provider such as a local mock issuer used by the integration tests. | ||
|
|
||
| ``SingleTenantAzureAuthorizationCodeBearer`` does not accept an | ||
| ``openid_config_url`` argument, so the base class is used directly in that case. | ||
| Issuer validation remains enabled either way; the expected issuer is taken from | ||
| the discovery document. | ||
|
|
||
| Returns | ||
| ------- | ||
| AzureAuthorizationCodeBearerBase | ||
| The configured security scheme. | ||
| """ | ||
| scopes: dict[str, str] = { | ||
| f"api://{settings.AZURE_CLIENT_ID}/user_impersonation": "user_impersonation", | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| if settings.OPENID_CONFIG_URL: | ||
| return AzureAuthorizationCodeBearerBase( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI-generated review comment: This observation was produced with AI assistance and should be validated and discussed by the team before deciding on an implementation. Overriding discovery on Possible directions are to add a separate generic JWT bearer implementation using PyJWT/Authlib with discovery-backed JWKS and explicit issuer, audience, lifetime, and algorithm validation; introduce an adapter that validates generic claims and maps only the fields ISAR actually needs; or keep this class but describe and test the feature as an Azure-shaped test issuer rather than generic OIDC. If algorithms are configurable, restrict them to an explicit allow-list and cross-check discovery/JWK metadata rather than trusting the token header. Please add integration tests using real discovery/JWKS tokens without Scores
|
||
| app_client_id=settings.AZURE_CLIENT_ID, | ||
| tenant_id=settings.AZURE_TENANT_ID, | ||
| scopes=scopes, | ||
| openid_config_url=settings.OPENID_CONFIG_URL, | ||
| openapi_authorization_url=settings.OPENAPI_AUTHORIZATION_URL, | ||
| openapi_token_url=settings.OPENAPI_TOKEN_URL, | ||
| ) | ||
|
|
||
| return SingleTenantAzureAuthorizationCodeBearer( | ||
| app_client_id=settings.AZURE_CLIENT_ID, | ||
| tenant_id=settings.AZURE_TENANT_ID, | ||
| scopes=scopes, | ||
| ) | ||
|
|
||
|
|
||
| azure_scheme: AzureAuthorizationCodeBearerBase = build_azure_scheme() | ||
|
|
||
|
|
||
| async def validate_has_role(user: User = Depends(azure_scheme)) -> None: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI-generated review comment: This observation was produced with AI assistance and should be validated and discussed by the team before deciding on an implementation.
This Entra-style scope name does not establish the access token audience. OAuth/OIDC providers and test servers differ in how they derive
aud: some use requested scopes, others require aresourceoraudienceparameter, and others need explicit claim mappings. ISAR then validatesaudexactly againstAZURE_CLIENT_ID, so a token requested withapi://<id>/user_impersonationcan be validly issued but rejected because its audience is the full scope or another configured resource. Authorization additionally depends on the non-standard top-levelrolesclaim containingMission.Control; declaring a scope here does not enforce that scope because the endpoint usesDependsrather thanSecurity(..., scopes=[...]).I recommend separating provider-neutral settings such as expected audience, authorization scopes, required role/claim name, and discovery URL instead of deriving all of them from
AZURE_CLIENT_ID. Configure the test issuer to emit the exact audience and role contract, or support a configurable resource/audience request parameter in the token-producing client. Then test matching and mismatching audience, missing and presentMission.Control, and, if the scope is intended as authorization, enforce and test it explicitly.Scores