Fix scope checking in refresh token flow

openid-connect-client/pom.xml

@@ -22,7 +22,7 @@
 	<parent>
 		<artifactId>openid-connect-parent</artifactId>
 		<groupId>org.mitre</groupId>
-		<version>1.3.5.cnaf.v20191003</version>
+		<version>1.3.5.cnaf.20200115</version>
 		<relativePath>..</relativePath>
 	</parent>
 	<artifactId>openid-connect-client</artifactId>

openid-connect-common/pom.xml

@@ -22,7 +22,7 @@
 	<parent>
 		<artifactId>openid-connect-parent</artifactId>
 		<groupId>org.mitre</groupId>
-		<version>1.3.5.cnaf.v20191003</version>
+		<version>1.3.5.cnaf.20200115</version>
 		<relativePath>..</relativePath>
 	</parent>
 	<artifactId>openid-connect-common</artifactId>

openid-connect-server/pom.xml

@@ -23,7 +23,7 @@
 	<parent>
 		<groupId>org.mitre</groupId>
 		<artifactId>openid-connect-parent</artifactId>
-		<version>1.3.5.cnaf.v20191003</version>
+		<version>1.3.5.cnaf.20200115</version>
 		<relativePath>..</relativePath>
 	</parent>
 	<build>

openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ProviderTokenService.java

@@ -29,7 +29,6 @@ import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.util.Collection;
 import java.util.Date;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.UUID;
@@ -66,6 +65,7 @@ import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
 
 import com.google.common.base.Strings;
+import com.google.common.collect.Sets;
 import com.nimbusds.jose.util.Base64URL;
 import com.nimbusds.jwt.JWTClaimsSet;
 import com.nimbusds.jwt.PlainJWT;
@@ -331,33 +331,52 @@ public class DefaultOAuth2ProviderTokenService implements OAuth2TokenEntityServi
 
 		OAuth2AccessTokenEntity token = new OAuth2AccessTokenEntity();
 		
-		// get the stored scopes from the authentication holder's authorization request; these are the scopes associated with the refresh token
+		Set<String> reservedScopes = scopeService.toStrings(scopeService.getReserved());
-		Set<String> refreshScopesRequested = new HashSet<>(refreshToken.getAuthenticationHolder().getAuthentication().getOAuth2Request().getScope());
-		Set<SystemScope> refreshScopes = scopeService.fromStrings(refreshScopesRequested);
-		// remove any of the special system scopes
-		refreshScopes = scopeService.removeReservedScopes(refreshScopes);
 
-		Set<String> scopeRequested = authRequest.getScope() == null ? new HashSet<String>() : new HashSet<>(authRequest.getScope());
+		// Scopes linked to the refresh token, i.e. authorized by the user
-		Set<SystemScope> scope = scopeService.fromStrings(scopeRequested);
+		Set<String> authorizedScopes = Sets.newHashSet(refreshToken.getAuthenticationHolder().getAuthentication().getOAuth2Request().getScope());
+		authorizedScopes.removeAll(reservedScopes);
 		
-		// remove any of the special system scopes
+		// Scopes requested in this refresh token flow
-		scope = scopeService.removeReservedScopes(scope);
+		Set<String> requestedScopes = Sets.newHashSet();
+		if (authRequest.getScope() != null) {
+		  requestedScopes.addAll(authRequest.getScope());
+		}
 		
-		if (scope != null && !scope.isEmpty()) {
+		requestedScopes.removeAll(reservedScopes);
-			// ensure a proper subset of scopes
+		
-			if (refreshScopes != null && refreshScopes.containsAll(scope)) {
+		if (!requestedScopes.isEmpty()) {
-				// set the scope of the new access token if requested
+		  // Check for upscoping
-				token.setScope(scopeService.toStrings(scope));
+		  if (scopeService.scopesMatch(authorizedScopes, requestedScopes)) {
+		    token.setScope(requestedScopes);
 		  } else {
 		    String errorMsg = "Up-scoping is not allowed.";
             logger.error(errorMsg);
             throw new InvalidScopeException(errorMsg);
 		  }
+		  
 		} else {
-			// otherwise inherit the scope of the refresh token (if it's there -- this can return a null scope set)
+		  // Preserve scopes linked to the original refresh token
-			token.setScope(scopeService.toStrings(refreshScopes));
+		  token.setScope(authorizedScopes);
 		}
 
+//		if (scope != null && !scope.isEmpty()) {
+//			// ensure a proper subset of scopes
+//		    // FIXME: ugly and inefficient translation to/from strings for no added value, just to work around
+//		    // a terribly designed API
+//			if (refreshScopes != null && scopeService.scopesMatch(scopeService.toStrings(refreshScopes), scopeService.toStrings(scope))) {
+//				// set the scope of the new access token if requested
+//				token.setScope(scopeService.toStrings(scope));
+//			} else {
+//				String errorMsg = "Up-scoping is not allowed.";
+//				logger.error(errorMsg);
+//				throw new InvalidScopeException(errorMsg);
+//			}
+//		} else {
+//			// otherwise inherit the scope of the refresh token (if it's there -- this can return a null scope set)
+//			token.setScope(scopeService.toStrings(refreshScopes));
+//		}
+
 		token.setClient(client);
 
 		if (client.getAccessTokenValiditySeconds() != null) {

openid-connect-server/src/test/java/org/mitre/oauth2/service/impl/TestDefaultOAuth2ProviderTokenService.java

@@ -17,6 +17,27 @@
  *******************************************************************************/
 package org.mitre.oauth2.service.impl;
 
+import static com.google.common.collect.Sets.newHashSet;
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.AdditionalAnswers.returnsFirstArg;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anySet;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
 import java.util.Date;
 import java.util.HashSet;
 import java.util.Set;
@@ -49,27 +70,6 @@ import org.springframework.security.oauth2.provider.OAuth2Request;
 import org.springframework.security.oauth2.provider.TokenRequest;
 import org.springframework.security.oauth2.provider.token.TokenEnhancer;
 
-import static com.google.common.collect.Sets.newHashSet;
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.not;
-import static org.hamcrest.CoreMatchers.notNullValue;
-import static org.hamcrest.CoreMatchers.nullValue;
-import static org.mockito.AdditionalAnswers.returnsFirstArg;
-import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.anySet;
-import static org.mockito.Matchers.anyString;
-import static org.mockito.Mockito.atLeastOnce;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.reset;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-
 /**
  * @author wkim
  *
@@ -125,7 +125,8 @@ public class TestDefaultOAuth2ProviderTokenService {
     reset(tokenRepository, authenticationHolderRepository, clientDetailsService, tokenEnhancer);
 
     authentication = Mockito.mock(OAuth2Authentication.class);
-		OAuth2Request clientAuth = new OAuth2Request(null, clientId, null, true, scope, null, null, null, null);
+    OAuth2Request clientAuth =
+        new OAuth2Request(null, clientId, null, true, scope, null, null, null, null);
     when(authentication.getOAuth2Request()).thenReturn(clientAuth);
 
     client = Mockito.mock(ClientDetailsEntity.class);
@@ -161,7 +162,8 @@ public class TestDefaultOAuth2ProviderTokenService {
     when(storedAuthHolder.getAuthentication()).thenReturn(storedAuthentication);
     when(storedAuthentication.getOAuth2Request()).thenReturn(storedAuthRequest);
 
-		when(authenticationHolderRepository.save(any(AuthenticationHolderEntity.class))).thenReturn(storedAuthHolder);
+    when(authenticationHolderRepository.save(any(AuthenticationHolderEntity.class)))
+      .thenReturn(storedAuthHolder);
 
     when(scopeService.fromStrings(anySet())).thenAnswer(new Answer<Set<SystemScope>>() {
       @Override
@@ -189,6 +191,16 @@ public class TestDefaultOAuth2ProviderTokenService {
       }
     });
 
+    when(scopeService.scopesMatch(anySet(), anySet())).thenAnswer(new Answer<Boolean>() {
+      @Override
+      public Boolean answer(InvocationOnMock invocation) throws Throwable {
+        Object[] args = invocation.getArguments();
+        Set<String> expected = (Set<String>) args[0];
+        Set<String> actual = (Set<String>) args[1];
+        return expected.containsAll(actual);
+      }
+    });
+    
     // we're not testing restricted or reserved scopes here, just pass through
     when(scopeService.removeReservedScopes(anySet())).then(returnsFirstArg());
     when(scopeService.removeRestrictedAndReservedScopes(anySet())).then(returnsFirstArg());
@@ -232,7 +244,8 @@ public class TestDefaultOAuth2ProviderTokenService {
 
     try {
       service.createAccessToken(null);
-			fail("Authentication parameter is null. Excpected a AuthenticationCredentialsNotFoundException.");
+      fail(
+          "Authentication parameter is null. Excpected a AuthenticationCredentialsNotFoundException.");
     } catch (AuthenticationCredentialsNotFoundException e) {
       assertThat(e, is(notNullValue()));
     }
@@ -280,13 +293,15 @@ public class TestDefaultOAuth2ProviderTokenService {
    */
   @Test
   public void createAccessToken_yesRefresh() {
-		OAuth2Request clientAuth = new OAuth2Request(null, clientId, null, true, newHashSet(SystemScopeService.OFFLINE_ACCESS), null, null, null, null);
+    OAuth2Request clientAuth = new OAuth2Request(null, clientId, null, true,
+        newHashSet(SystemScopeService.OFFLINE_ACCESS), null, null, null, null);
     when(authentication.getOAuth2Request()).thenReturn(clientAuth);
     when(client.isAllowRefresh()).thenReturn(true);
 
     OAuth2AccessTokenEntity token = service.createAccessToken(authentication);
 
-		// Note: a refactor may be appropriate to only save refresh tokens once to the repository during creation.
+    // Note: a refactor may be appropriate to only save refresh tokens once to the repository during
+    // creation.
     verify(tokenRepository, atLeastOnce()).saveRefreshToken(any(OAuth2RefreshTokenEntity.class));
     verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
 
@@ -294,7 +309,8 @@ public class TestDefaultOAuth2ProviderTokenService {
   }
 
   /**
-	 * Checks to see that the expiration date of new tokens is being set accurately to within some delta for time skew.
+   * Checks to see that the expiration date of new tokens is being set accurately to within some
+   * delta for time skew.
    */
   @Test
   public void createAccessToken_expiration() {
@@ -316,8 +332,10 @@ public class TestDefaultOAuth2ProviderTokenService {
 
     verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
 
-		assertTrue(token.getExpiration().after(lowerBoundAccessTokens) && token.getExpiration().before(upperBoundAccessTokens));
+    assertTrue(token.getExpiration().after(lowerBoundAccessTokens)
-		assertTrue(token.getRefreshToken().getExpiration().after(lowerBoundRefreshTokens) && token.getRefreshToken().getExpiration().before(upperBoundRefreshTokens));
+        && token.getExpiration().before(upperBoundAccessTokens));
+    assertTrue(token.getRefreshToken().getExpiration().after(lowerBoundRefreshTokens)
+        && token.getRefreshToken().getExpiration().before(upperBoundRefreshTokens));
   }
 
   @Test
@@ -343,7 +361,8 @@ public class TestDefaultOAuth2ProviderTokenService {
     AuthenticationHolderEntity authHolder = mock(AuthenticationHolderEntity.class);
     when(authHolder.getAuthentication()).thenReturn(authentication);
 
-		when(authenticationHolderRepository.save(any(AuthenticationHolderEntity.class))).thenReturn(authHolder);
+    when(authenticationHolderRepository.save(any(AuthenticationHolderEntity.class)))
+      .thenReturn(authHolder);
 
     OAuth2AccessTokenEntity token = service.createAccessToken(authentication);
 
@@ -392,7 +411,6 @@ public class TestDefaultOAuth2ProviderTokenService {
 
     verify(tokenEnhancer).enhance(token, storedAuthentication);
     verify(tokenRepository).saveAccessToken(token);
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
 
   }
 
@@ -411,7 +429,6 @@ public class TestDefaultOAuth2ProviderTokenService {
     verify(tokenEnhancer).enhance(token, storedAuthentication);
     verify(tokenRepository).saveAccessToken(token);
     verify(tokenRepository).removeRefreshToken(refreshToken);
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
 
   }
 
@@ -429,7 +446,6 @@ public class TestDefaultOAuth2ProviderTokenService {
 
     verify(tokenEnhancer).enhance(token, storedAuthentication);
     verify(tokenRepository).saveAccessToken(token);
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
 
   }
 
@@ -437,7 +453,6 @@ public class TestDefaultOAuth2ProviderTokenService {
   public void refreshAccessToken_requestingSameScope() {
     OAuth2AccessTokenEntity token = service.refreshAccessToken(refreshTokenValue, tokenRequest);
 
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
 
     assertThat(token.getScope(), equalTo(storedScope));
   }
@@ -450,8 +465,6 @@ public class TestDefaultOAuth2ProviderTokenService {
 
     OAuth2AccessTokenEntity token = service.refreshAccessToken(refreshTokenValue, tokenRequest);
 
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
-
     assertThat(token.getScope(), equalTo(lessScope));
   }
 
@@ -467,12 +480,13 @@ public class TestDefaultOAuth2ProviderTokenService {
   }
 
   /**
-	 * Tests the case where only some of the valid scope values are being requested along with
+   * Tests the case where only some of the valid scope values are being requested along with other
-	 * other extra unauthorized scope values.
+   * extra unauthorized scope values.
    */
   @Test(expected = InvalidScopeException.class)
   public void refreshAccessToken_requestingMixedScope() {
-		Set<String> mixedScope = newHashSet("openid", "profile", "address", "phone"); // no email or offline_access
+    Set<String> mixedScope = newHashSet("openid", "profile", "address", "phone"); // no email or
+                                                                                  // offline_access
 
     tokenRequest.setScope(mixedScope);
 
@@ -487,8 +501,6 @@ public class TestDefaultOAuth2ProviderTokenService {
 
     OAuth2AccessTokenEntity token = service.refreshAccessToken(refreshTokenValue, tokenRequest);
 
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
-
     assertThat(token.getScope(), equalTo(storedScope));
   }
 
@@ -498,14 +510,13 @@ public class TestDefaultOAuth2ProviderTokenService {
 
     OAuth2AccessTokenEntity token = service.refreshAccessToken(refreshTokenValue, tokenRequest);
 
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
-
     assertThat(token.getScope(), equalTo(storedScope));
 
   }
 
   /**
-	 * Checks to see that the expiration date of refreshed tokens is being set accurately to within some delta for time skew.
+   * Checks to see that the expiration date of refreshed tokens is being set accurately to within
+   * some delta for time skew.
    */
   @Test
   public void refreshAccessToken_expiration() {
@@ -521,9 +532,9 @@ public class TestDefaultOAuth2ProviderTokenService {
     Date lowerBoundAccessTokens = new Date(start + (accessTokenValiditySeconds * 1000L) - DELTA);
     Date upperBoundAccessTokens = new Date(end + (accessTokenValiditySeconds * 1000L) + DELTA);
 
-		verify(scopeService, atLeastOnce()).removeReservedScopes(anySet());
 
-		assertTrue(token.getExpiration().after(lowerBoundAccessTokens) && token.getExpiration().before(upperBoundAccessTokens));
+    assertTrue(token.getExpiration().after(lowerBoundAccessTokens)
+        && token.getExpiration().before(upperBoundAccessTokens));
   }
 
   @Test

pom.xml

@@ -20,7 +20,7 @@
 	<modelVersion>4.0.0</modelVersion>
 	<groupId>org.mitre</groupId>
 	<artifactId>openid-connect-parent</artifactId>
-	<version>1.3.5.cnaf.v20191003</version>
+	<version>1.3.5.cnaf.20200115</version>
 	<name>MITREid Connect</name>
 	<packaging>pom</packaging>
 	<parent>