-
-
Notifications
You must be signed in to change notification settings - Fork 201
Fix authorize against HostAuthorizationPolicy when set #591
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
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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||||||||||||
| using System.Text; | ||||||||||||||||
| using System.Threading.Tasks; | ||||||||||||||||
| using Microsoft.AspNetCore.Http; | ||||||||||||||||
| using Microsoft.Extensions.DependencyInjection; | ||||||||||||||||
| using Microsoft.Extensions.Logging; | ||||||||||||||||
|
|
||||||||||||||||
| namespace TickerQ.Dashboard.Authentication; | ||||||||||||||||
|
|
@@ -157,15 +158,27 @@ private Task<AuthResult> AuthenticateCustomAsync(string authHeader) | |||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| private Task<AuthResult> AuthenticateHostAsync(HttpContext context) | ||||||||||||||||
| private async Task<AuthResult> AuthenticateHostAsync(HttpContext context) | ||||||||||||||||
| { | ||||||||||||||||
| if (!string.IsNullOrEmpty(_config.HostAuthorizationPolicy)) | ||||||||||||||||
| { | ||||||||||||||||
| var authorizationService = context.RequestServices.GetRequiredService<Microsoft.AspNetCore.Authorization.IAuthorizationService>(); | ||||||||||||||||
| var authResult = await authorizationService.AuthorizeAsync(context.User, context, _config.HostAuthorizationPolicy); | ||||||||||||||||
| if (!authResult.Succeeded) | ||||||||||||||||
| { | ||||||||||||||||
| return AuthResult.Failure("Host authorization policy not satisfied"); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return AuthResult.Success(context.User.Identity?.Name ?? "host-user"); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Delegate to host application's authentication | ||||||||||||||||
| if (context.User?.Identity?.IsAuthenticated == true) | ||||||||||||||||
| { | ||||||||||||||||
| var username = context.User.Identity.Name ?? "host-user"; | ||||||||||||||||
|
Comment on lines
176
to
178
|
||||||||||||||||
| if (context.User?.Identity?.IsAuthenticated == true) | |
| { | |
| var username = context.User.Identity.Name ?? "host-user"; | |
| var authenticatedIdentity = context.User?.Identities?.FirstOrDefault(i => i.IsAuthenticated); | |
| if (authenticatedIdentity != null) | |
| { | |
| var username = authenticatedIdentity.Name ?? context.User.Identity?.Name ?? "host-user"; |
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.
Existing behavior/expectations i did not want to break.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| using FluentAssertions; | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Logging; | ||
| using NSubstitute; | ||
| using System.Security.Claims; | ||
| using TickerQ.Dashboard.Authentication; | ||
|
|
||
| namespace TickerQ.Tests; | ||
|
|
||
| public class AuthServiceHostTests | ||
| { | ||
| [Fact] | ||
| public async Task AuthenticateAsync_HostMode_UserAuthenticated_WithName_ReturnsSuccess() | ||
| { | ||
| var config = new AuthConfig { Mode = AuthMode.Host }; | ||
| var logger = Substitute.For<ILogger<AuthService>>(); | ||
| var svc = new AuthService(config, logger); | ||
|
|
||
| var context = new DefaultHttpContext(); | ||
| var identity = new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "alice") }, "TestAuth"); | ||
| context.User = new ClaimsPrincipal(identity); | ||
|
|
||
| var result = await svc.AuthenticateAsync(context); | ||
|
|
||
| result.IsAuthenticated.Should().BeTrue(); | ||
| result.Username.Should().Be("alice"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task AuthenticateAsync_HostMode_UserAuthenticated_WithoutName_ReturnsHostUser() | ||
| { | ||
| var config = new AuthConfig { Mode = AuthMode.Host }; | ||
| var logger = Substitute.For<ILogger<AuthService>>(); | ||
| var svc = new AuthService(config, logger); | ||
|
|
||
| var context = new DefaultHttpContext(); | ||
| // Create an authenticated identity without a name | ||
| var identity = new ClaimsIdentity(System.Array.Empty<Claim>(), "TestAuth"); | ||
| context.User = new ClaimsPrincipal(identity); | ||
|
|
||
| var result = await svc.AuthenticateAsync(context); | ||
|
|
||
| result.IsAuthenticated.Should().BeTrue(); | ||
| result.Username.Should().Be("host-user"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task AuthenticateAsync_HostMode_UserNotAuthenticated_ReturnsFailure() | ||
| { | ||
| var config = new AuthConfig { Mode = AuthMode.Host }; | ||
| var logger = Substitute.For<ILogger<AuthService>>(); | ||
| var svc = new AuthService(config, logger); | ||
|
|
||
| var context = new DefaultHttpContext(); | ||
| // Unauthenticated identity | ||
| context.User = new ClaimsPrincipal(new ClaimsIdentity()); | ||
|
|
||
| var result = await svc.AuthenticateAsync(context); | ||
|
|
||
| result.IsAuthenticated.Should().BeFalse(); | ||
| result.ErrorMessage.Should().Contain("Host authentication required"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task AuthenticateAsync_HostMode_WithPolicy_AuthorizationFails_But_DefaultIdentityAuthenticates_ShouldFail() | ||
| { | ||
| var config = new AuthConfig { Mode = AuthMode.Host, HostAuthorizationPolicy = "MyPolicy" }; | ||
| var logger = Substitute.For<ILogger<AuthService>>(); | ||
| var svc = new AuthService(config, logger); | ||
|
|
||
| var context = new DefaultHttpContext(); | ||
| var identity = new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "alice") }, "TestAuth"); | ||
| context.User = new ClaimsPrincipal(identity); | ||
|
|
||
| // Mock IAuthorizationService to return failure for the policy | ||
| var authorizationService = Substitute.For<IAuthorizationService>(); | ||
| authorizationService.AuthorizeAsync(Arg.Any<ClaimsPrincipal>(), Arg.Any<object?>(), "MyPolicy") | ||
| .Returns(Task.FromResult(AuthorizationResult.Failed())); | ||
|
|
||
| var services = new ServiceCollection(); | ||
| services.AddSingleton(authorizationService); | ||
| context.RequestServices = services.BuildServiceProvider(); | ||
|
|
||
| var result = await svc.AuthenticateAsync(context); | ||
|
|
||
| // Expected: authentication should fail because the policy was not satisfied. | ||
| result.IsAuthenticated.Should().BeFalse(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task AuthenticateAsync_HostMode_WithPolicy_Authorized_ReturnsSuccess() | ||
| { | ||
| var config = new AuthConfig { Mode = AuthMode.Host, HostAuthorizationPolicy = "MyPolicy" }; | ||
| var logger = Substitute.For<ILogger<AuthService>>(); | ||
| var svc = new AuthService(config, logger); | ||
|
|
||
| var context = new DefaultHttpContext(); | ||
| var identity = new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "alice") }, "TestAuth"); | ||
| context.User = new ClaimsPrincipal(identity); | ||
|
|
||
| // Mock IAuthorizationService to return success for the given policy | ||
| var authorizationService = Substitute.For<IAuthorizationService>(); | ||
| authorizationService.AuthorizeAsync(Arg.Any<ClaimsPrincipal>(), Arg.Any<object?>(), "MyPolicy") | ||
| .Returns(Task.FromResult(AuthorizationResult.Success())); | ||
|
|
||
| var services = new ServiceCollection(); | ||
| services.AddSingleton(authorizationService); | ||
| context.RequestServices = services.BuildServiceProvider(); | ||
|
|
||
| var result = await svc.AuthenticateAsync(context); | ||
|
|
||
| result.IsAuthenticated.Should().BeTrue(); | ||
| result.Username.Should().Be("alice"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task AuthenticateAsync_HostMode_WithMultipleIdentities_SecondaryIdentitySatisfiesPolicy_ReturnsSuccess() | ||
| { | ||
| var config = new AuthConfig { Mode = AuthMode.Host, HostAuthorizationPolicy = "MyPolicy" }; | ||
| var logger = Substitute.For<ILogger<AuthService>>(); | ||
| var svc = new AuthService(config, logger); | ||
|
|
||
| var context = new DefaultHttpContext(); | ||
|
|
||
| // First identity is unauthenticated (default) | ||
| var id1 = new ClaimsIdentity(); | ||
|
|
||
| // Second identity is authenticated | ||
| var id2 = new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "second-identity") }, "AuthType"); | ||
|
|
||
| context.User = new ClaimsPrincipal(new[] { id1, id2 }); | ||
|
|
||
| // Mock IAuthorizationService to return success when invoked with the policy | ||
| var authorizationService = Substitute.For<IAuthorizationService>(); | ||
| authorizationService.AuthorizeAsync(Arg.Any<ClaimsPrincipal>(), Arg.Any<object?>(), "MyPolicy") | ||
| .Returns(Task.FromResult(AuthorizationResult.Success())); | ||
|
|
||
| var services = new ServiceCollection(); | ||
| services.AddSingleton(authorizationService); | ||
| context.RequestServices = services.BuildServiceProvider(); | ||
|
|
||
| var result = await svc.AuthenticateAsync(context); | ||
|
|
||
| result.IsAuthenticated.Should().BeTrue(); | ||
| result.Username.Should().Be("host-user"); | ||
| } | ||
| } |
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.
When the policy succeeds, the username is taken from context.User.Identity.Name, but ClaimsPrincipal.Identity returns the first identity. With multiple identities (or when the authenticated identity isn’t first), this can return null and fall back to "host-user" even though an authenticated identity has a name. Consider deriving the username from the authenticated identity (e.g., the first identity with IsAuthenticated==true) or from a name claim across all identities.
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.
If this is wanted. Could add a loop for each identity and create a new ClaimsPrincipal with it to validate that against the policy so we know which identity did satisfy the policy and use it's Name claim if any with fallback to "host-user".