diff --git a/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs b/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs index 8c93a232..f9f91591 100644 --- a/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs +++ b/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs @@ -5,7 +5,6 @@ */ using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -88,12 +87,12 @@ namespace OpenIddict.Core OpenIddictConstants.ClientTypes.Confidential, cancellationToken); } - // If the client is a confidential application, throw an + // If the client is not a public application, throw an // exception as the client secret is required in this case. - if (string.IsNullOrEmpty(secret) && - string.Equals(type, OpenIddictConstants.ClientTypes.Confidential, StringComparison.OrdinalIgnoreCase)) + if (string.IsNullOrEmpty(secret) && !await IsPublicAsync(application, cancellationToken)) { - throw new InvalidOperationException("A client secret must be provided when creating a confidential application."); + throw new InvalidOperationException("A client secret must be provided when creating " + + "a confidential or hybrid application."); } if (!string.IsNullOrEmpty(secret)) @@ -133,12 +132,13 @@ namespace OpenIddict.Core OpenIddictConstants.ClientTypes.Confidential; } - // If the client is a confidential application, throw an + // If the client is not a public application, throw an // exception as the client secret is required in this case. if (string.IsNullOrEmpty(descriptor.ClientSecret) && - string.Equals(descriptor.Type, OpenIddictConstants.ClientTypes.Confidential, StringComparison.OrdinalIgnoreCase)) + !string.Equals(descriptor.Type, OpenIddictConstants.ClientTypes.Public, StringComparison.OrdinalIgnoreCase)) { - throw new InvalidOperationException("A client secret must be provided when creating a confidential application."); + throw new InvalidOperationException("A client secret must be provided when creating " + + "a confidential or hybrid application."); } // Obfuscate the provided client secret. @@ -265,9 +265,10 @@ namespace OpenIddict.Core // Ensure the application type returned by the store is supported by the manager. if (!string.Equals(type, OpenIddictConstants.ClientTypes.Confidential, StringComparison.OrdinalIgnoreCase) && + !string.Equals(type, OpenIddictConstants.ClientTypes.Hybrid, StringComparison.OrdinalIgnoreCase) && !string.Equals(type, OpenIddictConstants.ClientTypes.Public, StringComparison.OrdinalIgnoreCase)) { - throw new InvalidOperationException("Only 'confidential' or 'public' applications are " + + throw new InvalidOperationException("Only 'confidential', 'hybrid' or 'public' applications are " + "supported by the default application manager."); } @@ -351,31 +352,34 @@ namespace OpenIddict.Core } /// - /// Determines whether the specified application has a client secret. + /// Determines whether an application is a confidential client. /// /// The application. /// The that can be used to abort the operation. - /// - /// A that can be used to monitor the asynchronous operation, - /// whose result returns a boolean indicating whether a client secret is registered. - /// - public virtual async Task HasClientSecretAsync([NotNull] TApplication application, CancellationToken cancellationToken) + /// true if the application is a confidential client, false otherwise. + public async Task IsConfidentialAsync([NotNull] TApplication application, CancellationToken cancellationToken) { if (application == null) { throw new ArgumentNullException(nameof(application)); } - return !string.IsNullOrEmpty(await Store.GetClientSecretAsync(application, cancellationToken)); + var type = await GetClientTypeAsync(application, cancellationToken); + if (string.IsNullOrEmpty(type)) + { + return false; + } + + return string.Equals(type, OpenIddictConstants.ClientTypes.Confidential, StringComparison.OrdinalIgnoreCase); } /// - /// Determines whether an application is a confidential client. + /// Determines whether an application is a hybrid client. /// /// The application. /// The that can be used to abort the operation. - /// true if the application is a confidential client, false otherwise. - public async Task IsConfidentialAsync([NotNull] TApplication application, CancellationToken cancellationToken) + /// true if the application is a hybrid client, false otherwise. + public async Task IsHybridAsync([NotNull] TApplication application, CancellationToken cancellationToken) { if (application == null) { @@ -388,7 +392,7 @@ namespace OpenIddict.Core return false; } - return string.Equals(type, OpenIddictConstants.ClientTypes.Confidential, StringComparison.OrdinalIgnoreCase); + return string.Equals(type, OpenIddictConstants.ClientTypes.Hybrid, StringComparison.OrdinalIgnoreCase); } /// @@ -505,9 +509,9 @@ namespace OpenIddict.Core throw new ArgumentNullException(nameof(application)); } - if (!await IsConfidentialAsync(application, cancellationToken)) + if (await IsPublicAsync(application, cancellationToken)) { - Logger.LogWarning("Client authentication cannot be enforced for non-confidential applications."); + Logger.LogWarning("Client authentication cannot be enforced for public applications."); return false; } @@ -640,9 +644,10 @@ namespace OpenIddict.Core // Ensure the application type is supported by the manager. if (!string.Equals(descriptor.Type, OpenIddictConstants.ClientTypes.Confidential, StringComparison.OrdinalIgnoreCase) && + !string.Equals(descriptor.Type, OpenIddictConstants.ClientTypes.Hybrid, StringComparison.OrdinalIgnoreCase) && !string.Equals(descriptor.Type, OpenIddictConstants.ClientTypes.Public, StringComparison.OrdinalIgnoreCase)) { - throw new ArgumentException("Only 'confidential' or 'public' applications are " + + throw new ArgumentException("Only 'confidential', 'hybrid' or 'public' applications are " + "supported by the default application manager.", nameof(descriptor)); } diff --git a/src/OpenIddict.Core/OpenIddictConstants.cs b/src/OpenIddict.Core/OpenIddictConstants.cs index 65ad9ba7..0b3a02e0 100644 --- a/src/OpenIddict.Core/OpenIddictConstants.cs +++ b/src/OpenIddict.Core/OpenIddictConstants.cs @@ -16,6 +16,7 @@ namespace OpenIddict.Core public static class ClientTypes { public const string Confidential = "confidential"; + public const string Hybrid = "hybrid"; public const string Public = "public"; } diff --git a/src/OpenIddict/OpenIddictProvider.Authentication.cs b/src/OpenIddict/OpenIddictProvider.Authentication.cs index ad1422ca..9425c901 100644 --- a/src/OpenIddict/OpenIddictProvider.Authentication.cs +++ b/src/OpenIddict/OpenIddictProvider.Authentication.cs @@ -298,17 +298,17 @@ namespace OpenIddict return; } - // To prevent downgrade attacks, ensure that authorization requests returning an access token directly - // from the authorization endpoint are rejected if the client_id corresponds to a confidential application. + // To prevent downgrade attacks, ensure that authorization requests returning a token directly from + // the authorization endpoint are rejected if the client_id corresponds to a confidential application. // Note: when using the authorization code grant, ValidateTokenRequest is responsible of rejecting // the token request if the client_id corresponds to an unauthenticated confidential client. if (await applications.IsConfidentialAsync(application, context.HttpContext.RequestAborted) && - context.Request.HasResponseType(OpenIdConnectConstants.ResponseTypes.Token)) + (context.Request.HasResponseType(OpenIdConnectConstants.ResponseTypes.IdToken) || + context.Request.HasResponseType(OpenIdConnectConstants.ResponseTypes.Token))) { context.Reject( error: OpenIdConnectConstants.Errors.InvalidRequest, - description: "Confidential clients are not allowed to retrieve " + - "an access token from the authorization endpoint."); + description: "Confidential clients are not allowed to retrieve a token from the authorization endpoint."); return; } diff --git a/src/OpenIddict/OpenIddictProvider.Exchange.cs b/src/OpenIddict/OpenIddictProvider.Exchange.cs index e27bc180..948bd1da 100644 --- a/src/OpenIddict/OpenIddictProvider.Exchange.cs +++ b/src/OpenIddict/OpenIddictProvider.Exchange.cs @@ -102,6 +102,9 @@ namespace OpenIddict // Reject the request if client identification is mandatory. if (options.RequireClientIdentification) { + logger.LogError("The token request was rejected becaused the " + + "mandatory client_id parameter was missing or empty."); + context.Reject( error: OpenIdConnectConstants.Errors.InvalidRequest, description: "The mandatory 'client_id' parameter was missing."); @@ -159,8 +162,8 @@ namespace OpenIddict return; } - logger.LogInformation("The token request validation process was not fully validated because " + - "the client '{ClientId}' was a public application.", context.ClientId); + logger.LogDebug("The token request validation process was not fully validated because " + + "the client '{ClientId}' was a public application.", context.ClientId); // If client authentication cannot be enforced, call context.Skip() to inform // the OpenID Connect server middleware that the caller cannot be fully trusted. @@ -169,11 +172,11 @@ namespace OpenIddict return; } - // Confidential applications MUST authenticate + // Confidential and hybrid applications MUST authenticate // to protect them from impersonation attacks. if (string.IsNullOrEmpty(context.ClientSecret)) { - logger.LogError("The token request was rejected because the confidential application " + + logger.LogError("The token request was rejected because the confidential or hybrid application " + "'{ClientId}' didn't specify a client secret.", context.ClientId); context.Reject( @@ -185,7 +188,7 @@ namespace OpenIddict if (!await applications.ValidateClientSecretAsync(application, context.ClientSecret, context.HttpContext.RequestAborted)) { - logger.LogError("The token request was rejected because the confidential application " + + logger.LogError("The token request was rejected because the confidential or hybrid application " + "'{ClientId}' didn't specify valid client credentials.", context.ClientId); context.Reject( diff --git a/src/OpenIddict/OpenIddictProvider.Introspection.cs b/src/OpenIddict/OpenIddictProvider.Introspection.cs index 2dbb6706..b5042156 100644 --- a/src/OpenIddict/OpenIddictProvider.Introspection.cs +++ b/src/OpenIddict/OpenIddictProvider.Introspection.cs @@ -68,8 +68,8 @@ namespace OpenIddict return; } - // Reject non-confidential applications. - if (!await applications.IsConfidentialAsync(application, context.HttpContext.RequestAborted)) + // Reject introspection requests sent by public applications. + if (await applications.IsPublicAsync(application, context.HttpContext.RequestAborted)) { logger.LogError("The introspection request was rejected because the public application " + "'{ClientId}' was not allowed to use this endpoint.", context.ClientId); @@ -84,7 +84,7 @@ namespace OpenIddict // Validate the client credentials. if (!await applications.ValidateClientSecretAsync(application, context.ClientSecret, context.HttpContext.RequestAborted)) { - logger.LogError("The introspection request was rejected because the confidential application " + + logger.LogError("The introspection request was rejected because the confidential or hybrid application " + "'{ClientId}' didn't specify valid client credentials.", context.ClientId); context.Reject( diff --git a/src/OpenIddict/OpenIddictProvider.Revocation.cs b/src/OpenIddict/OpenIddictProvider.Revocation.cs index 0266c197..777ccbd5 100644 --- a/src/OpenIddict/OpenIddictProvider.Revocation.cs +++ b/src/OpenIddict/OpenIddictProvider.Revocation.cs @@ -73,8 +73,8 @@ namespace OpenIddict return; } - logger.LogInformation("The revocation request validation process was skipped " + - "because the client_id parameter was missing or empty."); + logger.LogDebug("The revocation request validation process was skipped " + + "because the client_id parameter was missing or empty."); context.Skip(); @@ -85,6 +85,9 @@ namespace OpenIddict var application = await applications.FindByClientIdAsync(context.ClientId, context.HttpContext.RequestAborted); if (application == null) { + logger.LogError("The revocation request was rejected because the client " + + "application was not found: '{ClientId}'.", context.ClientId); + context.Reject( error: OpenIdConnectConstants.Errors.InvalidClient, description: "Application not found in the database: ensure that your client_id is correct."); @@ -92,12 +95,15 @@ namespace OpenIddict return; } - // Reject revocation requests containing a client_secret if the client application is not confidential. + // Reject revocation requests containing a client_secret if the application is a public client. if (await applications.IsPublicAsync(application, context.HttpContext.RequestAborted)) { // Reject tokens requests containing a client_secret when the client is a public application. if (!string.IsNullOrEmpty(context.ClientSecret)) { + logger.LogError("The revocation request was rejected because the public application " + + "'{ClientId}' was not allowed to use this endpoint.", context.ClientId); + context.Reject( error: OpenIdConnectConstants.Errors.InvalidRequest, description: "Public clients are not allowed to send a client_secret."); @@ -105,8 +111,8 @@ namespace OpenIddict return; } - logger.LogInformation("The revocation request validation process was not fully validated because " + - "the client '{ClientId}' was a public application.", context.ClientId); + logger.LogDebug("The revocation request validation process was not fully validated because " + + "the client '{ClientId}' was a public application.", context.ClientId); // If client authentication cannot be enforced, call context.Skip() to inform // the OpenID Connect server middleware that the caller cannot be fully trusted. @@ -115,10 +121,13 @@ namespace OpenIddict return; } - // Confidential applications MUST authenticate + // Confidential and hybrid applications MUST authenticate // to protect them from impersonation attacks. if (string.IsNullOrEmpty(context.ClientSecret)) { + logger.LogError("The revocation request was rejected because the confidential or hybrid application " + + "'{ClientId}' didn't specify a client secret.", context.ClientId); + context.Reject( error: OpenIdConnectConstants.Errors.InvalidClient, description: "Missing credentials: ensure that you specified a client_secret."); @@ -128,6 +137,9 @@ namespace OpenIddict if (!await applications.ValidateClientSecretAsync(application, context.ClientSecret, context.HttpContext.RequestAborted)) { + logger.LogError("The revocation request was rejected because the confidential or hybrid application " + + "'{ClientId}' didn't specify valid client credentials.", context.ClientId); + context.Reject( error: OpenIdConnectConstants.Errors.InvalidClient, description: "Invalid credentials: ensure that you specified a correct client_secret."); diff --git a/test/OpenIddict.Tests/OpenIddictProviderTests.Authentication.cs b/test/OpenIddict.Tests/OpenIddictProviderTests.Authentication.cs index 91a1a292..a7cb5962 100644 --- a/test/OpenIddict.Tests/OpenIddictProviderTests.Authentication.cs +++ b/test/OpenIddict.Tests/OpenIddictProviderTests.Authentication.cs @@ -423,6 +423,7 @@ namespace OpenIddict.Tests [Theory] [InlineData("code id_token token")] [InlineData("code token")] + [InlineData("id_token")] [InlineData("id_token token")] [InlineData("token")] public async Task ValidateAuthorizationRequest_ImplicitOrHybridRequestIsRejectedWhenClientIsConfidential(string type) @@ -464,8 +465,7 @@ namespace OpenIddict.Tests // Assert Assert.Equal(OpenIdConnectConstants.Errors.InvalidRequest, response.Error); - Assert.Equal("Confidential clients are not allowed to retrieve " + - "an access token from the authorization endpoint.", response.ErrorDescription); + Assert.Equal("Confidential clients are not allowed to retrieve a token from the authorization endpoint.", response.ErrorDescription); Mock.Get(manager).Verify(mock => mock.FindByClientIdAsync("Fabrikam", It.IsAny()), Times.Once()); Mock.Get(manager).Verify(mock => mock.HasRedirectUriAsync(application, It.IsAny()), Times.Once()); diff --git a/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs b/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs index 6d73d928..dacb7958 100644 --- a/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs +++ b/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs @@ -313,6 +313,46 @@ namespace OpenIddict.Tests Mock.Get(manager).Verify(mock => mock.GetClientTypeAsync(application, It.IsAny()), Times.Once()); } + [Fact] + public async Task ValidateTokenRequest_ClientSecretIsRequiredForHybridClients() + { + // Arrange + var application = new OpenIddictApplication(); + + var manager = CreateApplicationManager(instance => + { + instance.Setup(mock => mock.FindByClientIdAsync("Fabrikam", It.IsAny())) + .ReturnsAsync(application); + + instance.Setup(mock => mock.GetClientTypeAsync(application, It.IsAny())) + .ReturnsAsync(OpenIddictConstants.ClientTypes.Hybrid); + }); + + var server = CreateAuthorizationServer(builder => + { + builder.Services.AddSingleton(manager); + }); + + var client = new OpenIdConnectClient(server.CreateClient()); + + // Act + var response = await client.PostAsync(TokenEndpoint, new OpenIdConnectRequest + { + ClientId = "Fabrikam", + ClientSecret = null, + GrantType = OpenIdConnectConstants.GrantTypes.Password, + Username = "johndoe", + Password = "A3ddj3w" + }); + + // Assert + Assert.Equal(OpenIdConnectConstants.Errors.InvalidClient, response.Error); + Assert.Equal("Missing credentials: ensure that you specified a client_secret.", 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_RequestIsRejectedWhenClientCredentialsAreInvalid() { diff --git a/test/OpenIddict.Tests/OpenIddictProviderTests.Revocation.cs b/test/OpenIddict.Tests/OpenIddictProviderTests.Revocation.cs index 0f1cda33..da6a3d46 100644 --- a/test/OpenIddict.Tests/OpenIddictProviderTests.Revocation.cs +++ b/test/OpenIddict.Tests/OpenIddictProviderTests.Revocation.cs @@ -195,6 +195,45 @@ namespace OpenIddict.Tests Mock.Get(manager).Verify(mock => mock.GetClientTypeAsync(application, It.IsAny()), Times.Once()); } + [Fact] + public async Task ValidateRevocationRequest_ClientSecretIsRequiredForHybridClients() + { + // Arrange + var application = new OpenIddictApplication(); + + var manager = CreateApplicationManager(instance => + { + instance.Setup(mock => mock.FindByClientIdAsync("Fabrikam", It.IsAny())) + .ReturnsAsync(application); + + instance.Setup(mock => mock.GetClientTypeAsync(application, It.IsAny())) + .ReturnsAsync(OpenIddictConstants.ClientTypes.Hybrid); + }); + + var server = CreateAuthorizationServer(builder => + { + builder.Services.AddSingleton(manager); + }); + + var client = new OpenIdConnectClient(server.CreateClient()); + + // Act + var response = await client.PostAsync(RevocationEndpoint, new OpenIdConnectRequest + { + ClientId = "Fabrikam", + ClientSecret = null, + Token = "SlAV32hkKG", + TokenTypeHint = OpenIdConnectConstants.TokenTypeHints.RefreshToken + }); + + // Assert + Assert.Equal(OpenIdConnectConstants.Errors.InvalidClient, response.Error); + Assert.Equal("Missing credentials: ensure that you specified a client_secret.", 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 ValidateRevocationRequest_RequestIsRejectedWhenClientCredentialsAreInvalid() {