From 15e55bb4990e6e09ce0671bcd0ee951bf8453fb9 Mon Sep 17 00:00:00 2001 From: Joseph Cosentino Date: Tue, 6 Feb 2024 15:12:01 -0800 Subject: [PATCH] refactor: policy validation on config change (#418) --- .../auth/ClientDevicesAuthService.java | 32 ++++++++----- .../configuration/GroupConfiguration.java | 46 +++++++++++-------- .../auth/ClientDevicesAuthServiceTest.java | 21 ++++----- .../auth/configuration/GroupManagerTest.java | 13 ++---- ...ml => groupConfigWithUnknownProperty.yaml} | 0 5 files changed, 62 insertions(+), 50 deletions(-) rename src/test/resources/com/aws/greengrass/clientdevices/auth/{badGroupConfig.yaml => groupConfigWithUnknownProperty.yaml} (100%) diff --git a/src/main/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthService.java b/src/main/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthService.java index 9aca40b13..275923517 100644 --- a/src/main/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthService.java +++ b/src/main/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthService.java @@ -5,7 +5,9 @@ package com.aws.greengrass.clientdevices.auth; +import com.amazon.aws.iot.greengrass.component.common.SerializerFactory; import com.aws.greengrass.authorization.AuthorizationHandler; +import com.aws.greengrass.authorization.exceptions.AuthorizationException; import com.aws.greengrass.clientdevices.auth.api.ClientDevicesAuthServiceApi; import com.aws.greengrass.clientdevices.auth.api.UseCases; import com.aws.greengrass.clientdevices.auth.certificate.CertificatesConfig; @@ -21,6 +23,7 @@ import com.aws.greengrass.clientdevices.auth.configuration.RuntimeConfiguration; import com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor; import com.aws.greengrass.clientdevices.auth.connectivity.ConnectivityInfoCache; +import com.aws.greengrass.clientdevices.auth.exception.PolicyException; import com.aws.greengrass.clientdevices.auth.infra.NetworkStateProvider; import com.aws.greengrass.clientdevices.auth.metrics.MetricsEmitter; import com.aws.greengrass.clientdevices.auth.metrics.handlers.AuthorizeClientDeviceActionsMetricHandler; @@ -44,7 +47,6 @@ import com.aws.greengrass.ipc.VerifyClientDeviceIdentityOperationHandler; import com.aws.greengrass.lifecyclemanager.PluginService; import com.aws.greengrass.util.Coerce; -import com.fasterxml.jackson.databind.MapperFeature; import com.fasterxml.jackson.databind.ObjectMapper; import software.amazon.awssdk.aws.greengrass.GreengrassCoreIPCService; @@ -68,6 +70,8 @@ @ImplementsService(name = ClientDevicesAuthService.CLIENT_DEVICES_AUTH_SERVICE_NAME) public class ClientDevicesAuthService extends PluginService { public static final String CLIENT_DEVICES_AUTH_SERVICE_NAME = "aws.greengrass.clientdevices.Auth"; + private static final ObjectMapper MAPPER = SerializerFactory.getConfigurationSerializerJson(); + private static final String KV_NODE = "node"; // TODO: Move configuration related constants to appropriate configuration class public static final String DEVICE_GROUPS_TOPICS = "deviceGroups"; @@ -174,7 +178,7 @@ private void configChangeHandler(WhatHappened whatHappened, Node node) { if (whatHappened == WhatHappened.timestampUpdated || whatHappened == WhatHappened.interiorAdded) { return; } - logger.atDebug().kv("why", whatHappened).kv("node", node).log(); + logger.atTrace().kv("why", whatHappened).kv(KV_NODE, node).log(); // NOTE: This should not live here. The service doesn't have to have knowledge about where/how // keys are stored Topics deviceGroupTopics = this.config.lookupTopics(CONFIGURATION_CONFIG_KEY, DEVICE_GROUPS_TOPICS); @@ -201,7 +205,7 @@ private void configChangeHandler(WhatHappened whatHappened, Node node) { } if (whatHappened == WhatHappened.initialized || node == null || node.childOf(DEVICE_GROUPS_TOPICS)) { - updateDeviceGroups(whatHappened, deviceGroupTopics); + updateDeviceGroups(deviceGroupTopics); } onConfigurationChanged(); @@ -229,7 +233,7 @@ public void postInject() { authorizationHandler.registerComponent(this.getName(), new HashSet<>( Arrays.asList(SUBSCRIBE_TO_CERTIFICATE_UPDATES, VERIFY_CLIENT_DEVICE_IDENTITY, GET_CLIENT_DEVICE_AUTH_TOKEN, AUTHORIZE_CLIENT_DEVICE_ACTION))); - } catch (com.aws.greengrass.authorization.exceptions.AuthorizationException e) { + } catch (AuthorizationException e) { logger.atError("initialize-cda-service-authorization-error", e) .log("Failed to initialize the client device auth service with the Authorization module."); } @@ -255,18 +259,24 @@ public CertificateManager getCertificateManager() { return context.get(CertificateManager.class); } - private void updateDeviceGroups(WhatHappened whatHappened, Topics deviceGroupsTopics) { - final ObjectMapper objectMapper = new ObjectMapper().enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS, - MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES); + private void updateDeviceGroups(Topics deviceGroupsTopics) { + GroupConfiguration groupConfiguration; try { - context.get(GroupManager.class).setGroupConfiguration( - objectMapper.convertValue(deviceGroupsTopics.toPOJO(), GroupConfiguration.class)); + groupConfiguration = MAPPER.convertValue(deviceGroupsTopics.toPOJO(), GroupConfiguration.class); } catch (IllegalArgumentException e) { - logger.atError().kv("event", whatHappened).kv("node", deviceGroupsTopics.getFullName()).setCause(e) - .log("Unable to parse group configuration"); serviceErrored(e); + return; } + + try { + groupConfiguration.validate(); + } catch (PolicyException e) { + serviceErrored(e); + return; + } + + context.get(GroupManager.class).setGroupConfiguration(groupConfiguration); } void updateCACertificateConfig(List caCerts) { diff --git a/src/main/java/com/aws/greengrass/clientdevices/auth/configuration/GroupConfiguration.java b/src/main/java/com/aws/greengrass/clientdevices/auth/configuration/GroupConfiguration.java index 4711be5ee..4284609cc 100644 --- a/src/main/java/com/aws/greengrass/clientdevices/auth/configuration/GroupConfiguration.java +++ b/src/main/java/com/aws/greengrass/clientdevices/auth/configuration/GroupConfiguration.java @@ -5,7 +5,7 @@ package com.aws.greengrass.clientdevices.auth.configuration; -import com.aws.greengrass.clientdevices.auth.exception.AuthorizationException; +import com.aws.greengrass.clientdevices.auth.exception.PolicyException; import com.aws.greengrass.logging.api.Logger; import com.aws.greengrass.logging.impl.LogManager; import com.aws.greengrass.util.Utils; @@ -15,12 +15,12 @@ import lombok.Value; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; @Value @JsonDeserialize(builder = GroupConfiguration.GroupConfigurationBuilder.class) @@ -45,31 +45,24 @@ public class GroupConfiguration { @Builder GroupConfiguration(ConfigurationFormatVersion formatVersion, Map definitions, - Map> policies) throws AuthorizationException { + Map> policies) { this.formatVersion = formatVersion == null ? ConfigurationFormatVersion.MAR_05_2021 : formatVersion; this.definitions = definitions == null ? Collections.emptyMap() : definitions; this.policies = policies == null ? Collections.emptyMap() : policies; - this.groupToPermissionsMap = constructGroupToPermissionsMap(); + this.groupToPermissionsMap = constructGroupPermissions(); } @JsonPOJOBuilder(withPrefix = "") public static class GroupConfigurationBuilder { } - private Map> constructGroupToPermissionsMap() throws AuthorizationException { - Map> groupToPermissionsMap = new HashMap<>(); - - for (Map.Entry groupDefinitionEntry : definitions.entrySet()) { - GroupDefinition groupDefinition = groupDefinitionEntry.getValue(); - if (!policies.containsKey(groupDefinition.getPolicyName())) { - throw new AuthorizationException( - String.format("Policies doesn't have policy named %s", groupDefinition.getPolicyName())); - } - groupToPermissionsMap.put(groupDefinitionEntry.getKey(), - constructGroupPermission(groupDefinitionEntry.getKey(), - policies.get(groupDefinition.getPolicyName()))); - } - return groupToPermissionsMap; + private Map> constructGroupPermissions() { + return definitions.entrySet().stream().collect(Collectors.toMap( + Map.Entry::getKey, + entry -> constructGroupPermission( + entry.getKey(), + policies.getOrDefault(entry.getValue().getPolicyName(), + Collections.emptyMap())))); } private Set constructGroupPermission(String groupName, @@ -114,4 +107,21 @@ private Set findPolicyVariables(String resource) { } return policyVariables; } + + /** + * Validate the deviceGroups configuration. + * + * @throws PolicyException if an invalid policy is detected + */ + public void validate() throws PolicyException { + String missingPolicy = definitions.values().stream() + .map(GroupDefinition::getPolicyName) + .filter(policyName -> !policies.containsKey(policyName)) + .findFirst() + .orElse(null); + if (missingPolicy != null) { + throw new PolicyException( + String.format("Policy definition %s does not have a corresponding policy", missingPolicy)); + } + } } diff --git a/src/test/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthServiceTest.java b/src/test/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthServiceTest.java index b119057c6..5a11f0bfd 100644 --- a/src/test/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthServiceTest.java +++ b/src/test/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthServiceTest.java @@ -16,8 +16,8 @@ import com.aws.greengrass.clientdevices.auth.configuration.GroupManager; import com.aws.greengrass.clientdevices.auth.configuration.Permission; import com.aws.greengrass.clientdevices.auth.configuration.RuntimeConfiguration; -import com.aws.greengrass.clientdevices.auth.exception.AuthorizationException; import com.aws.greengrass.clientdevices.auth.exception.CloudServiceInteractionException; +import com.aws.greengrass.clientdevices.auth.exception.PolicyException; import com.aws.greengrass.clientdevices.auth.exception.UseCaseException; import com.aws.greengrass.componentmanager.KernelConfigResolver; import com.aws.greengrass.config.Topic; @@ -29,7 +29,6 @@ import com.aws.greengrass.mqttclient.spool.SpoolerStoreException; import com.aws.greengrass.testcommons.testutilities.GGExtension; import com.aws.greengrass.util.GreengrassServiceClientFactory; -import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException; import org.hamcrest.collection.IsIterableContainingInAnyOrder; import org.hamcrest.collection.IsMapContaining; import org.hamcrest.collection.IsMapWithSize; @@ -166,14 +165,12 @@ void GIVEN_no_group_configuration_WHEN_start_service_change_THEN_empty_configura } @Test - void GIVEN_bad_group_configuration_WHEN_start_service_THEN_service_in_error_state(ExtensionContext context) - throws Exception { - ignoreExceptionOfType(context, IllegalArgumentException.class); - ignoreExceptionOfType(context, UnrecognizedPropertyException.class); - - startNucleusWithConfig("badGroupConfig.yaml", State.ERRORED); - - verify(groupManager, never()).setGroupConfiguration(any()); + void GIVEN_empty_group_with_unknown_property_WHEN_start_service_THEN_empty_group_created() throws Exception { + startNucleusWithConfig("groupConfigWithUnknownProperty.yaml", State.RUNNING); + verify(groupManager).setGroupConfiguration(configurationCaptor.capture()); + GroupConfiguration groupConfiguration = configurationCaptor.getValue(); + assertThat(groupConfiguration.getDefinitions(), IsMapWithSize.anEmptyMap()); + assertThat(groupConfiguration.getPolicies(), IsMapWithSize.anEmptyMap()); } @Test @@ -218,7 +215,7 @@ void GIVEN_valid_group_configuration_WHEN_start_service_THEN_instantiated_config void GIVEN_group_has_no_policy_WHEN_start_service_THEN_no_configuration_update(ExtensionContext context) throws Exception { ignoreExceptionOfType(context, IllegalArgumentException.class); - ignoreExceptionOfType(context, AuthorizationException.class); + ignoreExceptionOfType(context, PolicyException.class); startNucleusWithConfig("noGroupPolicyConfig.yaml", State.ERRORED); @@ -429,4 +426,4 @@ private X509Certificate pemToX509Certificate(String certPem) throws IOException, } return cert; } -} \ No newline at end of file +} diff --git a/src/test/java/com/aws/greengrass/clientdevices/auth/configuration/GroupManagerTest.java b/src/test/java/com/aws/greengrass/clientdevices/auth/configuration/GroupManagerTest.java index f973b8c7e..dfb3d3bbf 100644 --- a/src/test/java/com/aws/greengrass/clientdevices/auth/configuration/GroupManagerTest.java +++ b/src/test/java/com/aws/greengrass/clientdevices/auth/configuration/GroupManagerTest.java @@ -8,7 +8,6 @@ import com.aws.greengrass.clientdevices.auth.session.Session; import com.aws.greengrass.clientdevices.auth.session.SessionImpl; import com.aws.greengrass.clientdevices.auth.configuration.parser.ParseException; -import com.aws.greengrass.clientdevices.auth.exception.AuthorizationException; import com.aws.greengrass.clientdevices.auth.iot.Thing; import com.aws.greengrass.testcommons.testutilities.GGExtension; import org.junit.jupiter.api.Test; @@ -27,8 +26,7 @@ @ExtendWith({MockitoExtension.class, GGExtension.class}) public class GroupManagerTest { @Test - void GIVEN_emptyGroupConfiguration_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet() - throws AuthorizationException { + void GIVEN_emptyGroupConfiguration_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet() throws Exception { GroupManager groupManager = new GroupManager(); groupManager.setGroupConfiguration(new GroupConfiguration(null, null, null)); @@ -37,8 +35,7 @@ void GIVEN_emptyGroupConfiguration_WHEN_getApplicablePolicyPermissions_THEN_retu } @Test - void GIVEN_sessionInNoGroup_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet() - throws AuthorizationException, ParseException { + void GIVEN_sessionInNoGroup_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet() throws Exception { GroupConfiguration groupConfiguration = GroupConfiguration.builder() .definitions(Collections.singletonMap("group1", getGroupDefinition("differentThingName", "policy1"))) .policies(Collections.singletonMap("policy1", @@ -51,8 +48,7 @@ void GIVEN_sessionInNoGroup_WHEN_getApplicablePolicyPermissions_THEN_returnEmpty } @Test - void GIVEN_sessionInSingleGroup_WHEN_getApplicablePolicyPermissions_THEN_returnGroupPermissions() - throws AuthorizationException, ParseException { + void GIVEN_sessionInSingleGroup_WHEN_getApplicablePolicyPermissions_THEN_returnGroupPermissions() throws Exception { Session session = getSessionFromThing("thingName"); GroupConfiguration groupConfiguration = GroupConfiguration.builder() .definitions(Collections.singletonMap("group1", getGroupDefinition("thingName", "policy1"))).policies( @@ -70,8 +66,7 @@ void GIVEN_sessionInSingleGroup_WHEN_getApplicablePolicyPermissions_THEN_returnG } @Test - void GIVEN_sessionInMultipleGroups_WHEN_getApplicablePolicyPermissions_THEN_returnMergedGroupPermissions() - throws AuthorizationException, ParseException { + void GIVEN_sessionInMultipleGroups_WHEN_getApplicablePolicyPermissions_THEN_returnMergedGroupPermissions() throws Exception { Session session = getSessionFromThing("thingName"); GroupConfiguration groupConfiguration = GroupConfiguration.builder().definitions(new HashMap() {{ diff --git a/src/test/resources/com/aws/greengrass/clientdevices/auth/badGroupConfig.yaml b/src/test/resources/com/aws/greengrass/clientdevices/auth/groupConfigWithUnknownProperty.yaml similarity index 100% rename from src/test/resources/com/aws/greengrass/clientdevices/auth/badGroupConfig.yaml rename to src/test/resources/com/aws/greengrass/clientdevices/auth/groupConfigWithUnknownProperty.yaml