From bf289f6e0920b4736d16eade88f5fa2126f20bbc Mon Sep 17 00:00:00 2001 From: Federica Agostini Date: Wed, 26 Jul 2023 16:46:07 +0100 Subject: [PATCH 1/4] Update organizationName to match search results --- .../saml/DefaultMetadataLookupService.java | 34 +++++++++++++++++-- .../iam/authn/saml/model/IdpDescription.java | 18 ++++++++-- .../saml/MetadataLookupServiceTests.java | 7 ++-- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java index 8d0272bd0..c660e3d40 100644 --- a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java +++ b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java @@ -26,11 +26,13 @@ import java.util.Set; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Predicate; import java.util.stream.Collectors; import org.opensaml.common.xml.SAMLConstants; import org.opensaml.saml2.metadata.EntityDescriptor; import org.opensaml.saml2.metadata.IDPSSODescriptor; +import org.opensaml.saml2.metadata.LocalizedString; import org.opensaml.saml2.metadata.provider.MetadataProvider; import org.opensaml.saml2.metadata.provider.MetadataProviderException; import org.opensaml.saml2.metadata.provider.ObservableMetadataProvider; @@ -107,6 +109,7 @@ private IdpDescription descriptionFromMetadata(EntityDescriptor descriptor) { if (!uiInfo.getDisplayNames().isEmpty()) { result.setOrganizationName(uiInfo.getDisplayNames().get(0).getName().getLocalString()); + result.setDisplayNames(uiInfo.getDisplayNames().stream().map(dn -> dn.getName()).toList()); } } } @@ -140,6 +143,16 @@ private Optional> lookupByEntityId(String text) { public List lookupIdp(String text) { List result = new ArrayList<>(); + String textToFind = text.toLowerCase(); + + Predicate filterForDescriptions = description -> { + if (description.getDisplayNames() != null) { + return description.getDisplayNames().stream() + .anyMatch(name -> name.getLocalString().toLowerCase().contains(textToFind)); + } else { + return description.getEntityId().toLowerCase().contains(textToFind); + } + }; lookupByEntityId(text).ifPresent(result::addAll); @@ -149,10 +162,25 @@ public List lookupIdp(String text) { try { lock.readLock().lock(); + return descriptions.stream() - .filter(p -> p.getOrganizationName().toLowerCase().contains(text.toLowerCase())) - .limit(MAX_RESULTS) - .collect(Collectors.toList()); + .filter(filterForDescriptions) + .limit(MAX_RESULTS) + .map(description -> { + if (description.getDisplayNames() != null) { + List displayNames = description.getDisplayNames(); + + for (LocalizedString displayName : displayNames) { + String localString = displayName.getLocalString(); + if (localString.toLowerCase().contains(textToFind)) { + description.setOrganizationName(localString); + break; + } + } + } + return description; + }) + .collect(Collectors.toList()); } finally { lock.readLock().unlock(); } diff --git a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/model/IdpDescription.java b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/model/IdpDescription.java index 039b3572f..7b92df54d 100644 --- a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/model/IdpDescription.java +++ b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/model/IdpDescription.java @@ -15,15 +15,20 @@ */ package it.infn.mw.iam.authn.saml.model; +import java.util.List; + +import org.opensaml.saml2.metadata.LocalizedString; + import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonInclude.Include; @JsonInclude(Include.NON_EMPTY) -public class IdpDescription { +public class IdpDescription { private String entityId; private String organizationName; private String imageUrl; + private List displayNames; public String getEntityId() { return entityId; @@ -49,9 +54,18 @@ public void setImageUrl(String imageUrl) { this.imageUrl = imageUrl; } + public List getDisplayNames() { + return displayNames; + } + + public void setDisplayNames(List displayNames) { + this.displayNames = displayNames; + } + @Override public String toString() { return "IdpDescription [entityId=" + entityId + ", organizationName=" + organizationName - + ", imageUrl=" + imageUrl + "]"; + + ", imageUrl=" + imageUrl + ", displayNames=" + displayNames + "]"; } + } diff --git a/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java b/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java index 7c21a92e7..94c00fe39 100644 --- a/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java +++ b/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java @@ -58,6 +58,7 @@ public class MetadataLookupServiceTests { public static final String IDP1_ORGANIZATION_NAME = "IDP1 organization"; public static final String IDP2_ORGANIZATION_NAME = "IDP2 organization"; + public static final String IDP4_ORGANIZATION_NAME = "IDP4 organization"; @Mock MetadataManager manager; @@ -91,7 +92,7 @@ public void setup() throws MetadataProviderException { when(idp2DisplayName.getName()).thenReturn(idp2LocalizedString); when(idp2UIInfo.getDisplayNames()).thenReturn(asList(idp2DisplayName)); - when(idp4LocalizedString.getLocalString()).thenReturn(""); + when(idp4LocalizedString.getLocalString()).thenReturn(IDP4_ORGANIZATION_NAME); when(idp4DisplayName.getName()).thenReturn(idp4LocalizedString); when(idp4UIInfo.getDisplayNames()).thenReturn(asList(idp4DisplayName)); @@ -151,7 +152,7 @@ public void testServiceInitialization() throws MetadataProviderException { hasProperty("organizationName", is(IDP3_ENTITY_ID))))); assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP4_ENTITY_ID)), - hasProperty("organizationName", is(IDP4_ENTITY_ID))))); + hasProperty("organizationName", is(IDP4_ORGANIZATION_NAME))))); } @@ -191,7 +192,7 @@ public void testPartialLookupWorks() { hasProperty("organizationName", is(IDP3_ENTITY_ID))))); assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP4_ENTITY_ID)), - hasProperty("organizationName", is(IDP4_ENTITY_ID))))); + hasProperty("organizationName", is(IDP4_ORGANIZATION_NAME))))); } @Test From 5bc28cc059927f443bcea5a643993d4a04147c90 Mon Sep 17 00:00:00 2001 From: Federica Agostini Date: Thu, 17 Aug 2023 16:32:06 +0200 Subject: [PATCH 2/4] Use googlestyle formatter --- .../saml/DefaultMetadataLookupService.java | 43 ++++++++-------- .../saml/MetadataLookupServiceTests.java | 50 +++++++++---------- 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java index c660e3d40..060c8de74 100644 --- a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java +++ b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java @@ -51,7 +51,8 @@ @Component @Profile("saml") -public class DefaultMetadataLookupService implements MetadataLookupService, ObservableMetadataProvider.Observer { +public class DefaultMetadataLookupService + implements MetadataLookupService, ObservableMetadataProvider.Observer { private static final int MAX_RESULTS = 20; private static final Logger LOG = LoggerFactory.getLogger(DefaultMetadataLookupService.class); @@ -72,7 +73,7 @@ private void initializeMetadataSet() throws MetadataProviderException { final Instant startTime = Instant.now(); LOG.debug("Initializing IdP descriptor list from metadata"); - + Set newDescriptions = new HashSet<>(); for (String idpName : metadataManager.getIDPEntityNames()) { @@ -109,7 +110,8 @@ private IdpDescription descriptionFromMetadata(EntityDescriptor descriptor) { if (!uiInfo.getDisplayNames().isEmpty()) { result.setOrganizationName(uiInfo.getDisplayNames().get(0).getName().getLocalString()); - result.setDisplayNames(uiInfo.getDisplayNames().stream().map(dn -> dn.getName()).toList()); + result + .setDisplayNames(uiInfo.getDisplayNames().stream().map(dn -> dn.getName()).toList()); } } } @@ -147,8 +149,9 @@ public List lookupIdp(String text) { Predicate filterForDescriptions = description -> { if (description.getDisplayNames() != null) { - return description.getDisplayNames().stream() - .anyMatch(name -> name.getLocalString().toLowerCase().contains(textToFind)); + return description.getDisplayNames() + .stream() + .anyMatch(name -> name.getLocalString().toLowerCase().contains(textToFind)); } else { return description.getEntityId().toLowerCase().contains(textToFind); } @@ -164,23 +167,23 @@ public List lookupIdp(String text) { lock.readLock().lock(); return descriptions.stream() - .filter(filterForDescriptions) - .limit(MAX_RESULTS) - .map(description -> { - if (description.getDisplayNames() != null) { - List displayNames = description.getDisplayNames(); - - for (LocalizedString displayName : displayNames) { - String localString = displayName.getLocalString(); - if (localString.toLowerCase().contains(textToFind)) { - description.setOrganizationName(localString); - break; - } + .filter(filterForDescriptions) + .limit(MAX_RESULTS) + .map(description -> { + if (description.getDisplayNames() != null) { + List displayNames = description.getDisplayNames(); + + for (LocalizedString displayName : displayNames) { + String localString = displayName.getLocalString(); + if (localString.toLowerCase().contains(textToFind)) { + description.setOrganizationName(localString); + break; } } - return description; - }) - .collect(Collectors.toList()); + } + return description; + }) + .collect(Collectors.toList()); } finally { lock.readLock().unlock(); } diff --git a/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java b/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java index 94c00fe39..6f6225902 100644 --- a/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java +++ b/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java @@ -55,11 +55,11 @@ public class MetadataLookupServiceTests { public static final String IDP2_ENTITY_ID = "urn:test:idp2"; public static final String IDP3_ENTITY_ID = "urn:test:idp3"; public static final String IDP4_ENTITY_ID = "urn:test:idp4"; - + public static final String IDP1_ORGANIZATION_NAME = "IDP1 organization"; public static final String IDP2_ORGANIZATION_NAME = "IDP2 organization"; public static final String IDP4_ORGANIZATION_NAME = "IDP4 organization"; - + @Mock MetadataManager manager; @@ -103,7 +103,7 @@ public void setup() throws MetadataProviderException { .thenReturn(asList(idp2UIInfo)); when(idp4SsoExtensions.getUnknownXMLObjects(UIInfo.DEFAULT_ELEMENT_NAME)) - .thenReturn(asList(idp4UIInfo)); + .thenReturn(asList(idp4UIInfo)); when(idp1SsoDesc.getExtensions()).thenReturn(idp1SsoExtensions); @@ -116,7 +116,7 @@ public void setup() throws MetadataProviderException { when(idp2Desc.getEntityID()).thenReturn(IDP2_ENTITY_ID); when(idp2Desc.getIDPSSODescriptor(SAMLConstants.SAML20P_NS)).thenReturn(idp2SsoDesc); - + when(idp3Desc.getEntityID()).thenReturn(IDP3_ENTITY_ID); when(idp4Desc.getEntityID()).thenReturn(IDP4_ENTITY_ID); @@ -127,8 +127,8 @@ public void setup() throws MetadataProviderException { when(manager.getEntityDescriptor(IDP3_ENTITY_ID)).thenReturn(idp3Desc); when(manager.getEntityDescriptor(IDP4_ENTITY_ID)).thenReturn(idp4Desc); - when(manager.getIDPEntityNames()).thenReturn(Sets.newHashSet(IDP1_ENTITY_ID, IDP2_ENTITY_ID, - IDP3_ENTITY_ID, IDP4_ENTITY_ID)); + when(manager.getIDPEntityNames()) + .thenReturn(Sets.newHashSet(IDP1_ENTITY_ID, IDP2_ENTITY_ID, IDP3_ENTITY_ID, IDP4_ENTITY_ID)); } @@ -139,72 +139,72 @@ public void testServiceInitialization() throws MetadataProviderException { assertNotNull(service.listIdps()); List idps = service.listIdps(); - + assertThat(idps, hasSize(4)); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP1_ENTITY_ID)), hasProperty("organizationName", is(IDP1_ORGANIZATION_NAME))))); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP2_ENTITY_ID)), hasProperty("organizationName", is(IDP2_ORGANIZATION_NAME))))); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP3_ENTITY_ID)), hasProperty("organizationName", is(IDP3_ENTITY_ID))))); assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP4_ENTITY_ID)), hasProperty("organizationName", is(IDP4_ORGANIZATION_NAME))))); } - - + + @Test public void testEmptyMetadataInitialization() { when(manager.getIDPEntityNames()).thenReturn(emptySet()); DefaultMetadataLookupService service = new DefaultMetadataLookupService(manager); - + assertThat(service.listIdps(), hasSize(0)); } - + @Test public void testLookupByOrganizationNameWorks() { DefaultMetadataLookupService service = new DefaultMetadataLookupService(manager); - + List idps = service.lookupIdp(IDP1_ORGANIZATION_NAME); assertThat(idps, hasSize(1)); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP1_ENTITY_ID)), hasProperty("organizationName", is(IDP1_ORGANIZATION_NAME))))); } - + @Test public void testPartialLookupWorks() { DefaultMetadataLookupService service = new DefaultMetadataLookupService(manager); - + List idps = service.lookupIdp("idp"); assertThat(idps, hasSize(4)); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP1_ENTITY_ID)), hasProperty("organizationName", is(IDP1_ORGANIZATION_NAME))))); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP2_ENTITY_ID)), hasProperty("organizationName", is(IDP2_ORGANIZATION_NAME))))); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP3_ENTITY_ID)), hasProperty("organizationName", is(IDP3_ENTITY_ID))))); assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP4_ENTITY_ID)), hasProperty("organizationName", is(IDP4_ORGANIZATION_NAME))))); } - + @Test public void testEntityIdLookupWorks() { - + DefaultMetadataLookupService service = new DefaultMetadataLookupService(manager); List idps = service.lookupIdp(IDP1_ENTITY_ID); assertThat(idps, hasSize(1)); - + assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP1_ENTITY_ID)), hasProperty("organizationName", is(IDP1_ORGANIZATION_NAME))))); - + idps = service.lookupIdp("unknown"); assertThat(idps, hasSize(0)); } From 3da06be3bfdf7bc3cc6d28eb3ba66232124234c3 Mon Sep 17 00:00:00 2001 From: Federica Agostini Date: Thu, 17 Aug 2023 16:57:20 +0200 Subject: [PATCH 3/4] Define variable earlier --- .../mw/iam/authn/saml/DefaultMetadataLookupService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java index 060c8de74..3ab833ea6 100644 --- a/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java +++ b/iam-login-service/src/main/java/it/infn/mw/iam/authn/saml/DefaultMetadataLookupService.java @@ -170,9 +170,9 @@ public List lookupIdp(String text) { .filter(filterForDescriptions) .limit(MAX_RESULTS) .map(description -> { - if (description.getDisplayNames() != null) { - List displayNames = description.getDisplayNames(); - + List displayNames = description.getDisplayNames(); + if (displayNames != null) { + for (LocalizedString displayName : displayNames) { String localString = displayName.getLocalString(); if (localString.toLowerCase().contains(textToFind)) { From 83e59e43b0a005f8f920f1063003c2d8788b0b5a Mon Sep 17 00:00:00 2001 From: Federica Agostini Date: Thu, 17 Aug 2023 16:57:33 +0200 Subject: [PATCH 4/4] Add more tests following comments by Roberta --- .../saml/MetadataLookupServiceTests.java | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java b/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java index 6f6225902..5fdde229f 100644 --- a/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java +++ b/iam-login-service/src/test/java/it/infn/mw/iam/test/ext_authn/saml/MetadataLookupServiceTests.java @@ -76,17 +76,20 @@ public class MetadataLookupServiceTests { UIInfo idp1UIInfo, idp2UIInfo, idp4UIInfo; @Mock - DisplayName idp1DisplayName, idp2DisplayName, idp4DisplayName; + DisplayName idp1DisplayName, idp1ItDisplayName, idp2DisplayName, idp4DisplayName; @Mock - LocalizedString idp1LocalizedString, idp2LocalizedString, idp4LocalizedString; + LocalizedString idp1LocalizedString, idp1ItLocalizedString, idp2LocalizedString, + idp4LocalizedString; @Before public void setup() throws MetadataProviderException { when(idp1LocalizedString.getLocalString()).thenReturn(IDP1_ORGANIZATION_NAME); + when(idp1ItLocalizedString.getLocalString()).thenReturn("IDP1 organizzazione"); when(idp1DisplayName.getName()).thenReturn(idp1LocalizedString); - when(idp1UIInfo.getDisplayNames()).thenReturn(asList(idp1DisplayName)); + when(idp1ItDisplayName.getName()).thenReturn(idp1ItLocalizedString); + when(idp1UIInfo.getDisplayNames()).thenReturn(asList(idp1DisplayName, idp1ItDisplayName)); when(idp2LocalizedString.getLocalString()).thenReturn(IDP2_ORGANIZATION_NAME); when(idp2DisplayName.getName()).thenReturn(idp2LocalizedString); @@ -163,15 +166,29 @@ public void testEmptyMetadataInitialization() { assertThat(service.listIdps(), hasSize(0)); } + + @Test + public void testEmptyTextToFind() { + DefaultMetadataLookupService service = new DefaultMetadataLookupService(manager); + + List idps = service.lookupIdp("noMatchOnTextToFind"); + assertThat(idps, hasSize(0)); + } @Test public void testLookupByOrganizationNameWorks() { DefaultMetadataLookupService service = new DefaultMetadataLookupService(manager); - List idps = service.lookupIdp(IDP1_ORGANIZATION_NAME); - assertThat(idps, hasSize(1)); + List idpsIt = service.lookupIdp("organizz"); + assertThat(idpsIt, hasSize(1)); - assertThat(idps, hasItem(allOf(hasProperty("entityId", is(IDP1_ENTITY_ID)), + assertThat(idpsIt, hasItem(allOf(hasProperty("entityId", is(IDP1_ENTITY_ID)), + hasProperty("organizationName", is("IDP1 organizzazione"))))); + + List idpsEn = service.lookupIdp(IDP1_ORGANIZATION_NAME); + assertThat(idpsEn, hasSize(1)); + + assertThat(idpsEn, hasItem(allOf(hasProperty("entityId", is(IDP1_ENTITY_ID)), hasProperty("organizationName", is(IDP1_ORGANIZATION_NAME))))); }