Merge pull request #4243 from mposolda/KEYCLOAK-3316

KEYCLOAK-3316 Fixes for OAuth2 requests without 'scope=openid'

services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java

@@ -269,6 +269,12 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase {
     }
 
     private Response checkOIDCParams() {
+        // If request is not OIDC request, but pure OAuth2 request and response_type is just 'token', then 'nonce' is not mandatory
+        boolean isOIDCRequest = TokenUtil.isOIDCRequest(request.getScope());
+        if (!isOIDCRequest && parsedResponseType.toString().equals(OIDCResponseType.TOKEN)) {
+            return null;
+        }
+
         if (parsedResponseType.isImplicitOrHybridFlow() && request.getNonce() == null) {
             ServicesLogger.LOGGER.missingParameter(OIDCLoginProtocol.NONCE_PARAM);
             event.error(Errors.INVALID_REQUEST);
@@ -354,10 +360,12 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase {
 
     private void checkRedirectUri() {
         String redirectUriParam = request.getRedirectUriParam();
+        boolean isOIDCRequest = TokenUtil.isOIDCRequest(request.getScope());
 
         event.detail(Details.REDIRECT_URI, redirectUriParam);
 
-        redirectUri = RedirectUtils.verifyRedirectUri(uriInfo, redirectUriParam, realm, client);
+        // redirect_uri parameter is required per OpenID Connect, but optional per OAuth2
+        redirectUri = RedirectUtils.verifyRedirectUri(uriInfo, redirectUriParam, realm, client, isOIDCRequest);
         if (redirectUri == null) {
             event.error(Errors.INVALID_REDIRECT_URI);
             throw new ErrorPageException(session, Messages.INVALID_PARAMETER, OIDCLoginProtocol.REDIRECT_URI_PARAM);

services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java

@@ -26,6 +26,7 @@ import org.keycloak.services.Urls;
 
 import javax.ws.rs.core.UriInfo;
 import java.net.URI;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.Set;
 
@@ -38,12 +39,16 @@ public class RedirectUtils {
 
     public static String verifyRealmRedirectUri(UriInfo uriInfo, String redirectUri, RealmModel realm) {
         Set<String> validRedirects = getValidateRedirectUris(uriInfo, realm);
-        return verifyRedirectUri(uriInfo, null, redirectUri, realm, validRedirects);
+        return verifyRedirectUri(uriInfo, null, redirectUri, realm, validRedirects, true);
     }
 
     public static String verifyRedirectUri(UriInfo uriInfo, String redirectUri, RealmModel realm, ClientModel client) {
+        return verifyRedirectUri(uriInfo, redirectUri, realm, client, true);
+    }
+
+    public static String verifyRedirectUri(UriInfo uriInfo, String redirectUri, RealmModel realm, ClientModel client, boolean requireRedirectUri) {
         if (client != null)
-            return verifyRedirectUri(uriInfo, client.getRootUrl(), redirectUri, realm, client.getRedirectUris());
+            return verifyRedirectUri(uriInfo, client.getRootUrl(), redirectUri, realm, client.getRedirectUris(), requireRedirectUri);
         return null;
     }
 
@@ -69,10 +74,16 @@ public class RedirectUtils {
         return redirects;
     }
 
-    private static String verifyRedirectUri(UriInfo uriInfo, String rootUrl, String redirectUri, RealmModel realm, Set<String> validRedirects) {
+    private static String verifyRedirectUri(UriInfo uriInfo, String rootUrl, String redirectUri, RealmModel realm, Set<String> validRedirects, boolean requireRedirectUri) {
         if (redirectUri == null) {
-            logger.debug("No Redirect URI parameter specified");
+            if (!requireRedirectUri) {
-            return null;
+                redirectUri = getSingleValidRedirectUri(validRedirects);
+            }
+
+            if (redirectUri == null) {
+                logger.debug("No Redirect URI parameter specified");
+                return null;
+            }
         } else if (validRedirects.isEmpty()) {
             logger.debug("No Redirect URIs supplied");
             redirectUri = null;
@@ -149,4 +160,14 @@ public class RedirectUtils {
         return false;
     }
 
+    private static String getSingleValidRedirectUri(Collection<String> validRedirects) {
+        if (validRedirects.size() != 1) return null;
+        String validRedirect = validRedirects.iterator().next();
+        int idx = validRedirect.indexOf("/*");
+        if (idx > -1) {
+            validRedirect = validRedirect.substring(0, idx);
+        }
+        return validRedirect;
+    }
+
 }

services/src/main/java/org/keycloak/services/ServicesLogger.java

@@ -406,7 +406,7 @@ public interface ServicesLogger extends BasicLogger {
     void failedToCloseProviderSession(@Cause Throwable t);
 
     @LogMessage(level = WARN)
-    @Message(id=91, value="Request is missing scope 'openid' so it's not treated as OIDC, but just pure OAuth2 request. This can have impact in future versions (eg. removed IDToken from the Token Response)")
+    @Message(id=91, value="Request is missing scope 'openid' so it's not treated as OIDC, but just pure OAuth2 request.")
     @Once
     void oidcScopeMissing();
 

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java

@@ -102,7 +102,7 @@ public class OAuthClient {
 
     private String maxAge;
 
-    private String responseType = OAuth2Constants.CODE;
+    private String responseType;
 
     private String responseMode;
 
@@ -171,6 +171,8 @@ public class OAuthClient {
         clientSessionState = null;
         clientSessionHost = null;
         maxAge = null;
+        responseType = OAuth2Constants.CODE;
+        responseMode = null;
         nonce = null;
         request = null;
         requestUri = null;

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuth2OnlyTest.java

@@ -15,19 +15,25 @@
  * limitations under the License.
  */
 
-package org.keycloak.testsuite.oidc;
+package org.keycloak.testsuite.oauth;
 
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 
 import javax.ws.rs.core.UriBuilder;
 
+import org.hamcrest.Matchers;
 import org.jboss.arquillian.graphene.page.Page;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.keycloak.OAuth2Constants;
 import org.keycloak.events.Details;
+import org.keycloak.events.Errors;
+import org.keycloak.models.ClientModel;
 import org.keycloak.representations.AccessToken;
+import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.EventRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.testsuite.AbstractTestRealmKeycloakTest;
@@ -35,6 +41,7 @@ import org.keycloak.testsuite.ActionURIUtils;
 import org.keycloak.testsuite.Assert;
 import org.keycloak.testsuite.AssertEvents;
 import org.keycloak.testsuite.admin.AbstractAdminTest;
+import org.keycloak.testsuite.admin.ApiUtil;
 import org.keycloak.testsuite.pages.AccountUpdateProfilePage;
 import org.keycloak.testsuite.pages.AppPage;
 import org.keycloak.testsuite.pages.ErrorPage;
@@ -46,9 +53,11 @@ import org.keycloak.testsuite.util.OAuthClient;
 import static org.junit.Assert.assertEquals;
 
 /**
+ * Test for scenarios when 'scope=openid' is missing. Which means we have pure OAuth2 request (not OpenID Connect)
+ *
  * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
  */
-public class ScopeParameterTest extends AbstractTestRealmKeycloakTest {
+public class OAuth2OnlyTest extends AbstractTestRealmKeycloakTest {
 
     @Rule
     public AssertEvents events = new AssertEvents(this);
@@ -71,6 +80,18 @@ public class ScopeParameterTest extends AbstractTestRealmKeycloakTest {
 
     @Override
     public void configureTestRealm(RealmRepresentation testRealm) {
+        ClientRepresentation client = new ClientRepresentation();
+        client.setClientId("more-uris-client");
+        client.setEnabled(true);
+        client.setRedirectUris(Arrays.asList("http://localhost:8180/auth/realms/master/app/auth", "http://localhost:8180/foo"));
+        client.setBaseUrl("http://localhost:8180/auth/realms/master/app/auth");
+
+        testRealm.getClients().add(client);
+
+        ClientRepresentation testApp = testRealm.getClients().stream()
+                .filter(cl -> cl.getClientId().equals("test-app"))
+                .findFirst().get();
+        testApp.setImplicitFlowEnabled(true);
     }
 
     @Before
@@ -82,20 +103,13 @@ public class ScopeParameterTest extends AbstractTestRealmKeycloakTest {
          * will faile and the clientID will always be "sample-public-client
          * @see AccessTokenTest#testAuthorizationNegotiateHeaderIgnored()
          */
-        oauth.clientId("test-app");
+        oauth.init(adminClient, driver);
-        oauth.maxAge(null);
-    }
-
-    @Override
-    public void addTestRealms(List<RealmRepresentation> testRealms) {
-        RealmRepresentation realm = AbstractAdminTest.loadJson(getClass().getResourceAsStream("/testrealm.json"), RealmRepresentation.class);
-        testRealms.add(realm);
     }
 
 
     // If scope=openid is missing, IDToken won't be present
     @Test
-    public void testMissingScopeOpenid() {
+    public void testMissingIDToken() {
         String loginFormUrl = oauth.getLoginFormUrl();
         loginFormUrl = ActionURIUtils.removeQueryParamFromURI(loginFormUrl, OAuth2Constants.SCOPE);
 
@@ -139,4 +153,60 @@ public class ScopeParameterTest extends AbstractTestRealmKeycloakTest {
         Assert.assertEquals(accessToken.getPreferredUsername(), "test-user@localhost");
 
     }
+
+
+    // In OAuth2, it is allowed that redirect_uri is not mandatory as long as client has just 1 redirect_uri configured without wildcard
+    @Test
+    public void testMissingRedirectUri() throws Exception {
+        // OAuth2 login without redirect_uri. It will be allowed.
+        String loginFormUrl = oauth.getLoginFormUrl();
+        loginFormUrl = ActionURIUtils.removeQueryParamFromURI(loginFormUrl, OAuth2Constants.SCOPE);
+        loginFormUrl = ActionURIUtils.removeQueryParamFromURI(loginFormUrl, OAuth2Constants.REDIRECT_URI);
+
+        driver.navigate().to(loginFormUrl);
+        loginPage.assertCurrent();
+        oauth.fillLoginForm("test-user@localhost", "password");
+        events.expectLogin().assertEvent();
+
+        // Client 'more-uris-client' has 2 redirect uris. OAuth2 login without redirect_uri won't be allowed
+        oauth.clientId("more-uris-client");
+        loginFormUrl = oauth.getLoginFormUrl();
+        loginFormUrl = ActionURIUtils.removeQueryParamFromURI(loginFormUrl, OAuth2Constants.SCOPE);
+        loginFormUrl = ActionURIUtils.removeQueryParamFromURI(loginFormUrl, OAuth2Constants.REDIRECT_URI);
+
+        driver.navigate().to(loginFormUrl);
+        errorPage.assertCurrent();
+        Assert.assertEquals("Invalid parameter: redirect_uri", errorPage.getError());
+        events.expectLogin()
+                .error(Errors.INVALID_REDIRECT_URI)
+                .client("more-uris-client")
+                .user(Matchers.nullValue(String.class))
+                .session(Matchers.nullValue(String.class))
+                .removeDetail(Details.REDIRECT_URI)
+                .removeDetail(Details.CODE_ID)
+                .removeDetail(Details.CONSENT)
+                .assertEvent();
+    }
+
+
+    // In OAuth2 (when response_type=token and no scope=openid) we don't treat nonce parameter mandatory
+    @Test
+    public void testMissingNonceInOAuth2ImplicitFlow() throws Exception {
+        oauth.responseType("token");
+        oauth.nonce(null);
+        String loginFormUrl = oauth.getLoginFormUrl();
+        loginFormUrl = ActionURIUtils.removeQueryParamFromURI(loginFormUrl, OAuth2Constants.SCOPE);
+
+        driver.navigate().to(loginFormUrl);
+        loginPage.assertCurrent();
+        oauth.fillLoginForm("test-user@localhost", "password");
+        events.expectLogin().assertEvent();
+
+        OAuthClient.AuthorizationEndpointResponse response = new OAuthClient.AuthorizationEndpointResponse(oauth);
+        Assert.assertNull(response.getError());
+        Assert.assertNull(response.getCode());
+        Assert.assertNull(response.getIdToken());
+        Assert.assertNotNull(response.getAccessToken());
+    }
+
 }