From 2fd8f8d592e32fb46ea2616398145745ded9a0c0 Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Thu, 14 Feb 2019 14:46:40 -0700 Subject: [PATCH 1/4] [SHIBUI-1058][SHIBUI-1230] Fixed a casting bug related to converting a boolean attribute to a string value. --- .../tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java index ae443a055..9e32d371c 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java @@ -106,7 +106,8 @@ public List getAttributeListFromRelyingPartyOverridesRepresentation(M case BOOLEAN: if (overrideProperty.getPersistType() != null && !overrideProperty.getPersistType().equalsIgnoreCase("boolean") && - ((entry.getValue() instanceof Boolean && (Boolean)entry.getValue()) || Boolean.valueOf((String)entry.getValue()))) { + ((entry.getValue() instanceof Boolean && (Boolean)entry.getValue()) || + ((entry.getValue() instanceof String) && Boolean.valueOf((String)entry.getValue())))) { list.add(attributeUtility.createAttributeWithStringValues(overrideProperty.getAttributeName(), overrideProperty.getAttributeFriendlyName(), overrideProperty.getPersistValue())); From 9cc005015c02fff28eb68ed8c35632050606b37f Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Thu, 14 Feb 2019 16:26:09 -0700 Subject: [PATCH 2/4] [SHIBUI-1058][SHIBUI-1231] Fixed a bug where we were persisting boolean relying party overrides improperly. We now only persist when the value from the UI is true. Note that we still persist based on how the override is defined in the yaml. In other words, the UI controls whether or not to persist and the yaml controls what is actually persisted. --- .../ui/service/JPAEntityServiceImpl.java | 29 ++++++++++--------- .../admin/ui/util/TestHelpers.groovy | 4 +-- .../admin/ui/util/TestObjectGenerator.groovy | 7 +---- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java index 9e32d371c..7d9e9388c 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityServiceImpl.java @@ -104,22 +104,23 @@ public List getAttributeListFromRelyingPartyOverridesRepresentation(M RelyingPartyOverrideProperty overrideProperty = overridePropertyList.stream().filter(op -> op.getName().equals(key)).findFirst().get(); switch (ModelRepresentationConversions.AttributeTypes.valueOf(overrideProperty.getDisplayType().toUpperCase())) { case BOOLEAN: - if (overrideProperty.getPersistType() != null && - !overrideProperty.getPersistType().equalsIgnoreCase("boolean") && - ((entry.getValue() instanceof Boolean && (Boolean)entry.getValue()) || - ((entry.getValue() instanceof String) && Boolean.valueOf((String)entry.getValue())))) { - list.add(attributeUtility.createAttributeWithStringValues(overrideProperty.getAttributeName(), - overrideProperty.getAttributeFriendlyName(), - overrideProperty.getPersistValue())); - } else { - if (entry.getValue() instanceof String) { - list.add(attributeUtility.createAttributeWithBooleanValue(overrideProperty.getAttributeName(), + if ((entry.getValue() instanceof Boolean && (Boolean)entry.getValue()) || + ((entry.getValue() instanceof String) && Boolean.valueOf((String)entry.getValue()))) { + if (overrideProperty.getPersistType() != null && + !overrideProperty.getPersistType().equalsIgnoreCase("boolean")) { + list.add(attributeUtility.createAttributeWithStringValues(overrideProperty.getAttributeName(), overrideProperty.getAttributeFriendlyName(), - Boolean.valueOf((String) entry.getValue()))); + overrideProperty.getPersistValue())); } else { - list.add(attributeUtility.createAttributeWithBooleanValue(overrideProperty.getAttributeName(), - overrideProperty.getAttributeFriendlyName(), - (Boolean) entry.getValue())); + if (entry.getValue() instanceof String) { + list.add(attributeUtility.createAttributeWithBooleanValue(overrideProperty.getAttributeName(), + overrideProperty.getAttributeFriendlyName(), + Boolean.valueOf((String) entry.getValue()))); + } else { + list.add(attributeUtility.createAttributeWithBooleanValue(overrideProperty.getAttributeName(), + overrideProperty.getAttributeFriendlyName(), + (Boolean) entry.getValue())); + } } } break; diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestHelpers.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestHelpers.groovy index 672618b30..0b175c5ad 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestHelpers.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestHelpers.groovy @@ -20,8 +20,8 @@ class TestHelpers { count += ((Collection)entry.value).size() != 0 ? 1 : 0 } else if (entry.value instanceof String) { count += StringUtils.isNotBlank((String)entry.value) ? 1 : 0 - } else { - count++ + } else if (entry.value instanceof Boolean) { + count += Boolean.valueOf((Boolean)entry.value) ? 1 : 0 } } diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy index 8eb8426f4..11b0d7b36 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy @@ -348,12 +348,7 @@ class TestObjectGenerator { customPropertiesConfiguration.getOverrides().each { override -> switch (ModelRepresentationConversions.AttributeTypes.valueOf(override.getDisplayType().toUpperCase())) { case ModelRepresentationConversions.AttributeTypes.BOOLEAN: - if (override.getPersistType() != null && - override.getPersistType() != override.getDisplayType()) { - representation.put(override.getName(), generator.randomString(30)) - } else { - representation.put(override.getName(), generator.randomBoolean()) - } + representation.put(override.getName(), generator.randomBoolean()) break case ModelRepresentationConversions.AttributeTypes.INTEGER: representation.put(override.getName(), generator.randomInt(0, 999999)) From 9d6d2c434181e560d6d4a4d45dc221e9f6d13021 Mon Sep 17 00:00:00 2001 From: Jj! Date: Thu, 14 Feb 2019 18:08:51 -0600 Subject: [PATCH 3/4] [SHIBUI-1058] unit test --- ...JPAEntityDescriptorServiceImplTests.groovy | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/AuxiliaryJPAEntityDescriptorServiceImplTests.groovy diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/AuxiliaryJPAEntityDescriptorServiceImplTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/AuxiliaryJPAEntityDescriptorServiceImplTests.groovy new file mode 100644 index 000000000..85f1202c6 --- /dev/null +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/AuxiliaryJPAEntityDescriptorServiceImplTests.groovy @@ -0,0 +1,158 @@ +package edu.internet2.tier.shibboleth.admin.ui.service + +import edu.internet2.tier.shibboleth.admin.ui.domain.EntityDescriptor +import edu.internet2.tier.shibboleth.admin.ui.domain.SPSSODescriptor +import edu.internet2.tier.shibboleth.admin.ui.domain.SingleLogoutService +import edu.internet2.tier.shibboleth.admin.ui.domain.frontend.EntityDescriptorRepresentation +import edu.internet2.tier.shibboleth.admin.ui.domain.frontend.LogoutEndpointRepresentation +import edu.internet2.tier.shibboleth.admin.ui.opensaml.OpenSamlObjects +import spock.lang.Shared +import spock.lang.Specification +import spock.lang.Unroll + +class AuxiliaryJPAEntityDescriptorServiceImplTests extends Specification { + @Shared + def openSAMLObjects = new OpenSamlObjects().with { + it.init() + it + } + + @Shared + JPAEntityDescriptorServiceImpl entityDescriptorService + + void setup() { + entityDescriptorService = new JPAEntityDescriptorServiceImpl(openSAMLObjects, null, null) + } + + def "simple test"() { + assert true + } + + @Unroll + def "test #method(#description)"() { + setup: + expected.setResourceId(starter.getResourceId()) + entityDescriptorService."$method"(starter, representation) + + expect: + assert starter == expected + + where: + [method, description, representation, starter, expected] << Data.getData(openSAMLObjects) + } + + static class Data { + static def getData(OpenSamlObjects openSAMLObjects) { + def data = [] + + data << new DataField( + method: 'setupLogout', + description: 'no change', + representation: new EntityDescriptorRepresentation(), + starter: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class), + expected: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class) + ) + data << new DataField( + method: 'setupLogout', + description: 'add logout', + representation: new EntityDescriptorRepresentation().with { + it.logoutEndpoints = [new LogoutEndpointRepresentation(bindingType: 'testBinding', url: 'testLocation')] + it + }, + starter: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class), + expected: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class).with { + it.getRoleDescriptors().add( + openSAMLObjects.buildDefaultInstanceOfType(SPSSODescriptor.class).with { + it.getSingleLogoutServices().add( + openSAMLObjects.buildDefaultInstanceOfType(SingleLogoutService.class).with { + it.binding = 'testBinding' + it.location = 'testLocation' + it + } + ) + it + } + ) + it + } + ) + data << new DataField( + method: 'setupLogout', + description: 'remove logout', + representation: new EntityDescriptorRepresentation(), + starter: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class).with { + it.getRoleDescriptors().add( + openSAMLObjects.buildDefaultInstanceOfType(SPSSODescriptor.class).with { + it.getSingleLogoutServices().add( + openSAMLObjects.buildDefaultInstanceOfType(SingleLogoutService.class).with { + it.binding = 'testBinding' + it.location = 'testLocation' + it + } + ) + it + } + ) + it + }, + expected: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class).with { + it.getRoleDescriptors().add(openSAMLObjects.buildDefaultInstanceOfType(SPSSODescriptor.class)) + it + } + ) + data << new DataField( + method: 'setupLogout', + description: 'modify logout', + representation: new EntityDescriptorRepresentation().with { + it.logoutEndpoints = [new LogoutEndpointRepresentation(bindingType: 'testBinding', url: 'testLocation')] + it + }, + starter: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class).with { + it.getRoleDescriptors().add( + openSAMLObjects.buildDefaultInstanceOfType(SPSSODescriptor.class).with { + it.getSingleLogoutServices().add( + openSAMLObjects.buildDefaultInstanceOfType(SingleLogoutService.class).with { + it.binding = 'startBinding' + it.location = 'startLocation' + it + } + ) + it + } + ) + it + }, + expected: openSAMLObjects.buildDefaultInstanceOfType(EntityDescriptor.class).with { + it.getRoleDescriptors().add( + openSAMLObjects.buildDefaultInstanceOfType(SPSSODescriptor.class).with { + it.getSingleLogoutServices().add( + openSAMLObjects.buildDefaultInstanceOfType(SingleLogoutService.class).with { + it.binding = 'testBinding' + it.location = 'testLocation' + it + } + ) + it + } + ) + it + }, + ) + + return data + } + + static class DataField implements Iterable { + String method + String description + EntityDescriptorRepresentation representation + EntityDescriptor starter + EntityDescriptor expected + + @Override + Iterator iterator() { + return [this.method, this.description, this.representation, this.starter, this.expected].iterator() + } + } + } +} From 254ca661cb3d61f888e01ed56336e1485593c9d8 Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Thu, 14 Feb 2019 18:36:33 -0700 Subject: [PATCH 4/4] [SHIBUI-1058] Updated boolean override to always be true if present. --- .../tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy index 11b0d7b36..5da927e25 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/util/TestObjectGenerator.groovy @@ -307,7 +307,7 @@ class TestObjectGenerator { override.getPersistType() != override.getDisplayType()) { attributes.add(attributeUtility.createAttributeWithStringValues(override.getAttributeName(), override.getAttributeFriendlyName(), generator.randomString(30))) } else { - attributes.add(attributeUtility.createAttributeWithBooleanValue(override.getAttributeName(), override.getAttributeFriendlyName(), generator.randomBoolean())) + attributes.add(attributeUtility.createAttributeWithBooleanValue(override.getAttributeName(), override.getAttributeFriendlyName(), true)) } break case ModelRepresentationConversions.AttributeTypes.INTEGER: