Better error handling, especially during developer authorization

src/main/java/se/su/dsv/oauth2/staging/CustomAuthorizationEndpointFilter.java

@@ -9,11 +9,14 @@ import jakarta.servlet.http.HttpFilter;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletRequestWrapper;
 import jakarta.servlet.http.HttpServletResponse;
+import org.springframework.http.HttpStatus;
 import org.springframework.security.authentication.AuthenticationManager;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.security.core.context.SecurityContextHolder;
+import org.springframework.security.oauth2.core.OAuth2Error;
 import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
+import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationException;
 import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationToken;
 import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2AuthorizationCodeRequestAuthenticationConverter;
 import org.springframework.security.web.DefaultRedirectStrategy;
@@ -72,7 +75,11 @@ public class CustomAuthorizationEndpointFilter extends HttpFilter {
         }
 
         if (loggedInUser.getAuthorities().contains(new Entitlement(developerEntitlement))) {
-            proceedWithDeveloperAuthorization(request, response, loggedInUser);
+            try {
+                proceedWithDeveloperAuthorization(request, response, loggedInUser);
+            } catch (OAuth2AuthorizationCodeRequestAuthenticationException exception) {
+                sendAuthorizationError(request, response, exception);
+            }
         } else {
             chain.doFilter(request, response);
         }
@@ -193,4 +200,40 @@ public class CustomAuthorizationEndpointFilter extends HttpFilter {
         String redirectUri = uriBuilder.build(true).toUriString();
         this.redirectStrategy.sendRedirect(request, response, redirectUri);
     }
+
+    private void sendAuthorizationError(
+            HttpServletRequest request,
+            HttpServletResponse response,
+            OAuth2AuthorizationCodeRequestAuthenticationException authorizationCodeRequestAuthenticationException)
+            throws IOException
+    {
+        OAuth2Error error = authorizationCodeRequestAuthenticationException.getError();
+        OAuth2AuthorizationCodeRequestAuthenticationToken authorizationCodeRequestAuthentication = authorizationCodeRequestAuthenticationException
+                .getAuthorizationCodeRequestAuthentication();
+
+        if (authorizationCodeRequestAuthentication == null
+            || !StringUtils.hasText(authorizationCodeRequestAuthentication.getRedirectUri())) {
+            response.sendError(HttpStatus.BAD_REQUEST.value(), error.toString());
+            return;
+        }
+
+        UriComponentsBuilder uriBuilder = UriComponentsBuilder
+                .fromUriString(authorizationCodeRequestAuthentication.getRedirectUri())
+                .queryParam(OAuth2ParameterNames.ERROR, error.getErrorCode());
+        if (StringUtils.hasText(error.getDescription())) {
+            uriBuilder.queryParam(OAuth2ParameterNames.ERROR_DESCRIPTION,
+                    UriUtils.encode(error.getDescription(), StandardCharsets.UTF_8));
+        }
+        if (StringUtils.hasText(error.getUri())) {
+            uriBuilder.queryParam(OAuth2ParameterNames.ERROR_URI,
+                    UriUtils.encode(error.getUri(), StandardCharsets.UTF_8));
+        }
+        if (StringUtils.hasText(authorizationCodeRequestAuthentication.getState())) {
+            uriBuilder.queryParam(OAuth2ParameterNames.STATE,
+                    UriUtils.encode(authorizationCodeRequestAuthentication.getState(), StandardCharsets.UTF_8));
+        }
+        // build(true) -> Components are explicitly encoded
+        String redirectUri = uriBuilder.build(true).toUriString();
+        this.redirectStrategy.sendRedirect(request, response, redirectUri);
+    }
 }

src/main/resources/templates/error.jte

@@ -0,0 +1,8 @@
+@param Integer status
+@param String error
+@param String message
+
+@template.base(title = message, content = @`
+    <h1>${status} ${error}</h1>
+    <p>${message}</p>
+`)

src/test/java/se/su/dsv/oauth2/StagingProfileTest.java

@@ -6,8 +6,14 @@ import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.security.oauth2.core.oidc.StandardClaimNames;
 import org.springframework.test.context.ActiveProfiles;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.*;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
 import static se.su.dsv.oauth2.ShibbolethRequestProcessor.remoteUser;
+import static se.su.dsv.oauth2.TestRegisteredClientConfiguration.CLIENT_ID;
+import static se.su.dsv.oauth2.TestRegisteredClientConfiguration.REDIRECT_URI;
 
 @SpringBootTest(
         classes = TestRegisteredClientConfiguration.class,
@@ -155,4 +161,67 @@ public class StagingProfileTest extends AbstractMetadataCodeFlowTest {
         assertTrue(claims.getStringListClaim("entitlements").contains(customEntitlement),
                 "Does not contain custom entitlement");
     }
+
+    @Test
+    public void correctly_handles_missing_response_type_parameter() throws Exception {
+        mockMvc.perform(post(getAuthorizationEndpoint())
+                .with(remoteUser("developer")
+                        .entitlement(DEVELOPER_ENTITLEMENT))
+                .queryParam("client_id", CLIENT_ID)
+                .queryParam("redirect_uri", REDIRECT_URI)
+                .formField("principal", "developer"))
+                .andExpect(status().isBadRequest())
+                .andExpect(status().reason(containsString("response_type")));
+    }
+
+    @Test
+    public void redirects_back_to_client_if_there_are_errors_but_valid_redirect_uri() throws Exception {
+        mockMvc.perform(post(getAuthorizationEndpoint())
+                .with(remoteUser("developer")
+                        .entitlement(DEVELOPER_ENTITLEMENT))
+                .queryParam("response_type", "code")
+                .queryParam("client_id", CLIENT_ID)
+                .queryParam("redirect_uri", REDIRECT_URI)
+                .queryParam("scope", "invalid")
+                .formField("principal", "developer"))
+                .andExpect(status().is3xxRedirection())
+                .andExpect(result -> {
+                    String redirectedUrl = result.getResponse().getRedirectedUrl();
+                    assertThat(redirectedUrl, containsString("error=invalid_scope"));
+                });
+    }
+
+    @Test
+    public void does_not_redirect_with_invalid_client_id() throws Exception {
+        mockMvc.perform(post(getAuthorizationEndpoint())
+                .with(remoteUser("developer")
+                        .entitlement(DEVELOPER_ENTITLEMENT))
+                .queryParam("response_type", "code")
+                .queryParam("client_id", "invalid-client-id")
+                .queryParam("redirect_uri", REDIRECT_URI)
+                .formField("principal", "developer"))
+                .andExpect(status().isBadRequest())
+                .andExpect(status().reason(containsString("client_id")));
+    }
+
+    @Test
+    public void maintains_state_during_error_redirect() throws Exception {
+        String state = "state123";
+
+        mockMvc.perform(post(getAuthorizationEndpoint())
+                .with(remoteUser("developer")
+                        .entitlement(DEVELOPER_ENTITLEMENT))
+                .queryParam("response_type", "code")
+                .queryParam("client_id", CLIENT_ID)
+                .queryParam("redirect_uri", REDIRECT_URI)
+                .queryParam("state", state)
+                .queryParam("scope", "invalid")
+                .formField("principal", "developer"))
+                .andExpect(status().is3xxRedirection())
+                .andExpect(result -> {
+                    String redirectedUrl = result.getResponse().getRedirectedUrl();
+                    assertThat(redirectedUrl, containsString("error=invalid_scope"));
+                    assertThat(redirectedUrl, containsString("state=" + state));
+                });
+    }
 }