diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index ecef8e15e..af5cda208 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -47,6 +47,7 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient private string? _tokenEndpointAuthMethod; private ITokenCache _tokenCache; private AuthorizationServerMetadata? _authServerMetadata; + private string? _lastRequestedScopes; /// /// Initializes a new instance of the class using the specified options. @@ -714,16 +715,47 @@ private async Task PerformDynamicClientRegistrationAsync( private string? GetScopeParameter(ProtectedResourceMetadata protectedResourceMetadata) { + string? newScopes; + if (!string.IsNullOrEmpty(protectedResourceMetadata.WwwAuthenticateScope)) { - return protectedResourceMetadata.WwwAuthenticateScope; + newScopes = protectedResourceMetadata.WwwAuthenticateScope; } else if (protectedResourceMetadata.ScopesSupported.Count > 0) { - return string.Join(" ", protectedResourceMetadata.ScopesSupported); + newScopes = string.Join(" ", protectedResourceMetadata.ScopesSupported); + } + else + { + newScopes = _configuredScopes; + } + + // Union with previously requested scopes to avoid losing permissions during step-up + // authorization. Per the MCP spec (aligned with RFC 6750 ยง3.1), servers should emit only + // per-operation scopes in 403 insufficient_scope challenges, so clients SHOULD accumulate + // all previously requested scopes to prevent losing previously granted permissions. + if (_lastRequestedScopes is not null && newScopes is not null) + { + var combined = new HashSet(StringComparer.Ordinal); + foreach (var scope in _lastRequestedScopes.Split(' ')) + { + if (scope.Length > 0) + { + combined.Add(scope); + } + } + foreach (var scope in newScopes.Split(' ')) + { + if (scope.Length > 0) + { + combined.Add(scope); + } + } + newScopes = string.Join(" ", combined); } - return _configuredScopes; + _lastRequestedScopes = newScopes; + return newScopes; } /// diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index c4979fb10..4c8cac4fb 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -475,7 +475,10 @@ public async Task AuthorizationFlow_UsesScopeFromForbiddenHeader() { // Tool now just checks if user has the required scopes // If they don't, it shouldn't get here due to middleware - Assert.True(user.HasClaim("scope", adminScopes), "User should have admin scopes when tool executes"); + // The token scope may include accumulated scopes, so check for containment + var scopeClaim = user.FindFirst("scope")?.Value ?? ""; + var userScopes = new HashSet(scopeClaim.Split(' ', StringSplitOptions.RemoveEmptyEntries), StringComparer.Ordinal); + Assert.True(adminScopes.Split(' ', StringSplitOptions.RemoveEmptyEntries).All(s => userScopes.Contains(s)), "User should have admin scopes when tool executes"); return "Admin tool executed."; }), ]); @@ -510,9 +513,11 @@ public async Task AuthorizationFlow_UsesScopeFromForbiddenHeader() if (toolCallParams?.Name == "admin-tool") { - // Check if user has required scopes + // Check if user has all required scopes (token may include accumulated scopes) var user = context.User; - if (!user.HasClaim("scope", adminScopes)) + var scopeClaim = user.FindFirst("scope")?.Value ?? ""; + var userScopes = new HashSet(scopeClaim.Split(' ', StringSplitOptions.RemoveEmptyEntries), StringComparer.Ordinal); + if (!adminScopes.Split(' ', StringSplitOptions.RemoveEmptyEntries).All(s => userScopes.Contains(s))) { // User lacks required scopes, return 403 before MapMcp processes the request context.Response.StatusCode = StatusCodes.Status403Forbidden; @@ -554,7 +559,12 @@ public async Task AuthorizationFlow_UsesScopeFromForbiddenHeader() var adminResult = await client.CallToolAsync("admin-tool", cancellationToken: TestContext.Current.CancellationToken); Assert.Equal("Admin tool executed.", adminResult.Content[0].ToString()); - Assert.Equal(adminScopes, requestedScope); + // After the 403 insufficient_scope challenge with adminScopes, the client should re-authorize + // with the union of previously requested scopes ("mcp:tools") and the newly challenged scopes. + var accumulatedScopes = requestedScope!.Split(' '); + Assert.Contains("mcp:tools", accumulatedScopes); + Assert.Contains("admin:read", accumulatedScopes); + Assert.Contains("admin:write", accumulatedScopes); } [Fact]