From 3e6ba6fd038febebaaa59396bd9ca9f80b32f6dc Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Fri, 25 Aug 2023 16:43:37 -0700 Subject: [PATCH] Add a secret to the AccessToken --- docs/api.md | 14 ++++++++----- .../rdp/controllers/AdminController.java | 14 ++++++++----- .../ubc/pavlab/rdp/model/AccessToken.java | 2 ++ .../TokenBasedAuthentication.java | 8 +++++--- .../TokenBasedAuthenticationConverter.java | 10 +++++++++- .../TokenBasedAuthenticationManager.java | 5 +++-- .../ubc/pavlab/rdp/services/UserService.java | 19 +++++++++++++++--- .../pavlab/rdp/services/UserServiceImpl.java | 17 ++++++++++------ .../V1.6.0__add_secret_to_access_token.sql | 2 ++ .../rdp/controllers/AdminControllerTest.java | 16 ++++++++++----- ...TokenBasedAuthenticationConverterTest.java | 20 +++++++++++++++---- .../TokenBasedAuthenticationManagerTest.java | 2 +- 12 files changed, 94 insertions(+), 35 deletions(-) create mode 100644 src/main/resources/db/migration/common/V1.6.0__add_secret_to_access_token.sql diff --git a/docs/api.md b/docs/api.md index 242068a3..0104b797 100644 --- a/docs/api.md +++ b/docs/api.md @@ -33,13 +33,16 @@ the [International data](customization.md#international-data) customization section. For external services, authentication should instead be performed with using a [service account](service-accounts.md) -with a secret access token. +with an access token and a secret. ```http GET /api/** HTTP/1.1 -Authorization: Bearer {accessToken} +Authorization: Bearer {accessToken}:{secret} ``` +Tokens created prior to the 1.6 release may omit the `:{secret}` part. We highly recommend that you revoke and +regenerate new tokens with secrets. + Passing the authorization token via `auth` query parameter is deprecated as of 1.4.0. ```http @@ -160,7 +163,6 @@ GET /api/ontologies/{ontologyName}/terms HTTP/1.1 - `page` the page to query starting from zero to `totalPages` - ## List specific terms in a category/ontology !!! note @@ -171,8 +173,10 @@ GET /api/ontologies/{ontologyName}/terms HTTP/1.1 GET /api/ontologies/{ontologyName}/terms?ontologyTermIds HTTP/1.1 ``` -To retrieve specific terms, you may use `ontologyTermIds` query parameter and pass it as many time as you want. The output is -not paginated and the `page` parameter from [List all terms in a category/ontology](#list-all-terms-in-a-categoryontology) is ignored. +To retrieve specific terms, you may use `ontologyTermIds` query parameter and pass it as many time as you want. The +output is +not paginated and the `page` parameter +from [List all terms in a category/ontology](#list-all-terms-in-a-categoryontology) is ignored. ## Retrieve a single category/ontology term diff --git a/src/main/java/ubc/pavlab/rdp/controllers/AdminController.java b/src/main/java/ubc/pavlab/rdp/controllers/AdminController.java index e311bd4b..9134553d 100644 --- a/src/main/java/ubc/pavlab/rdp/controllers/AdminController.java +++ b/src/main/java/ubc/pavlab/rdp/controllers/AdminController.java @@ -114,7 +114,7 @@ public ModelAndView viewCreateServiceAccount() { } @PostMapping(value = "/admin/create-service-account") - public Object createServiceAccount( @Validated(User.ValidationServiceAccount.class) User user, BindingResult bindingResult ) { + public Object createServiceAccount( @Validated(User.ValidationServiceAccount.class) User user, BindingResult bindingResult, RedirectAttributes redirectAttributes ) { String serviceEmail = user.getEmail() + '@' + siteSettings.getHostUrl().getHost(); if ( userService.findUserByEmailNoAuth( serviceEmail ) != null ) { @@ -136,9 +136,13 @@ public Object createServiceAccount( @Validated(User.ValidationServiceAccount.cla profile.setContactEmailVerified( false ); user.setProfile( profile ); - user = userService.createServiceAccount( user ); + UserService.ServiceAccountAndAccessToken userAndAccessToken = userService.createServiceAccount( user ); - return "redirect:/admin/users/" + user.getId(); + redirectAttributes.addFlashAttribute( "message", String.format( "Created service account with token %s and secret %s.", + userAndAccessToken.getAccessTokenWithRawSecret().getAccessToken().getToken(), + userAndAccessToken.getAccessTokenWithRawSecret().getRawSecret() ) ); + + return "redirect:/admin/users/" + userAndAccessToken.getUser().getId(); } @PostMapping(value = "/admin/users/{user}/roles") @@ -184,8 +188,8 @@ public Object createAccessTokenForUser( @PathVariable(required = false) User use return new ModelAndView( "error/404", HttpStatus.NOT_FOUND ) .addObject( "message", messageSource.getMessage( "AdminController.userNotFoundById", null, locale ) ); } - AccessToken accessToken = userService.createAccessTokenForUser( user ); - redirectAttributes.addFlashAttribute( "message", MessageFormat.format( "Successfully created an access token {0}.", accessToken.getToken() ) ); + UserService.AccessTokenWithRawSecret accessToken = userService.createAccessTokenForUser( user ); + redirectAttributes.addFlashAttribute( "message", MessageFormat.format( "Successfully created an access token {0} with secret {1}.", accessToken.getAccessToken().getToken(), accessToken.getRawSecret() ) ); return "redirect:/admin/users/" + user.getId(); } diff --git a/src/main/java/ubc/pavlab/rdp/model/AccessToken.java b/src/main/java/ubc/pavlab/rdp/model/AccessToken.java index 875fafd6..1803cc74 100644 --- a/src/main/java/ubc/pavlab/rdp/model/AccessToken.java +++ b/src/main/java/ubc/pavlab/rdp/model/AccessToken.java @@ -28,6 +28,8 @@ public class AccessToken extends Token implements UserContent { @JoinColumn(name = "user_id", nullable = false) private User user; + private String secret; + @CreatedDate private Instant createdAt; diff --git a/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthentication.java b/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthentication.java index 6fa7ba1b..6af34b2f 100644 --- a/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthentication.java +++ b/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthentication.java @@ -5,19 +5,21 @@ public class TokenBasedAuthentication extends AbstractAuthenticationToken { private final String token; + private final String secret; - public TokenBasedAuthentication( String token ) { + public TokenBasedAuthentication( String token, String secret ) { super( null ); this.token = token; + this.secret = secret; } @Override public Object getCredentials() { - return token; + return secret; } @Override public Object getPrincipal() { - return null; + return token; } } diff --git a/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverter.java b/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverter.java index 19086cc7..6fe85c1b 100644 --- a/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverter.java +++ b/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverter.java @@ -16,6 +16,7 @@ class TokenBasedAuthenticationConverter implements AuthenticationConverter { public final TokenBasedAuthentication convert( HttpServletRequest request ) throws IllegalArgumentException { String authorizationHeader = request.getHeader( "Authorization" ); String authToken = request.getParameter( "auth" ); + String secret; if ( authToken == null && authorizationHeader != null ) { String[] pieces = authorizationHeader.split( " ", 2 ); if ( pieces[0].equalsIgnoreCase( "Bearer" ) ) { @@ -28,6 +29,13 @@ public final TokenBasedAuthentication convert( HttpServletRequest request ) thro return null; /* unsupported authentication scheme */ } } - return authToken != null ? new TokenBasedAuthentication( authToken ) : null; + if ( authToken != null && authToken.contains( ":" ) ) { + String[] pieces = authToken.split( ":", 2 ); + authToken = pieces[0]; + secret = pieces[1]; + } else { + secret = null; + } + return authToken != null ? new TokenBasedAuthentication( authToken, secret ) : null; } } diff --git a/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManager.java b/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManager.java index 2702858e..7e63752a 100644 --- a/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManager.java +++ b/src/main/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManager.java @@ -31,7 +31,8 @@ public TokenBasedAuthenticationManager( UserService userService, ApplicationSett @Override public Authentication authenticate( Authentication authentication ) throws AuthenticationException { - String authToken = (String) authentication.getCredentials(); + String authToken = (String) authentication.getPrincipal(); + String secret = (String) authentication.getCredentials(); User u; if ( applicationSettings.getIsearch().getAuthTokens().contains( authToken ) ) { // remote admin authentication @@ -42,7 +43,7 @@ public Authentication authenticate( Authentication authentication ) throws Authe } else { // authentication via access token try { - u = userService.findUserByAccessTokenNoAuth( authToken ); + u = userService.findUserByAccessTokenNoAuth( authToken, secret ); } catch ( ExpiredTokenException e ) { throw new CredentialsExpiredException( "API token is expired.", e ); } catch ( TokenException e ) { diff --git a/src/main/java/ubc/pavlab/rdp/services/UserService.java b/src/main/java/ubc/pavlab/rdp/services/UserService.java index 60d6fadf..24817c4a 100644 --- a/src/main/java/ubc/pavlab/rdp/services/UserService.java +++ b/src/main/java/ubc/pavlab/rdp/services/UserService.java @@ -1,5 +1,6 @@ package ubc.pavlab.rdp.services; +import lombok.Value; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.security.authentication.BadCredentialsException; @@ -26,7 +27,13 @@ public interface UserService { User createAdmin( User admin ); - User createServiceAccount( User user ); + ServiceAccountAndAccessToken createServiceAccount( User user ); + + @Value + class ServiceAccountAndAccessToken { + User user; + AccessTokenWithRawSecret accessTokenWithRawSecret; + } List findAllRoles(); @@ -61,7 +68,7 @@ public interface UserService { User findUserByEmailNoAuth( String email ); - User findUserByAccessTokenNoAuth( String accessToken ) throws TokenException; + User findUserByAccessTokenNoAuth( String accessToken, String secret ) throws TokenException; /** * Anonymize the given user. @@ -113,7 +120,13 @@ public interface UserService { void revokeAccessToken( AccessToken accessToken ); - AccessToken createAccessTokenForUser( User user ); + AccessTokenWithRawSecret createAccessTokenForUser( User user ); + + @Value + class AccessTokenWithRawSecret { + AccessToken accessToken; + String rawSecret; + } Optional getRemoteSearchUser(); diff --git a/src/main/java/ubc/pavlab/rdp/services/UserServiceImpl.java b/src/main/java/ubc/pavlab/rdp/services/UserServiceImpl.java index f3d5a24f..4c122af8 100644 --- a/src/main/java/ubc/pavlab/rdp/services/UserServiceImpl.java +++ b/src/main/java/ubc/pavlab/rdp/services/UserServiceImpl.java @@ -143,13 +143,13 @@ public User createAdmin( User admin ) { @Override @Secured("ROLE_ADMIN") @Transactional - public User createServiceAccount( User user ) { + public ServiceAccountAndAccessToken createServiceAccount( User user ) { user.setPassword( bCryptPasswordEncoder.encode( createSecureRandomToken() ) ); Role serviceAccountRole = roleRepository.findByRole( "ROLE_SERVICE_ACCOUNT" ); user.getRoles().add( serviceAccountRole ); user = userRepository.save( user ); - createAccessTokenForUser( user ); - return user; + AccessTokenWithRawSecret accessToken = createAccessTokenForUser( user ); + return new ServiceAccountAndAccessToken( user, accessToken ); } @Override @@ -320,7 +320,7 @@ public User findUserByEmailNoAuth( String email ) { } @Override - public User findUserByAccessTokenNoAuth( String accessToken ) throws TokenException { + public User findUserByAccessTokenNoAuth( String accessToken, String secret ) throws TokenException { AccessToken token = accessTokenRepository.findByToken( accessToken ); if ( token == null ) { return null; @@ -329,6 +329,9 @@ public User findUserByAccessTokenNoAuth( String accessToken ) throws TokenExcept // token is expired throw new ExpiredTokenException( "Token is expired." ); } + if ( token.getSecret() != null && !bCryptPasswordEncoder.matches( secret, token.getSecret() ) ) { + throw new TokenException( "Provided secret does not match the token." ); + } return token.getUser(); } @@ -386,11 +389,13 @@ public void revokeAccessToken( AccessToken accessToken ) { } @Override - public AccessToken createAccessTokenForUser( User user ) { + public AccessTokenWithRawSecret createAccessTokenForUser( User user ) { AccessToken token = new AccessToken(); token.updateToken( createSecureRandomToken() ); token.setUser( user ); - return accessTokenRepository.save( token ); + String secret = createSecureRandomToken(); + token.setSecret( bCryptPasswordEncoder.encode( secret ) ); + return new AccessTokenWithRawSecret( accessTokenRepository.save( token ), secret ); } @Override diff --git a/src/main/resources/db/migration/common/V1.6.0__add_secret_to_access_token.sql b/src/main/resources/db/migration/common/V1.6.0__add_secret_to_access_token.sql new file mode 100644 index 00000000..4925e82f --- /dev/null +++ b/src/main/resources/db/migration/common/V1.6.0__add_secret_to_access_token.sql @@ -0,0 +1,2 @@ +alter table access_token + add column secret varchar(255); -- may be null for pre-1.6 tokens \ No newline at end of file diff --git a/src/test/java/ubc/pavlab/rdp/controllers/AdminControllerTest.java b/src/test/java/ubc/pavlab/rdp/controllers/AdminControllerTest.java index bca4cea6..8f28bdb7 100644 --- a/src/test/java/ubc/pavlab/rdp/controllers/AdminControllerTest.java +++ b/src/test/java/ubc/pavlab/rdp/controllers/AdminControllerTest.java @@ -27,7 +27,6 @@ import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.test.context.support.WithMockUser; import org.springframework.test.context.TestPropertySource; -import org.springframework.test.context.junit4.SpringRunner; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; import org.springframework.util.LinkedMultiValueMap; @@ -181,13 +180,17 @@ public void givenLoggedIn_whenCreateServiceAccount_thenRedirect3xx() throws Exce when( userService.createServiceAccount( any() ) ).thenAnswer( answer -> { User createdUser = answer.getArgument( 0, User.class ); createdUser.setId( 1 ); - return createdUser; + AccessToken accessToken = new AccessToken(); + accessToken.setToken( "1234" ); + return new UserService.ServiceAccountAndAccessToken( createdUser, new UserService.AccessTokenWithRawSecret( accessToken, "5678" ) ); } ); mvc.perform( post( "/admin/create-service-account" ) .param( "profile.name", "Service Account" ) .param( "email", "service-account" ) ) .andExpect( status().is3xxRedirection() ) - .andExpect( redirectedUrl( "/admin/users/1" ) ); + .andExpect( redirectedUrl( "/admin/users/1" ) ) + .andExpect( flash().attribute( "message", containsString( "1234" ) ) ) + .andExpect( flash().attribute( "message", containsString( "5678" ) ) ); ArgumentCaptor captor = ArgumentCaptor.forClass( User.class ); verify( userService ).createServiceAccount( captor.capture() ); assertThat( captor.getValue() ) @@ -200,12 +203,15 @@ public void givenLoggedIn_whenCreateServiceAccount_thenRedirect3xx() throws Exce public void givenLoggedIn_whenCreateAccessToken_thenRedirect3xx() throws Exception { User user = createUser( 1 ); AccessToken accessToken = TestUtils.createAccessToken( 1, user, "1234" ); + UserService.AccessTokenWithRawSecret accessTokenWithSecret = new UserService.AccessTokenWithRawSecret( accessToken, "5678" ); when( userService.findUserById( 1 ) ).thenReturn( user ); - when( userService.createAccessTokenForUser( user ) ).thenReturn( accessToken ); + when( userService.createAccessTokenForUser( user ) ).thenReturn( accessTokenWithSecret ); when( roleRepository.findByRole( "ROLE_USER" ) ).thenReturn( createRole( 1, "ROLE_USER" ) ); mvc.perform( post( "/admin/users/{user}/create-access-token", user.getId() ) ) .andExpect( status().is3xxRedirection() ) - .andExpect( redirectedUrl( "/admin/users/1" ) ); + .andExpect( redirectedUrl( "/admin/users/1" ) ) + .andExpect( flash().attribute( "message", containsString( "1234" ) ) ) + .andExpect( flash().attribute( "message", containsString( "5678" ) ) ); verify( userService ).createAccessTokenForUser( user ); } diff --git a/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverterTest.java b/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverterTest.java index e86dab9c..6e1dd40f 100644 --- a/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverterTest.java +++ b/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationConverterTest.java @@ -16,18 +16,30 @@ public class TokenBasedAuthenticationConverterTest { @Test public void convert_withAuthorizationHeader() { + MockHttpServletRequest request = request( HttpMethod.GET, "/" ) + .header( "Authorization", "Bearer 1234:5678" ) + .buildRequest( servletContext ); + assertThat( new TokenBasedAuthenticationConverter().convert( request ) ) + .isNotNull() + .hasFieldOrPropertyWithValue( "principal", "1234" ) + .hasFieldOrPropertyWithValue( "credentials", "5678" ); + } + + @Test + public void convert_withAuthorizationHeaderWithoutSecret() { MockHttpServletRequest request = request( HttpMethod.GET, "/" ) .header( "Authorization", "Bearer 1234" ) .buildRequest( servletContext ); assertThat( new TokenBasedAuthenticationConverter().convert( request ) ) .isNotNull() - .hasFieldOrPropertyWithValue( "credentials", "1234" ); + .hasFieldOrPropertyWithValue( "principal", "1234" ) + .hasFieldOrPropertyWithValue( "credentials", null ); } @Test public void convert_withUnsupportedAuthenticationScheme_thenIgnoreAndReturnNull() { MockHttpServletRequest request = request( HttpMethod.GET, "/" ) - .header( "Authorization", "Basic 1234" ) + .header( "Authorization", "Basic 1234:5678" ) .buildRequest( servletContext ); assertThat( new TokenBasedAuthenticationConverter().convert( request ) ) .isNull(); @@ -40,11 +52,11 @@ public void convert_withAuthRequestParam() { .buildRequest( servletContext ); assertThat( new TokenBasedAuthenticationConverter().convert( request ) ) .isNotNull() - .hasFieldOrPropertyWithValue( "credentials", "1234" ); + .hasFieldOrPropertyWithValue( "principal", "1234" ); } @Test - public void convert_whenCredentialsAreMissing_thenReturnNull() { + public void convert_whenPrincipalIsMissing_thenReturnNull() { MockHttpServletRequest request = request( HttpMethod.GET, "/" ) .buildRequest( servletContext ); assertThat( new TokenBasedAuthenticationConverter().convert( request ) ) diff --git a/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManagerTest.java b/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManagerTest.java index deeb567e..fd2c07d2 100644 --- a/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManagerTest.java +++ b/src/test/java/ubc/pavlab/rdp/security/authentication/TokenBasedAuthenticationManagerTest.java @@ -73,7 +73,7 @@ public void authenticate_whenRemoteUserIsRemovedAtRuntime() { when( userService.getRemoteSearchUser() ).thenReturn( Optional.empty() ); Authentication auth = mock( Authentication.class ); - when( auth.getCredentials() ).thenReturn( "test" ); + when( auth.getPrincipal() ).thenReturn( "test" ); assertThatThrownBy( () -> manager.authenticate( auth ) ) .isInstanceOf( InternalAuthenticationServiceException.class ); }