From 833dc0aa58b717578aca78621ef71b826341e83d Mon Sep 17 00:00:00 2001 From: "ADVENT\\cwhite" Date: Fri, 5 May 2017 15:59:33 -0400 Subject: [PATCH] Consistent expiration handling Fixes #1084 Flip expression in DateTimeExtensions.HasExpired() to read like HasExceeded(). Check expiration and require consent for expired consent. Also remove consent from store. Remove expiration check from DefaultGrantStore that was returning null and causing callers to return invalid error statuses rather than expired. Also allows expired consents, refresh tokens, and reference tokens to be removed from the store. Change reference token expiration check to use DateTimeExtensions.HasExceeded(). Remove DefaultGrantStore expired grant "should not load" tests. Add test for expired consent. Update Expired_Reference_Token test. --- .../Extensions/DateTimeExtensions.cs | 2 +- .../Services/DefaultConsentService.cs | 14 ++- .../Stores/Default/DefaultGrantStore.cs | 15 +--- .../Validation/TokenValidator.cs | 2 +- .../Default/DefaultConsentServiceTests.cs | 18 ++++ .../DefaultPersistedGrantStoreTests.cs | 89 ------------------- .../Validation/AccessTokenValidation.cs | 4 +- 7 files changed, 39 insertions(+), 105 deletions(-) diff --git a/src/IdentityServer4/Extensions/DateTimeExtensions.cs b/src/IdentityServer4/Extensions/DateTimeExtensions.cs index 8e8f632de..c851c5256 100644 --- a/src/IdentityServer4/Extensions/DateTimeExtensions.cs +++ b/src/IdentityServer4/Extensions/DateTimeExtensions.cs @@ -36,7 +36,7 @@ public static bool HasExpired(this DateTime? expirationTime) [DebuggerStepThrough] public static bool HasExpired(this DateTime expirationTime) { - if (expirationTime < IdentityServerDateTime.UtcNow) + if (IdentityServerDateTime.UtcNow > expirationTime) { return true; } diff --git a/src/IdentityServer4/Services/DefaultConsentService.cs b/src/IdentityServer4/Services/DefaultConsentService.cs index f15a11a6e..62f9c5bdf 100644 --- a/src/IdentityServer4/Services/DefaultConsentService.cs +++ b/src/IdentityServer4/Services/DefaultConsentService.cs @@ -75,7 +75,19 @@ public virtual async Task RequiresConsentAsync(ClaimsPrincipal subject, Cl } var consent = await _userConsentStore.GetUserConsentAsync(subject.GetSubjectId(), client.ClientId); - if (consent?.Scopes != null) + + if (consent == null) + { + return true; + } + + if (consent.Expiration.HasExpired()) + { + await _userConsentStore.RemoveUserConsentAsync(consent.SubjectId, consent.ClientId); + return true; + } + + if (consent.Scopes != null) { var intersect = scopes.Intersect(consent.Scopes); return !(scopes.Count() == intersect.Count()); diff --git a/src/IdentityServer4/Stores/Default/DefaultGrantStore.cs b/src/IdentityServer4/Stores/Default/DefaultGrantStore.cs index e138f8503..2a1b27e52 100644 --- a/src/IdentityServer4/Stores/Default/DefaultGrantStore.cs +++ b/src/IdentityServer4/Stores/Default/DefaultGrantStore.cs @@ -72,20 +72,13 @@ protected async Task GetItemAsync(string key) var grant = await _store.GetAsync(hashedKey); if (grant != null && grant.Type == _grantType) { - if (!grant.Expiration.HasExpired()) + try { - try - { - return _serializer.Deserialize(grant.Data); - } - catch (Exception ex) - { - _logger.LogError("Failed to deserailize JSON from grant store. Exception: {0}", ex.Message); - } + return _serializer.Deserialize(grant.Data); } - else + catch (Exception ex) { - _logger.LogDebug("{grantType} grant with value: {key} found in store, but has expired.", _grantType, key); + _logger.LogError("Failed to deserailize JSON from grant store. Exception: {0}", ex.Message); } } else diff --git a/src/IdentityServer4/Validation/TokenValidator.cs b/src/IdentityServer4/Validation/TokenValidator.cs index 19b7b8829..ebc5ed15d 100644 --- a/src/IdentityServer4/Validation/TokenValidator.cs +++ b/src/IdentityServer4/Validation/TokenValidator.cs @@ -277,7 +277,7 @@ private async Task ValidateReferenceAccessTokenAsync(stri return Invalid(OidcConstants.ProtectedResourceErrors.InvalidToken); } - if (IdentityServerDateTime.UtcNow >= token.CreationTime.AddSeconds(token.Lifetime)) + if (token.CreationTime.HasExceeded(token.Lifetime)) { LogError("Token expired."); diff --git a/test/IdentityServer.UnitTests/Services/Default/DefaultConsentServiceTests.cs b/test/IdentityServer.UnitTests/Services/Default/DefaultConsentServiceTests.cs index d89779b9a..9d425c059 100644 --- a/test/IdentityServer.UnitTests/Services/Default/DefaultConsentServiceTests.cs +++ b/test/IdentityServer.UnitTests/Services/Default/DefaultConsentServiceTests.cs @@ -8,6 +8,7 @@ using IdentityServer4.Models; using IdentityServer4.Services; using IdentityServer4.UnitTests.Common; +using System; using System.Linq; using System.Security.Claims; using System.Threading.Tasks; @@ -149,5 +150,22 @@ public async Task RequiresConsentAsync_prior_consent_with_too_few_scopes_should_ result.Should().BeTrue(); } + [Fact] + public async Task RequiresConsentAsync_expired_consent_should_require_consent() + { + var scopes = new string[] { "foo", "bar" }; + await _userConsentStore.StoreUserConsentAsync(new Consent() + { + ClientId = _client.ClientId, + SubjectId = _user.GetSubjectId(), + Scopes = scopes, + CreationTime = DateTime.UtcNow.AddHours(-1), + Expiration = DateTime.UtcNow.AddSeconds(-1) + }); + + var result = await _subject.RequiresConsentAsync(_user, _client, scopes); + + result.Should().BeTrue(); + } } } diff --git a/test/IdentityServer.UnitTests/Stores/Default/DefaultPersistedGrantStoreTests.cs b/test/IdentityServer.UnitTests/Stores/Default/DefaultPersistedGrantStoreTests.cs index 830f97b25..8226494c9 100644 --- a/test/IdentityServer.UnitTests/Stores/Default/DefaultPersistedGrantStoreTests.cs +++ b/test/IdentityServer.UnitTests/Stores/Default/DefaultPersistedGrantStoreTests.cs @@ -97,26 +97,6 @@ public async Task RemoveAuthorizationCodeAsync_should_remove_grant() code2.Should().BeNull(); } - [Fact] - public async Task expired_code_should_not_load() - { - var code1 = new AuthorizationCode() - { - ClientId = "test", - CreationTime = DateTime.UtcNow.AddHours(-1), - Lifetime = 10, - Subject = _user, - CodeChallenge = "challenge", - RedirectUri = "http://client/cb", - Nonce = "nonce", - RequestedScopes = new string[] { "scope1", "scope2" } - }; - var handle = await _codes.StoreAuthorizationCodeAsync(code1); - - var code2 = await _codes.GetAuthorizationCodeAsync(handle); - code2.Should().BeNull(); - } - [Fact] public async Task StoreRefreshTokenAsync_should_persist_grant() { @@ -183,34 +163,6 @@ public async Task RemoveRefreshTokenAsync_should_remove_grant() token2.Should().BeNull(); } - [Fact] - public async Task expired_refresh_token_should_not_load() - { - var token1 = new RefreshToken() - { - CreationTime = DateTime.UtcNow.AddHours(-1), - Lifetime = 10, - AccessToken = new Token - { - ClientId = "client", - Audiences = { "aud" }, - CreationTime = DateTime.UtcNow, - Type = "type", - Claims = new List - { - new Claim("sub", "123"), - new Claim("scope", "foo") - } - }, - Version = 1 - }; - - var handle = await _refreshTokens.StoreRefreshTokenAsync(token1); - var token2 = await _refreshTokens.GetRefreshTokenAsync(handle); - - token2.Should().BeNull(); - } - [Fact] public async Task RemoveRefreshTokenAsync_by_sub_and_client_should_remove_grant() { @@ -296,29 +248,6 @@ public async Task RemoveReferenceTokenAsync_should_remove_grant() token2.Should().BeNull(); } - [Fact] - public async Task expired_reference_token_should_not_load() - { - var token1 = new Token() - { - ClientId = "client", - Audiences = { "aud" }, - CreationTime = DateTime.UtcNow.AddHours(-1), - Type = "type", - Claims = new List - { - new Claim("sub", "123"), - new Claim("scope", "foo") - }, - Version = 1 - }; - - var handle = await _referenceTokens.StoreReferenceTokenAsync(token1); - - var token2 = await _referenceTokens.GetReferenceTokenAsync(handle); - token2.Should().BeNull(); - } - [Fact] public async Task RemoveReferenceTokenAsync_by_sub_and_client_should_remove_grant() { @@ -380,24 +309,6 @@ public async Task RemoveUserConsentAsync_should_remove_grant() consent2.Should().BeNull(); } - [Fact] - public async Task expired_user_consent_should_not_load() - { - var consent1 = new Consent() - { - ClientId = "client", - SubjectId = "123", - Scopes = new string[] { "foo", "bar" }, - CreationTime = DateTime.UtcNow.AddHours(-1), - Expiration = DateTime.UtcNow.AddSeconds(-1) - }; - - await _userConsent.StoreUserConsentAsync(consent1); - - var consent2 = await _userConsent.GetUserConsentAsync("123", "client"); - consent2.Should().BeNull(); - } - [Fact] public async Task same_key_for_different_grant_types_should_not_interfere_with_each_other() { diff --git a/test/IdentityServer.UnitTests/Validation/AccessTokenValidation.cs b/test/IdentityServer.UnitTests/Validation/AccessTokenValidation.cs index dc546adfe..3ca662326 100644 --- a/test/IdentityServer.UnitTests/Validation/AccessTokenValidation.cs +++ b/test/IdentityServer.UnitTests/Validation/AccessTokenValidation.cs @@ -143,8 +143,8 @@ public async Task Expired_Reference_Token() var token = TokenFactory.CreateAccessToken(new Client { ClientId = "roclient" }, "valid", 2, "read", "write"); var handle = await store.StoreReferenceTokenAsync(token); - - now = now.AddMilliseconds(2000); + + now = now.AddSeconds(3); var result = await validator.ValidateAccessTokenAsync(handle);