Revise SP contacts settings validation

The contact type check now becomes useful (and so it was restored), because with the new full Contacts support the user may indeed specify invalid contact types in settings. The "not enough data" check, instead, was fixed and it is now raised only if ALL of the contact data (company, given name, surname, e-mail addresses and phone names) are empty, reflecting the actual SAML 2.0 metadata schema constraint. Fixes #353.
SAML-Toolkits · Nov 5, 2021 · 6900702 · 6900702
1 parent f7364f7
commit 6900702
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 18 deletions.
diff --git a/core/src/main/java/com/onelogin/saml2/settings/Saml2Settings.java b/core/src/main/java/com/onelogin/saml2/settings/Saml2Settings.java
@@ -5,8 +5,10 @@
 import java.security.cert.CertificateEncodingException;
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Set;
 
 import com.onelogin.saml2.model.hsm.HSM;
 
@@ -1017,27 +1019,25 @@ public List<String> checkSPSettings() {
 
 		List<Contact> contacts = this.getContacts();
 		if (!contacts.isEmpty()) {
-/*			
-			List<String> validTypes = new ArrayList<String>();
-			validTypes.add("technical");
-			validTypes.add("support");
-			validTypes.add("administrative");
-			validTypes.add("billing");
-			validTypes.add("other");
-*/
+			Set<String> validTypes = new HashSet<>();
+			validTypes.add(Constants.CONTACT_TYPE_TECHNICAL);
+			validTypes.add(Constants.CONTACT_TYPE_SUPPORT);
+			validTypes.add(Constants.CONTACT_TYPE_ADMINISTRATIVE);
+			validTypes.add(Constants.CONTACT_TYPE_BILLING);
+			validTypes.add(Constants.CONTACT_TYPE_OTHER);
 			for (Contact contact : contacts) {
-/*
 				if (!validTypes.contains(contact.getContactType())) {
 					errorMsg = "contact_type_invalid";
 					errors.add(errorMsg);
 					LOGGER.error(errorMsg);
 				}
-*/
-
-				if (contact.getEmailAddresses().isEmpty() || contact.getEmailAddresses().stream().allMatch(StringUtils::isEmpty) || 
-						(StringUtils.isEmpty(contact.getCompany()) && 
-						StringUtils.isEmpty(contact.getGivenName()) && 
-						StringUtils.isEmpty(contact.getSurName()))) {
+				if ((contact.getEmailAddresses().isEmpty()
+				            || contact.getEmailAddresses().stream().allMatch(StringUtils::isEmpty))
+				            && (contact.getTelephoneNumbers().isEmpty() || contact.getTelephoneNumbers()
+				                        .stream().allMatch(StringUtils::isEmpty))
+				            && StringUtils.isEmpty(contact.getCompany())
+				            && StringUtils.isEmpty(contact.getGivenName())
+				            && StringUtils.isEmpty(contact.getSurName())) {
 					errorMsg = "contact_not_enough_data";
 					errors.add(errorMsg);
 					LOGGER.error(errorMsg);

diff --git a/core/src/test/java/com/onelogin/saml2/test/settings/Saml2SettingsTest.java b/core/src/test/java/com/onelogin/saml2/test/settings/Saml2SettingsTest.java
@@ -114,6 +114,7 @@ public void testCheckSPSettingsAllErrors() throws IOException, Error {
 		assertThat(settingsErrors, hasItem("sp_entityId_not_found"));
 		assertThat(settingsErrors, hasItem("sp_acs_not_found"));
 		assertThat(settingsErrors, hasItem("sp_cert_not_found_and_required"));
+		assertThat(settingsErrors, hasItem("contact_type_invalid"));
 		assertThat(settingsErrors, hasItem("contact_not_enough_data"));
 		assertThat(settingsErrors, hasItem("organization_not_enough_data"));
 	}
@@ -151,6 +152,7 @@ public void testCheckSettingsAllErrors() throws IOException, Error {
 		assertThat(settingsErrors, hasItem("sp_entityId_not_found"));
 		assertThat(settingsErrors, hasItem("sp_acs_not_found"));
 		assertThat(settingsErrors, hasItem("sp_cert_not_found_and_required"));
+		assertThat(settingsErrors, hasItem("contact_type_invalid"));
 		assertThat(settingsErrors, hasItem("contact_not_enough_data"));
 		assertThat(settingsErrors, hasItem("organization_not_enough_data"));
 		assertThat(settingsErrors, hasItem("idp_entityId_not_found"));

diff --git a/core/src/test/resources/config/config.allerrors.properties b/core/src/test/resources/config/config.allerrors.properties
@@ -19,4 +19,6 @@ onelogin.saml2.organization.name = SP Java
 onelogin.saml2.organization.url = http://sp.example.com
 
 # Contacts
-onelogin.saml2.contacts.support.email_address = support@example.com
+onelogin.saml2.sp.contact[0].contactType=administrative
+onelogin.saml2.sp.contact[1].contactType=nonexistent
+onelogin.saml2.sp.contact[1].company=ACME
diff --git a/core/src/test/resources/config/config.sperrors.properties b/core/src/test/resources/config/config.sperrors.properties
@@ -30,4 +30,6 @@ onelogin.saml2.organization.name = SP Java
 onelogin.saml2.organization.displayname = SP Java Example
 
 # Contacts
-onelogin.saml2.contacts.support.given_name = Support Guy
+onelogin.saml2.sp.contact[0].contactType=administrative
+onelogin.saml2.sp.contact[1].contactType=nonexistent
+onelogin.saml2.sp.contact[1].company=ACME
diff --git a/toolkit/src/test/java/com/onelogin/saml2/test/AuthTest.java b/toolkit/src/test/java/com/onelogin/saml2/test/AuthTest.java
@@ -303,7 +303,7 @@ public void testConstructorInvalidSettings() throws IOException, SettingsExcepti
 		Saml2Settings settings = new SettingsBuilder().fromFile("config/config.sperrors.properties").build();
 
 		expectedEx.expect(SettingsException.class);
-		expectedEx.expectMessage("Invalid settings: sp_entityId_not_found, sp_acs_not_found, sp_cert_not_found_and_required, contact_not_enough_data, organization_not_enough_data, idp_cert_or_fingerprint_not_found_and_required, idp_cert_not_found_and_required");
+		expectedEx.expectMessage("Invalid settings: sp_entityId_not_found, sp_acs_not_found, sp_cert_not_found_and_required, contact_not_enough_data, contact_type_invalid, organization_not_enough_data, idp_cert_or_fingerprint_not_found_and_required, idp_cert_not_found_and_required");
 		new Auth(settings, request, response);
 	}