From 021482749224b243b71c65a20314bb51f58d55db Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Tue, 11 Mar 2014 20:03:38 +0000 Subject: [PATCH] KEYCLOAK-364 Show access denied if social login is cancelled --- .../java/org/keycloak/login/LoginForms.java | 2 + .../freemarker/FreeMarkerLoginForms.java | 8 +++- .../services/resources/SocialResource.java | 38 ++++++++++++------- .../social/AbstractOAuth2Provider.java | 14 ++++--- .../social/SocialAccessDeniedException.java | 7 ++++ .../org/keycloak/social/SocialProvider.java | 2 - .../social/SocialProviderException.java | 3 ++ .../social/twitter/TwitterProvider.java | 10 ++--- .../org/keycloak/testsuite/DummySocial.java | 11 +++--- .../testsuite/DummySocialServlet.java | 4 +- .../keycloak/testsuite/pages/LoginPage.java | 8 ++++ .../testsuite/social/SocialLoginTest.java | 21 ++++------ 12 files changed, 80 insertions(+), 48 deletions(-) create mode 100644 social/core/src/main/java/org/keycloak/social/SocialAccessDeniedException.java diff --git a/forms/login-api/src/main/java/org/keycloak/login/LoginForms.java b/forms/login-api/src/main/java/org/keycloak/login/LoginForms.java index 2eb370418e..a02df450b9 100755 --- a/forms/login-api/src/main/java/org/keycloak/login/LoginForms.java +++ b/forms/login-api/src/main/java/org/keycloak/login/LoginForms.java @@ -43,6 +43,8 @@ public interface LoginForms { public LoginForms setClient(ClientModel client); + public LoginForms setQueryParams(MultivaluedMap queryParams); + public LoginForms setFormData(MultivaluedMap formData); public LoginForms setStatus(Response.Status status); diff --git a/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/FreeMarkerLoginForms.java b/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/FreeMarkerLoginForms.java index 69e17fcfd7..a91aaec3b6 100755 --- a/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/FreeMarkerLoginForms.java +++ b/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/FreeMarkerLoginForms.java @@ -51,6 +51,7 @@ public class FreeMarkerLoginForms implements LoginForms { private Response.Status status = Response.Status.OK; private List realmRolesRequested; private MultivaluedMap resourceRolesRequested; + private MultivaluedMap queryParams; public static enum MessageType {SUCCESS, WARNING, ERROR} @@ -114,7 +115,7 @@ public class FreeMarkerLoginForms implements LoginForms { } private Response createResponse(LoginFormsPages page) { - MultivaluedMap queryParameterMap = uriInfo.getQueryParameters(); + MultivaluedMap queryParameterMap = queryParams != null ? queryParams : uriInfo.getQueryParameters(); String requestURI = uriInfo.getBaseUri().getPath(); UriBuilder uriBuilder = UriBuilder.fromUri(requestURI); @@ -276,4 +277,9 @@ public class FreeMarkerLoginForms implements LoginForms { return this; } + @Override + public LoginForms setQueryParams(MultivaluedMap queryParams) { + this.queryParams = queryParams; + return this; + } } diff --git a/services/src/main/java/org/keycloak/services/resources/SocialResource.java b/services/src/main/java/org/keycloak/services/resources/SocialResource.java index 4566c2ec75..d8ff9b1cc4 100755 --- a/services/src/main/java/org/keycloak/services/resources/SocialResource.java +++ b/services/src/main/java/org/keycloak/services/resources/SocialResource.java @@ -23,7 +23,6 @@ package org.keycloak.services.resources; import org.jboss.resteasy.logging.Logger; import org.jboss.resteasy.spi.HttpRequest; -import org.jboss.resteasy.spi.HttpResponse; import org.keycloak.models.AccountRoles; import org.keycloak.models.ClientModel; import org.keycloak.models.Constants; @@ -32,22 +31,20 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.SocialLinkModel; import org.keycloak.models.UserModel; import org.keycloak.models.utils.KeycloakModelUtils; -import org.keycloak.services.managers.AppAuthManager; -import org.keycloak.services.managers.Auth; import org.keycloak.services.managers.AuthenticationManager; import org.keycloak.services.managers.RealmManager; +import org.keycloak.services.managers.SocialRequestManager; import org.keycloak.services.managers.TokenManager; import org.keycloak.services.resources.flows.Flows; import org.keycloak.services.resources.flows.OAuthFlows; import org.keycloak.services.resources.flows.Urls; import org.keycloak.social.AuthCallback; -import org.keycloak.social.AuthRequest; import org.keycloak.social.RequestDetails; +import org.keycloak.social.SocialAccessDeniedException; import org.keycloak.social.SocialLoader; import org.keycloak.social.SocialProvider; import org.keycloak.social.SocialProviderConfig; import org.keycloak.social.SocialProviderException; -import org.keycloak.services.managers.SocialRequestManager; import org.keycloak.social.SocialUser; import javax.ws.rs.GET; @@ -57,6 +54,7 @@ import javax.ws.rs.QueryParam; import javax.ws.rs.container.ResourceContext; import javax.ws.rs.core.Context; import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.MultivaluedHashMap; import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; import javax.ws.rs.core.UriBuilder; @@ -141,6 +139,14 @@ public class SocialResource { SocialUser socialUser; try { socialUser = provider.processCallback(config, callback); + } catch (SocialAccessDeniedException e) { + MultivaluedHashMap queryParms = new MultivaluedHashMap(); + queryParms.putSingle("client_id", requestData.getClientAttribute("clientId")); + queryParms.putSingle("state", requestData.getClientAttribute("state")); + queryParms.putSingle("scope", requestData.getClientAttribute("scope")); + queryParms.putSingle("redirect_uri", requestData.getClientAttribute("redirectUri")); + queryParms.putSingle("response_type", requestData.getClientAttribute("responseType")); + return Flows.forms(realm, request, uriInfo).setQueryParams(queryParms).setWarning("Access denied").createLogin(); } catch (SocialProviderException e) { logger.warn("Failed to process social callback", e); return oauth.forwardToSecurityFailure("Failed to process social callback"); @@ -210,7 +216,7 @@ public class SocialResource { public Response redirectToProviderAuth(@PathParam("realm") final String realmName, @QueryParam("provider_id") final String providerId, @QueryParam("client_id") final String clientId, @QueryParam("scope") final String scope, @QueryParam("state") final String state, - @QueryParam("redirect_uri") String redirectUri) { + @QueryParam("redirect_uri") String redirectUri, @QueryParam("response_type") String responseType) { RealmManager realmManager = new RealmManager(session); RealmModel realm = realmManager.getRealmByName(realmName); @@ -239,20 +245,24 @@ public class SocialResource { .putClientAttribute("realm", realmName) .putClientAttribute("clientId", clientId).putClientAttribute("scope", scope) .putClientAttribute("state", state).putClientAttribute("redirectUri", redirectUri) - .redirectToSocialProvider(); + .putClientAttribute("responseType", responseType).redirectToSocialProvider(); } catch (Throwable t) { return Flows.forms(realm, request, uriInfo).setError("Failed to redirect to social auth").createErrorPage(); } } private RequestDetails getRequestDetails(Map queryParams) { - for (SocialProvider provider : SocialLoader.load()) { - if (queryParams.containsKey(provider.getRequestIdParamName())) { - String requestId = queryParams.get(provider.getRequestIdParamName())[0]; - if (socialRequestManager.isRequestId(requestId)) { - return socialRequestManager.retrieveData(requestId); - } - } + String requestId = null; + if (queryParams.containsKey("state")) { + requestId = queryParams.get("state")[0]; + } else if (queryParams.containsKey("oauth_token")) { + requestId = queryParams.get("oauth_token")[0]; + } else if (queryParams.containsKey("denied")) { + requestId = queryParams.get("denied")[0]; + } + + if (requestId != null && socialRequestManager.isRequestId(requestId)) { + return socialRequestManager.retrieveData(requestId); } return null; diff --git a/social/core/src/main/java/org/keycloak/social/AbstractOAuth2Provider.java b/social/core/src/main/java/org/keycloak/social/AbstractOAuth2Provider.java index ae8a0a5d49..7c6d2d706a 100644 --- a/social/core/src/main/java/org/keycloak/social/AbstractOAuth2Provider.java +++ b/social/core/src/main/java/org/keycloak/social/AbstractOAuth2Provider.java @@ -51,6 +51,15 @@ public abstract class AbstractOAuth2Provider implements SocialProvider { @Override public SocialUser processCallback(SocialProviderConfig config, AuthCallback callback) throws SocialProviderException { + String error = callback.getQueryParam("error"); + if (error != null) { + if (error.equals("access_denied")) { + throw new SocialAccessDeniedException(); + } else { + throw new SocialProviderException(error); + } + } + try { String code = callback.getQueryParam(CODE); @@ -82,9 +91,4 @@ public abstract class AbstractOAuth2Provider implements SocialProvider { } } - @Override - public String getRequestIdParamName() { - return STATE; - } - } diff --git a/social/core/src/main/java/org/keycloak/social/SocialAccessDeniedException.java b/social/core/src/main/java/org/keycloak/social/SocialAccessDeniedException.java new file mode 100644 index 0000000000..6f9d0c7bd1 --- /dev/null +++ b/social/core/src/main/java/org/keycloak/social/SocialAccessDeniedException.java @@ -0,0 +1,7 @@ +package org.keycloak.social; + +/** + * @author Stian Thorgersen + */ +public class SocialAccessDeniedException extends SocialProviderException { +} diff --git a/social/core/src/main/java/org/keycloak/social/SocialProvider.java b/social/core/src/main/java/org/keycloak/social/SocialProvider.java index 70da76de24..73a8f21d93 100644 --- a/social/core/src/main/java/org/keycloak/social/SocialProvider.java +++ b/social/core/src/main/java/org/keycloak/social/SocialProvider.java @@ -31,8 +31,6 @@ public interface SocialProvider { AuthRequest getAuthUrl(SocialProviderConfig config) throws SocialProviderException; - String getRequestIdParamName(); - String getName(); SocialUser processCallback(SocialProviderConfig config, AuthCallback callback) throws SocialProviderException; diff --git a/social/core/src/main/java/org/keycloak/social/SocialProviderException.java b/social/core/src/main/java/org/keycloak/social/SocialProviderException.java index 98cd2c35c9..5697046f7e 100644 --- a/social/core/src/main/java/org/keycloak/social/SocialProviderException.java +++ b/social/core/src/main/java/org/keycloak/social/SocialProviderException.java @@ -28,6 +28,9 @@ public class SocialProviderException extends Exception { private static final long serialVersionUID = 1L; + protected SocialProviderException() { + } + public SocialProviderException(String message) { super(message); } diff --git a/social/twitter/src/main/java/org/keycloak/social/twitter/TwitterProvider.java b/social/twitter/src/main/java/org/keycloak/social/twitter/TwitterProvider.java index 7842ff40c1..10df5cda46 100755 --- a/social/twitter/src/main/java/org/keycloak/social/twitter/TwitterProvider.java +++ b/social/twitter/src/main/java/org/keycloak/social/twitter/TwitterProvider.java @@ -23,6 +23,7 @@ package org.keycloak.social.twitter; import org.keycloak.social.AuthCallback; import org.keycloak.social.AuthRequest; +import org.keycloak.social.SocialAccessDeniedException; import org.keycloak.social.SocialProvider; import org.keycloak.social.SocialProviderConfig; import org.keycloak.social.SocialProviderException; @@ -67,6 +68,10 @@ public class TwitterProvider implements SocialProvider { @Override public SocialUser processCallback(SocialProviderConfig config, AuthCallback callback) throws SocialProviderException { + if (callback.getQueryParam("denied") != null) { + throw new SocialAccessDeniedException(); + } + try { Twitter twitter = new TwitterFactory().getInstance(); twitter.setOAuthConsumer(config.getKey(), config.getSecret()); @@ -86,9 +91,4 @@ public class TwitterProvider implements SocialProvider { } } - @Override - public String getRequestIdParamName() { - return "oauth_token"; - } - } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocial.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocial.java index 71a3563e02..5ad7cf6c54 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocial.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocial.java @@ -2,6 +2,7 @@ package org.keycloak.testsuite; import org.keycloak.social.AuthCallback; import org.keycloak.social.AuthRequest; +import org.keycloak.social.SocialAccessDeniedException; import org.keycloak.social.SocialProvider; import org.keycloak.social.SocialProviderConfig; import org.keycloak.social.SocialProviderException; @@ -26,11 +27,6 @@ public class DummySocial implements SocialProvider { .setQueryParam("redirect_uri", config.getCallbackUrl()).setQueryParam("state", state).setAttribute("state", state).build(); } - @Override - public String getRequestIdParamName() { - return "state"; - } - @Override public String getName() { return "Dummy Provider"; @@ -38,6 +34,11 @@ public class DummySocial implements SocialProvider { @Override public SocialUser processCallback(SocialProviderConfig config, AuthCallback callback) throws SocialProviderException { + String error = callback.getQueryParam("error"); + if (error != null) { + throw new SocialAccessDeniedException(); + } + if (!callback.getQueryParam("state").equals(callback.getAttribute("state"))) { throw new SocialProviderException("Invalid state"); } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocialServlet.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocialServlet.java index cc93a39bb0..e87ce1d47c 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocialServlet.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/DummySocialServlet.java @@ -27,8 +27,8 @@ public class DummySocialServlet extends HttpServlet { pw.print(""); pw.print(""); pw.print(""); - pw.print(""); - pw.print(""); + pw.print(""); + pw.print(""); pw.print(""); pw.print(""); pw.print(""); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginPage.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginPage.java index 3671ae0b63..a266b8a0c2 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginPage.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginPage.java @@ -62,6 +62,9 @@ public class LoginPage extends AbstractPage { @FindBy(className = "feedback-error") private WebElement loginErrorMessage; + @FindBy(className = "feedback-warning") + private WebElement loginWarningMessage; + public void login(String username, String password) { usernameInput.clear(); usernameInput.sendKeys(username); @@ -80,6 +83,11 @@ public class LoginPage extends AbstractPage { return loginErrorMessage != null ? loginErrorMessage.getText() : null; } + public String getWarning() { + return loginWarningMessage != null ? loginWarningMessage.getText() : null; + } + + public boolean isCurrent() { return driver.getTitle().equals("Log in to test"); } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java index 01d136ff74..18c4281588 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/social/SocialLoginTest.java @@ -102,7 +102,7 @@ public class SocialLoginTest { driver.findElement(By.id("firstname")).sendKeys("Bob"); driver.findElement(By.id("lastname")).sendKeys("Builder"); driver.findElement(By.id("email")).sendKeys("bob@builder.com"); - driver.findElement(By.id("submit")).click(); + driver.findElement(By.id("login")).click(); Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); @@ -128,19 +128,12 @@ public class SocialLoginTest { driver.findElement(By.id("cancel")).click(); + Assert.assertTrue(loginPage.isCurrent()); + Assert.assertEquals("Access denied", loginPage.getWarning()); + + loginPage.login("test-user@localhost", "password"); + Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); - - AccessTokenResponse response = oauth.doAccessTokenRequest(oauth.getCurrentQuery().get("code"), "password"); - - AccessToken token = oauth.verifyToken(response.getAccessToken()); - Assert.assertEquals(36, token.getSubject().length()); - - UserRepresentation profile = keycloakRule.getUserById("test", token.getSubject()); - Assert.assertEquals(36, profile.getUsername().length()); - - Assert.assertEquals("Bob", profile.getFirstName()); - Assert.assertEquals("Builder", profile.getLastName()); - Assert.assertEquals("bob@builder.com", profile.getEmail()); } @Test @@ -162,7 +155,7 @@ public class SocialLoginTest { driver.findElement(By.id("firstname")).sendKeys("Bob"); driver.findElement(By.id("lastname")).sendKeys("Builder"); driver.findElement(By.id("email")).sendKeys("bob@builder.com"); - driver.findElement(By.id("submit")).click(); + driver.findElement(By.id("login")).click(); profilePage.isCurrent();