From 214c429fc4c7900c19b3a01a17fb775b99bd7895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Sun, 19 Mar 2017 17:28:02 +0100 Subject: [PATCH] Optimization: reject grant_type=authorization_code requests that don't specify a redirect_uri directly from ValidateTokenRequest --- src/OpenIddict/OpenIddictProvider.Exchange.cs | 34 ++++++++++--------- .../OpenIddictProviderTests.Exchange.cs | 32 +++++++++++++++-- 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/src/OpenIddict/OpenIddictProvider.Exchange.cs b/src/OpenIddict/OpenIddictProvider.Exchange.cs index 56c9471d..06bb3400 100644 --- a/src/OpenIddict/OpenIddictProvider.Exchange.cs +++ b/src/OpenIddict/OpenIddictProvider.Exchange.cs @@ -50,6 +50,20 @@ namespace OpenIddict return; } + // Optimization: the OpenID Connect server middleware automatically rejects grant_type=authorization_code + // requests missing the redirect_uri parameter if one was specified in the initial authorization request. + // Since OpenIddict doesn't allow redirect_uri-less authorization requests, an earlier check can be made here, + // which saves the OpenID Connect server middleware from having to deserialize the authorization code ticket. + // See http://openid.net/specs/openid-connect-core-1_0.html#TokenRequestValidation for more information. + if (context.Request.IsAuthorizationCodeGrantType() && string.IsNullOrEmpty(context.Request.RedirectUri)) + { + context.Reject( + error: OpenIdConnectConstants.Errors.InvalidRequest, + description: "The mandatory 'redirect_uri' parameter was missing."); + + return; + } + // Note: the OpenID Connect server middleware allows returning a refresh token with grant_type=client_credentials, // though it's usually not recommended by the OAuth2 specification. To encourage developers to make a new // grant_type=client_credentials request instead of using refresh tokens, OpenIddict uses a stricter policy @@ -65,14 +79,12 @@ namespace OpenIddict return; } - // Note: the OpenID Connect server middleware rejects grant_type=client_credentials requests - // when validation is skipped but an early check is made here to avoid making unnecessary + // Optimization: the OpenID Connect server middleware automatically rejects grant_type=client_credentials + // requests when validation is skipped but an earlier check is made here to avoid making unnecessary // database roundtrips to retrieve the client application corresponding to the client_id. if (context.Request.IsClientCredentialsGrantType() && (string.IsNullOrEmpty(context.Request.ClientId) || string.IsNullOrEmpty(context.Request.ClientSecret))) { - logger.LogError("The token request was rejected because the client credentials were missing."); - context.Reject( error: OpenIdConnectConstants.Errors.InvalidRequest, description: "Client applications must be authenticated to use the client credentials grant."); @@ -80,13 +92,6 @@ namespace OpenIddict return; } - // Note: though required by the OpenID Connect specification for the refresh token grant, - // client authentication is not mandatory for non-confidential client applications in OAuth2. - // To avoid breaking OAuth2 scenarios, OpenIddict uses a relaxed policy that allows - // public applications to use the refresh token grant without having to authenticate. - // See http://openid.net/specs/openid-connect-core-1_0.html#RefreshingAccessToken - // and https://tools.ietf.org/html/rfc6749#section-6 for more information. - // At this stage, skip client authentication if the client identifier is missing // or reject the token request if client identification is set as required. // Note: the OpenID Connect server middleware will automatically ensure that @@ -97,9 +102,6 @@ namespace OpenIddict // Reject the request if client identification is mandatory. if (options.Value.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."); @@ -107,8 +109,8 @@ namespace OpenIddict return; } - logger.LogInformation("The token request validation process was skipped " + - "because the client_id parameter was missing or empty."); + logger.LogDebug("The token request validation process was partially skipped " + + "because the 'client_id' parameter was missing or empty."); context.Skip(); diff --git a/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs b/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs index 6e7bb66c..b36feaee 100644 --- a/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs +++ b/test/OpenIddict.Tests/OpenIddictProviderTests.Exchange.cs @@ -74,6 +74,28 @@ namespace OpenIddict.Tests Assert.Equal("The 'offline_access' scope is not allowed.", response.ErrorDescription); } + [Fact] + public async Task ValidateTokenRequest_AuthorizationCodeRequestIsRejectedWhenRedirectUriIsMissing() + { + // Arrange + var server = CreateAuthorizationServer(); + + var client = new OpenIdConnectClient(server.CreateClient()); + + // Act + var response = await client.PostAsync(TokenEndpoint, new OpenIdConnectRequest + { + ClientId = "Fabrikam", + Code = "SplxlOBeZQQYbYS6WxSbIA", + GrantType = OpenIdConnectConstants.GrantTypes.AuthorizationCode, + RedirectUri = null + }); + + // Assert + Assert.Equal(OpenIdConnectConstants.Errors.InvalidRequest, response.Error); + Assert.Equal("The mandatory 'redirect_uri' parameter was missing.", response.ErrorDescription); + } + [Fact] public async Task ValidateTokenRequest_ClientCredentialsRequestWithOfflineAccessScopeIsRejected() { @@ -378,7 +400,8 @@ namespace OpenIddict.Tests { ClientId = "Fabrikam", Code = "SplxlOBeZQQYbYS6WxSbIA", - GrantType = OpenIdConnectConstants.GrantTypes.AuthorizationCode + GrantType = OpenIdConnectConstants.GrantTypes.AuthorizationCode, + RedirectUri = "http://www.fabrikam.com/path" }); // Assert @@ -483,7 +506,8 @@ namespace OpenIddict.Tests { ClientId = "Fabrikam", Code = "SplxlOBeZQQYbYS6WxSbIA", - GrantType = OpenIdConnectConstants.GrantTypes.AuthorizationCode + GrantType = OpenIdConnectConstants.GrantTypes.AuthorizationCode, + RedirectUri = "http://www.fabrikam.com/path" }); // Assert @@ -604,7 +628,8 @@ namespace OpenIddict.Tests { ClientId = "Fabrikam", Code = "SplxlOBeZQQYbYS6WxSbIA", - GrantType = OpenIdConnectConstants.GrantTypes.AuthorizationCode + GrantType = OpenIdConnectConstants.GrantTypes.AuthorizationCode, + RedirectUri = "http://www.fabrikam.com/path" }); // Assert @@ -749,6 +774,7 @@ namespace OpenIddict.Tests ClientSecret = "7Fjfp0ZBr1KtDRbnfVdmIw", Code = "8xLOxBtZp8", GrantType = flow, + RedirectUri = "http://www.fabrikam.com/path", RefreshToken = "8xLOxBtZp8", Username = "johndoe", Password = "A3ddj3w"