Skip to content

Allow openid for integration tests - #449

Open
oysand wants to merge 1 commit into
equinor:mainfrom
oysand:allow-openid-for-integration-tests
Open

Allow openid for integration tests#449
oysand wants to merge 1 commit into
equinor:mainfrom
oysand:allow-openid-for-integration-tests

Conversation

@oysand

@oysand oysand commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Ready for review checklist:

  • A self-review has been performed
  • All commits run individually
  • Temporary changes have been removed, like logging, TODO, etc.
  • The PR has been tested locally
  • A test has been written
    • This change doesn't need a new test
  • Relevant issues are linked
  • Remaining work is documented in issues
    • There is no remaining work from this PR that requires new issues
  • The changes do not introduce dead code as unused imports, functions etc.

@olaals

olaals commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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 backend validation override alone does not make SARA usable with an arbitrary OIDC provider in local orchestration. The frontend still builds https://login.microsoftonline.com/{tenantId}, /api/config exposes only Entra-oriented client and tenant IDs, and Swagger constructs Microsoft authorization/token URLs and sends the Entra-specific Resource parameter. Interactive browser login will therefore continue to depend on Entra even when backend validation points elsewhere.

Audience and authorization claims also need an explicit contract. Different providers and test servers derive aud differently from scopes, and SARA endpoints depend on application roles such as WorkflowStatus.Write. The issuer configuration should deterministically map browser and service clients to SARA’s expected audience and roles rather than relying on scope naming conventions.

Possible directions are to expose provider-neutral authority, authorization endpoint, token endpoint, client ID, and scopes through runtime config; derive endpoints from discovery where practical; omit provider-specific parameters unless configured; and define explicit audience/role mappings for test tokens. If MSAL cannot cleanly support the desired providers, a provider-neutral OIDC browser client may be more maintainable. Please add an end-to-end authorization-code-with-PKCE test and a service-token test that verifies exact audience and required-role behavior.

Scores

  • Overall importance: 9/10
  • Correctness-related: 9/10
  • Security-related: 7/10
  • Interoperability-related: 10/10
  • Flexibility-related: 9/10
  • Confidence: 9/10

options.Authority = authority;
options.Audience = audience;
// The mock issuer is plain HTTP on the test network.
options.RequireHttpsMetadata = false;

@olaals olaals Aug 5, 2026

Copy link
Copy Markdown
Contributor

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.

RequireHttpsMetadata is disabled for every generic provider. This is convenient for a local HTTP test issuer, but it combines provider selection with weakening the transport protection for discovery metadata and JWKS. If generic OIDC becomes usable in local, staging, or other deployments, a configuration mistake could allow signing keys to be fetched over HTTP and permit key substitution.

I recommend keeping HTTPS metadata required by default and making insecure HTTP a separate, explicit opt-in such as AllowInsecureHttpMetadata=false. Local/integration orchestration can enable that setting, while other deployments should reject it at startup. Tests should verify that HTTP metadata is rejected by default, accepted only with the explicit opt-in, and that HTTPS providers continue using strict metadata validation.

Scores

  • Overall importance: 8/10
  • Correctness-related: 7/10
  • Security-related: 10/10
  • Interoperability-related: 5/10
  • Flexibility-related: 6/10
  • Confidence: 9/10

@oysand

oysand commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #456, which uses Keycloak instead of oauth2-mock-server. Leaving this open as the fallback — close it if that one lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants