From e7c4942777984d677b745af90a25c9352ac90cf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Tue, 23 Oct 2018 18:18:06 +0200 Subject: [PATCH] Move the scope permissions validation logic to ensure it also applies to public clients --- .../OpenIddictServerProvider.Exchange.cs | 58 +++++------ .../OpenIddictServerProviderTests.Exchange.cs | 99 +++++++++---------- 2 files changed, 75 insertions(+), 82 deletions(-) diff --git a/src/OpenIddict.Server/Internal/OpenIddictServerProvider.Exchange.cs b/src/OpenIddict.Server/Internal/OpenIddictServerProvider.Exchange.cs index 3a9b5029..2f3ee4b0 100644 --- a/src/OpenIddict.Server/Internal/OpenIddictServerProvider.Exchange.cs +++ b/src/OpenIddict.Server/Internal/OpenIddictServerProvider.Exchange.cs @@ -215,6 +215,35 @@ namespace OpenIddict.Server.Internal } } + // Unless permission enforcement was explicitly disabled, ensure + // the client application is allowed to use the specified scopes. + if (!options.IgnoreScopePermissions) + { + foreach (var scope in context.Request.GetScopes()) + { + // Avoid validating the "openid" and "offline_access" scopes as they represent protocol scopes. + if (string.Equals(scope, OpenIddictConstants.Scopes.OfflineAccess, StringComparison.Ordinal) || + string.Equals(scope, OpenIddictConstants.Scopes.OpenId, StringComparison.Ordinal)) + { + continue; + } + + // Reject the request if the application is not allowed to use the iterated scope. + if (!await _applicationManager.HasPermissionAsync(application, + OpenIddictConstants.Permissions.Prefixes.Scope + scope)) + { + _logger.LogError("The token request was rejected because the application '{ClientId}' " + + "was not allowed to use the scope {Scope}.", context.ClientId, scope); + + context.Reject( + error: OpenIddictConstants.Errors.InvalidRequest, + description: "This client application is not allowed to use the specified scope."); + + return; + } + } + } + if (await _applicationManager.IsPublicAsync(application)) { // Note: public applications are not allowed to use the client credentials grant. @@ -279,35 +308,6 @@ namespace OpenIddict.Server.Internal return; } - // Unless permission enforcement was explicitly disabled, ensure - // the client application is allowed to use the specified scopes. - if (!options.IgnoreScopePermissions) - { - foreach (var scope in context.Request.GetScopes()) - { - // Avoid validating the "openid" and "offline_access" scopes as they represent protocol scopes. - if (string.Equals(scope, OpenIddictConstants.Scopes.OfflineAccess, StringComparison.Ordinal) || - string.Equals(scope, OpenIddictConstants.Scopes.OpenId, StringComparison.Ordinal)) - { - continue; - } - - // Reject the request if the application is not allowed to use the iterated scope. - if (!await _applicationManager.HasPermissionAsync(application, - OpenIddictConstants.Permissions.Prefixes.Scope + scope)) - { - _logger.LogError("The token request was rejected because the application '{ClientId}' " + - "was not allowed to use the scope {Scope}.", context.ClientId, scope); - - context.Reject( - error: OpenIddictConstants.Errors.InvalidRequest, - description: "This client application is not allowed to use the specified scope."); - - return; - } - } - } - context.Validate(); await _eventService.PublishAsync(new OpenIddictServerEvents.ValidateTokenRequest(context)); diff --git a/test/OpenIddict.Server.Tests/Internal/OpenIddictServerProviderTests.Exchange.cs b/test/OpenIddict.Server.Tests/Internal/OpenIddictServerProviderTests.Exchange.cs index 580aaf31..09a43134 100644 --- a/test/OpenIddict.Server.Tests/Internal/OpenIddictServerProviderTests.Exchange.cs +++ b/test/OpenIddict.Server.Tests/Internal/OpenIddictServerProviderTests.Exchange.cs @@ -473,7 +473,7 @@ namespace OpenIddict.Server.Internal.Tests } [Fact] - public async Task ValidateTokenRequest_ClientSecretCannotBeUsedByPublicClients() + public async Task ValidateTokenRequest_RequestIsRejectedWhenScopePermissionIsNotGranted() { // Arrange var application = new OpenIddictApplication(); @@ -485,11 +485,23 @@ namespace OpenIddict.Server.Internal.Tests instance.Setup(mock => mock.GetClientTypeAsync(application, It.IsAny())) .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Public)); + + instance.Setup(mock => mock.HasPermissionAsync(application, + OpenIddictConstants.Permissions.Prefixes.Scope + + OpenIddictConstants.Scopes.Profile, It.IsAny())) + .ReturnsAsync(true); + + instance.Setup(mock => mock.HasPermissionAsync(application, + OpenIddictConstants.Permissions.Prefixes.Scope + + OpenIddictConstants.Scopes.Email, It.IsAny())) + .ReturnsAsync(false); }); var server = CreateAuthorizationServer(builder => { builder.Services.AddSingleton(manager); + builder.RegisterScopes(OpenIddictConstants.Scopes.Email, OpenIddictConstants.Scopes.Profile); + builder.Configure(options => options.IgnoreScopePermissions = false); }); var client = new OpenIdConnectClient(server.CreateClient()); @@ -498,22 +510,32 @@ namespace OpenIddict.Server.Internal.Tests var response = await client.PostAsync(TokenEndpoint, new OpenIdConnectRequest { ClientId = "Fabrikam", - ClientSecret = "7Fjfp0ZBr1KtDRbnfVdmIw", GrantType = OpenIddictConstants.GrantTypes.Password, Username = "johndoe", - Password = "A3ddj3w" + Password = "A3ddj3w", + Scope = "openid offline_access profile email" }); // Assert Assert.Equal(OpenIddictConstants.Errors.InvalidRequest, response.Error); - Assert.Equal("The 'client_secret' parameter is not valid for this client application.", response.ErrorDescription); + Assert.Equal("This client application is not allowed to use the specified scope.", response.ErrorDescription); - Mock.Get(manager).Verify(mock => mock.FindByClientIdAsync("Fabrikam", It.IsAny()), Times.Once()); - Mock.Get(manager).Verify(mock => mock.GetClientTypeAsync(application, It.IsAny()), Times.Once()); + Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, + OpenIddictConstants.Permissions.Prefixes.Scope + + OpenIddictConstants.Scopes.OpenId, It.IsAny()), Times.Never()); + Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, + OpenIddictConstants.Permissions.Prefixes.Scope + + OpenIddictConstants.Scopes.OfflineAccess, It.IsAny()), Times.Never()); + Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, + OpenIddictConstants.Permissions.Prefixes.Scope + + OpenIddictConstants.Scopes.Profile, It.IsAny()), Times.Once()); + Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, + OpenIddictConstants.Permissions.Prefixes.Scope + + OpenIddictConstants.Scopes.Email, It.IsAny()), Times.Once()); } [Fact] - public async Task ValidateTokenRequest_ClientSecretIsRequiredForConfidentialClients() + public async Task ValidateTokenRequest_ClientSecretCannotBeUsedByPublicClients() { // Arrange var application = new OpenIddictApplication(); @@ -524,7 +546,7 @@ namespace OpenIddict.Server.Internal.Tests .ReturnsAsync(application); instance.Setup(mock => mock.GetClientTypeAsync(application, It.IsAny())) - .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Confidential)); + .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Public)); }); var server = CreateAuthorizationServer(builder => @@ -538,22 +560,22 @@ namespace OpenIddict.Server.Internal.Tests var response = await client.PostAsync(TokenEndpoint, new OpenIdConnectRequest { ClientId = "Fabrikam", - ClientSecret = null, + ClientSecret = "7Fjfp0ZBr1KtDRbnfVdmIw", GrantType = OpenIddictConstants.GrantTypes.Password, Username = "johndoe", Password = "A3ddj3w" }); // Assert - Assert.Equal(OpenIddictConstants.Errors.InvalidClient, response.Error); - Assert.Equal("The 'client_secret' parameter required for this client application is missing.", response.ErrorDescription); + Assert.Equal(OpenIddictConstants.Errors.InvalidRequest, response.Error); + Assert.Equal("The 'client_secret' parameter is not valid for this client application.", response.ErrorDescription); Mock.Get(manager).Verify(mock => mock.FindByClientIdAsync("Fabrikam", It.IsAny()), Times.Once()); Mock.Get(manager).Verify(mock => mock.GetClientTypeAsync(application, It.IsAny()), Times.Once()); } [Fact] - public async Task ValidateTokenRequest_ClientSecretIsRequiredForHybridClients() + public async Task ValidateTokenRequest_ClientSecretIsRequiredForConfidentialClients() { // Arrange var application = new OpenIddictApplication(); @@ -564,7 +586,7 @@ namespace OpenIddict.Server.Internal.Tests .ReturnsAsync(application); instance.Setup(mock => mock.GetClientTypeAsync(application, It.IsAny())) - .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Hybrid)); + .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Confidential)); }); var server = CreateAuthorizationServer(builder => @@ -593,7 +615,7 @@ namespace OpenIddict.Server.Internal.Tests } [Fact] - public async Task ValidateTokenRequest_RequestIsRejectedWhenClientCredentialsAreInvalid() + public async Task ValidateTokenRequest_ClientSecretIsRequiredForHybridClients() { // Arrange var application = new OpenIddictApplication(); @@ -604,10 +626,7 @@ namespace OpenIddict.Server.Internal.Tests .ReturnsAsync(application); instance.Setup(mock => mock.GetClientTypeAsync(application, It.IsAny())) - .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Confidential)); - - instance.Setup(mock => mock.ValidateClientSecretAsync(application, "7Fjfp0ZBr1KtDRbnfVdmIw", It.IsAny())) - .ReturnsAsync(false); + .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Hybrid)); }); var server = CreateAuthorizationServer(builder => @@ -621,7 +640,7 @@ namespace OpenIddict.Server.Internal.Tests var response = await client.PostAsync(TokenEndpoint, new OpenIdConnectRequest { ClientId = "Fabrikam", - ClientSecret = "7Fjfp0ZBr1KtDRbnfVdmIw", + ClientSecret = null, GrantType = OpenIddictConstants.GrantTypes.Password, Username = "johndoe", Password = "A3ddj3w" @@ -629,15 +648,14 @@ namespace OpenIddict.Server.Internal.Tests // Assert Assert.Equal(OpenIddictConstants.Errors.InvalidClient, response.Error); - Assert.Equal("The specified client credentials are invalid.", response.ErrorDescription); + Assert.Equal("The 'client_secret' parameter required for this client application is missing.", response.ErrorDescription); Mock.Get(manager).Verify(mock => mock.FindByClientIdAsync("Fabrikam", It.IsAny()), Times.Once()); Mock.Get(manager).Verify(mock => mock.GetClientTypeAsync(application, It.IsAny()), Times.Once()); - Mock.Get(manager).Verify(mock => mock.ValidateClientSecretAsync(application, "7Fjfp0ZBr1KtDRbnfVdmIw", It.IsAny()), Times.Once()); } [Fact] - public async Task ValidateTokenRequest_RequestIsRejectedWhenScopePermissionIsNotGranted() + public async Task ValidateTokenRequest_RequestIsRejectedWhenClientCredentialsAreInvalid() { // Arrange var application = new OpenIddictApplication(); @@ -651,27 +669,12 @@ namespace OpenIddict.Server.Internal.Tests .Returns(new ValueTask(OpenIddictConstants.ClientTypes.Confidential)); instance.Setup(mock => mock.ValidateClientSecretAsync(application, "7Fjfp0ZBr1KtDRbnfVdmIw", It.IsAny())) - .ReturnsAsync(true); - - instance.Setup(mock => mock.HasPermissionAsync(application, - OpenIddictConstants.Permissions.Prefixes.Scope + - OpenIddictConstants.Scopes.Profile, It.IsAny())) - .ReturnsAsync(true); - - instance.Setup(mock => mock.HasPermissionAsync(application, - OpenIddictConstants.Permissions.Prefixes.Scope + - OpenIddictConstants.Scopes.Email, It.IsAny())) .ReturnsAsync(false); - - instance.Setup(mock => mock.ValidateRedirectUriAsync(application, "http://www.fabrikam.com/path", It.IsAny())) - .ReturnsAsync(true); }); var server = CreateAuthorizationServer(builder => { builder.Services.AddSingleton(manager); - builder.RegisterScopes(OpenIddictConstants.Scopes.Email, OpenIddictConstants.Scopes.Profile); - builder.Configure(options => options.IgnoreScopePermissions = false); }); var client = new OpenIdConnectClient(server.CreateClient()); @@ -683,26 +686,16 @@ namespace OpenIddict.Server.Internal.Tests ClientSecret = "7Fjfp0ZBr1KtDRbnfVdmIw", GrantType = OpenIddictConstants.GrantTypes.Password, Username = "johndoe", - Password = "A3ddj3w", - Scope = "openid offline_access profile email" + Password = "A3ddj3w" }); // Assert - Assert.Equal(OpenIddictConstants.Errors.InvalidRequest, response.Error); - Assert.Equal("This client application is not allowed to use the specified scope.", response.ErrorDescription); + Assert.Equal(OpenIddictConstants.Errors.InvalidClient, response.Error); + Assert.Equal("The specified client credentials are invalid.", response.ErrorDescription); - Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, - OpenIddictConstants.Permissions.Prefixes.Scope + - OpenIddictConstants.Scopes.OpenId, It.IsAny()), Times.Never()); - Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, - OpenIddictConstants.Permissions.Prefixes.Scope + - OpenIddictConstants.Scopes.OfflineAccess, It.IsAny()), Times.Never()); - Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, - OpenIddictConstants.Permissions.Prefixes.Scope + - OpenIddictConstants.Scopes.Profile, It.IsAny()), Times.Once()); - Mock.Get(manager).Verify(mock => mock.HasPermissionAsync(application, - OpenIddictConstants.Permissions.Prefixes.Scope + - OpenIddictConstants.Scopes.Email, It.IsAny()), Times.Once()); + Mock.Get(manager).Verify(mock => mock.FindByClientIdAsync("Fabrikam", It.IsAny()), Times.Once()); + Mock.Get(manager).Verify(mock => mock.GetClientTypeAsync(application, It.IsAny()), Times.Once()); + Mock.Get(manager).Verify(mock => mock.ValidateClientSecretAsync(application, "7Fjfp0ZBr1KtDRbnfVdmIw", It.IsAny()), Times.Once()); } [Fact]