From 1ddb66b89b2b2db2e7830f52823dcd9001435830 Mon Sep 17 00:00:00 2001 From: evgomes Date: Sun, 31 Jul 2022 22:24:20 -0300 Subject: [PATCH] Fixed security issue for refresh tokens. --- .../JWTAPI/Controllers/LoginController.cs | 4 +-- .../Resources/RevokeTokenResource.cs | 5 ++- .../Core/Security/Tokens/ITokenHandler.cs | 4 +-- .../Security/Tokens/RefreshTokenWithEmail.cs | 8 +++++ .../Core/Services/IAuthenticationService.cs | 2 +- .../JWTAPI/Security/Tokens/TokenHandler.cs | 29 +++++++++------ .../JWTAPI/Services/AuthenticationService.cs | 6 ++-- .../Security/Tokens/TokenHandlerTests.cs | 35 ++++++++++++++----- .../Services/AuthenticationServiceTests.cs | 10 +++--- 9 files changed, 69 insertions(+), 34 deletions(-) create mode 100644 src/JWTAPI/JWTAPI/Core/Security/Tokens/RefreshTokenWithEmail.cs diff --git a/src/JWTAPI/JWTAPI/Controllers/LoginController.cs b/src/JWTAPI/JWTAPI/Controllers/LoginController.cs index e15769b..38ed033 100644 --- a/src/JWTAPI/JWTAPI/Controllers/LoginController.cs +++ b/src/JWTAPI/JWTAPI/Controllers/LoginController.cs @@ -58,14 +58,14 @@ public async Task RefreshTokenAsync([FromBody] RefreshTokenResour [Route("/api/token/revoke")] [HttpPost] - public IActionResult RevokeToken([FromBody] RevokeTokenResource revokeTokenResource) + public IActionResult RevokeToken([FromBody] RevokeTokenResource resource) { if (!ModelState.IsValid) { return BadRequest(ModelState); } - _authenticationService.RevokeRefreshToken(revokeTokenResource.Token); + _authenticationService.RevokeRefreshToken(resource.Token, resource.Email); return NoContent(); } } diff --git a/src/JWTAPI/JWTAPI/Controllers/Resources/RevokeTokenResource.cs b/src/JWTAPI/JWTAPI/Controllers/Resources/RevokeTokenResource.cs index ac130d5..58e3067 100644 --- a/src/JWTAPI/JWTAPI/Controllers/Resources/RevokeTokenResource.cs +++ b/src/JWTAPI/JWTAPI/Controllers/Resources/RevokeTokenResource.cs @@ -6,5 +6,8 @@ public class RevokeTokenResource { [Required] public string Token { get; set; } - } + + [Required] + public string Email { get; set; } + } } \ No newline at end of file diff --git a/src/JWTAPI/JWTAPI/Core/Security/Tokens/ITokenHandler.cs b/src/JWTAPI/JWTAPI/Core/Security/Tokens/ITokenHandler.cs index 2f0bb71..deff843 100644 --- a/src/JWTAPI/JWTAPI/Core/Security/Tokens/ITokenHandler.cs +++ b/src/JWTAPI/JWTAPI/Core/Security/Tokens/ITokenHandler.cs @@ -5,7 +5,7 @@ namespace JWTAPI.Core.Security.Tokens public interface ITokenHandler { AccessToken CreateAccessToken(User user); - RefreshToken TakeRefreshToken(string token); - void RevokeRefreshToken(string token); + RefreshToken TakeRefreshToken(string token, string userEmail); + void RevokeRefreshToken(string token, string userEmail); } } \ No newline at end of file diff --git a/src/JWTAPI/JWTAPI/Core/Security/Tokens/RefreshTokenWithEmail.cs b/src/JWTAPI/JWTAPI/Core/Security/Tokens/RefreshTokenWithEmail.cs new file mode 100644 index 0000000..de0b779 --- /dev/null +++ b/src/JWTAPI/JWTAPI/Core/Security/Tokens/RefreshTokenWithEmail.cs @@ -0,0 +1,8 @@ +namespace JWTAPI.Core.Security.Tokens +{ + public class RefreshTokenWithEmail + { + public string Email { get; set; } + public RefreshToken RefreshToken { get; set; } + } +} diff --git a/src/JWTAPI/JWTAPI/Core/Services/IAuthenticationService.cs b/src/JWTAPI/JWTAPI/Core/Services/IAuthenticationService.cs index 5e69d1e..a3a99aa 100644 --- a/src/JWTAPI/JWTAPI/Core/Services/IAuthenticationService.cs +++ b/src/JWTAPI/JWTAPI/Core/Services/IAuthenticationService.cs @@ -6,6 +6,6 @@ public interface IAuthenticationService { Task CreateAccessTokenAsync(string email, string password); Task RefreshTokenAsync(string refreshToken, string userEmail); - void RevokeRefreshToken(string refreshToken); + void RevokeRefreshToken(string refreshToken, string userEmail); } } \ No newline at end of file diff --git a/src/JWTAPI/JWTAPI/Security/Tokens/TokenHandler.cs b/src/JWTAPI/JWTAPI/Security/Tokens/TokenHandler.cs index 3d90c92..86a328c 100644 --- a/src/JWTAPI/JWTAPI/Security/Tokens/TokenHandler.cs +++ b/src/JWTAPI/JWTAPI/Security/Tokens/TokenHandler.cs @@ -9,7 +9,7 @@ namespace JWTAPI.Security.Tokens { public class TokenHandler : ITokenHandler { - private readonly ISet _refreshTokens = new HashSet(); + private readonly ISet _refreshTokens = new HashSet(); private readonly TokenOptions _tokenOptions; private readonly SigningConfigurations _signingConfigurations; @@ -26,26 +26,33 @@ public AccessToken CreateAccessToken(User user) { var refreshToken = BuildRefreshToken(); var accessToken = BuildAccessToken(user, refreshToken); - _refreshTokens.Add(refreshToken); + _refreshTokens.Add(new RefreshTokenWithEmail + { + Email = user.Email, + RefreshToken = refreshToken, + }); return accessToken; } - public RefreshToken TakeRefreshToken(string token) + public RefreshToken TakeRefreshToken(string token, string userEmail) { - if (string.IsNullOrWhiteSpace(token)) + if (string.IsNullOrWhiteSpace(token) || string.IsNullOrWhiteSpace(userEmail)) return null; - var refreshToken = _refreshTokens.SingleOrDefault(t => t.Token == token); - if (refreshToken != null) - _refreshTokens.Remove(refreshToken); - - return refreshToken; + var refreshTokenWithEmail = _refreshTokens.SingleOrDefault(t => t.RefreshToken.Token == token && t.Email == userEmail); + if(refreshTokenWithEmail == null) + { + return null; + } + + _refreshTokens.Remove(refreshTokenWithEmail); + return refreshTokenWithEmail.RefreshToken; } - public void RevokeRefreshToken(string token) + public void RevokeRefreshToken(string token, string userEmail) { - TakeRefreshToken(token); + TakeRefreshToken(token, userEmail); } private RefreshToken BuildRefreshToken() diff --git a/src/JWTAPI/JWTAPI/Services/AuthenticationService.cs b/src/JWTAPI/JWTAPI/Services/AuthenticationService.cs index 1921bf8..e0bde64 100644 --- a/src/JWTAPI/JWTAPI/Services/AuthenticationService.cs +++ b/src/JWTAPI/JWTAPI/Services/AuthenticationService.cs @@ -34,7 +34,7 @@ public async Task CreateAccessTokenAsync(string email, string pas public async Task RefreshTokenAsync(string refreshToken, string userEmail) { - var token = _tokenHandler.TakeRefreshToken(refreshToken); + var token = _tokenHandler.TakeRefreshToken(refreshToken, userEmail); if (token == null) { @@ -56,9 +56,9 @@ public async Task RefreshTokenAsync(string refreshToken, string u return new TokenResponse(true, null, accessToken); } - public void RevokeRefreshToken(string refreshToken) + public void RevokeRefreshToken(string refreshToken, string userEmail) { - _tokenHandler.RevokeRefreshToken(refreshToken); + _tokenHandler.RevokeRefreshToken(refreshToken, userEmail); } } } \ No newline at end of file diff --git a/tests/JWTAPI.Tests/Security/Tokens/TokenHandlerTests.cs b/tests/JWTAPI.Tests/Security/Tokens/TokenHandlerTests.cs index 377db3a..1a805d9 100644 --- a/tests/JWTAPI.Tests/Security/Tokens/TokenHandlerTests.cs +++ b/tests/JWTAPI.Tests/Security/Tokens/TokenHandlerTests.cs @@ -79,7 +79,7 @@ public void Should_Create_Access_Token() public void Should_Take_Existing_Refresh_Token() { var accessToken = _tokenHandler.CreateAccessToken(_user); - var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token); + var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token, "test@test.com"); Assert.NotNull(refreshToken); Assert.Equal(accessToken.RefreshToken.Token, refreshToken.Token); @@ -87,16 +87,33 @@ public void Should_Take_Existing_Refresh_Token() } [Fact] - public void Should_Return_Null_For_Empty_Refresh_Token_When_Trying_To_Take() + public void Should_Return_Null_For_Empty_Refresh_Token_When_Trying_To_Take_Refresh_Token() { - var refreshToken = _tokenHandler.TakeRefreshToken(""); + var refreshToken = _tokenHandler.TakeRefreshToken(string.Empty, "test@test.com"); + Assert.Null(refreshToken); + } + + [Fact] + public void Should_Return_Null_For_Empty_Email_When_Trying_To_Take_Refresh_Token() + { + var accessToken = _tokenHandler.CreateAccessToken(_user); + var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token, string.Empty); + Assert.Null(refreshToken); } [Fact] - public void Should_Return_Null_For_Invalid_Refresh_Token_When_Trying_To_Take() + public void Should_Return_Null_For_Invalid_Refresh_Token_When_Trying_To_Take_Refresh_oken() { - var refreshToken = _tokenHandler.TakeRefreshToken("invalid_token"); + var refreshToken = _tokenHandler.TakeRefreshToken("invalid_token", "test@test.com"); + Assert.Null(refreshToken); + } + + [Fact] + public void Should_Return_Null_For_Invalid_Email_When_Trying_To_Take_Refresh_Token() + { + var accessToken = _tokenHandler.CreateAccessToken(_user); + var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token, "admin@admin.com"); Assert.Null(refreshToken); } @@ -104,8 +121,8 @@ public void Should_Return_Null_For_Invalid_Refresh_Token_When_Trying_To_Take() public void Should_Not_Take_Refresh_Token_That_Was_Already_Taken() { var accessToken = _tokenHandler.CreateAccessToken(_user); - var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token); - var refreshTokenSecondTime = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token); + var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token, "test@test.com"); + var refreshTokenSecondTime = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token, "test@test.com"); Assert.NotNull(refreshToken); Assert.Null(refreshTokenSecondTime); @@ -115,8 +132,8 @@ public void Should_Not_Take_Refresh_Token_That_Was_Already_Taken() public void Should_Revoke_Refresh_Token() { var accessToken = _tokenHandler.CreateAccessToken(_user); - _tokenHandler.RevokeRefreshToken(accessToken.RefreshToken.Token); - var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token); + _tokenHandler.RevokeRefreshToken(accessToken.RefreshToken.Token, "test@test.com"); + var refreshToken = _tokenHandler.TakeRefreshToken(accessToken.RefreshToken.Token, "test@test.com"); Assert.Null(refreshToken); } diff --git a/tests/JWTAPI.Tests/Services/AuthenticationServiceTests.cs b/tests/JWTAPI.Tests/Services/AuthenticationServiceTests.cs index b3f7230..9ac9b40 100644 --- a/tests/JWTAPI.Tests/Services/AuthenticationServiceTests.cs +++ b/tests/JWTAPI.Tests/Services/AuthenticationServiceTests.cs @@ -72,16 +72,16 @@ private void SetupMocks() ) ); - _tokenHandler.Setup(h => h.TakeRefreshToken("abc")) + _tokenHandler.Setup(h => h.TakeRefreshToken("abc", It.IsAny())) .Returns(new RefreshToken("abc", DateTime.UtcNow.AddSeconds(60).Ticks)); - _tokenHandler.Setup(h => h.TakeRefreshToken("expired")) + _tokenHandler.Setup(h => h.TakeRefreshToken("expired", It.IsAny())) .Returns(new RefreshToken("expired", DateTime.UtcNow.AddSeconds(-60).Ticks)); - _tokenHandler.Setup(h => h.TakeRefreshToken("invalid")) + _tokenHandler.Setup(h => h.TakeRefreshToken("invalid", It.IsAny())) .Returns(null); - _tokenHandler.Setup(h => h.RevokeRefreshToken("abc")) + _tokenHandler.Setup(h => h.RevokeRefreshToken("abc", It.IsAny())) .Callback(() => _calledRefreshToken = true); } @@ -153,7 +153,7 @@ public async Task Should_Not_Refresh_Token_For_Invalid_User_Data() [Fact] public void Should_Revoke_Refresh_Token() { - _authenticationService.RevokeRefreshToken("abc"); + _authenticationService.RevokeRefreshToken("abc", "test@test.com"); Assert.True(_calledRefreshToken); }