When doing implicit grant with prompt=none, put error code in fragment

[#132259715] https://www.pivotaltracker.com/story/show/132259715 Signed-off-by: Priyata Agrawal <pagrawal@pivotal.io>
cloudfoundry · Oct 17, 2016 · a0b6efe · a0b6efe
1 parent 0b9dd84
commit a0b6efe
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 8 deletions.
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/AuthorizePromptNoneEntryPoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/AuthorizePromptNoneEntryPoint.java
@@ -34,8 +34,10 @@
 import java.io.IOException;
 import java.util.Set;
 
+import static java.util.Arrays.stream;
 import static java.util.Collections.EMPTY_SET;
 import static java.util.Optional.ofNullable;
+import static org.cloudfoundry.identity.uaa.util.UaaUrlUtils.addFragmentComponent;
 import static org.cloudfoundry.identity.uaa.util.UaaUrlUtils.addQueryParameter;
 import static org.springframework.util.StringUtils.hasText;
 
@@ -60,6 +62,7 @@ public AuthorizePromptNoneEntryPoint(AuthenticationFailureHandler failureHandler
     public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException) throws IOException, ServletException {
         String clientId = request.getParameter(OAuth2Utils.CLIENT_ID);
         String redirectUri = request.getParameter(OAuth2Utils.REDIRECT_URI);
+        String[] responseTypes = ofNullable(request.getParameter(OAuth2Utils.RESPONSE_TYPE)).map(rt -> rt.split(" ")).orElse(new String[0]);
 
         //client_id is a required parameter
         if (!hasText(clientId)) {
@@ -96,6 +99,8 @@ public void commence(HttpServletRequest request, HttpServletResponse response, A
         }
 
         failureHandler.onAuthenticationFailure(request, response, authException);
-        response.sendRedirect(addQueryParameter(resolvedRedirect, "error", "login_required"));
+        boolean implicit = stream(responseTypes).noneMatch("code"::equalsIgnoreCase);
+        String redirectLocation = implicit ? addFragmentComponent(resolvedRedirect, "error=login_required") : addQueryParameter(resolvedRedirect, "error", "login_required");
+        response.sendRedirect(redirectLocation);
     }
 }
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java b/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtils.java
@@ -23,6 +23,7 @@
 
 import javax.servlet.http.HttpServletRequest;
 import java.net.MalformedURLException;
+import java.net.URI;
 import java.net.URL;
 import java.util.Collection;
 
@@ -97,6 +98,13 @@ public static String addQueryParameter(String url, String name, String value) {
         return builder.build().toUriString();
     }
 
+    public static String addFragmentComponent(String urlString, String component) {
+        URI uri = URI.create(urlString);
+        UriComponentsBuilder builder = UriComponentsBuilder.fromUri(uri);
+        builder.fragment(StringUtils.hasText(uri.getFragment()) ? uri.getFragment() + "&" + component : component);
+        return builder.build().toUriString();
+    }
+
     public static String addSubdomainToUrl(String url) {
         UriComponentsBuilder builder = UriComponentsBuilder.fromHttpUrl(url);
         builder.host(getSubdomain() + builder.build().getHost());

diff --git a/.../src/test/java/org/cloudfoundry/identity/uaa/oauth/AuthorizePromptNoneEntryPointTest.java b/.../src/test/java/org/cloudfoundry/identity/uaa/oauth/AuthorizePromptNoneEntryPointTest.java
@@ -17,6 +17,8 @@
 
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 import org.springframework.http.HttpStatus;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
@@ -30,6 +32,8 @@
 import org.springframework.security.web.authentication.AuthenticationFailureHandler;
 import org.springframework.security.web.authentication.session.SessionAuthenticationException;
 
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 
 import static org.junit.Assert.assertEquals;
@@ -41,11 +45,13 @@
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
+import static org.springframework.util.StringUtils.hasText;
 
+@RunWith(Parameterized.class)
 public class AuthorizePromptNoneEntryPointTest {
 
-    public static final String REDIRECT_URI = "http://sub.domain.com/callback?oauth=true";
-    public static final String HTTP_SOME_OTHER_SITE_CALLBACK = "http://some.other.site/callback";
+    private static final String REDIRECT_URI = "http://sub.domain.com/callback?oauth=true";
+    private static final String HTTP_SOME_OTHER_SITE_CALLBACK = "http://some.other.site/callback";
     private final SessionAuthenticationException authException = new SessionAuthenticationException("");
     private AuthenticationFailureHandler failureHandler;
     private AuthenticationEntryPoint entryPoint;
@@ -55,21 +61,46 @@ public class AuthorizePromptNoneEntryPointTest {
     private ClientDetailsService clientDetailsService;
     private RedirectResolver redirectResolver;
 
+    private final String responseType;
+    private final String redirectHash;
+    private final String redirectUrl;
+
+    public AuthorizePromptNoneEntryPointTest(String responseType, String redirectHash) {
+        this.responseType = responseType;
+        this.redirectHash = redirectHash;
+        redirectUrl = REDIRECT_URI + redirectHash;
+    }
+
+    @Parameterized.Parameters(name = "{index}: {0} {1}")
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(new Object[][]{
+            {"code", ""},
+            {"token", ""},
+            {"id_token", ""},
+            {"token id_token", ""},
+            {"code", "#frag"},
+            {"token", "#frag"},
+            {"id_token", "#frag"},
+            {"token id_token", "#frag"}
+        });
+    }
+
     @Before
     public void setup() {
-        client = new BaseClientDetails("id", "", "openid", "authorization_code", "", REDIRECT_URI);
+        client = new BaseClientDetails("id", "", "openid", "authorization_code", "", redirectUrl);
         clientDetailsService = mock(ClientDetailsService.class);
         redirectResolver = mock(RedirectResolver.class);
         failureHandler = mock(AuthenticationFailureHandler.class);
 
         when(clientDetailsService.loadClientByClientId(eq(client.getClientId()))).thenReturn(client);
-        when(redirectResolver.resolveRedirect(eq(REDIRECT_URI), same(client))).thenReturn(REDIRECT_URI);
+        when(redirectResolver.resolveRedirect(eq(redirectUrl), same(client))).thenReturn(redirectUrl);
         when(redirectResolver.resolveRedirect(eq(HTTP_SOME_OTHER_SITE_CALLBACK), same(client))).thenThrow(new RedirectMismatchException(""));
 
         entryPoint = new AuthorizePromptNoneEntryPoint(failureHandler, clientDetailsService, redirectResolver);
 
         request = new MockHttpServletRequest("GET", "/oauth/authorize");
         request.setParameter(OAuth2Utils.CLIENT_ID, client.getClientId());
+        request.setParameter(OAuth2Utils.RESPONSE_TYPE, responseType);
         response = new MockHttpServletResponse();
     }
 
@@ -104,15 +135,17 @@ public void test_redirect_mismatch() throws Exception {
 
     @Test
     public void test_redirect_contains_error() throws Exception {
-        request.setParameter(OAuth2Utils.REDIRECT_URI, REDIRECT_URI);
+        request.setParameter(OAuth2Utils.REDIRECT_URI, redirectUrl);
         entryPoint.commence(request, response, authException);
         assertEquals(HttpStatus.FOUND.value(), response.getStatus());
-        assertEquals(REDIRECT_URI+"&error=login_required", response.getHeader("Location"));
+        String paramValue = "error=login_required";
+        String expectedRedirect = REDIRECT_URI + (responseType.equals("code") ? "&" + paramValue + redirectHash : (hasText(redirectHash) ? redirectHash + "&" : "#") + paramValue);
+        assertEquals(expectedRedirect, response.getHeader("Location"));
     }
 
     @Test
     public void test_failure_handler_is_invoked() throws Exception {
-        request.setParameter(OAuth2Utils.REDIRECT_URI, REDIRECT_URI);
+        request.setParameter(OAuth2Utils.REDIRECT_URI, redirectUrl);
         entryPoint.commence(request, response, authException);
         verify(failureHandler, times(1)).onAuthenticationFailure(same(request), same(response), same(authException));
     }

diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaUrlUtilsTest.java
@@ -227,6 +227,20 @@ public void test_add_query_parameter() {
         assertEquals("http://sub.domain.com?name=value#frag=fragvalue", UaaUrlUtils.addQueryParameter(url+"#frag=fragvalue", name, value));
     }
 
+    @Test
+    public void test_add_fragment_component() {
+        String url = "http://sub.domain.com";
+        String component = "name=value";
+        assertEquals("http://sub.domain.com#name=value", UaaUrlUtils.addFragmentComponent(url, component));
+    }
+
+    @Test
+    public void test_add_fragment_component_to_prior_fragment() {
+        String url = "http://sub.domain.com#frag";
+        String component = "name=value";
+        assertEquals("http://sub.domain.com#frag&name=value", UaaUrlUtils.addFragmentComponent(url, component));
+    }
+
     private void setIdentityZone(String subdomain) {
         IdentityZone zone = MultitenancyFixture.identityZone(subdomain, subdomain);
         IdentityZoneHolder.set(zone);