Repository: gravitee-io/graviteeio-access-management (java)

Issue:
[oauth2] Client can ask for any scope, even if scope are not defined from domain settings

Pull request:
fix(oauth2): handle unknown requested scopes
fixes #337

Patch:
diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/request/AbstractRequestResolver.java b/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/request/AbstractRequestResolver.java
index d27f9535dd..60db72258f 100644
--- a/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/request/AbstractRequestResolver.java
+++ b/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/request/AbstractRequestResolver.java
@@ -46,7 +46,7 @@ public abstract class AbstractRequestResolver<R extends BaseRequest> {
         final List<String> clientScopes = client.getScopes();
         final Set<String> requestScopes = request.getScopes();
         Set<String> clientResolvedScopes = new HashSet<>();
-        Set<String> resolvedScopes = requestScopes == null ? new HashSet<>() : new HashSet<>(requestScopes);
+        Set<String> resolvedScopes = new HashSet<>();
         Set<String> invalidScopes = new HashSet<>();
         // client scopes
         if (clientScopes != null && !clientScopes.isEmpty()) {
@@ -57,9 +57,9 @@ public abstract class AbstractRequestResolver<R extends BaseRequest> {
                 // filter the actual scopes granted by the client
                 for (String scope : requestScopes) {
                     if (clientScopes.contains(scope)) {
+                        resolvedScopes.add(scope);
                         clientResolvedScopes.add(scope);
                     } else {
-                        resolvedScopes.remove(scope);
                         invalidScopes.add(scope);
                     }
                 }
@@ -98,8 +98,8 @@ public abstract class AbstractRequestResolver<R extends BaseRequest> {
             return Single.error(new InvalidScopeException("Invalid scope(s): " + invalidScopes.stream().collect(Collectors.joining(" "))));
         }
 
-        if (resolvedScopes == null || resolvedScopes.isEmpty()) {
-            return Single.error(new InvalidScopeException("Empty scope (either the client or the user is not allowed the requested scopes)"));
+        if (resolvedScopes.isEmpty() && (requestScopes != null && !requestScopes.isEmpty())) {
+            return Single.error(new InvalidScopeException("Invalid scope(s): " + requestScopes.stream().collect(Collectors.joining(" "))));
         }
 
         // set resolved scopes
diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/token/jackson/AccessTokenSerializer.java b/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/token/jackson/AccessTokenSerializer.java
index 0316519f85..88ee4e3961 100644
--- a/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/token/jackson/AccessTokenSerializer.java
+++ b/gravitee-am-gateway/gravitee-am-gateway-handler/src/main/java/io/gravitee/am/gateway/handler/oauth2/token/jackson/AccessTokenSerializer.java
@@ -39,7 +39,10 @@ public class AccessTokenSerializer extends StdSerializer<AccessToken> {
         jsonGenerator.writeStringField(AccessToken.ACCESS_TOKEN, token.getValue());
         jsonGenerator.writeStringField(AccessToken.TOKEN_TYPE, token.getTokenType());
         jsonGenerator.writeNumberField(AccessToken.EXPIRES_IN, token.getExpiresIn());
-        jsonGenerator.writeStringField(AccessToken.SCOPE, token.getScope());
+
+        if (token.getScope() != null) {
+            jsonGenerator.writeStringField(AccessToken.SCOPE, token.getScope());
+        }
 
         if (token.getRefreshToken() != null) {
             jsonGenerator.writeStringField(AccessToken.REFRESH_TOKEN, token.getRefreshToken());


Test patch:
diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/AuthorizationRequestResolverTest.java b/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/AuthorizationRequestResolverTest.java
index 563f9ce6db..cfa95ba182 100644
--- a/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/AuthorizationRequestResolverTest.java
+++ b/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/AuthorizationRequestResolverTest.java
@@ -37,7 +37,7 @@ public class AuthorizationRequestResolverTest {
     private final AuthorizationRequestResolver authorizationRequestResolver = new AuthorizationRequestResolver();
 
     @Test
-    public void shouldResolveAuthorizationRequest() {
+    public void shouldNotResolveAuthorizationRequest_unknownScope() {
         final String scope = "read";
         final String redirectUri = "http://localhost:8080/callback";
         AuthorizationRequest authorizationRequest = new AuthorizationRequest();
@@ -46,8 +46,8 @@ public class AuthorizationRequestResolverTest {
         Client client = new Client();
 
         TestObserver<AuthorizationRequest> testObserver = authorizationRequestResolver.resolve(authorizationRequest, client, null).test();
-        testObserver.assertComplete();
-        testObserver.assertNoErrors();
+        testObserver.assertNotComplete();
+        testObserver.assertError(InvalidScopeException.class);
     }
 
     @Test
@@ -88,8 +88,8 @@ public class AuthorizationRequestResolverTest {
         Client client = new Client();
 
         TestObserver<AuthorizationRequest> testObserver = authorizationRequestResolver.resolve(authorizationRequest, client, null).test();
-        testObserver.assertNotComplete();
-        testObserver.assertError(InvalidScopeException.class);
+        testObserver.assertComplete();
+        testObserver.assertNoErrors();
     }
 
     @Test
@@ -173,6 +173,7 @@ public class AuthorizationRequestResolverTest {
         authorizationRequest.setScopes(Collections.singleton(scope));
         authorizationRequest.setRedirectUri(redirectUri);
         Client client = new Client();
+        client.setScopes(Collections.singletonList(scope));
         client.setRedirectUris(Arrays.asList("http://localhost:8080/callback", "http://localhost:8080/callback2"));
 
         TestObserver<AuthorizationRequest> testObserver = authorizationRequestResolver.resolve(authorizationRequest, client, null).test();
@@ -188,6 +189,7 @@ public class AuthorizationRequestResolverTest {
         authorizationRequest.setScopes(Collections.singleton(scope));
         authorizationRequest.setRedirectUri(redirectUri);
         Client client = new Client();
+        client.setScopes(Collections.singletonList(scope));
         client.setRedirectUris(Arrays.asList("http://localhost:8080/allowRedirect", "http://localhost:8080/allowRedirect2"));
 
         TestObserver<AuthorizationRequest> testObserver = authorizationRequestResolver.resolve(authorizationRequest, client, null).test();
diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/TokenRequestResolverTest.java b/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/TokenRequestResolverTest.java
index 329f91fc06..588adf7d56 100644
--- a/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/TokenRequestResolverTest.java
+++ b/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/oauth2/request/TokenRequestResolverTest.java
@@ -35,15 +35,15 @@ public class TokenRequestResolverTest {
     private final TokenRequestResolver tokenRequestResolver = new TokenRequestResolver();
 
     @Test
-    public void shouldResolveTokenRequest() {
+    public void shouldNotResolveTokenRequest_unknownScope() {
         final String scope = "read";
         TokenRequest tokenRequest = new TokenRequest();
         tokenRequest.setScopes(Collections.singleton(scope));
         Client client = new Client();
 
         TestObserver<TokenRequest> testObserver = tokenRequestResolver.resolve(tokenRequest, client, null).test();
-        testObserver.assertComplete();
-        testObserver.assertNoErrors();
+        testObserver.assertNotComplete();
+        testObserver.assertError(InvalidScopeException.class);
     }
 
     @Test
@@ -67,13 +67,13 @@ public class TokenRequestResolverTest {
     }
 
     @Test
-    public void shouldNotResolveTokenRequest_emptyScope() {
+    public void shouldResolveTokenRequest_emptyScope() {
         TokenRequest tokenRequest = new TokenRequest();
         Client client = new Client();
 
         TestObserver<TokenRequest> testObserver = tokenRequestResolver.resolve(tokenRequest, client, null).test();
-        testObserver.assertNotComplete();
-        testObserver.assertError(InvalidScopeException.class);
+        testObserver.assertComplete();
+        testObserver.assertNoErrors();
     }
 
     @Test
diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/vertx/endpoint/AuthorizationEndpointHandlerTest.java b/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/vertx/endpoint/AuthorizationEndpointHandlerTest.java
index 738c163ce9..1c05400b49 100644
--- a/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/vertx/endpoint/AuthorizationEndpointHandlerTest.java
+++ b/gravitee-am-gateway/gravitee-am-gateway-handler/src/test/java/io/gravitee/am/gateway/handler/vertx/endpoint/AuthorizationEndpointHandlerTest.java
@@ -144,12 +144,12 @@ public class AuthorizationEndpointHandlerTest  extends RxWebTestBase {
 
         testRequest(
                 HttpMethod.GET,
-                "/oauth/authorize?response_type=code&client_id=client-id&redirect_uri=http://localhost:9999/callback",
+                "/oauth/authorize?response_type=code&client_id=client-id&redirect_uri=http://localhost:9999/callback&scope=unknown",
                 null,
                 resp -> {
                     String location = resp.headers().get("location");
                     assertNotNull(location);
-                    assertEquals("http://localhost:9999/callback?error=invalid_scope&error_description=Empty+scope+%2528either+the+client+or+the+user+is+not+allowed+the+requested+scopes%2529", location);
+                    assertEquals("http://localhost:9999/callback?error=invalid_scope&error_description=Invalid+scope%2528s%2529%253A+unknown", location);
                 },
                 HttpStatusCode.FOUND_302, "Found", null);
     }