#63: address PR comments - improved error messages

Author: miszobi

core/src/main/java/com/onelogin/saml2/authn/SamlResponse.java

@@ -10,6 +10,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.xml.xpath.XPathExpressionException;
 
+import com.onelogin.saml2.model.SubjectConfirmationIssue;
 import org.joda.time.DateTime;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -341,7 +342,7 @@ private void validateSubjectConfirmation(String responseInResponseTo) throws Exc
 		}
 
 		if (!validSubjectConfirmation) {
-			throw new Exception(SubjectConfirmationIssue.prettyPrint(validationIssues));
+			throw new Exception(SubjectConfirmationIssue.prettyPrintIssues(validationIssues));
 		}
 	}
 

…aml2/authn/SubjectConfirmationIssue.java …aml2/model/SubjectConfirmationIssue.javacore/src/main/java/com/onelogin/saml2/authn/SubjectConfirmationIssue.java renamed to core/src/main/java/com/onelogin/saml2/model/SubjectConfirmationIssue.java core/src/main/java/com/onelogin/saml2/authn/SubjectConfirmationIssue.java renamed to core/src/main/java/com/onelogin/saml2/model/SubjectConfirmationIssue.java

@@ -1,29 +1,29 @@
-package com.onelogin.saml2.authn;
+package com.onelogin.saml2.model;
 
 import java.util.List;
 
-class SubjectConfirmationIssue {
+public class SubjectConfirmationIssue {
     private final int subjectConfirmationIndex;
     private final String message;
 
-    SubjectConfirmationIssue(int subjectConfirmationIndex, String message) {
+    public SubjectConfirmationIssue(int subjectConfirmationIndex, String message) {
         this.subjectConfirmationIndex = subjectConfirmationIndex;
         this.message = message;
     }
 
-    static String prettyPrint(List<SubjectConfirmationIssue> subjectConfirmationDataIssues) {
+    public static String prettyPrintIssues(List<SubjectConfirmationIssue> subjectConfirmationDataIssues) {
         StringBuilder subjectConfirmationDataIssuesMsg = new StringBuilder("A valid SubjectConfirmation was not found on this Response");
         if (subjectConfirmationDataIssues.size() > 0) {
-            subjectConfirmationDataIssuesMsg.append(" - ");
+            subjectConfirmationDataIssuesMsg.append(": ");
         }
         for (int i = 0; i < subjectConfirmationDataIssues.size(); i++) {
             final SubjectConfirmationIssue issue = subjectConfirmationDataIssues.get(i);
-            subjectConfirmationDataIssuesMsg.append(issue.message);
             if (subjectConfirmationDataIssues.size() > 1) {
-                subjectConfirmationDataIssuesMsg.append(" [")
+                subjectConfirmationDataIssuesMsg.append("\n[")
                         .append(issue.subjectConfirmationIndex)
-                        .append("]");
+                        .append("] ");
             }
+            subjectConfirmationDataIssuesMsg.append(issue.message);
             if (i != subjectConfirmationDataIssues.size() - 1) {
                 subjectConfirmationDataIssuesMsg.append(", ");
             }

core/src/test/java/com/onelogin/saml2/test/authn/AuthnResponseTest.java

@@ -1054,7 +1054,7 @@ public void testIsValidSubjectConfirmation_invalidInResponseTo() throws Exceptio
 		final String samlResponseEncoded = Util.getFileAsString("data/responses/invalids/invalid_subjectconfirmation_inresponse.xml.base64");
 
 		assertResponseValid(settings, samlResponseEncoded, false, false, "No Signature found. SAML Response rejected");
-		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData has an invalid InResponseTo value");
+		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response: SubjectConfirmationData has an invalid InResponseTo value");
 	}
 
 	@Test
@@ -1063,7 +1063,7 @@ public void testIsValidSubjectConfirmation_invalidRecipient() throws Exception {
 		final String samlResponseEncoded = Util.getFileAsString("data/responses/invalids/invalid_subjectconfirmation_recipient.xml.base64");
 
 		assertResponseValid(settings, samlResponseEncoded, false, false, "No Signature found. SAML Response rejected");
-		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData doesn't match a valid Recipient");
+		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response: SubjectConfirmationData doesn't match a valid Recipient");
 	}
 
 	@Test
@@ -1072,7 +1072,7 @@ public void testIsValidSubjectConfirmation_noLongerValid() throws Exception {
 		final String samlResponseEncoded = Util.getFileAsString("data/responses/invalids/invalid_subjectconfirmation_noa.xml.base64");
 
 		assertResponseValid(settings, samlResponseEncoded, false, false, "No Signature found. SAML Response rejected");
-		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData is no longer valid");
+		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response: SubjectConfirmationData is no longer valid");
 	}
 
 	@Test
@@ -1081,7 +1081,7 @@ public void testIsValidSubjectConfirmation_notYetValid() throws Exception {
 		final String samlResponseEncoded = loadAndEncode("data/responses/invalids/invalid_subjectconfirmation_nb.xml");
 
 		assertResponseValid(settings, samlResponseEncoded, false, false, "No Signature found. SAML Response rejected");
-		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData is not yet valid");
+		assertResponseValid(settings, samlResponseEncoded, true, false, "A valid SubjectConfirmation was not found on this Response: SubjectConfirmationData is not yet valid");
 	}
 
 	@Test
@@ -1094,7 +1094,7 @@ public void testIsValidSubjectConfirmation_missingRecipient() throws Exception {
 
 		assertResponseValid(settings, samlResponseEncoded, false, true, null);
 		assertResponseValid(settings, samlResponseEncoded, true, false,
-				"A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData doesn't contain a Recipient");
+				"A valid SubjectConfirmation was not found on this Response: SubjectConfirmationData doesn't contain a Recipient");
 	}
 
 	@Test
@@ -1107,7 +1107,7 @@ public void testIsValidSubjectConfirmation_missingNotOnOrAfter() throws Exceptio
 
 		assertResponseValid(settings, samlResponseEncoded, false, true, null);
 		assertResponseValid(settings, samlResponseEncoded, true, false,
-				"A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData doesn't contain a NotOnOrAfter attribute");
+				"A valid SubjectConfirmation was not found on this Response: SubjectConfirmationData doesn't contain a NotOnOrAfter attribute");
 	}
 
 	@Test
@@ -1120,9 +1120,10 @@ public void testIsValidSubjectConfirmation_multipleIssues() throws Exception {
 
 		assertResponseValid(settings, samlResponseEncoded, false, true, null);
 		assertResponseValid(settings, samlResponseEncoded, true, false,
-				"A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData doesn't contain a NotOnOrAfter attribute [0], " +
-						"SubjectConfirmationData doesn't contain a Recipient [1], " +
-						"SubjectConfirmationData is no longer valid [2]");
+				"A valid SubjectConfirmation was not found on this Response: " +
+						"\n[0] SubjectConfirmationData doesn't contain a NotOnOrAfter attribute, " +
+						"\n[1] SubjectConfirmationData doesn't contain a Recipient, " +
+						"\n[2] SubjectConfirmationData is no longer valid");
 	}
 
 	/**