Allow openid for integration tests - #2856
Conversation
e474704 to
6e713e0
Compare
6e713e0 to
88884b4
Compare
| ); | ||
| // Configures JWT bearer authentication, the token caches (Redis-backed when | ||
| // Redis:UseRedis is set) and the ISAR / SARA / Pointilla downstream API clients. | ||
| builder.Services.ConfigureAuthentication(builder.Configuration, builder.Environment); |
There was a problem hiding this comment.
Nice cleaning up the main Program.cs !
| /// <summary> | ||
| /// The environment in which the backend validates tokens against a generic | ||
| /// OpenID Connect issuer instead of Microsoft Entra ID. | ||
| /// | ||
| /// This exists solely so the armada integration tests can run against a local | ||
| /// mock issuer, with no Entra app registrations and no client secrets, while | ||
| /// still exercising authentication for real. | ||
| /// | ||
| /// Gating on the environment name rather than on a configuration flag is | ||
| /// deliberate: it keeps the generic-issuer path unreachable from Development, | ||
| /// Staging and Production regardless of which environment variables are set. | ||
| /// </summary> | ||
| public const string IntegrationTestEnvironment = "IntegrationTest"; | ||
|
|
||
| public static bool UsesGenericOidc(this IHostEnvironment environment) => |
There was a problem hiding this comment.
Not sure I agree that generic oidc should be gated on environment. I want to use this for local development as well. Having environment variables on environments directly is also not recommended according to the 12 factor app principles:
Another aspect of config management is grouping. Sometimes apps batch config into named groups (often called “environments”) named after specific deploys, such as the development, test, and production environments in Rails. This method does not scale cleanly: as more deploys of the app are created, new environment names are necessary, such as staging or qa. As the project grows further, developers may add their own special environments like joes-staging, resulting in a combinatorial explosion of config which makes managing deploys of the app very brittle.
In a twelve-factor app, env vars are granular controls, each fully orthogonal to other env vars. They are never grouped together as “environments”, but instead are independently managed for each deploy. This is a model that scales up smoothly as the app naturally expands into more deploys over its lifetime.
Since it is an open source repo it might be useful for others to use this in their dev/staging/prod
| new Dictionary<string, string> | ||
| { | ||
| ["grant_type"] = "client_credentials", | ||
| ["scope"] = scopes, |
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.
There is an implicit assumption that a requested scope such as isar-test/.default causes the authorization server to issue aud=isar-test. OAuth/OIDC does not define that transformation. Providers and test servers differ: some derive aud from scopes, some require a resource or audience parameter, and some require configured claim mappings. The resulting token can therefore be correctly signed but rejected by ISAR because ISAR validates an exact audience. Required application roles are a related issue: downstream APIs still need claims such as Mission.Control, and SARA workflow callbacks need WorkflowStatus.Write.
I suggest making the requested scope and expected resource/audience explicit per downstream API, supporting a configurable resource/audience token-request parameter where the provider requires it, and configuring deterministic role claims in the test issuer. An end-to-end test should acquire separate tokens for SARA and ISAR and verify both exact audience validation and required-role authorization.
Scores
- Overall importance: 9/10
- Correctness-related: 9/10
- Security-related: 7/10
- Interoperability-related: 9/10
- Flexibility-related: 8/10
- Confidence: 9/10
| { | ||
| ["grant_type"] = "client_credentials", | ||
| ["scope"] = scopes, | ||
| ["client_id"] = configuration["AzureAd:ClientId"] ?? "flotilla", |
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.
The client-credentials request identifies the client but does not authenticate it. Client credentials is intended for confidential clients, and real providers commonly require client_secret_basic, client_secret_post, private_key_jwt, or another advertised token-endpoint authentication method. This works only with permissive test servers and is therefore not a generic implementation.
Possible directions are to configure a client secret and use Basic authentication by default, make the client authentication method explicit and support the methods actually needed by deployments, or deliberately model this as an unauthenticated test client and fail startup outside a clearly selected test mode. Ideally, consult token_endpoint_auth_methods_supported from discovery and fail clearly when the configured method is unsupported. Tests should inspect the outgoing request and cover both authenticated and intentionally unauthenticated clients.
Scores
- Overall importance: 7/10
- Correctness-related: 7/10
- Security-related: 8/10
- Interoperability-related: 8/10
- Flexibility-related: 7/10
- Confidence: 8/10
| options.Authority = authority; | ||
| options.Audience = audience; | ||
| // The mock issuer is plain HTTP on the test network. | ||
| options.RequireHttpsMetadata = false; |
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.
RequireHttpsMetadata is disabled for every generic provider. That is useful for a local HTTP test server, but it couples provider selection to transport-security relaxation. If generic OIDC becomes configuration-driven, a mistaken production configuration could fetch discovery metadata and signing keys over HTTP, allowing key substitution and forged-token validation.
I recommend keeping HTTPS metadata required by default and introducing a separate explicit setting such as AllowInsecureHttpMetadata, defaulting to false. It can be enabled only in local/integration deployment configuration, ideally with an additional startup guard that rejects it outside an explicitly permitted context. Tests should verify that an HTTP authority fails by default, succeeds only with the opt-in, and that HTTPS providers retain metadata enforcement.
Scores
- Overall importance: 8/10
- Correctness-related: 7/10
- Security-related: 10/10
- Interoperability-related: 5/10
- Flexibility-related: 6/10
- Confidence: 9/10
| AuthorizationHeaderProviderOptions? authorizationHeaderProviderOptions = null, | ||
| ClaimsPrincipal? claimsPrincipal = null, | ||
| CancellationToken cancellationToken = default | ||
| ) => GetAuthorizationHeaderAsync(string.Join(' ', scopes), cancellationToken); |
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 changes the semantics of every user-token request into client credentials while ignoring claimsPrincipal. PointillaService calls CallApiForUserAsync, so the downstream request would no longer represent the signed-in user and could receive application permissions instead of delegated permissions. That can hide authorization defects in integration tests and would be a security regression if this provider becomes usable outside that narrow test setup.
Possible directions are to implement the appropriate delegated/token-exchange flow for providers that support it, preserve the existing Entra provider for user calls while overriding only CreateAuthorizationHeaderForAppAsync, or explicitly throw NotSupportedException for user acquisition so the change cannot silently elevate or alter identity. Please add a test exercising CallApiForUserAsync and asserting either preserved user identity or the intended explicit failure.
Scores
- Overall importance: 9/10
- Correctness-related: 10/10
- Security-related: 9/10
- Interoperability-related: 6/10
- Flexibility-related: 6/10
- Confidence: 9/10
| ); | ||
|
|
||
| using var client = httpClientFactory.CreateClient(); | ||
| using var request = new HttpRequestMessage(HttpMethod.Post, $"{Authority}/token") |
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.
The token endpoint is constructed by appending /token to the authority. Endpoint paths are not fixed by OAuth/OIDC: different providers and test servers may place the token endpoint under an issuer-specific path or expose a completely different path. This makes an otherwise standards-compatible provider fail even though its discovery document advertises the correct token_endpoint.
I recommend resolving the OpenID Provider Configuration from the configured issuer/authority and using its advertised token_endpoint. In .NET this could use ConfigurationManager<OpenIdConnectConfiguration>, with the same HTTPS and refresh controls as inbound metadata retrieval, or a dedicated typed client that caches discovery metadata. A separately configured token endpoint can be an escape hatch, but discovery should be the default. Please add a test where the token endpoint is not ${Authority}/token so this interoperability remains covered.
Scores
- Overall importance: 8/10
- Correctness-related: 9/10
- Security-related: 4/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 generic-provider path currently covers backend bearer validation and downstream token acquisition, but the browser and Swagger paths remain Entra-specific. The Flotilla frontend still constructs its authority as Consider exposing provider-neutral runtime settings such as issuer/authority, authorization endpoint, token endpoint, client ID, and scopes to the frontend and Swagger. Where possible, derive endpoints from discovery rather than duplicating them. MSAL may also need provider-specific compatibility settings, or the frontend could use a provider-neutral OIDC client if supporting non-Entra providers is a long-term requirement. An end-to-end browser test using authorization code plus PKCE would give much stronger confidence than configuration-shape tests. Scores
|
|
Superseded by #2863, 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: