Enhance error handling

apereo · Nov 30, 2015 · 152d910 · 152d910
1 parent 6f611ee
commit 152d910
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 6 deletions.
diff --git a/cas-server-support-rest/build.gradle b/cas-server-support-rest/build.gradle
@@ -7,6 +7,7 @@ dependencies {
     testCompile project(':cas-server-core-util')
     testCompile project(path: ":cas-server-core-authentication", configuration: "tests")
     testCompile project(path: ":cas-server-core-services", configuration: "tests")
+    testCompile 'org.skyscreamer:jsonassert:1.2.3'
 }
 
 
diff --git a/cas-server-support-rest/src/main/java/org/jasig/cas/support/rest/TicketsResource.java b/cas-server-support-rest/src/main/java/org/jasig/cas/support/rest/TicketsResource.java
@@ -1,7 +1,10 @@
 package org.jasig.cas.support.rest;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import org.jasig.cas.CasProtocolConstants;
 import org.jasig.cas.CentralAuthenticationService;
+import org.jasig.cas.authentication.AuthenticationException;
 import org.jasig.cas.authentication.Credential;
 import org.jasig.cas.authentication.UsernamePasswordCredential;
 import org.jasig.cas.authentication.principal.WebApplicationServiceFactory;
@@ -27,6 +30,10 @@
 import javax.validation.constraints.NotNull;
 import java.net.URI;
 import java.util.Formatter;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
 
 /**
  * {@link RestController} implementation of CAS' REST API.
@@ -54,6 +61,11 @@ public class TicketsResource {
     @Autowired(required = false)
     private final CredentialFactory credentialFactory = new DefaultCredentialFactory();
 
+    /**
+     * JSON ObjectMapper.
+     */
+    private final ObjectMapper jacksonObjectMapper = new ObjectMapper();
+
 
     /**
      * Create new ticket granting ticket.
@@ -64,24 +76,42 @@ public class TicketsResource {
      */
     @RequestMapping(value = "/tickets", method = RequestMethod.POST, consumes = MediaType.APPLICATION_FORM_URLENCODED_VALUE)
     public final ResponseEntity<String> createTicketGrantingTicket(@RequestBody final MultiValueMap<String, String> requestBody,
-                                                                   final HttpServletRequest request) {
+                                                                   final HttpServletRequest request) throws JsonProcessingException {
         try (Formatter fmt = new Formatter()) {
             final TicketGrantingTicket tgtId = this.cas.createTicketGrantingTicket(this.credentialFactory.fromRequestBody(requestBody));
             final URI ticketReference = new URI(request.getRequestURL().toString() + '/' + tgtId.getId());
             final HttpHeaders headers = new HttpHeaders();
             headers.setLocation(ticketReference);
             headers.setContentType(MediaType.TEXT_HTML);
             fmt.format("<!DOCTYPE HTML PUBLIC \\\"-//IETF//DTD HTML 2.0//EN\\\"><html><head><title>");
-            //IETF//DTD HTML 2.0//EN\\\"><html><head><title>");
             fmt.format("%s %s", HttpStatus.CREATED, HttpStatus.CREATED.getReasonPhrase())
                     .format("</title></head><body><h1>TGT Created</h1><form action=\"%s", ticketReference.toString())
                     .format("\" method=\"POST\">Service:<input type=\"text\" name=\"service\" value=\"\">")
                     .format("<br><input type=\"submit\" value=\"Submit\"></form></body></html>");
             return new ResponseEntity<>(fmt.toString(), headers, HttpStatus.CREATED);
-        } catch (final Throwable e) {
+        }
+        catch(AuthenticationException e) {
+            final List<String> authnExceptions = new LinkedList<>();
+            for (Map.Entry<String, Class<? extends Exception>> handlerErrorEntry: e.getHandlerErrors().entrySet()) {
+                authnExceptions.add(handlerErrorEntry.getValue().getSimpleName());
+            }
+            final Map<String, List<String>> errorsMap = new HashMap<>();
+            errorsMap.put("authentication_exceptions", authnExceptions);
+            LOGGER.error(e.getMessage(), e);
+            LOGGER.error(String.format("Caused by: %s", authnExceptions));
+            return new ResponseEntity<>(this.jacksonObjectMapper
+                    .writer()
+                    .withDefaultPrettyPrinter()
+                    .writeValueAsString(errorsMap), HttpStatus.UNAUTHORIZED);
+        }
+        catch(BadRequestException e) {
             LOGGER.error(e.getMessage(), e);
             return new ResponseEntity<>(e.getMessage(), HttpStatus.BAD_REQUEST);
         }
+        catch (final Throwable e) {
+            LOGGER.error(e.getMessage(), e);
+            return new ResponseEntity<>(e.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
+        }
     }
 
     /**
@@ -102,7 +132,7 @@ public final ResponseEntity<String> createServiceTicket(@RequestBody final Multi
             return new ResponseEntity<>("TicketGrantingTicket could not be found", HttpStatus.NOT_FOUND);
         } catch (final Exception e) {
             LOGGER.error(e.getMessage(), e);
-            return new ResponseEntity<>(e.getMessage(), HttpStatus.BAD_REQUEST);
+            return new ResponseEntity<>(e.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
         }
     }
 
@@ -127,7 +157,21 @@ public static class DefaultCredentialFactory implements CredentialFactory {
 
         @Override
         public Credential fromRequestBody(@NotNull final MultiValueMap<String, String> requestBody) {
+            final String username = requestBody.getFirst("username");
+            final String password = requestBody.getFirst("password");
+            if(username == null || password == null) {
+                throw new BadRequestException("Invalid payload. 'username' and 'password' form fields are required.");
+            }
             return new UsernamePasswordCredential(requestBody.getFirst("username"), requestBody.getFirst("password"));
         }
     }
+
+    /**
+     * Exception to indicate bad payload.
+     */
+    public static class BadRequestException extends IllegalArgumentException {
+        public BadRequestException(String s) {
+            super(s);
+        }
+    }
 }
diff --git a/cas-server-support-rest/src/test/java/org/jasig/cas/support/rest/TicketsResourceTests.java b/cas-server-support-rest/src/test/java/org/jasig/cas/support/rest/TicketsResourceTests.java
@@ -78,8 +78,30 @@ public void creationOfTGTWithAuthenticationException() throws Throwable {
         this.mockMvc.perform(post("/cas/v1/tickets")
                 .param("username", "test")
                 .param("password", "test"))
+                .andExpect(status().isUnauthorized())
+                .andExpect(content().json("{\"authentication_exceptions\" : [ \"LoginException\" ]}"));
+    }
+
+    @Test
+    public void creationOfTGTWithUnexpectedRuntimeException() throws Throwable {
+        configureCasMockTGTCreationToThrow(new RuntimeException("Other exception"));
+
+        this.mockMvc.perform(post("/cas/v1/tickets")
+                .param("username", "test")
+                .param("password", "test"))
+                .andExpect(status().is5xxServerError())
+                .andExpect(content().string("Other exception"));
+    }
+
+    @Test
+    public void creationOfTGTWithBadPayload() throws Throwable {
+        configureCasMockTGTCreationToThrow(new RuntimeException("Other exception"));
+
+        this.mockMvc.perform(post("/cas/v1/tickets")
+                .param("no_username_param", "test")
+                .param("no_password_param", "test"))
                 .andExpect(status().is4xxClientError())
-                .andExpect(content().string("1 errors, 0 successes"));
+                .andExpect(content().string("Invalid payload. 'username' and 'password' form fields are required."));
     }
 
     @Test
@@ -109,7 +131,7 @@ public void creationOfSTWithGeneralException() throws Throwable {
 
         this.mockMvc.perform(post("/cas/v1/tickets/TGT-1")
                 .param("service", "https://www.google.com"))
-                .andExpect(status().is4xxClientError())
+                .andExpect(status().is5xxServerError())
                 .andExpect(content().string("Other exception"));
     }
 
@@ -131,6 +153,10 @@ private void configureCasMockTGTCreationToThrowAuthenticationException() throws
         when(this.casMock.createTicketGrantingTicket(any(Credential.class))).thenThrow(new AuthenticationException(handlerErrors));
     }
 
+    private void configureCasMockTGTCreationToThrow(final Throwable e) throws Throwable {
+        when(this.casMock.createTicketGrantingTicket(any(Credential.class))).thenThrow(e);
+    }
+
     private void configureCasMockSTCreationToThrow(final Throwable e) throws Throwable {
         when(this.casMock.grantServiceTicket(anyString(), any(Service.class))).thenThrow(e);
     }