Add a secret to the AccessToken

arteymix · arteymix · commit 43c0cd00daed · 2023-08-26T01:00:17.000-07:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<Role> 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<User> getRemoteSearchUser();
 
diff --git 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
@@ -0,0 +1,2 @@
+alter table access_token
+    add column secret varchar(255); -- may be null for pre-1.6 tokens
diff --git 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<User> 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
@@ -21,7 +21,7 @@ public void convert_withAuthorizationHeader() {
                 .buildRequest( servletContext );
         assertThat( new TokenBasedAuthenticationConverter().convert( request ) )
                 .isNotNull()
-                .hasFieldOrPropertyWithValue( "credentials", "1234" );
+                .hasFieldOrPropertyWithValue( "principal", "1234" );
     }
 
     @Test
@@ -40,7 +40,7 @@ public void convert_withAuthRequestParam() {
                 .buildRequest( servletContext );
         assertThat( new TokenBasedAuthenticationConverter().convert( request ) )
                 .isNotNull()
-                .hasFieldOrPropertyWithValue( "credentials", "1234" );
+                .hasFieldOrPropertyWithValue( "principal", "1234" );
     }
 
     @Test
diff --git 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 );
     }

Original file line number	Diff line number	Diff line change
`@@ -5,19 +5,21 @@`
`5`	`5`	`public class TokenBasedAuthentication extends AbstractAuthenticationToken {`
`6`	`6`
`7`	`7`	`private final String token;`
	`8`	`+ private final String secret;`
`8`	`9`
`9`		`- public TokenBasedAuthentication( String token ) {`
	`10`	`+ public TokenBasedAuthentication( String token, String secret ) {`
`10`	`11`	`super( null );`
`11`	`12`	`this.token = token;`
	`13`	`+ this.secret = secret;`
`12`	`14`	`}`
`13`	`15`
`14`	`16`	`@Override`
`15`	`17`	`public Object getCredentials() {`
`16`		`- return token;`
	`18`	`+ return secret;`
`17`	`19`	`}`
`18`	`20`
`19`	`21`	`@Override`
`20`	`22`	`public Object getPrincipal() {`
`21`		`- return null;`
	`23`	`+ return token;`
`22`	`24`	`}`
`23`	`25`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,7 @@ class TokenBasedAuthenticationConverter implements AuthenticationConverter {`
`16`	`16`	`public final TokenBasedAuthentication convert( HttpServletRequest request ) throws IllegalArgumentException {`
`17`	`17`	`String authorizationHeader = request.getHeader( "Authorization" );`
`18`	`18`	`String authToken = request.getParameter( "auth" );`
	`19`	`+ String secret;`
`19`	`20`	`if ( authToken == null && authorizationHeader != null ) {`
`20`	`21`	`String[] pieces = authorizationHeader.split( " ", 2 );`
`21`	`22`	`if ( pieces[0].equalsIgnoreCase( "Bearer" ) ) {`
`@@ -28,6 +29,13 @@ public final TokenBasedAuthentication convert( HttpServletRequest request ) thro`
`28`	`29`	`return null; /* unsupported authentication scheme */`
`29`	`30`	`}`
`30`	`31`	`}`
`31`		`- return authToken != null ? new TokenBasedAuthentication( authToken ) : null;`
	`32`	`+ if ( authToken != null && authToken.contains( ":" ) ) {`
	`33`	`+ String[] pieces = authToken.split( ":", 2 );`
	`34`	`+ authToken = pieces[0];`
	`35`	`+ secret = pieces[1];`
	`36`	`+ } else {`
	`37`	`+ secret = null;`
	`38`	`+ }`
	`39`	`+ return authToken != null ? new TokenBasedAuthentication( authToken, secret ) : null;`
`32`	`40`	`}`
`33`	`41`	`}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+alter table access_token`
	`2`	`+ add column secret varchar(255); -- may be null for pre-1.6 tokens`