From 4e309929b5fe3747e446df320ebe16384736d734 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Fri, 31 Jul 2020 10:37:43 +0200 Subject: [PATCH] Don't revoke the authorization when detecting an authorization code/refresh token replay --- .../OpenIddictServerHandlers.cs | 29 +-- ...enIddictServerIntegrationTests.Exchange.cs | 178 ------------------ 2 files changed, 8 insertions(+), 199 deletions(-) diff --git a/src/OpenIddict.Server/OpenIddictServerHandlers.cs b/src/OpenIddict.Server/OpenIddictServerHandlers.cs index cfa1c4b7..7ff7c1ea 100644 --- a/src/OpenIddict.Server/OpenIddictServerHandlers.cs +++ b/src/OpenIddict.Server/OpenIddictServerHandlers.cs @@ -798,18 +798,12 @@ namespace OpenIddict.Server /// public class ValidateTokenEntry : IOpenIddictServerHandler { - private readonly IOpenIddictAuthorizationManager _authorizationManager; private readonly IOpenIddictTokenManager _tokenManager; public ValidateTokenEntry() => throw new InvalidOperationException(SR.GetResourceString(SR.ID1015)); - public ValidateTokenEntry( - IOpenIddictAuthorizationManager authorizationManager, - IOpenIddictTokenManager tokenManager) - { - _authorizationManager = authorizationManager; - _tokenManager = tokenManager; - } + public ValidateTokenEntry(IOpenIddictTokenManager tokenManager) + => _tokenManager = tokenManager; /// /// Gets the default descriptor definition assigned to this handler. @@ -872,15 +866,16 @@ namespace OpenIddict.Server context.Request.IsRefreshTokenGrantType())) { // If the authorization code/device code/refresh token is already marked as redeemed, this may indicate - // that it was compromised. In this case, revoke the authorization and all the associated tokens. + // that it was compromised. In this case, revoke the entire chain of tokens associated with the authorization. + // Note: the authorization itself is not revoked to allow the legitimate client to start a new flow. // See https://tools.ietf.org/html/rfc6749#section-10.5 for more information. if (await _tokenManager.HasStatusAsync(token, Statuses.Redeemed)) { // First, mark the redeemed token submitted by the client as revoked. await _tokenManager.TryRevokeAsync(token); - // Then, try to revoke the authorization and the associated token entries. - await TryRevokeAuthorizationChainAsync(context.Principal.GetAuthorizationId()); + // Then, try to revoke the token entries associated with the authorization. + await TryRevokeChainAsync(context.Principal.GetAuthorizationId()); context.Logger.LogError(SR.GetResourceString(SR.ID7002), identifier); @@ -965,21 +960,13 @@ namespace OpenIddict.Server .SetTokenId(await _tokenManager.GetIdAsync(token)) .SetTokenType(await _tokenManager.GetTypeAsync(token)); - async ValueTask TryRevokeAuthorizationChainAsync(string? identifier) + async ValueTask TryRevokeChainAsync(string? identifier) { - if (context.Options.DisableAuthorizationStorage || string.IsNullOrEmpty(identifier)) + if (string.IsNullOrEmpty(identifier)) { return; } - // Then, try to revoke the authorization and the associated token entries. - - var authorization = await _authorizationManager.FindByIdAsync(identifier); - if (authorization != null) - { - await _authorizationManager.TryRevokeAsync(authorization); - } - await foreach (var token in _tokenManager.FindByAuthorizationIdAsync(identifier)) { // Don't change the status of the token used in the token request. diff --git a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs index 00e42fc9..3c4b6312 100644 --- a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs +++ b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs @@ -2353,184 +2353,6 @@ namespace OpenIddict.Server.IntegrationTests Mock.Get(manager).Verify(manager => manager.HasStatusAsync(token, Statuses.Redeemed, It.IsAny()), Times.Once()); } - [Fact] - public async Task HandleTokenRequest_RevokesAuthorizationWhenAuthorizationCodeIsAlreadyRedeemed() - { - // Arrange - var authorization = new OpenIddictAuthorization(); - - var manager = CreateAuthorizationManager(mock => - { - mock.Setup(manager => manager.FindByIdAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0", It.IsAny())) - .ReturnsAsync(authorization); - }); - - await using var server = await CreateServerAsync(options => - { - options.AddEventHandler(builder => - { - builder.UseInlineHandler(context => - { - Assert.Equal("SplxlOBeZQQYbYS6WxSbIA", context.Token); - Assert.Equal(TokenTypeHints.AuthorizationCode, context.TokenType); - - context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) - .SetTokenType(TokenTypeHints.AuthorizationCode) - .SetPresenters("Fabrikam") - .SetTokenId("3E228451-1555-46F7-A471-951EFBA23A56") - .SetAuthorizationId("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0") - .SetClaim(Claims.Subject, "Bob le Bricoleur"); - - return default; - }); - - builder.SetOrder(ValidateIdentityModelToken.Descriptor.Order - 500); - }); - - options.AddEventHandler(builder => - builder.UseInlineHandler(context => - { - context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) - .SetClaim(Claims.Subject, "Bob le Magnifique"); - - return default; - })); - - options.Services.AddSingleton(CreateApplicationManager(mock => - { - var application = new OpenIddictApplication(); - - mock.Setup(manager => manager.FindByClientIdAsync("Fabrikam", It.IsAny())) - .ReturnsAsync(application); - - mock.Setup(manager => manager.HasClientTypeAsync(application, ClientTypes.Public, It.IsAny())) - .ReturnsAsync(true); - })); - - options.Services.AddSingleton(CreateTokenManager(mock => - { - var token = new OpenIddictToken(); - - mock.Setup(manager => manager.FindByIdAsync("3E228451-1555-46F7-A471-951EFBA23A56", It.IsAny())) - .ReturnsAsync(token); - - mock.Setup(manager => manager.GetIdAsync(token, It.IsAny())) - .ReturnsAsync("3E228451-1555-46F7-A471-951EFBA23A56"); - - mock.Setup(manager => manager.GetAuthorizationIdAsync(token, It.IsAny())) - .ReturnsAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0"); - - mock.Setup(manager => manager.HasStatusAsync(token, Statuses.Redeemed, It.IsAny())) - .ReturnsAsync(true); - - mock.Setup(manager => manager.FindByAuthorizationIdAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0", It.IsAny())) - .Returns(AsyncEnumerable.Empty()); - })); - - options.Services.AddSingleton(manager); - }); - - await using var client = await server.CreateClientAsync(); - - // Act - var response = await client.PostAsync("/connect/token", new OpenIddictRequest - { - ClientId = "Fabrikam", - Code = "SplxlOBeZQQYbYS6WxSbIA", - GrantType = GrantTypes.AuthorizationCode, - RedirectUri = "http://www.fabrikam.com/path" - }); - - // Assert - Assert.Equal(Errors.InvalidGrant, response.Error); - Assert.Equal(SR.GetResourceString(SR.ID3010), response.ErrorDescription); - - Mock.Get(manager).Verify(manager => manager.FindByIdAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0", It.IsAny()), Times.Once()); - Mock.Get(manager).Verify(manager => manager.TryRevokeAsync(authorization, It.IsAny()), Times.Once()); - } - - [Fact] - public async Task HandleTokenRequest_RevokesAuthorizationWhenRefreshTokenIsAlreadyRedeemed() - { - // Arrange - var authorization = new OpenIddictAuthorization(); - - var manager = CreateAuthorizationManager(mock => - { - mock.Setup(manager => manager.FindByIdAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0", It.IsAny())) - .ReturnsAsync(authorization); - }); - - await using var server = await CreateServerAsync(options => - { - options.AddEventHandler(builder => - { - builder.UseInlineHandler(context => - { - Assert.Equal("8xLOxBtZp8", context.Token); - Assert.Equal(TokenTypeHints.RefreshToken, context.TokenType); - - context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) - .SetTokenType(TokenTypeHints.RefreshToken) - .SetTokenId("60FFF7EA-F98E-437B-937E-5073CC313103") - .SetAuthorizationId("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0") - .SetClaim(Claims.Subject, "Bob le Bricoleur"); - - return default; - }); - - builder.SetOrder(ValidateIdentityModelToken.Descriptor.Order - 500); - }); - - options.AddEventHandler(builder => - builder.UseInlineHandler(context => - { - context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) - .SetClaim(Claims.Subject, "Bob le Magnifique"); - - return default; - })); - - options.Services.AddSingleton(CreateTokenManager(mock => - { - var token = new OpenIddictToken(); - - mock.Setup(manager => manager.FindByIdAsync("60FFF7EA-F98E-437B-937E-5073CC313103", It.IsAny())) - .ReturnsAsync(token); - - mock.Setup(manager => manager.GetIdAsync(token, It.IsAny())) - .ReturnsAsync("60FFF7EA-F98E-437B-937E-5073CC313103"); - - mock.Setup(manager => manager.GetAuthorizationIdAsync(token, It.IsAny())) - .ReturnsAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0"); - - mock.Setup(manager => manager.HasStatusAsync(token, Statuses.Redeemed, It.IsAny())) - .ReturnsAsync(true); - - mock.Setup(manager => manager.FindByAuthorizationIdAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0", It.IsAny())) - .Returns(AsyncEnumerable.Empty()); - })); - - options.Services.AddSingleton(manager); - }); - - await using var client = await server.CreateClientAsync(); - - // Act - var response = await client.PostAsync("/connect/token", new OpenIddictRequest - { - GrantType = GrantTypes.RefreshToken, - RefreshToken = "8xLOxBtZp8" - }); - - // Assert - Assert.Equal(Errors.InvalidGrant, response.Error); - Assert.Equal(SR.GetResourceString(SR.ID3012), response.ErrorDescription); - - Mock.Get(manager).Verify(manager => manager.FindByIdAsync("18D15F73-BE2B-6867-DC01-B3C1E8AFDED0", It.IsAny()), Times.Once()); - Mock.Get(manager).Verify(manager => manager.TryRevokeAsync(authorization, It.IsAny()), Times.Once()); - } - [Fact] public async Task HandleTokenRequest_RevokesTokensWhenAuthorizationCodeIsAlreadyRedeemed() {