diff --git a/src/Yavsc.Org.Tests/Mandatory/Remoting.cs b/src/Yavsc.Org.Tests/Mandatory/Remoting.cs index 7def1a43f..5caf905b4 100644 --- a/src/Yavsc.Org.Tests/Mandatory/Remoting.cs +++ b/src/Yavsc.Org.Tests/Mandatory/Remoting.cs @@ -1,7 +1,10 @@ using System.Security.Cryptography.X509Certificates; using System.Net.Security; +using System.IdentityModel.Tokens.Jwt; +using System.Security.Claims; using IdentityModel.Client; using Xunit; +using Yavsc.Server.Helpers; namespace Yavsc.Org.Tests { @@ -70,6 +73,72 @@ namespace Yavsc.Org.Tests } + /// + /// Regression for the MyFiles upload 500 / null + /// User.Identity.Name: the access token minted by the + /// real IdentityServer (resource-owner password grant) must + /// carry a name claim whose value is the user's login + /// (UserName), and + /// — which reads FindFirstValue("name") — must resolve + /// to it on a principal built the way + /// AddYavscJwtBearer builds one + /// (MapInboundClaims=false, NameClaimType="name"). + /// + /// Prerequisites wired by : + /// the test client's AllowedScopes include + /// openid+profile, and the standard identity + /// resources are seeded by the production pipeline. Requesting + /// openid profile makes ProfileService emit + /// name = user.UserName. + /// + [Fact] + public async Task PasswordToken_carries_name_claim_equal_to_login() + { + var serverUrl = GetServerUrl(); + var cancellationToken = TestContext.Current.CancellationToken; + + var client = NewHttpClient(); + var tokenEndpoint = await ResolveTokenEndpointAsync(client, serverUrl, cancellationToken); + + var response = await client.RequestPasswordTokenAsync(new PasswordTokenRequest + { + Address = tokenEndpoint, + ClientId = RequireNonEmpty(_serverFixture.TestClientId, nameof(_serverFixture.TestClientId)), + ClientSecret = RequireNonEmpty(_serverFixture.TestClientSecret, nameof(_serverFixture.TestClientSecret)), + UserName = RequireNonEmpty(_serverFixture.TestingUserName, nameof(_serverFixture.TestingUserName)), + Password = RequireNonEmpty(_serverFixture.TestingUserPassword, nameof(_serverFixture.TestingUserPassword)), + // "openid profile" triggers ProfileService to emit the + // "name" claim (user.UserName). Without profile, the + // token carries only sub + scopes and GetUserName() + // resolves to null — the exact regression behind the + // MyFiles upload 500. "test" is the API scope, so a real + // JWT access token (not just an id_token) is issued. + Scope = "openid profile test" + }, cancellationToken); + + Assert.False(response.IsError, + $"Token request failed: {response.Error} ({response.ErrorDescription})"); + + // Decode the access token straight from the payload so short + // claim names ("name", "sub") stay as-emitted — mirrors + // AddYavscJwtBearer with MapInboundClaims=false. + var jwt = new JwtSecurityToken(response.AccessToken); + var nameClaim = jwt.Claims.FirstOrDefault(c => c.Type == "name"); + Assert.True(nameClaim is not null, + "access_token has no 'name' claim; ProfileService did not emit it " + + "(is the 'profile' scope in the client's AllowedScopes and requested?)."); + Assert.Equal(_serverFixture.TestingUserName, nameClaim!.Value); + + // Rebuild the principal the way a bearer consumer host does + // and exercise the real GetUserName() helper used by the + // file-system / avatar paths. + var identity = new ClaimsIdentity( + jwt.Claims, authenticationType: "Bearer", + nameType: "name", roleType: null); + var principal = new ClaimsPrincipal(identity); + Assert.Equal(_serverFixture.TestingUserName, principal.GetUserName()); + } + [Fact] public async Task GetSignin_returns_a_page() { diff --git a/src/Yavsc.Org.Tests/NonRegression/BillingServiceTests.cs b/src/Yavsc.Org.Tests/NonRegression/BillingServiceTests.cs index 4e8ae0d6f..6504be24d 100644 --- a/src/Yavsc.Org.Tests/NonRegression/BillingServiceTests.cs +++ b/src/Yavsc.Org.Tests/NonRegression/BillingServiceTests.cs @@ -8,6 +8,16 @@ using Xunit; namespace Yavsc { + // BillingService and Config expose static mutable registries that + // WorkflowHelpers.ConfigureBillingService() also mutates from the + // WebServerFixture host build (HostingExtensions.cs). Without a + // collection, xUnit runs this class in parallel with the + // "Yavsc Server" collection, and the host build's ConfigureBilling + // call can land between this test's two ConfigureBilling calls, + // breaking the idempotency count assertion (a flaky failure that + // surfaces whenever the fixture's timing shifts). Pinning the class + // to the same collection serializes it with the host build. + [Collection("Yavsc Server")] [Trait("regression", "II")] public class BillingServiceTests { diff --git a/src/Yavsc.Org.Tests/WebServerFixture.cs b/src/Yavsc.Org.Tests/WebServerFixture.cs index 5ca1daa06..522103023 100644 --- a/src/Yavsc.Org.Tests/WebServerFixture.cs +++ b/src/Yavsc.Org.Tests/WebServerFixture.cs @@ -1,4 +1,5 @@ using IdentityServer8.EntityFramework.Entities; +using IdentityServer8.EntityFramework.Mappers; using IdentityServer8.Models; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Builder; @@ -238,6 +239,26 @@ public sealed class WebServerFixture : WebHostFixture AddAuthorizedClient(scope, TestClientId, TestClientSecret); } + // EnsureDefaultApplicationScopes seeds the standard identity + // resources (openid, profile) once at host startup, but a + // ResetAndMigrateDatabase call does EnsureDeleted on the shared + // in-memory store, which wipes them — and ReseedAuthTestData + // previously only re-added the client. Without openid/profile + // present, any token request asking for those scopes returns + // invalid_scope, so a password-grant token carrying the "name" + // claim (scope "openid profile") could not be minted after a + // reset. Re-seed them here to mirror the startup seed. + var identityResources = configDb.Set(); + if (!identityResources.Any(r => r.Name == "openid")) + { + identityResources.Add(new IdentityResources.OpenId().ToEntity()); + } + if (!identityResources.Any(r => r.Name == "profile")) + { + identityResources.Add(new IdentityResources.Profile().ToEntity()); + } + configDb.SaveChanges(); + _sharedTestClientId = TestClientId; _sharedTestClientSecret = TestClientSecret; _sharedTestingUserName = TestingUserName; @@ -284,7 +305,12 @@ public sealed class WebServerFixture : WebHostFixture { ClientId = testClientId, AccessTokenLifetime = 3600000, - AccessTokenType = 1, + // AccessTokenType = 0 → Jwt in IdentityServer8 (1 = Reference, + // the inverse of Duende's enum). The real PostIt client issues + // JWT access tokens so the Blogs/Api bearer hosts can validate + // them with JwtBearer; mirror that here so the token can be + // decoded and its "name" claim asserted. + AccessTokenType = 0, ClientName = "Testing client", Enabled = true, RequireClientSecret = true @@ -327,6 +353,23 @@ public sealed class WebServerFixture : WebHostFixture ClientId = testingClient.Id, Scope = "test" }); + // Allow the identity scopes so a resource-owner-password token + // can request "openid profile" and trigger ProfileService to + // emit the "name" claim (user.UserName). Without profile in the + // client's AllowedScopes, IdentityServer strips the scope from + // the request and the access_token carries no "name" — the + // regression that left GetUserName() / the MyFiles upload root + // null. Additive only: existing tests request "test" alone. + configDb.Set().Add(new ClientScope + { + ClientId = testingClient.Id, + Scope = "openid" + }); + configDb.Set().Add(new ClientScope + { + ClientId = testingClient.Id, + Scope = "profile" + }); configDb.SaveChanges(); } diff --git a/src/Yavsc.Org/Extensions/HostingExtensions.cs b/src/Yavsc.Org/Extensions/HostingExtensions.cs index ea035001c..b5bb65cc5 100644 --- a/src/Yavsc.Org/Extensions/HostingExtensions.cs +++ b/src/Yavsc.Org/Extensions/HostingExtensions.cs @@ -368,6 +368,17 @@ public static class HostingExtensions }) .AddAspNetIdentity() + // Register the custom ProfileService so IdentityServer actually + // calls it instead of the default UserProfileService. Without + // this, the class is dead code: IS8 falls back to the default, + // which only emits claims listed in each API resource's + // UserClaims (RequestedClaimTypes) — "name" is never among + // them, so every access token lacks "name" and the Blogs/Api + // bearer hosts get GetUserName() == null (the MyFiles upload + // 500). Our ProfileService emits name/email/roles for any + // user grant whose client is allowed "profile", independent + // of the caller, so the access token carries the login. + .AddProfileService() .AddClientStore() .AddClientConfigurationValidator() .AddCorsPolicyService() diff --git a/src/Yavsc.Server/Helpers/FileSystemHelpers.cs b/src/Yavsc.Server/Helpers/FileSystemHelpers.cs index 77e691ea5..762c18f36 100644 --- a/src/Yavsc.Server/Helpers/FileSystemHelpers.cs +++ b/src/Yavsc.Server/Helpers/FileSystemHelpers.cs @@ -1,19 +1,17 @@ #nullable enable annotations - - - using System.Security.Claims; using Microsoft.AspNetCore.Html; using Microsoft.Extensions.FileProviders; +using Microsoft.AspNetCore.Http; +using ImageMagick; using Yavsc.Models; using Yavsc.Models.Streaming; using Yavsc.ViewModels; -using Microsoft.AspNetCore.Http; using Yavsc.Exceptions; using Yavsc.Abstract.Helpers; -using ImageMagick; using Yavsc.Server.Models.FileSystem; using Yavsc.ViewModels.UserFiles; + namespace Yavsc.Server.Helpers { public static class FileSystemHelpers @@ -46,7 +44,7 @@ namespace Yavsc.Server.Helpers this ClaimsPrincipal user, string subpath, SiteSettings siteSettings) { - var root = Path.Combine(siteSettings.Blog, user.Identity.Name); + var root = Path.Combine(siteSettings.Blog, user.GetUserName()); if (!string.IsNullOrWhiteSpace(subpath)) { if (!subpath.IsValidYavscPath()) diff --git a/src/Yavsc.Server/Helpers/ServiceExtensions.cs b/src/Yavsc.Server/Helpers/ServiceExtensions.cs index 297084c28..397eada85 100644 --- a/src/Yavsc.Server/Helpers/ServiceExtensions.cs +++ b/src/Yavsc.Server/Helpers/ServiceExtensions.cs @@ -1,5 +1,6 @@ #nullable enable annotations +using IdentityModel; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.Extensions.Configuration; @@ -107,9 +108,23 @@ public static class ServiceExtensions ValidateAudience = true, ValidAudiences = audiences, RoleClaimType = Constants.RoleClaimType, - NameClaimType = Constants.NameClaimType, + // IdentityServer's ProfileService emits the name claim as + // JwtClaimTypes.Name ("name", short form). MapInboundClaims + // (below) is disabled, so it stays "name" on the principal — + // NameClaimType must therefore be "name" too, or + // User.Identity.Name / GetUserName() resolve to null. The + // previous Constants.NameClaimType (the Microsoft long URI) + // never matched the remapped xmlsoap URI either, so the name + // was unreachable on every bearer host regardless of scope. + NameClaimType = JwtClaimTypes.Name, }; - options.MapInboundClaims = true; + // Keep JWT claim names as-emitted ("sub", "name", …). The default + // inbound map rewrites "name" → xmlsoap name URI and "sub" → + // nameid URI, which neither Constants.NameClaimType (microsoft + // URI) nor GetUserName()'s FindFirstValue("name") could match. + // Roles are already emitted as Constants.RoleClaimType (a long + // URI), so disabling the map leaves IsInMsRole() working. + options.MapInboundClaims = false; options.ClaimsIssuer = authority; options.Audience = audiences[0]; diff --git a/src/Yavsc.Server/Services/ProfileService.cs b/src/Yavsc.Server/Services/ProfileService.cs index 3b7b5121a..4c915f5c9 100644 --- a/src/Yavsc.Server/Services/ProfileService.cs +++ b/src/Yavsc.Server/Services/ProfileService.cs @@ -28,35 +28,40 @@ namespace Yavsc.Services var claims = new List { new Claim(JwtClaimTypes.Subject,user.Id.ToString()), }; - List claimAdds = new List(); foreach (var scope in context.RequestedResources.ParsedScopes) { if (context.Client.AllowedScopes.Contains(scope.ParsedName)) { claims.Add(new Claim(JwtClaimTypes.Scope, scope.ParsedName)); - claimAdds.Add(scope.ParsedName); } } - if (claimAdds.Contains(JwtClaimTypes.Profile)) + // name / email / roles are gated on the client being granted + // the "profile" identity scope. They MUST land in the *access + // token*: the Blogs/Api bearer resource hosts read the login + // via GetUserName() (FindFirstValue("name")) and roles via + // IsInMsRole() from the access token. But "profile" is an + // identity scope, which only appears in the id-token caller's + // ParsedScopes — never in the access-token caller's — so the + // previous gate (claimAdds.Contains("profile")) left name, + // email and roles out of every access token, and GetUserName() + // resolved to null on every bearer host (the MyFiles upload + // 500). Gate on the client's AllowedScopes instead, which is + // caller-independent, so both the access token and the id + // token carry these claims for any user grant whose client is + // allowed "profile". + if (context.Client.AllowedScopes.Contains(JwtClaimTypes.Profile)) { - claimAdds.Remove("profile"); - claimAdds.Add(JwtClaimTypes.Name); - claimAdds.Add(JwtClaimTypes.Email); - claimAdds.Add(Constants.RoleClaimType); - } - - if (claimAdds.Contains(JwtClaimTypes.Name)) - claims.Add(new Claim(JwtClaimTypes.Name, user.FullName)); - - if (claimAdds.Contains(JwtClaimTypes.Email)) + // "name" carries the login (UserName), not the display + // FullName: GetUserName() / User.Identity.Name feed + // filesystem paths (user files root, avatar file name), + // and FullName can contain spaces. No bearer-host + // consumer displays this claim as a human name today. + claims.Add(new Claim(JwtClaimTypes.Name, user.UserName)); claims.Add(new Claim(JwtClaimTypes.Email, user.Email)); - - if (claimAdds.Contains(Constants.RoleClaimType)) - { var roles = await this._userManager.GetRolesAsync(user); - if (roles.Count()>0) + if (roles.Count() > 0) { claims.AddRange(roles.Select(r => new Claim(Constants.RoleClaimType, r))); } diff --git a/src/Yavsc.Tests.Shared/TestTokenIssuer.cs b/src/Yavsc.Tests.Shared/TestTokenIssuer.cs index 18aa2e00c..0046fc346 100644 --- a/src/Yavsc.Tests.Shared/TestTokenIssuer.cs +++ b/src/Yavsc.Tests.Shared/TestTokenIssuer.cs @@ -74,6 +74,14 @@ public static class TestTokenIssuer var claims = new List { new("sub", subject), + // Mirror production: IdentityServer's ProfileService emits the + // "name" claim as the user's login (UserName) when the profile + // scope is granted, and the file-system helpers resolve the + // per-user storage root via GetUserName() (FindFirstValue("name")) + // — not via User.Identity.Name. A test token without "name" would + // make ReceiveUserFile / EnsureDestinationDirectory compute a + // null owner root and 500 on upload, so stamp it here. + new("name", subject), new("scope", scope), }; if (extraClaims is not null) claims.AddRange(extraClaims);