Skip to content

Commit a72f128

Browse files
miszobimiszobi
committed
#63: better error message + test for multiple SubjectConfirmation issues
1 parent c28390b commit a72f128

4 files changed

Lines changed: 95 additions & 19 deletions

File tree

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

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package com.onelogin.saml2.authn;
22

3-
import java.io.IOException;
43
import java.security.PrivateKey;
54
import java.security.cert.X509Certificate;
65
import java.util.ArrayList;
@@ -9,13 +8,8 @@
98
import java.util.Map;
109

1110
import javax.servlet.http.HttpServletRequest;
12-
import javax.xml.parsers.ParserConfigurationException;
13-
import javax.xml.xpath.XPath;
14-
import javax.xml.xpath.XPathConstants;
1511
import javax.xml.xpath.XPathExpressionException;
16-
import javax.xml.xpath.XPathFactory;
1712

18-
import org.apache.commons.lang3.StringUtils;
1913
import org.joda.time.DateTime;
2014
import org.slf4j.Logger;
2115
import org.slf4j.LoggerFactory;
@@ -24,7 +18,6 @@
2418
import org.w3c.dom.NamedNodeMap;
2519
import org.w3c.dom.Node;
2620
import org.w3c.dom.NodeList;
27-
import org.xml.sax.SAXException;
2821

2922
import com.onelogin.saml2.settings.Saml2Settings;
3023
import com.onelogin.saml2.model.SamlResponseStatus;
@@ -289,7 +282,7 @@ public boolean isValid(String requestId) {
289282

290283
// Check SubjectConfirmation, at least one SubjectConfirmation must be valid
291284
private void validateSubjectConfirmation(String responseInResponseTo) throws Exception {
292-
final List<String> subjectConfirmationDataIssues = new ArrayList<>();
285+
final List<SubjectConfirmationIssue> validationIssues = new ArrayList<>();
293286
boolean validSubjectConfirmation = false;
294287
NodeList subjectConfirmationNodes = this.queryAssertion("/saml:Subject/saml:SubjectConfirmation");
295288
for (int i = 0; i < subjectConfirmationNodes.getLength(); i++) {
@@ -306,50 +299,49 @@ private void validateSubjectConfirmation(String responseInResponseTo) throws Exc
306299

307300
Node recipient = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("Recipient");
308301
if (recipient == null) {
309-
subjectConfirmationDataIssues.add("SubjectConfirmationData doesn't contain a Recipient");
302+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData doesn't contain a Recipient"));
310303
continue;
311304
}
312305

313306
if (!recipient.getNodeValue().equals(currentUrl)) {
314-
subjectConfirmationDataIssues.add("SubjectConfirmationData doesn't match a valid Recipient");
307+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData doesn't match a valid Recipient"));
315308
continue;
316309
}
317310

318311
Node inResponseTo = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("InResponseTo");
319312
if (inResponseTo != null && !inResponseTo.getNodeValue().equals(responseInResponseTo)) {
320-
subjectConfirmationDataIssues.add("SubjectConfirmationData has an invalid InResponseTo value");
313+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData has an invalid InResponseTo value"));
321314
continue;
322315
}
323316

324317

325318
Node notOnOrAfter = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("NotOnOrAfter");
326319
if (notOnOrAfter == null) {
327-
subjectConfirmationDataIssues.add("SubjectConfirmationData doesn't contain a NotOnOrAfter attribute");
320+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData doesn't contain a NotOnOrAfter attribute"));
328321
continue;
329322
}
330323

331324
DateTime noa = Util.parseDateTime(notOnOrAfter.getNodeValue());
332325
if (noa.isEqualNow() || noa.isBeforeNow()) {
333-
subjectConfirmationDataIssues.add("SubjectConfirmationData is no longer valid");
326+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData is no longer valid"));
334327
continue;
335328
}
336329

337330
Node notBefore = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("NotBefore");
338331
if (notBefore != null) {
339332
DateTime nb = Util.parseDateTime(notBefore.getNodeValue());
340333
if (nb.isAfterNow()) {
341-
subjectConfirmationDataIssues.add("SubjectConfirmationData is not yet valid");
334+
validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData is not yet valid"));
342335
continue;
343336
}
344337
}
345338
validSubjectConfirmation = true;
346339
}
347340
}
348341
}
342+
349343
if (!validSubjectConfirmation) {
350-
String subjectConfirmationDataIssuesMsg = subjectConfirmationDataIssues.isEmpty() ? "" :
351-
" - " + StringUtils.join(subjectConfirmationDataIssues, ", ");
352-
throw new Exception("A valid SubjectConfirmation was not found on this Response" + subjectConfirmationDataIssuesMsg);
344+
throw new Exception(SubjectConfirmationIssue.prettyPrint(validationIssues));
353345
}
354346
}
355347

‎core/src/main/java/com/onelogin/saml2/authn/SubjectConfirmationIssue.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package com.onelogin.saml2.authn;
2+
3+
import java.util.List;
4+
5+
class SubjectConfirmationIssue {
6+
private final int subjectConfirmationIndex;
7+
private final String message;
8+
9+
SubjectConfirmationIssue(int subjectConfirmationIndex, String message) {
10+
this.subjectConfirmationIndex = subjectConfirmationIndex;
11+
this.message = message;
12+
}
13+
14+
static String prettyPrint(List<SubjectConfirmationIssue> subjectConfirmationDataIssues) {
15+
StringBuilder subjectConfirmationDataIssuesMsg = new StringBuilder("A valid SubjectConfirmation was not found on this Response");
16+
if (subjectConfirmationDataIssues.size() > 0) {
17+
subjectConfirmationDataIssuesMsg.append(" - ");
18+
}
19+
for (int i = 0; i < subjectConfirmationDataIssues.size(); i++) {
20+
final SubjectConfirmationIssue issue = subjectConfirmationDataIssues.get(i);
21+
subjectConfirmationDataIssuesMsg.append(issue.message);
22+
if (subjectConfirmationDataIssues.size() > 1) {
23+
subjectConfirmationDataIssuesMsg.append(" [")
24+
.append(issue.subjectConfirmationIndex)
25+
.append("]");
26+
}
27+
if (i != subjectConfirmationDataIssues.size() - 1) {
28+
subjectConfirmationDataIssuesMsg.append(", ");
29+
}
30+
}
31+
32+
return subjectConfirmationDataIssuesMsg.toString();
33+
}
34+
}

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1086,7 +1086,6 @@ public void testIsValidSubjectConfirmation_notYetValid() throws Exception {
10861086

10871087
@Test
10881088
public void testIsValidSubjectConfirmation_missingRecipient() throws Exception {
1089-
// having
10901089
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
10911090
settings.setWantAssertionsSigned(false);
10921091
settings.setWantMessagesSigned(true);
@@ -1105,13 +1104,27 @@ public void testIsValidSubjectConfirmation_missingNotOnOrAfter() throws Exceptio
11051104
settings.setWantMessagesSigned(true);
11061105

11071106
final String samlResponseEncoded = loadSignMessageAndEncode("data/responses/invalids/invalid_subjectconfirmation_no_notonorafter.xml");
1108-
HttpServletRequest request = mockRequestWithSamlResponse(samlResponseEncoded);
11091107

11101108
assertResponseValid(settings, samlResponseEncoded, false, true, null);
11111109
assertResponseValid(settings, samlResponseEncoded, true, false,
11121110
"A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData doesn't contain a NotOnOrAfter attribute");
11131111
}
11141112

1113+
@Test
1114+
public void testIsValidSubjectConfirmation_multipleIssues() throws Exception {
1115+
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
1116+
settings.setWantAssertionsSigned(false);
1117+
settings.setWantMessagesSigned(true);
1118+
1119+
final String samlResponseEncoded = loadSignMessageAndEncode("data/responses/invalids/invalid_subjectconfirmation_multiple_issues.xml");
1120+
1121+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1122+
assertResponseValid(settings, samlResponseEncoded, true, false,
1123+
"A valid SubjectConfirmation was not found on this Response - SubjectConfirmationData doesn't contain a NotOnOrAfter attribute [0], " +
1124+
"SubjectConfirmationData doesn't contain a Recipient [1], " +
1125+
"SubjectConfirmationData is no longer valid [2]");
1126+
}
1127+
11151128
/**
11161129
* Tests the isValid method of SamlResponse
11171130
* Case: Datetime with Miliseconds

‎core/src/test/resources/data/responses/invalids/invalid_subjectconfirmation_multiple_issues.xml‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?xml version="1.0"?>
2+
<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="pfxc32aed67-820f-4296-0c20-205a10dd5787" Version="2.0" IssueInstant="2011-06-17T14:54:14Z" Destination="http://localhost:8080/java-saml-jspsample/acs.jsp" InResponseTo="_57bcbf70-7b1f-012e-c821-782bcb13bb38">
3+
<saml:Issuer>https://pitbulk.no-ip.org/simplesaml/saml2/idp/metadata.php</saml:Issuer>
4+
<samlp:Status>
5+
<samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
6+
</samlp:Status>
7+
<saml:Assertion xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xs="http://www.w3.org/2001/XMLSchema" ID="pfx7841991c-c73f-4035-e2ee-c170c0e1d3e4" Version="2.0" IssueInstant="2011-06-17T14:54:14Z">
8+
<saml:Issuer>https://pitbulk.no-ip.org/simplesaml/saml2/idp/metadata.php</saml:Issuer>
9+
<saml:Subject>
10+
<saml:NameID SPNameQualifier="hello.com" Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress">someone@example.com</saml:NameID>
11+
<saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
12+
<saml:SubjectConfirmationData Recipient="http://localhost:8080/java-saml-jspsample/acs.jsp"/>
13+
</saml:SubjectConfirmation>
14+
<saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
15+
<saml:SubjectConfirmationData NotOnOrAfter="2120-06-17T14:53:44Z"/>
16+
</saml:SubjectConfirmation>
17+
<saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
18+
<saml:SubjectConfirmationData NotOnOrAfter="2010-06-17T14:53:44Z" Recipient="http://localhost:8080/java-saml-jspsample/acs.jsp"/>
19+
</saml:SubjectConfirmation>
20+
</saml:Subject>
21+
<saml:Conditions NotBefore="2010-06-17T14:53:44Z" NotOnOrAfter="2121-06-17T14:59:14Z">
22+
<saml:AudienceRestriction>
23+
<saml:Audience>http://localhost:8080/java-saml-jspsample/metadata.jsp</saml:Audience>
24+
</saml:AudienceRestriction>
25+
</saml:Conditions>
26+
<saml:AuthnStatement AuthnInstant="2011-06-17T14:54:07Z" SessionNotOnOrAfter="2121-06-17T22:54:14Z" SessionIndex="_51be37965feb5579d803141076936dc2e9d1d98ebf">
27+
<saml:AuthnContext>
28+
<saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:Password</saml:AuthnContextClassRef>
29+
</saml:AuthnContext>
30+
</saml:AuthnStatement>
31+
<saml:AttributeStatement>
32+
<saml:Attribute Name="mail" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
33+
<saml:AttributeValue xsi:type="xs:string">someone@example.com</saml:AttributeValue>
34+
</saml:Attribute>
35+
</saml:AttributeStatement>
36+
</saml:Assertion>
37+
</samlp:Response>

0 commit comments

Comments
 (0)