Auth: Prepare test infrastructure for multiple test scenarios#9
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Prepares the auth integration test infrastructure for exercising additional OAuth/OpenID flows by tightening client configuration and adding a test-only login helper endpoint.
Changes:
- Explicitly configures
AllowedGrantTypes(and a redirect URI where needed) for test OAuth clients across fixtures/tests. - Adds a test-only
/test-loginendpoint via anIStartupFilterinIgnisApiFactory. - Refactors env var cleanup in the shared integration fixture and adjusts token tests to create/dispose
HttpClientper test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Ignis.Api.Tests/IntegrationFixture.cs | Centralizes env var keys and cleans them up during fixture disposal; sets AllowedGrantTypes/redirect URI for the shared test client. |
| tests/Ignis.Api.Tests/IgnisApiFactory.cs | Adds in-memory config for AllowedGrantTypes/redirect URI and injects a /test-login middleware via startup filter. |
| tests/Ignis.Api.Tests/AuthorizationControllerTests.cs | Creates/disposes a fresh HttpClient per test rather than holding a field. |
| tests/Ignis.Api.Tests/AuthConfigurationTests.cs | Updates test configs/env vars to include AllowedGrantTypes for clients. |
| src/Ignis.Auth/AuthConstants.cs | Introduces a shared constant for the session cookie scheme name. |
Comments suppressed due to low confidence (3)
tests/Ignis.Api.Tests/IntegrationFixture.cs:66
- DisposeAsync clears each environment variable by setting it to null, which can clobber values that were already set on the machine/CI agent before the test run. Consider capturing the previous values for each key in InitializeAsync and restoring them here (instead of always nulling) to avoid leaking side effects across test runs or developer environments.
foreach (var key in EnvVarKeys)
Environment.SetEnvironmentVariable(key, null);
tests/Ignis.Api.Tests/IgnisApiFactory.cs:76
- A new
/test-loginendpoint is introduced as test infrastructure, but there’s no test verifying it works (e.g., returns 200 and emits a Set-Cookie header for the session scheme). Adding a small integration test that exercises/test-loginwould prevent silent breakages if the auth pipeline changes.
/// <summary>
/// Adds a <c>/test-login</c> endpoint that signs in as a test user
/// using the <see cref="AuthConstants.SessionScheme"/> cookie scheme.
/// </summary>
private sealed class TestLoginStartupFilter : IStartupFilter
{
public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
{
return app =>
{
app.Use(async (context, nextMiddleware) =>
{
if (context.Request.Path == "/test-login")
{
var claims = new List<Claim>
{
new(ClaimTypes.NameIdentifier, "test-user-id"),
new(ClaimTypes.Name, "Test User"),
};
var identity = new ClaimsIdentity(claims, AuthConstants.SessionScheme);
await context.SignInAsync(
AuthConstants.SessionScheme, new ClaimsPrincipal(identity));
context.Response.StatusCode = StatusCodes.Status200OK;
return;
}
src/Ignis.Auth/AuthConstants.cs:8
- The XML doc states this cookie scheme is used for the authorization code flow, but the current auth server configuration only enables the client credentials flow (and explicitly throws for
authorization_code). Either adjust the comment to reflect current behavior/planned status, or implement the supporting auth-code/session-cookie plumbing before documenting it as in-use.
/// <summary>
/// Authentication scheme used for the user session cookie during the authorization code flow.
/// </summary>
public const string SessionScheme = "IgnisAuth.Session";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kennethmyhra
approved these changes
Feb 25, 2026
2f1a146 to
aef90d0
Compare
Implement test login endpoint and update configuration for client credentials
aef90d0 to
ad03056
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enhance test fixture for allowing to test multiple flows scenarios. Also adds a test login endpoint, for establishing a cookie (without logging in on an working identity provided).
Update all test fixtures to explicitly set AllowedGrantTypes on test clients, and also provides fixtures with separate clients for each test to prevent cookie clutter.
AuthConstants.SessionScheme may look a bit odd, but the reason for doing it that way is to prepare for sharing it across app-code and test-code in upcoming PRs.