Allow openid for integration tests - #1156
Conversation
683cdf0 to
4a7d705
Compare
| The configured security scheme. | ||
| """ | ||
| scopes: dict[str, str] = { | ||
| f"api://{settings.AZURE_CLIENT_ID}/user_impersonation": "user_impersonation", |
There was a problem hiding this comment.
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 a resource or audience parameter, and others need explicit claim mappings. ISAR then validates aud exactly against AZURE_CLIENT_ID, so a token requested with api://<id>/user_impersonation can be validly issued but rejected because its audience is the full scope or another configured resource. Authorization additionally depends on the non-standard top-level roles claim containing Mission.Control; declaring a scope here does not enforce that scope because the endpoint uses Depends rather than Security(..., 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 present Mission.Control, and, if the scope is intended as authorization, enforce and test it explicitly.
Scores
- Overall importance: 9/10
- Correctness-related: 10/10
- Security-related: 9/10
- Interoperability-related: 9/10
- Flexibility-related: 8/10
- Confidence: 9/10
| } | ||
|
|
||
| if settings.OPENID_CONFIG_URL: | ||
| return AzureAuthorizationCodeBearerBase( |
There was a problem hiding this comment.
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 AzureAuthorizationCodeBearerBase does not make token validation provider-neutral. The dependency still parses tokens into its Azure-specific User model, which requires claims such as ver and models aud as a single string, and its validator currently hard-codes RS256. A standards-compatible provider or test server may omit Azure’s ver claim, emit a valid array-form aud, or advertise another signing algorithm. Such a correctly signed token would still be rejected with 401.
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 ver, with string and array audiences, and with accepted/rejected algorithms.
Scores
- Overall importance: 10/10
- Correctness-related: 10/10
- Security-related: 9/10
- Interoperability-related: 10/10
- Flexibility-related: 9/10
- Confidence: 10/10
|
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. The constructor-level tests verify that URLs are assigned, but they do not prove interoperability or security. A stronger acceptance test would start a standards-compatible OIDC test issuer, load its discovery/JWKS, acquire or build a signed token, and call a protected ISAR endpoint. The matrix should include a valid token, wrong issuer, wrong audience, expired token, unsupported signing algorithm, missing required role, and valid array-form audience if the generic path intends to support the JWT standard broadly. This would also make the intended contract explicit: whether the feature supports arbitrary OIDC access tokens or only Azure-shaped claims from a configurable issuer. That distinction should be documented because the implementation and operational configuration differ substantially. Scores
|
|
Superseded by #1158, which uses Keycloak instead of oauth2-mock-server. Leaving this open as the fallback — close it if that one lands. |
2 participants
Ready for review checklist: