diff --git a/.checkov.baseline b/.checkov.baseline index 8b066203e..72fe61a69 100644 --- a/.checkov.baseline +++ b/.checkov.baseline @@ -62,12 +62,6 @@ "CKV_AWS_109", "CKV_AWS_111" ] - }, - { - "resource": "AWS::IAM::ManagedPolicy.PivotRolepolicy3", - "check_ids": [ - "CKV_AWS_109" - ] } ] }, diff --git a/backend/dataall/base/aws/iam.py b/backend/dataall/base/aws/iam.py index b3cc1c636..fdf32906c 100644 --- a/backend/dataall/base/aws/iam.py +++ b/backend/dataall/base/aws/iam.py @@ -1,8 +1,8 @@ import logging +from botocore.exceptions import ClientError from .sts import SessionHelper - log = logging.getLogger(__name__) @@ -16,12 +16,14 @@ def client(account_id: str, role=None): def get_role(account_id: str, role_arn: str, role=None): log.info(f"Getting IAM role = {role_arn}") try: - iamcli = IAM.client(account_id=account_id, role=role) - response = iamcli.get_role( + client = IAM.client(account_id=account_id, role=role) + response = client.get_role( RoleName=role_arn.split("/")[-1] ) assert response['Role']['Arn'] == role_arn, "Arn doesn't match the role name. Check Arn and try again." - except Exception as e: + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to get role {role_arn}: {e}') log.error( f'Failed to get role {role_arn} due to: {e}' ) @@ -33,71 +35,247 @@ def get_role(account_id: str, role_arn: str, role=None): def get_role_arn_by_name(account_id: str, role_name: str, role=None): log.info(f"Getting IAM role name= {role_name}") try: - iamcli = IAM.client(account_id=account_id, role=role) - response = iamcli.get_role( + client = IAM.client(account_id=account_id, role=role) + response = client.get_role( RoleName=role_name ) - except Exception as e: + return response["Role"]["Arn"] + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to get role {role_name}: {e}') log.error( f'Failed to get role {role_name} due to: {e}' ) return None - else: - return response["Role"]["Arn"] @staticmethod - def update_role_policy( - account_id: str, - role_name: str, - policy_name: str, - policy: str, + def get_role_policy( + account_id: str, + role_name: str, + policy_name: str, ): try: - iamcli = IAM.client(account_id) - iamcli.put_role_policy( + client = IAM.client(account_id) + response = client.get_role_policy( RoleName=role_name, PolicyName=policy_name, - PolicyDocument=policy, ) - except Exception as e: + return response["PolicyDocument"] + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to get policy {policy_name} of role {role_name}: {e}') log.error( - f'Failed to add S3 bucket access to target role {account_id}/{role_name} : {e}' + f'Failed to get policy {policy_name} of role {role_name} : {e}' ) - raise e + return None @staticmethod - def get_role_policy( - account_id: str, - role_name: str, - policy_name: str, + def delete_role_policy( + account_id: str, + role_name: str, + policy_name: str, ): try: - iamcli = IAM.client(account_id) - response = iamcli.get_role_policy( + client = IAM.client(account_id) + client.delete_role_policy( RoleName=role_name, PolicyName=policy_name, ) - except Exception as e: + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to delete policy {policy_name} of role {role_name}: {e}') log.error( - f'Failed to get policy {policy_name} of role {role_name} : {e}' + f'Failed to delete policy {policy_name} of role {role_name} : {e}' + ) + + @staticmethod + def get_managed_policy_by_name( + account_id: str, + policy_name: str + ): + try: + arn = f'arn:aws:iam::{account_id}:policy/{policy_name}' + client = IAM.client(account_id) + response = client.get_policy(PolicyArn=arn) + return response['Policy'] + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception( + f'Data.all Environment Pivot Role does not have permissions to to get policy {policy_name}: {e}') + log.error( + f'Failed to get policy {policy_name}: {e}' ) return None - else: - return response["PolicyDocument"] @staticmethod - def delete_role_policy( - account_id: str, - role_name: str, - policy_name: str, + def create_managed_policy( + account_id: str, + policy_name: str, + policy: str ): try: - iamcli = IAM.client(account_id) - iamcli.delete_role_policy( - RoleName=role_name, + client = IAM.client(account_id) + response = client.create_policy( PolicyName=policy_name, + PolicyDocument=policy, + ) + arn = response['Policy']['Arn'] + log.info( + f'Created managed policy {arn}' + ) + return arn + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to create managed policy {policy_name}: {e}') + raise Exception(f'Failed to create managed policy {policy_name} : {e}') + + @staticmethod + def delete_managed_policy_by_name( + account_id: str, + policy_name + ): + try: + arn = f'arn:aws:iam::{account_id}:policy/{policy_name}' + client = IAM.client(account_id) + client.delete_policy( + PolicyArn=arn + ) + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to delete managed policy {policy_name}: {e}') + raise Exception(f'Failed to delete managed policy {policy_name} : {e}') + + @staticmethod + def get_managed_policy_default_version( + account_id: str, + policy_name: str + ): + try: + arn = f'arn:aws:iam::{account_id}:policy/{policy_name}' + client = IAM.client(account_id) + response = client.get_policy(PolicyArn=arn) + versionId = response['Policy']['DefaultVersionId'] + policyVersion = client.get_policy_version(PolicyArn=arn, VersionId=versionId) + policyDocument = policyVersion['PolicyVersion']['Document'] + return versionId, policyDocument + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to get policy {policy_name}: {e}') + log.error(f'Failed to get policy {policy_name} : {e}') + return None, None + + @staticmethod + def update_managed_policy_default_version( + account_id: str, + policy_name: str, + old_version_id: str, + policy_document: str): + try: + arn = f'arn:aws:iam::{account_id}:policy/{policy_name}' + client = IAM.client(account_id) + client.create_policy_version( + PolicyArn=arn, + PolicyDocument=policy_document, + SetAsDefault=True + ) + + client.delete_policy_version(PolicyArn=arn, VersionId=old_version_id) + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception( + f'Data.all Environment Pivot Role does not have permissions to update policy {policy_name}: {e}') + raise Exception(f'Failed to update policy {policy_name} : {e}') + + @staticmethod + def delete_managed_policy_non_default_versions( + account_id: str, + policy_name: str, + ): + try: + arn = f'arn:aws:iam::{account_id}:policy/{policy_name}' + client = IAM.client(account_id) + + # List all policy versions + paginator = client.get_paginator('list_policy_versions') + pages = paginator.paginate( + PolicyArn=arn + ) + versions = [] + for page in pages: + versions += page['Versions'] + non_default_versions = [version['VersionId'] for version in versions if version['IsDefaultVersion'] is False] + # Delete all non-default versions + for version_id in non_default_versions: + client.delete_policy_version(PolicyArn=arn, VersionId=version_id) + + return True + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception(f'Data.all Environment Pivot Role does not have permissions to get policy {policy_name}: {e}') + log.error(f'Failed to get policy {policy_name} : {e}') + return None, None + + @staticmethod + def is_policy_attached( + account_id: str, + policy_name: str, + role_name: str + ): + try: + client = IAM.client(account_id) + paginator = client.get_paginator('list_attached_role_policies') + pages = paginator.paginate( + RoleName=role_name ) - except Exception as e: + policies = [] + for page in pages: + policies += page['AttachedPolicies'] + return policy_name in [p['PolicyName'] for p in policies] + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception( + f'Data.all Environment Pivot Role does not have permissions to to get the list of attached policies to the role {role_name}: {e}') log.error( - f'Failed to delete policy {policy_name} of role {role_name} : {e}' + f'Failed to get the list of attached policies to the role {role_name}: {e}' + ) + return False + + @staticmethod + def attach_role_policy( + account_id, + role_name, + policy_arn + ): + try: + client = IAM.client(account_id) + response = client.attach_role_policy( + RoleName=role_name, + PolicyArn=policy_arn + ) + return True + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception( + f'Data.all Environment Pivot Role does not have permissions to to attach policy {policy_arn} to the role {role_name}: {e}') + log.error( + f'Failed to attach policy {policy_arn} to the role {role_name}: {e}' + ) + raise e + + @staticmethod + def detach_policy_from_role( + account_id: str, + role_name: str, + policy_name: str): + try: + arn = f'arn:aws:iam::{account_id}:policy/{policy_name}' + client = IAM.client(account_id) + client.detach_role_policy( + RoleName=role_name, + PolicyArn=arn ) + except ClientError as e: + if e.response['Error']['Code'] == 'AccessDenied': + raise Exception( + f'Data.all Environment Pivot Role does not have permissions to detach policy {policy_name} from role {role_name}: {e}') + raise Exception(f'Failed to detach policy {policy_name} from role {role_name}: {e}') diff --git a/backend/dataall/base/utils/naming_convention.py b/backend/dataall/base/utils/naming_convention.py index 6de387f18..51743f229 100644 --- a/backend/dataall/base/utils/naming_convention.py +++ b/backend/dataall/base/utils/naming_convention.py @@ -6,7 +6,8 @@ class NamingConventionPattern(Enum): S3 = {'regex': '[^a-zA-Z0-9-]', 'separator': '-', 'max_length': 63} - IAM = {'regex': '[^a-zA-Z0-9-_]', 'separator': '-', 'max_length': 63} + IAM = {'regex': '[^a-zA-Z0-9-_]', 'separator': '-', 'max_length': 63} # Role names up to 64 chars + IAM_POLICY = {'regex': '[^a-zA-Z0-9-_]', 'separator': '-', 'max_length': 128} # Policy names up to 128 chars GLUE = {'regex': '[^a-zA-Z0-9_]', 'separator': '_', 'max_length': 240} # Limit 255 - 15 extra chars buffer GLUE_ETL = {'regex': '[^a-zA-Z0-9-]', 'separator': '-', 'max_length': 52} NOTEBOOK = {'regex': '[^a-zA-Z0-9-]', 'separator': '-', 'max_length': 63} diff --git a/backend/dataall/core/environment/api/input_types.py b/backend/dataall/core/environment/api/input_types.py index 8662408f4..d13d7e056 100644 --- a/backend/dataall/core/environment/api/input_types.py +++ b/backend/dataall/core/environment/api/input_types.py @@ -106,6 +106,8 @@ class EnvironmentSortField(GraphQLEnumMapper): gql.Argument('groupUri', gql.NonNullableType(gql.String)), gql.Argument('IAMRoleArn', gql.NonNullableType(gql.String)), gql.Argument('environmentUri', gql.NonNullableType(gql.String)), + gql.Argument('dataallManaged', gql.NonNullableType(gql.Boolean)), + ], ) diff --git a/backend/dataall/core/environment/api/resolvers.py b/backend/dataall/core/environment/api/resolvers.py index c0cab2f11..6d050bb5c 100644 --- a/backend/dataall/core/environment/api/resolvers.py +++ b/backend/dataall/core/environment/api/resolvers.py @@ -12,6 +12,7 @@ from dataall.base.aws.sts import SessionHelper from dataall.base.utils import Parameter from dataall.core.environment.db.environment_models import Environment, EnvironmentGroup +from dataall.core.environment.services.managed_iam_policies import PolicyManager from dataall.core.environment.services.environment_resource_manager import EnvironmentResourceManager from dataall.core.environment.services.environment_service import EnvironmentService from dataall.core.environment.api.enums import EnvironmentPermission @@ -39,7 +40,8 @@ def get_trust_account(context: Context, source, **kwargs): def get_pivot_role_as_part_of_environment(context: Context, source, **kwargs): - ssm_param = ParameterStoreManager.get_parameter_value(region=os.getenv('AWS_REGION', 'eu-west-1'), parameter_path=f"/dataall/{os.getenv('envname', 'local')}/pivotRole/enablePivotRoleAutoCreate") + ssm_param = ParameterStoreManager.get_parameter_value(region=os.getenv('AWS_REGION', 'eu-west-1'), + parameter_path=f"/dataall/{os.getenv('envname', 'local')}/pivotRole/enablePivotRoleAutoCreate") return True if ssm_param == "True" else False @@ -122,7 +124,7 @@ def create_environment(context: Context, source, input={}): def update_environment( - context: Context, source, environmentUri: str = None, input: dict = None + context: Context, source, environmentUri: str = None, input: dict = None ): if input.get('SamlGroupName') and input.get('SamlGroupName') not in context.groups: raise exceptions.UnauthorizedOperation( @@ -233,7 +235,7 @@ def update_consumption_role(context: Context, source, environmentUri=None, consu def list_environment_invited_groups( - context: Context, source, environmentUri=None, filter=None + context: Context, source, environmentUri=None, filter=None ): if filter is None: filter = {} @@ -257,7 +259,7 @@ def list_environment_groups(context: Context, source, environmentUri=None, filte def list_all_environment_groups( - context: Context, source, environmentUri=None, filter=None + context: Context, source, environmentUri=None, filter=None ): if filter is None: filter = {} @@ -270,7 +272,7 @@ def list_all_environment_groups( def list_environment_consumption_roles( - context: Context, source, environmentUri=None, filter=None + context: Context, source, environmentUri=None, filter=None ): if filter is None: filter = {} @@ -283,7 +285,7 @@ def list_environment_consumption_roles( def list_all_environment_consumption_roles( - context: Context, source, environmentUri=None, filter=None + context: Context, source, environmentUri=None, filter=None ): if filter is None: filter = {} @@ -296,9 +298,9 @@ def list_all_environment_consumption_roles( def list_environment_group_invitation_permissions( - context: Context, - source, - environmentUri=None, + context: Context, + source, + environmentUri=None, ): with context.engine.scoped_session() as session: return EnvironmentService.list_group_invitation_permissions( @@ -331,7 +333,7 @@ def list_groups(context: Context, source, filter=None): def list_consumption_roles( - context: Context, source, environmentUri=None, filter=None + context: Context, source, environmentUri=None, filter=None ): if filter is None: filter = {} @@ -343,7 +345,7 @@ def list_consumption_roles( def list_environment_networks( - context: Context, source, environmentUri=None, filter=None + context: Context, source, environmentUri=None, filter=None ): if filter is None: filter = {} @@ -360,6 +362,17 @@ def get_parent_organization(context: Context, source, **kwargs): return org +def get_policies(context: Context, source, **kwargs): + with context.engine.scoped_session() as session: + environment = EnvironmentService.get_environment_by_uri(session, source.environmentUri) + return PolicyManager( + role_name=source.IAMRoleName, + environmentUri=environment.environmentUri, + account=environment.AwsAccountId, + resource_prefix=environment.resourcePrefix + ).get_all_policies() + + def resolve_environment_networks(context: Context, source, **kwargs): return VpcService.get_environment_networks(environment_uri=source.environmentUri) @@ -392,7 +405,7 @@ def resolve_user_role(context: Context, source: Environment): def list_environment_group_permissions( - context, source, environmentUri: str = None, groupUri: str = None + context, source, environmentUri: str = None, groupUri: str = None ): with context.engine.scoped_session() as session: return EnvironmentService.list_group_permissions( @@ -404,7 +417,7 @@ def list_environment_group_permissions( @is_feature_enabled('core.features.env_aws_actions') def _get_environment_group_aws_session( - session, username, groups, environment, groupUri=None + session, username, groups, environment, groupUri=None ): if groupUri and groupUri not in groups: raise exceptions.UnauthorizedOperation( @@ -452,10 +465,10 @@ def _get_environment_group_aws_session( @is_feature_enabled('core.features.env_aws_actions') def get_environment_assume_role_url( - context: Context, - source, - environmentUri: str = None, - groupUri: str = None, + context: Context, + source, + environmentUri: str = None, + groupUri: str = None, ): with context.engine.scoped_session() as session: ResourcePolicy.check_user_resource_permission( @@ -481,7 +494,7 @@ def get_environment_assume_role_url( @is_feature_enabled('core.features.env_aws_actions') def generate_environment_access_token( - context, source, environmentUri: str = None, groupUri: str = None + context, source, environmentUri: str = None, groupUri: str = None ): with context.engine.scoped_session() as session: ResourcePolicy.check_user_resource_permission( @@ -515,7 +528,7 @@ def get_environment_stack(context: Context, source: Environment, **kwargs): def delete_environment( - context: Context, source, environmentUri: str = None, deleteFromAWS: bool = False + context: Context, source, environmentUri: str = None, deleteFromAWS: bool = False ): with context.engine.scoped_session() as session: environment = EnvironmentService.get_environment_by_uri(session, environmentUri) @@ -537,7 +550,7 @@ def delete_environment( def enable_subscriptions( - context: Context, source, environmentUri: str = None, input: dict = None + context: Context, source, environmentUri: str = None, input: dict = None ): with context.engine.scoped_session() as session: ResourcePolicy.check_user_resource_permission( diff --git a/backend/dataall/core/environment/api/types.py b/backend/dataall/core/environment/api/types.py index 5286cbd6a..2cde4ef06 100644 --- a/backend/dataall/core/environment/api/types.py +++ b/backend/dataall/core/environment/api/types.py @@ -152,6 +152,15 @@ ], ) +RoleManagedPolicy = gql.ObjectType( + name='RoleManagedPolicy', + fields=[ + gql.Field(name='policy_name', type=gql.String), + gql.Field(name='policy_type', type=gql.String), + gql.Field(name='exists', type=gql.Boolean), + gql.Field(name='attached', type=gql.Boolean), + ], +) ConsumptionRole = gql.ObjectType( name='ConsumptionRole', @@ -162,9 +171,15 @@ gql.Field(name='environmentUri', type=gql.String), gql.Field(name='IAMRoleArn', type=gql.String), gql.Field(name='IAMRoleName', type=gql.String), + gql.Field(name='dataallManaged', type=gql.Boolean), gql.Field(name='created', type=gql.String), gql.Field(name='updated', type=gql.String), gql.Field(name='deleted', type=gql.String), + gql.Field( + name='managedPolicies', + type=gql.ArrayType(RoleManagedPolicy), + resolver=get_policies + ), ], ) diff --git a/backend/dataall/core/environment/cdk/environment_stack.py b/backend/dataall/core/environment/cdk/environment_stack.py index e55971c45..8a8c6e89a 100644 --- a/backend/dataall/core/environment/cdk/environment_stack.py +++ b/backend/dataall/core/environment/cdk/environment_stack.py @@ -21,6 +21,7 @@ from dataall.core.stacks.services.runtime_stacks_tagging import TagsUtil from dataall.core.environment.db.environment_models import Environment, EnvironmentGroup from dataall.core.environment.services.environment_service import EnvironmentService +from dataall.core.environment.services.managed_iam_policies import PolicyManager from dataall.base.cdkproxy.stacks.manager import stack from dataall.core.environment.cdk.pivot_role_stack import PivotRole from dataall.core.environment.cdk.env_role_core_policies.data_policy import S3Policy @@ -402,13 +403,13 @@ def create_or_import_environment_groups_roles(self): for group in self.environment_groups: if not group.environmentIAMRoleImported: group_role = self.create_group_environment_role(group=group, id=f'{group.environmentIAMRoleName}') - group_roles.append(group_role) else: - iam.Role.from_role_arn( + group_role = iam.Role.from_role_arn( self, f'{group.groupUri + group.environmentIAMRoleName}', role_arn=group.environmentIAMRoleArn, ) + group_roles.append(group_role) return group_roles def create_group_environment_role(self, group: EnvironmentGroup, id: str): @@ -431,6 +432,28 @@ def create_group_environment_role(self, group: EnvironmentGroup, id: str): permissions=group_permissions, ).generate_policies() + external_managed_policies = [] + policy_manager = PolicyManager( + environmentUri=self._environment.environmentUri, + resource_prefix=self._environment.resourcePrefix, + role_name=group.environmentIAMRoleName, + account=self._environment.AwsAccountId + ) + try: + managed_policies = policy_manager.get_all_policies() + except Exception as e: + logger.info(f"Not adding any managed policy because of exception: {e}") + # Known exception raised in first deployment because pivot role does not exist and cannot be assumed + managed_policies = [] + for policy in managed_policies: + # If there is a managed policy that exist it should be attached to the IAM role + if policy.get("exists", False): + external_managed_policies.append(iam.ManagedPolicy.from_managed_policy_name( + self, + id=f'{self._environment.resourcePrefix}-managed-policy-{policy.get("policy_name")}', + managed_policy_name=policy.get("policy_name") + )) + with self.engine.scoped_session() as session: data_policy = S3Policy( stack=self, @@ -452,7 +475,7 @@ def create_group_environment_role(self, group: EnvironmentGroup, id: str): inline_policies={ f'{group.environmentIAMRoleName}DataPolicy': data_policy.document, }, - managed_policies=services_policies, + managed_policies=services_policies + external_managed_policies, assumed_by=iam.CompositePrincipal( iam.ServicePrincipal('glue.amazonaws.com'), iam.ServicePrincipal('lambda.amazonaws.com'), diff --git a/backend/dataall/core/environment/db/environment_models.py b/backend/dataall/core/environment/db/environment_models.py index 658e02a91..c5ac4d101 100644 --- a/backend/dataall/core/environment/db/environment_models.py +++ b/backend/dataall/core/environment/db/environment_models.py @@ -84,6 +84,7 @@ class ConsumptionRole(Base): groupUri = Column(String, nullable=False) IAMRoleName = Column(String, nullable=False) IAMRoleArn = Column(String, nullable=False) + dataallManaged = Column(Boolean, nullable=False, default=True) created = Column(DateTime, default=datetime.datetime.now) updated = Column(DateTime, onupdate=datetime.datetime.now) deleted = Column(DateTime) diff --git a/backend/dataall/core/environment/services/environment_service.py b/backend/dataall/core/environment/services/environment_service.py index b4751851f..8db77dd24 100644 --- a/backend/dataall/core/environment/services/environment_service.py +++ b/backend/dataall/core/environment/services/environment_service.py @@ -30,6 +30,7 @@ from dataall.core.stacks.db.keyvaluetag_repositories import KeyValueTag from dataall.core.stacks.db.stack_models import Stack from dataall.core.stacks.db.enums import StackStatus +from dataall.core.environment.services.managed_iam_policies import PolicyManager log = logging.getLogger(__name__) @@ -148,7 +149,7 @@ def _validate_creation_params(data, uri, session): @staticmethod def _validate_resource_prefix(data): if data.get('resourcePrefix') and not bool( - re.match(r'^[a-z-]+$', data.get('resourcePrefix')) + re.match(r'^[a-z-]+$', data.get('resourcePrefix')) ): raise exceptions.InvalidInput( 'resourcePrefix', @@ -158,7 +159,9 @@ def _validate_resource_prefix(data): @staticmethod def _validate_account_region(data, session): - environment = EnvironmentRepository.find_environment_by_account_region(session=session, account_id=data.get('AwsAccountId'), region=data.get('region')) + environment = EnvironmentRepository.find_environment_by_account_region(session=session, + account_id=data.get('AwsAccountId'), + region=data.get('region')) if environment: raise exceptions.InvalidInput( 'AwsAccount/region', @@ -226,7 +229,6 @@ def invite_group(session, uri, data=None) -> (Environment, EnvironmentGroup): action='INVITE_TEAM', message=f'Team {group} is already a member of the environment {environment.name}', ) - if data.get('environmentIAMRoleArn'): env_group_iam_role_arn = data['environmentIAMRoleArn'] env_group_iam_role_name = data['environmentIAMRoleArn'].split("/")[-1] @@ -241,6 +243,15 @@ def invite_group(session, uri, data=None) -> (Environment, EnvironmentGroup): env_group_iam_role_arn = f'arn:aws:iam::{environment.AwsAccountId}:role/{env_group_iam_role_name}' env_role_imported = False + # If environment role is imported, then data.all should attach the policies at import time + # If environment role is created in environment stack, then data.all should attach the policies in the env stack + PolicyManager( + role_name=env_group_iam_role_name, + environmentUri=environment.environmentUri, + account=environment.AwsAccountId, + resource_prefix=environment.resourcePrefix + ).create_all_policies(managed=env_role_imported) + athena_workgroup = NamingConventionService( target_uri=environment.environmentUri, target_label=group, @@ -266,6 +277,7 @@ def invite_group(session, uri, data=None) -> (Environment, EnvironmentGroup): permissions=data['permissions'], resource_type=Environment.__name__, ) + return environment, environment_group @staticmethod @@ -331,6 +343,14 @@ def remove_group(session, uri, group): group_membership = EnvironmentService.find_environment_group( session, group, environment.environmentUri ) + + PolicyManager( + role_name=group_membership.environmentIAMRoleName, + environmentUri=environment.environmentUri, + account=environment.AwsAccountId, + resource_prefix=environment.resourcePrefix + ).delete_all_policies() + if group_membership: session.delete(group_membership) session.commit() @@ -398,7 +418,7 @@ def list_group_permissions_internal(session, uri, group_uri): @staticmethod def list_group_invitation_permissions( - session, username, groups, uri, data=None, check_perm=None + session, username, groups, uri, data=None, check_perm=None ): group_invitation_permissions = [] for p in permissions.ENVIRONMENT_INVITATION_REQUEST: @@ -435,8 +455,16 @@ def add_consumption_role(session, uri, data=None) -> (Environment, EnvironmentGr groupUri=group, IAMRoleArn=IAMRoleArn, IAMRoleName=IAMRoleArn.split("/")[-1], + dataallManaged=data['dataallManaged'] ) + PolicyManager( + role_name=consumption_role.IAMRoleName, + environmentUri=environment.environmentUri, + account=environment.AwsAccountId, + resource_prefix=environment.resourcePrefix + ).create_all_policies(managed=consumption_role.dataallManaged) + session.add(consumption_role) session.commit() @@ -454,6 +482,7 @@ def add_consumption_role(session, uri, data=None) -> (Environment, EnvironmentGr @has_resource_permission(permissions.REMOVE_ENVIRONMENT_CONSUMPTION_ROLE) def remove_consumption_role(session, uri, env_uri): consumption_role = EnvironmentService.get_environment_consumption_role(session, uri, env_uri) + environment = EnvironmentService.get_environment_by_uri(session, env_uri) num_resources = EnvironmentResourceManager.count_consumption_role_resources(session, uri) if num_resources > 0: @@ -463,15 +492,23 @@ def remove_consumption_role(session, uri, env_uri): ) if consumption_role: + PolicyManager( + role_name=consumption_role.IAMRoleName, + environmentUri=environment.environmentUri, + account=environment.AwsAccountId, + resource_prefix=environment.resourcePrefix + ).delete_all_policies() + + ResourcePolicy.delete_resource_policy( + session=session, + group=consumption_role.groupUri, + resource_uri=consumption_role.consumptionRoleUri, + resource_type=ConsumptionRole.__name__, + ) + session.delete(consumption_role) session.commit() - ResourcePolicy.delete_resource_policy( - session=session, - group=consumption_role.groupUri, - resource_uri=consumption_role.consumptionRoleUri, - resource_type=ConsumptionRole.__name__, - ) return True @staticmethod @@ -776,7 +813,7 @@ def query_all_environment_consumption_roles(session, uri, filter) -> Query: @staticmethod @has_resource_permission(permissions.LIST_ENVIRONMENT_CONSUMPTION_ROLES) def paginated_all_environment_consumption_roles( - session, uri, data=None + session, uri, data=None ) -> dict: return paginate( query=EnvironmentService.query_all_environment_consumption_roles( @@ -940,6 +977,16 @@ def delete_environment(session, uri, environment): message=f'Found {env_resources} resources on environment {environment.label} - Delete all environment related objects before proceeding', ) else: + PolicyManager( + role_name=environment.EnvironmentDefaultIAMRoleName, + environmentUri=environment.environmentUri, + account=environment.AwsAccountId, + resource_prefix=environment.resourcePrefix + ).delete_all_policies() + + KeyValueTag.delete_key_value_tags( + session, environment.environmentUri, 'environment' + ) EnvironmentResourceManager.delete_env(session, environment) EnvironmentParameterRepository(session).delete_params(environment.environmentUri) @@ -955,10 +1002,6 @@ def delete_environment(session, uri, environment): for role in env_roles: session.delete(role) - KeyValueTag.delete_key_value_tags( - session, environment.environmentUri, 'environment' - ) - return session.delete(environment) @staticmethod diff --git a/backend/dataall/core/environment/services/managed_iam_policies.py b/backend/dataall/core/environment/services/managed_iam_policies.py new file mode 100644 index 000000000..7a1c8d3bd --- /dev/null +++ b/backend/dataall/core/environment/services/managed_iam_policies.py @@ -0,0 +1,166 @@ +from typing import List +import logging +import json +from abc import ABC, abstractmethod +from dataall.base.aws.iam import IAM + +logger = logging.getLogger(__name__) + + +class ManagedPolicy(ABC): + """ + Abstract class for any IAM Managed policy that needs to be atached to data.all team IAM roles and consumption roles + """ + + @abstractmethod + def __init__( + self, + role_name, + account, + environmentUri, + resource_prefix + ): + self.role_name = role_name + self.account = account + self.environmentUri = environmentUri + self.resource_prefix = resource_prefix + + @property + @abstractmethod + def policy_type(self): + """ + Returns string and needs to be implemented in the ManagedPolicies inherited classes + """ + raise NotImplementedError + + @abstractmethod + def generate_policy_name(self) -> str: + """ + Returns string and needs to be implemented in the ManagedPolicies inherited classes + """ + raise NotImplementedError + + @abstractmethod + def generate_empty_policy(self) -> dict: + """ + Returns dict and needs to be implemented in the ManagedPolicies inherited classes + """ + raise NotImplementedError + + def check_if_policy_exists(self) -> bool: + policy_name = self.generate_policy_name() + share_policy = IAM.get_managed_policy_by_name(self.account, policy_name) + return (share_policy is not None) + + def check_if_policy_attached(self): + policy_name = self.generate_policy_name() + return IAM.is_policy_attached(self.account, policy_name, self.role_name) + + def attach_policy(self): + policy_arn = f"arn:aws:iam::{self.account}:policy/{self.generate_policy_name()}" + try: + IAM.attach_role_policy( + self.account, + self.role_name, + policy_arn + ) + except Exception as e: + raise Exception( + f"Required customer managed policy {policy_arn} can't be attached: {e}") + + +class PolicyManager(object): + def __init__( + self, + role_name, + account, + environmentUri, + resource_prefix, + ): + self.role_name = role_name + self.account = account + self.environmentUri = environmentUri + self.resource_prefix = resource_prefix + self.ManagedPolicies = ManagedPolicy.__subclasses__() + logger.info(f'Found {len(self.ManagedPolicies)} subclasses of ManagedPolicy') + self.initializedPolicies = [self._initialize_policy(policy) for policy in self.ManagedPolicies] + + def _initialize_policy(self, managedPolicy): + return managedPolicy( + role_name=self.role_name, + account=self.account, + environmentUri=self.environmentUri, + resource_prefix=self.resource_prefix + ) + + def create_all_policies(self, managed) -> bool: + """ + Manager that registers and calls all policies created by data.all modules and that + need to be created for consumption roles and team roles + """ + try: + for Policy in self.initializedPolicies: + empty_policy = Policy.generate_empty_policy() + policy_name = Policy.generate_policy_name() + logger.info(f'Creating policy {policy_name}') + + IAM.create_managed_policy( + account_id=self.account, + policy_name=policy_name, + policy=json.dumps(empty_policy) + ) + + if managed: + IAM.attach_role_policy( + account_id=self.account, + role_name=self.role_name, + policy_arn=f"arn:aws:iam::{self.account}:policy/{policy_name}" + ) + except Exception as e: + raise e + return True + + def delete_all_policies(self) -> bool: + """ + Manager that registers and calls all policies created by data.all modules and that + need to be deleted for consumption roles and team roles + """ + try: + for Policy in self.initializedPolicies: + policy_name = Policy.generate_policy_name() + logger.info(f'Deleting policy {policy_name}') + if Policy.check_if_policy_attached(): + IAM.detach_policy_from_role( + account_id=self.account, + role_name=self.role_name, + policy_name=policy_name + ) + if Policy.check_if_policy_exists(): + IAM.delete_managed_policy_non_default_versions( + account_id=self.account, + policy_name=policy_name + ) + IAM.delete_managed_policy_by_name( + account_id=self.account, + policy_name=policy_name + ) + except Exception as e: + raise e + return True + + def get_all_policies(self) -> List[dict]: + """ + Manager that registers and calls all policies created by data.all modules and that + need to be listed for consumption roles and team roles + """ + all_policies = [] + for Policy in self.initializedPolicies: + policy_dict = { + "policy_name": Policy.generate_policy_name(), + "policy_type": Policy.policy_type, + "exists": Policy.check_if_policy_exists(), + "attached": Policy.check_if_policy_attached() + } + all_policies.append(policy_dict) + logger.info(f'All policies currently added to role {str(all_policies)}') + return all_policies diff --git a/backend/dataall/modules/dataset_sharing/__init__.py b/backend/dataall/modules/dataset_sharing/__init__.py index 99dd6c01e..d71590175 100644 --- a/backend/dataall/modules/dataset_sharing/__init__.py +++ b/backend/dataall/modules/dataset_sharing/__init__.py @@ -22,6 +22,7 @@ def depends_on() -> List[Type['ModuleInterface']]: def __init__(self): from dataall.modules.dataset_sharing import api + from dataall.modules.dataset_sharing.services.managed_share_policy_service import SharePolicyService EnvironmentResourceManager.register(ShareEnvironmentResource()) log.info("API of dataset sharing has been imported") @@ -53,5 +54,6 @@ def is_supported(modes): def __init__(self): import dataall.modules.dataset_sharing.cdk + from dataall.modules.dataset_sharing.services.managed_share_policy_service import SharePolicyService log.info("CDK module data_sharing has been imported") diff --git a/backend/dataall/modules/dataset_sharing/api/input_types.py b/backend/dataall/modules/dataset_sharing/api/input_types.py index 3490700d2..4918141f9 100644 --- a/backend/dataall/modules/dataset_sharing/api/input_types.py +++ b/backend/dataall/modules/dataset_sharing/api/input_types.py @@ -10,6 +10,7 @@ gql.Argument(name='principalId', type=gql.NonNullableType(gql.String)), gql.Argument(name='principalType', type=gql.NonNullableType(gql.String)), gql.Argument(name='requestPurpose', type=gql.String), + gql.Argument(name='attachMissingPolicies', type=gql.NonNullableType(gql.Boolean)) ], ) diff --git a/backend/dataall/modules/dataset_sharing/api/resolvers.py b/backend/dataall/modules/dataset_sharing/api/resolvers.py index 517a1a11e..dd19817d0 100644 --- a/backend/dataall/modules/dataset_sharing/api/resolvers.py +++ b/backend/dataall/modules/dataset_sharing/api/resolvers.py @@ -56,7 +56,8 @@ def create_share_object( group_uri=input['groupUri'], principal_id=input['principalId'], principal_type=input['principalType'], - requestPurpose=input.get('requestPurpose') + requestPurpose=input.get('requestPurpose'), + attachMissingPolicies=input.get('attachMissingPolicies') ) diff --git a/backend/dataall/modules/dataset_sharing/cdk/pivot_role_data_sharing_policy.py b/backend/dataall/modules/dataset_sharing/cdk/pivot_role_data_sharing_policy.py index 8e5d76286..d70196a34 100644 --- a/backend/dataall/modules/dataset_sharing/cdk/pivot_role_data_sharing_policy.py +++ b/backend/dataall/modules/dataset_sharing/cdk/pivot_role_data_sharing_policy.py @@ -12,13 +12,32 @@ def get_statements(self): statements = [ # For access point sharing and S3 bucket sharing iam.PolicyStatement( - sid='IAMRolePolicy', + sid='IAMRolePolicy1', effect=iam.Effect.ALLOW, actions=[ 'iam:PutRolePolicy', - 'iam:DeleteRolePolicy' + 'iam:DeleteRolePolicy', + 'iam:AttachRolePolicy', + 'iam:DetachRolePolicy', + 'iam:ListAttachedRolePolicies', + ], + resources=[f'arn:aws:iam::{self.account}:role/*'], + ), + iam.PolicyStatement( + sid='IAMRolePolicy2', + effect=iam.Effect.ALLOW, + actions=[ + 'iam:ListPolicyVersions', + 'iam:CreatePolicy', + 'iam:DeletePolicy', + 'iam:CreatePolicyVersion', + 'iam:DeletePolicyVersion' + ], + resources=[ + f'arn:aws:iam::{self.account}:policy/{self.env_resource_prefix}*', + f'arn:aws:iam::{self.account}:policy/targetDatasetAccessControlPolicy', + f'arn:aws:iam::{self.account}:policy/dataall-targetDatasetS3Bucket-AccessControlPolicy', ], - resources=['*'], ), iam.PolicyStatement( sid='ManagedAccessPoints', diff --git a/backend/dataall/modules/dataset_sharing/services/managed_share_policy_service.py b/backend/dataall/modules/dataset_sharing/services/managed_share_policy_service.py new file mode 100644 index 000000000..0ad9ac4b2 --- /dev/null +++ b/backend/dataall/modules/dataset_sharing/services/managed_share_policy_service.py @@ -0,0 +1,295 @@ +import json +from dataall.base.aws.iam import IAM +from dataall.base.utils.naming_convention import NamingConventionService, NamingConventionPattern +from dataall.core.environment.services.managed_iam_policies import ManagedPolicy +import logging + +log = logging.getLogger(__name__) + +logging.basicConfig( + format='%(asctime)s - %(levelname)s - %(funcName)s - %(message)s ', + datefmt='%d-%b-%y %H:%M:%S', + level=logging.INFO +) + +OLD_IAM_ACCESS_POINT_ROLE_POLICY = "targetDatasetAccessControlPolicy" +OLD_IAM_S3BUCKET_ROLE_POLICY = "dataall-targetDatasetS3Bucket-AccessControlPolicy" + +IAM_S3_ACCESS_POINTS_STATEMENT_SID = "AccessPointsStatement" +IAM_S3_BUCKETS_STATEMENT_SID = "BucketStatement" +EMPTY_STATEMENT_SID = "EmptyStatement" + + +class SharePolicyService(ManagedPolicy): + def __init__( + self, + role_name, + account, + environmentUri, + resource_prefix + ): + self.role_name = role_name + self.account = account + self.environmentUri = environmentUri + self.resource_prefix = resource_prefix + + @property + def policy_type(self) -> str: + return "SharePolicy" + + def generate_policy_name(self) -> str: + # In this case it is not possible to build a too long policy because the IAM role can be max 64 chars + # However it is good practice to use the standard utility to build the name + return NamingConventionService( + target_label=f'env-{self.environmentUri}-share-policy', + target_uri=self.role_name, + pattern=NamingConventionPattern.IAM_POLICY, + resource_prefix=self.resource_prefix + ).build_compliant_name() + + def generate_empty_policy(self) -> dict: + return { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": EMPTY_STATEMENT_SID, + "Effect": "Allow", + "Action": "none:null", + "Resource": "*" + } + ] + } + + @staticmethod + def remove_empty_statement(policy_doc: dict, statement_sid: str) -> dict: + statement_index = SharePolicyService._get_statement_by_sid(policy_doc, statement_sid) + if statement_index is not None: + policy_doc["Statement"].pop(statement_index) + return policy_doc + + def add_missing_resources_to_policy_statement( + self, + resource_type: str, + target_resources: list, + statement_sid: str, + policy_document: dict + ): + """ + Checks if the resources are in the existing statement. Otherwise, it will add it. + :param target_resources: list + :param existing_policy_statement: dict + :return + """ + policy_name = self.generate_policy_name() + index = self._get_statement_by_sid(policy_document, statement_sid) + if index is None: + log.info( + f'{statement_sid} does NOT exists for Managed policy {policy_name} ' + f'creating statement...' + ) + additional_policy = { + "Sid": statement_sid, + "Effect": "Allow", + "Action": [ + f"{resource_type}:*" + ], + "Resource": target_resources + } + policy_document["Statement"].append(additional_policy) + else: + for target_resource in target_resources: + if target_resource not in policy_document["Statement"][index]["Resource"]: + log.info( + f'{statement_sid} exists for Managed policy {policy_name} ' + f'but {target_resource} is not included, updating...' + ) + policy_document["Statement"][index]["Resource"].extend([target_resource]) + else: + log.info( + f'{statement_sid} exists for Managed policy {policy_name} ' + f'and {target_resource} is included, skipping...' + ) + + def remove_resource_from_statement( + self, + target_resources: list, + statement_sid: str, + policy_document: dict + ): + policy_name = self.generate_policy_name() + index = self._get_statement_by_sid(policy_document, statement_sid) + log.info( + f'Removing {target_resources} from Statement[{index}] in Managed policy {policy_name} ' + f'skipping...' + ) + if index is None: + log.info( + f'{statement_sid} does NOT exists for Managed policy {policy_name} ' + f'skipping...' + ) + else: + policy_statement = policy_document["Statement"][index] + for target_resource in target_resources: + if target_resource in policy_statement["Resource"]: + log.info( + f'{statement_sid} exists for Managed policy {policy_name} ' + f'and {target_resource} is included, removing...' + ) + policy_statement["Resource"].remove(target_resource) + if len(policy_statement["Resource"]) == 0: + log.info( + f'No more resources in {statement_sid}, removing statement...' + ) + policy_document["Statement"].pop(index) + if len(policy_document["Statement"]) == 0: + log.info( + f'No more statements in {policy_document}, adding empty statement...' + ) + empty_policy_document = self.generate_empty_policy() + policy_document["Statement"] = empty_policy_document["Statement"] + + @staticmethod + def check_resource_in_policy_statement( + target_resources: list, + existing_policy_statement: dict + ) -> bool: + """ + Checks if the resources are in the existing policy + :param target_resources: list + :param existing_policy_statement: dict + :return True if all target_resources in the existing policy else False + """ + for target_resource in target_resources: + if target_resource not in existing_policy_statement["Resource"]: + return False + return True + + @staticmethod + def _get_statement_by_sid(policy, sid): + for index, statement in enumerate(policy["Statement"]): + if statement["Sid"] == sid: + return index + return None + + # Backwards compatibility + + def create_managed_policy_from_inline_and_delete_inline(self): + """ + For existing consumption and team roles, the IAM managed policy won't be created. + We need to create the policy based on the inline statements + Finally, delete the old obsolete inline policies from the role + """ + try: + policy_document = self._generate_managed_policy_from_inline_policies() + log.info(f'Creating policy from inline backwards compatibility. Policy = {str(policy_document)}') + policy_arn = IAM.create_managed_policy(self.account, self.generate_policy_name(), + json.dumps(policy_document)) + + # Delete obsolete inline policies + log.info(f'Deleting {OLD_IAM_ACCESS_POINT_ROLE_POLICY} and {OLD_IAM_S3BUCKET_ROLE_POLICY}') + self._delete_old_inline_policies() + except Exception as e: + raise Exception(f"Error creating policy from inline policies: {e}") + return policy_arn + + def _generate_managed_policy_from_inline_policies(self): + """ + Get resources shared in previous inline policies + If there are already shared resources, add them to the empty policy and remove the fake statement + return: IAM policy document + """ + existing_bucket_s3, existing_bucket_kms = self._get_policy_resources_from_inline_policy(OLD_IAM_S3BUCKET_ROLE_POLICY) + existing_access_points_s3, existing_access_points_kms = self._get_policy_resources_from_inline_policy(OLD_IAM_ACCESS_POINT_ROLE_POLICY) + log.info( + f"Back-filling S3BUCKET sharing resources: S3={existing_bucket_s3}, KMS={existing_bucket_kms}" + ) + log.info( + f"Back-filling S3ACCESS POINTS sharing resources: S3={existing_access_points_s3}, KMS={existing_access_points_kms}" + ) + if len(existing_bucket_s3 + existing_access_points_s3) > 0: + new_policy = { + "Version": "2012-10-17", + "Statement": [ + ] + } + updated_policy = self._update_policy_resources_from_inline_policy( + policy=new_policy, + statement_sid=IAM_S3_BUCKETS_STATEMENT_SID, + existing_s3=existing_bucket_s3, + existing_kms=existing_bucket_kms + ) + updated_policy = self._update_policy_resources_from_inline_policy( + policy=updated_policy, + statement_sid=IAM_S3_ACCESS_POINTS_STATEMENT_SID, + existing_s3=existing_access_points_s3, + existing_kms=existing_access_points_kms + ) + else: + updated_policy = self.generate_empty_policy() + return updated_policy + + def _get_policy_resources_from_inline_policy(self, policy_name): + # This function can only be used for backwards compatibility where policies had statement[0] for s3 + # and statement[1] for KMS permissions + try: + existing_policy = IAM.get_role_policy( + self.account, + self.role_name, + policy_name + ) + if existing_policy is not None: + kms_resources = existing_policy["Statement"][1]["Resource"] if len(existing_policy["Statement"]) > 1 else [] + return existing_policy["Statement"][0]["Resource"], kms_resources + else: + return [], [] + except Exception as e: + log.error( + f'Failed to retrieve the existing policy {policy_name}: {e} ' + ) + return [], [] + + def _update_policy_resources_from_inline_policy(self, policy, statement_sid, existing_s3, existing_kms): + # This function can only be used for backwards compatibility where policies had statement[0] for s3 + # and statement[1] for KMS permissions + if len(existing_s3) > 0: + additional_policy = { + "Sid": f"{statement_sid}S3", + "Effect": "Allow", + "Action": [ + "s3:*" + ], + "Resource": existing_s3 + } + policy["Statement"].append(additional_policy) + if len(existing_kms) > 0: + additional_policy = { + "Sid": f"{statement_sid}KMS", + "Effect": "Allow", + "Action": [ + "kms:*" + ], + "Resource": existing_kms + } + policy["Statement"].append(additional_policy) + return policy + + def _delete_old_inline_policies(self): + for policy_name in [OLD_IAM_S3BUCKET_ROLE_POLICY, OLD_IAM_ACCESS_POINT_ROLE_POLICY]: + try: + existing_policy = IAM.get_role_policy( + self.account, + self.role_name, + policy_name + ) + if existing_policy is not None: + log.info( + f'Deleting inline policy {policy_name}...' + ) + IAM.delete_role_policy(self.account, self.role_name, policy_name) + else: + pass + except Exception as e: + log.error( + f'Failed to retrieve the existing policy {policy_name}: {e} ' + ) + return True diff --git a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py index 4de2cec46..758192a3a 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py +++ b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py @@ -13,17 +13,14 @@ from dataall.modules.dataset_sharing.db.share_object_models import ShareObject from dataall.modules.dataset_sharing.services.dataset_alarm_service import DatasetAlarmService from dataall.modules.dataset_sharing.db.share_object_repositories import ShareObjectRepository -from dataall.modules.dataset_sharing.services.share_managers.share_manager_utils import ( - ShareManagerUtils, - ShareErrorFormatter, -) +from dataall.modules.dataset_sharing.services.share_managers.share_manager_utils import ShareErrorFormatter +from dataall.modules.dataset_sharing.services.managed_share_policy_service import SharePolicyService, IAM_S3_ACCESS_POINTS_STATEMENT_SID, EMPTY_STATEMENT_SID from dataall.modules.datasets_base.db.dataset_models import DatasetStorageLocation, Dataset logger = logging.getLogger(__name__) ACCESS_POINT_CREATION_TIME = 30 ACCESS_POINT_CREATION_RETRIES = 5 -IAM_ACCESS_POINT_ROLE_POLICY = "targetDatasetAccessControlPolicy" DATAALL_ALLOW_OWNER_SID = "AllowAllToAdmin" DATAALL_ACCESS_POINT_KMS_DECRYPT_SID = "DataAll-Access-Point-KMS-Decrypt" DATAALL_KMS_PIVOT_ROLE_PERMISSIONS_SID = "KMSPivotRolePermissions" @@ -172,64 +169,93 @@ def check_target_role_access_policy(self) -> None: and add to folder errors if check fails :return: None """ + logger.info(f"Check target role {self.target_requester_IAMRoleName} access policy") + key_alias = f"alias/{self.dataset.KmsAlias}" kms_client = KmsClient(self.dataset_account_id, self.source_environment.region) kms_key_id = kms_client.get_key_id(key_alias) - existing_policy = IAM.get_role_policy( - self.target_account_id, - self.target_requester_IAMRoleName, - IAM_ACCESS_POINT_ROLE_POLICY, + share_policy_service = SharePolicyService( + environmentUri=self.target_environment.environmentUri, + account=self.target_environment.AwsAccountId, + role_name=self.target_requester_IAMRoleName, + resource_prefix=self.target_environment.resourcePrefix ) - if not existing_policy: + share_resource_policy_name = share_policy_service.generate_policy_name() + + if not share_policy_service.check_if_policy_exists(): + logger.info(f"IAM Policy {share_resource_policy_name} does not exist") self.folder_errors.append( - ShareErrorFormatter.dne_error_msg(IAM_ACCESS_POINT_ROLE_POLICY, self.target_requester_IAMRoleName) + ShareErrorFormatter.dne_error_msg("IAM Policy", share_resource_policy_name) ) return - share_manager = ShareManagerUtils( - self.session, - self.dataset, - self.share, - self.source_environment, - self.target_environment, - self.source_env_group, - self.env_group, - ) - s3_target_resources = [ f"arn:aws:s3:::{self.bucket_name}", f"arn:aws:s3:::{self.bucket_name}/*", f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}", f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}/*", ] - if not share_manager.check_resource_in_policy_statement(s3_target_resources, existing_policy["Statement"][0]): + + version_id, policy_document = IAM.get_managed_policy_default_version( + self.target_environment.AwsAccountId, + share_resource_policy_name + ) + + s3_statement_index = SharePolicyService._get_statement_by_sid(policy_document, f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3") + + if s3_statement_index is None: + logger.info(f"IAM Policy Statement {IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3 does not exist") self.folder_errors.append( ShareErrorFormatter.missing_permission_error_msg( self.target_requester_IAMRoleName, - "IAM Policy", - IAM_ACCESS_POINT_ROLE_POLICY, + "IAM Policy Statement", + f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", + "S3 Bucket", + f"{self.bucket_name}", + ) + ) + + elif not share_policy_service.check_resource_in_policy_statement( + target_resources=s3_target_resources, + existing_policy_statement=policy_document["Statement"][s3_statement_index], + ): + logger.info(f"IAM Policy Statement {IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3 does not contain resources {s3_target_resources}") + self.folder_errors.append( + ShareErrorFormatter.missing_permission_error_msg( + self.target_requester_IAMRoleName, + "IAM Policy Resource", + f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", "S3 Bucket", f"{self.bucket_name}", ) ) if kms_key_id: - kms_error = False + kms_statement_index = SharePolicyService._get_statement_by_sid(policy_document, f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS") kms_target_resources = [f"arn:aws:kms:{self.dataset_region}:{self.dataset_account_id}:key/{kms_key_id}"] - if len(existing_policy["Statement"]) <= 1: - kms_error = True - elif not share_manager.check_resource_in_policy_statement( - kms_target_resources, - existing_policy["Statement"][1], - ): - kms_error = True + if not kms_statement_index: + logger.info(f"IAM Policy Statement {IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS does not exist") + self.folder_errors.append( + ShareErrorFormatter.missing_permission_error_msg( + self.target_requester_IAMRoleName, + "IAM Policy Statement", + f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", + "KMS Key", + f"{kms_key_id}", + ) + ) - if kms_error: + elif not share_policy_service.check_resource_in_policy_statement( + target_resources=kms_target_resources, + existing_policy_statement=policy_document["Statement"][kms_statement_index], + ): + logger.info( + f"IAM Policy Statement {IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS does not contain resources {kms_target_resources}") self.folder_errors.append( ShareErrorFormatter.missing_permission_error_msg( self.target_requester_IAMRoleName, - "IAM Policy", - IAM_ACCESS_POINT_ROLE_POLICY, + "IAM Policy Resource", + f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", "KMS Key", f"{kms_key_id}", ) @@ -240,78 +266,70 @@ def grant_target_role_access_policy(self): Updates requester IAM role policy to include requested S3 bucket and access point :return: """ - logger.info(f"Grant target role {self.target_requester_IAMRoleName} access policy") + logger.info( + f'Grant target role {self.target_requester_IAMRoleName} access policy' + ) + + share_policy_service = SharePolicyService( + environmentUri=self.target_environment.environmentUri, + account=self.target_environment.AwsAccountId, + role_name=self.target_requester_IAMRoleName, + resource_prefix=self.target_environment.resourcePrefix + ) + + # Backwards compatibility + # we check if a managed share policy exists. If False, the role was introduced to data.all before this update + # We create the policy from the inline statements and attach it to the role + if not share_policy_service.check_if_policy_exists(): + share_policy_service.create_managed_policy_from_inline_and_delete_inline() + share_policy_service.attach_policy() + # End of backwards compatibility + + share_resource_policy_name = share_policy_service.generate_policy_name() + version_id, policy_document = IAM.get_managed_policy_default_version( + self.target_account_id, + share_resource_policy_name + ) + key_alias = f"alias/{self.dataset.KmsAlias}" kms_client = KmsClient(self.dataset_account_id, self.source_environment.region) kms_key_id = kms_client.get_key_id(key_alias) - existing_policy = IAM.get_role_policy( - self.target_account_id, - self.target_requester_IAMRoleName, - IAM_ACCESS_POINT_ROLE_POLICY, + s3_target_resources = [ + f"arn:aws:s3:::{self.bucket_name}", + f"arn:aws:s3:::{self.bucket_name}/*", + f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}", + f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}/*" + ] + + share_policy_service.add_missing_resources_to_policy_statement( + resource_type="s3", + target_resources=s3_target_resources, + statement_sid=f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", + policy_document=policy_document ) - if existing_policy: # type dict - s3_target_resources = [ - f"arn:aws:s3:::{self.bucket_name}", - f"arn:aws:s3:::{self.bucket_name}/*", - f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}", - f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}/*", + + share_policy_service.remove_empty_statement( + policy_doc=policy_document, + statement_sid=EMPTY_STATEMENT_SID + ) + + if kms_key_id: + kms_target_resources = [ + f"arn:aws:kms:{self.dataset_region}:{self.dataset_account_id}:key/{kms_key_id}" ] - share_manager = ShareManagerUtils( - self.session, - self.dataset, - self.share, - self.source_environment, - self.target_environment, - self.source_env_group, - self.env_group, - ) - share_manager.add_missing_resources_to_policy_statement( - self.bucket_name, s3_target_resources, existing_policy["Statement"][0], IAM_ACCESS_POINT_ROLE_POLICY + share_policy_service.add_missing_resources_to_policy_statement( + resource_type="kms", + target_resources=kms_target_resources, + statement_sid=f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", + policy_document=policy_document, ) - if kms_key_id: - kms_target_resources = [f"arn:aws:kms:{self.dataset_region}:{self.dataset_account_id}:key/{kms_key_id}"] - if len(existing_policy["Statement"]) > 1: - share_manager.add_missing_resources_to_policy_statement( - kms_key_id, kms_target_resources, existing_policy["Statement"][1], IAM_ACCESS_POINT_ROLE_POLICY - ) - else: - additional_policy = {"Effect": "Allow", "Action": ["kms:*"], "Resource": kms_target_resources} - existing_policy["Statement"].append(additional_policy) - policy = existing_policy - else: - logger.info( - f"{IAM_ACCESS_POINT_ROLE_POLICY} does not exists for IAM role {self.target_requester_IAMRoleName}, creating..." - ) - policy = { - "Version": "2012-10-17", - "Statement": [ - { - "Effect": "Allow", - "Action": ["s3:*"], - "Resource": [ - f"arn:aws:s3:::{self.bucket_name}", - f"arn:aws:s3:::{self.bucket_name}/*", - f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}", - f"arn:aws:s3:{self.dataset_region}:{self.dataset_account_id}:accesspoint/{self.access_point_name}/*", - ], - } - ], - } - if kms_key_id: - additional_policy = { - "Effect": "Allow", - "Action": ["kms:*"], - "Resource": [f"arn:aws:kms:{self.dataset_region}:{self.dataset_account_id}:key/{kms_key_id}"], - } - policy["Statement"].append(additional_policy) - - IAM.update_role_policy( + IAM.update_managed_policy_default_version( self.target_account_id, - self.target_requester_IAMRoleName, - IAM_ACCESS_POINT_ROLE_POLICY, - json.dumps(policy), + share_resource_policy_name, + version_id, + json.dumps(policy_document) ) def check_access_point_and_policy(self) -> None: @@ -598,48 +616,63 @@ def delete_target_role_access_policy( dataset: Dataset, target_environment: Environment, ): - logger.info("Deleting target role IAM policy...") - access_point_name = S3AccessPointShareManager.build_access_point_name(share) - existing_policy = IAM.get_role_policy( - target_environment.AwsAccountId, - share.principalIAMRoleName, - IAM_ACCESS_POINT_ROLE_POLICY, + logger.info( + 'Deleting target role IAM statements...' + ) + + share_policy_service = SharePolicyService( + role_name=share.principalIAMRoleName, + account=target_environment.AwsAccountId, + environmentUri=target_environment.environmentUri, + resource_prefix=target_environment.resourcePrefix ) + + # Backwards compatibility + # we check if a managed share policy exists. If False, the role was introduced to data.all before this update + # We create the policy from the inline statements and attach it to the role + if not share_policy_service.check_if_policy_exists(): + share_policy_service.create_managed_policy_from_inline_and_delete_inline() + share_policy_service.attach_policy() + # End of backwards compatibility + + share_resource_policy_name = share_policy_service.generate_policy_name() + version_id, policy_document = IAM.get_managed_policy_default_version( + target_environment.AwsAccountId, + share_resource_policy_name) + + access_point_name = S3AccessPointShareManager.build_access_point_name(share) + key_alias = f"alias/{dataset.KmsAlias}" kms_client = KmsClient(dataset.AwsAccountId, dataset.region) kms_key_id = kms_client.get_key_id(key_alias) - if existing_policy: - s3_target_resources = [ - f"arn:aws:s3:::{dataset.S3BucketName}", - f"arn:aws:s3:::{dataset.S3BucketName}/*", - f"arn:aws:s3:{dataset.region}:{dataset.AwsAccountId}:accesspoint/{access_point_name}", - f"arn:aws:s3:{dataset.region}:{dataset.AwsAccountId}:accesspoint/{access_point_name}/*", - ] - ShareManagerUtils.remove_resource_from_statement(existing_policy["Statement"][0], s3_target_resources) - if kms_key_id: - kms_target_resources = [f"arn:aws:kms:{dataset.region}:{dataset.AwsAccountId}:key/{kms_key_id}"] - if len(existing_policy["Statement"]) > 1: - ShareManagerUtils.remove_resource_from_statement( - existing_policy["Statement"][1], kms_target_resources - ) - policy_statements = [] - for statement in existing_policy["Statement"]: - if len(statement["Resource"]) != 0: - policy_statements.append(statement) + s3_target_resources = [ + f"arn:aws:s3:::{dataset.S3BucketName}", + f"arn:aws:s3:::{dataset.S3BucketName}/*", + f"arn:aws:s3:{dataset.region}:{dataset.AwsAccountId}:accesspoint/{access_point_name}", + f"arn:aws:s3:{dataset.region}:{dataset.AwsAccountId}:accesspoint/{access_point_name}/*" + ] - existing_policy["Statement"] = policy_statements - if len(existing_policy["Statement"]) == 0: - IAM.delete_role_policy( - target_environment.AwsAccountId, share.principalIAMRoleName, IAM_ACCESS_POINT_ROLE_POLICY - ) - else: - IAM.update_role_policy( - target_environment.AwsAccountId, - share.principalIAMRoleName, - IAM_ACCESS_POINT_ROLE_POLICY, - json.dumps(existing_policy), - ) + share_policy_service.remove_resource_from_statement( + target_resources=s3_target_resources, + statement_sid=f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", + policy_document=policy_document + ) + if kms_key_id: + kms_target_resources = [ + f"arn:aws:kms:{dataset.region}:{dataset.AwsAccountId}:key/{kms_key_id}" + ] + share_policy_service.remove_resource_from_statement( + target_resources=kms_target_resources, + statement_sid=f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", + policy_document=policy_document + ) + IAM.update_managed_policy_default_version( + target_environment.AwsAccountId, + share_resource_policy_name, + version_id, + json.dumps(policy_document) + ) def delete_dataset_bucket_key_policy( self, diff --git a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py index 91940961b..279022b8b 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py +++ b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py @@ -9,11 +9,9 @@ from dataall.modules.dataset_sharing.aws.kms_client import KmsClient from dataall.modules.dataset_sharing.aws.s3_client import S3ControlClient, S3Client from dataall.modules.dataset_sharing.db.share_object_models import ShareObject -from dataall.modules.dataset_sharing.services.share_managers.share_manager_utils import ( - ShareManagerUtils, - ShareErrorFormatter, -) +from dataall.modules.dataset_sharing.services.share_managers.share_manager_utils import ShareErrorFormatter from dataall.modules.dataset_sharing.services.dataset_alarm_service import DatasetAlarmService +from dataall.modules.dataset_sharing.services.managed_share_policy_service import SharePolicyService, IAM_S3_BUCKETS_STATEMENT_SID, EMPTY_STATEMENT_SID from dataall.modules.datasets_base.db.dataset_models import Dataset, DatasetBucket from dataall.modules.dataset_sharing.db.share_object_repositories import ShareObjectRepository @@ -21,7 +19,6 @@ DATAALL_READ_ONLY_SID = "DataAll-Bucket-ReadOnly" DATAALL_ALLOW_OWNER_SID = "AllowAllToAdmin" -IAM_S3BUCKET_ROLE_POLICY = "dataall-targetDatasetS3Bucket-AccessControlPolicy" DATAALL_BUCKET_KMS_DECRYPT_SID = "DataAll-Bucket-KMS-Decrypt" DATAALL_KMS_PIVOT_ROLE_PERMISSIONS_SID = "KMSPivotRolePermissions" @@ -79,64 +76,86 @@ def check_s3_iam_access(self) -> None: :return: None """ logger.info(f"Check target role {self.target_requester_IAMRoleName} access policy") - existing_policy = IAM.get_role_policy( - self.target_account_id, - self.target_requester_IAMRoleName, - IAM_S3BUCKET_ROLE_POLICY, - ) - if not existing_policy: - self.bucket_errors.append( - ShareErrorFormatter.dne_error_msg(IAM_S3BUCKET_ROLE_POLICY, self.target_requester_IAMRoleName) - ) - return key_alias = f"alias/{self.target_bucket.KmsAlias}" kms_client = KmsClient(self.source_account_id, self.source_environment.region) kms_key_id = kms_client.get_key_id(key_alias) - share_manager = ShareManagerUtils( - self.session, - self.dataset, - self.share, - self.source_environment, - self.target_environment, - self.source_env_group, - self.env_group, + share_policy_service = SharePolicyService( + environmentUri=self.target_environment.environmentUri, + account=self.target_environment.AwsAccountId, + role_name=self.target_requester_IAMRoleName, + resource_prefix=self.target_environment.resourcePrefix ) + share_resource_policy_name = share_policy_service.generate_policy_name() + + if not share_policy_service.check_if_policy_exists(): + logger.info(f"IAM Policy {share_resource_policy_name} does not exist") + self.bucket_errors.append( + ShareErrorFormatter.dne_error_msg("IAM Policy", share_resource_policy_name) + ) + return s3_target_resources = [f"arn:aws:s3:::{self.bucket_name}", f"arn:aws:s3:::{self.bucket_name}/*"] - if not share_manager.check_resource_in_policy_statement( - target_resources=s3_target_resources, - existing_policy_statement=existing_policy["Statement"][0], + version_id, policy_document = IAM.get_managed_policy_default_version( + self.target_environment.AwsAccountId, + share_resource_policy_name + ) + s3_statement_index = SharePolicyService._get_statement_by_sid(policy_document, f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + + if s3_statement_index is None: + logger.info(f"IAM Policy Statement {IAM_S3_BUCKETS_STATEMENT_SID}S3 does not exist") + self.bucket_errors.append( + ShareErrorFormatter.missing_permission_error_msg( + self.target_requester_IAMRoleName, + "IAM Policy Statement", + f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", + "S3 Bucket", + f"{self.bucket_name}", + ) + ) + elif not share_policy_service.check_resource_in_policy_statement( + target_resources=s3_target_resources, + existing_policy_statement=policy_document["Statement"][s3_statement_index], ): + logger.info(f"IAM Policy Statement {IAM_S3_BUCKETS_STATEMENT_SID}KMS does not contain resources {s3_target_resources}") self.bucket_errors.append( ShareErrorFormatter.missing_permission_error_msg( self.target_requester_IAMRoleName, - "IAM Policy", - IAM_S3BUCKET_ROLE_POLICY, + "IAM Policy Resource", + f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "S3 Bucket", f"{self.bucket_name}", ) ) if kms_key_id: - kms_error = False + kms_statement_index = SharePolicyService._get_statement_by_sid(policy_document, f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") kms_target_resources = [f"arn:aws:kms:{self.bucket_region}:{self.source_account_id}:key/{kms_key_id}"] - if len(existing_policy["Statement"]) <= 1: - kms_error = True - elif not share_manager.check_resource_in_policy_statement( - target_resources=kms_target_resources, - existing_policy_statement=existing_policy["Statement"][1], - ): - kms_error = True + if kms_statement_index is None: + logger.info(f"IAM Policy Statement {IAM_S3_BUCKETS_STATEMENT_SID}S3 does not exist") + self.bucket_errors.append( + ShareErrorFormatter.missing_permission_error_msg( + self.target_requester_IAMRoleName, + "IAM Policy Statement", + f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", + "KMS Key", + f"{kms_key_id}", + ) + ) - if kms_error: + elif not share_policy_service.check_resource_in_policy_statement( + target_resources=kms_target_resources, + existing_policy_statement=policy_document["Statement"][kms_statement_index], + ): + logger.info( + f"IAM Policy Statement {IAM_S3_BUCKETS_STATEMENT_SID}KMS does not contain resources {kms_target_resources}") self.bucket_errors.append( ShareErrorFormatter.missing_permission_error_msg( self.target_requester_IAMRoleName, - "IAM Policy", - IAM_S3BUCKET_ROLE_POLICY, + "IAM Policy Resource", + f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "KMS Key", f"{kms_key_id}", ) @@ -148,76 +167,69 @@ def grant_s3_iam_access(self): Updates requester IAM role policy to include requested S3 bucket and kms key :return: """ - logger.info(f"Grant target role {self.target_requester_IAMRoleName} access policy") - existing_policy = IAM.get_role_policy( - self.target_account_id, - self.target_requester_IAMRoleName, - IAM_S3BUCKET_ROLE_POLICY, + logger.info( + f'Grant target role {self.target_requester_IAMRoleName} access policy' ) + + share_policy_service = SharePolicyService( + environmentUri=self.target_environment.environmentUri, + account=self.target_environment.AwsAccountId, + role_name=self.target_requester_IAMRoleName, + resource_prefix=self.target_environment.resourcePrefix + ) + + # Backwards compatibility + # we check if a managed share policy exists. If False, the role was introduced to data.all before this update + # We create the policy from the inline statements and attach it to the role + if not share_policy_service.check_if_policy_exists(): + share_policy_service.create_managed_policy_from_inline_and_delete_inline() + share_policy_service.attach_policy() + # End of backwards compatibility + + share_resource_policy_name = share_policy_service.generate_policy_name() + + logger.info(f'Share policy name is {share_resource_policy_name}') + version_id, policy_document = IAM.get_managed_policy_default_version( + self.target_account_id, + share_resource_policy_name) + key_alias = f"alias/{self.target_bucket.KmsAlias}" kms_client = KmsClient(self.source_account_id, self.source_environment.region) kms_key_id = kms_client.get_key_id(key_alias) - if existing_policy: # type dict - s3_target_resources = [f"arn:aws:s3:::{self.bucket_name}", f"arn:aws:s3:::{self.bucket_name}/*"] - - share_manager = ShareManagerUtils( - self.session, - self.dataset, - self.share, - self.source_environment, - self.target_environment, - self.source_env_group, - self.env_group, - ) - share_manager.add_missing_resources_to_policy_statement( - resource_type=self.bucket_name, - target_resources=s3_target_resources, - existing_policy_statement=existing_policy["Statement"][0], - iam_role_policy_name=IAM_S3BUCKET_ROLE_POLICY, - ) + s3_target_resources = [ + f"arn:aws:s3:::{self.bucket_name}", + f"arn:aws:s3:::{self.bucket_name}/*" + ] - if kms_key_id: - kms_target_resources = [f"arn:aws:kms:{self.bucket_region}:{self.source_account_id}:key/{kms_key_id}"] - if len(existing_policy["Statement"]) > 1: - share_manager.add_missing_resources_to_policy_statement( - resource_type=kms_key_id, - target_resources=kms_target_resources, - existing_policy_statement=existing_policy["Statement"][1], - iam_role_policy_name=IAM_S3BUCKET_ROLE_POLICY, - ) - else: - additional_policy = {"Effect": "Allow", "Action": ["kms:*"], "Resource": kms_target_resources} - existing_policy["Statement"].append(additional_policy) + share_policy_service.add_missing_resources_to_policy_statement( + resource_type="s3", + target_resources=s3_target_resources, + statement_sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", + policy_document=policy_document + ) - policy = existing_policy - else: - logger.info( - f"{IAM_S3BUCKET_ROLE_POLICY} does not exists for IAM role {self.target_requester_IAMRoleName}, creating..." + share_policy_service.remove_empty_statement( + policy_doc=policy_document, + statement_sid=EMPTY_STATEMENT_SID + ) + + if kms_key_id: + kms_target_resources = [ + f"arn:aws:kms:{self.bucket_region}:{self.source_account_id}:key/{kms_key_id}" + ] + share_policy_service.add_missing_resources_to_policy_statement( + resource_type="kms", + target_resources=kms_target_resources, + statement_sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", + policy_document=policy_document, ) - policy = { - "Version": "2012-10-17", - "Statement": [ - { - "Effect": "Allow", - "Action": ["s3:*"], - "Resource": [f"arn:aws:s3:::{self.bucket_name}", f"arn:aws:s3:::{self.bucket_name}/*"], - } - ], - } - if kms_key_id: - additional_policy = { - "Effect": "Allow", - "Action": ["kms:*"], - "Resource": [f"arn:aws:kms:{self.bucket_region}:{self.source_account_id}:key/{kms_key_id}"], - } - policy["Statement"].append(additional_policy) - IAM.update_role_policy( + IAM.update_managed_policy_default_version( self.target_account_id, - self.target_requester_IAMRoleName, - IAM_S3BUCKET_ROLE_POLICY, - json.dumps(policy), + share_resource_policy_name, + version_id, + json.dumps(policy_document) ) def get_bucket_policy_or_default(self): @@ -468,54 +480,58 @@ def delete_target_role_access_policy( target_bucket: DatasetBucket, target_environment: Environment, ): - logger.info("Deleting target role IAM policy...") - existing_policy = IAM.get_role_policy( - target_environment.AwsAccountId, - share.principalIAMRoleName, - IAM_S3BUCKET_ROLE_POLICY, + logger.info( + 'Deleting target role IAM statements...' ) + + share_policy_service = SharePolicyService( + role_name=share.principalIAMRoleName, + account=target_environment.AwsAccountId, + environmentUri=target_environment.environmentUri, + resource_prefix=target_environment.resourcePrefix + ) + # Backwards compatibility + # we check if a managed share policy exists. If False, the role was introduced to data.all before this update + # We create the policy from the inline statements and attach it to the role + if not share_policy_service.check_if_policy_exists(): + share_policy_service.create_managed_policy_from_inline_and_delete_inline() + share_policy_service.attach_policy() + # End of backwards compatibility + + share_resource_policy_name = share_policy_service.generate_policy_name() + version_id, policy_document = IAM.get_managed_policy_default_version( + self.target_account_id, + share_resource_policy_name) + key_alias = f"alias/{target_bucket.KmsAlias}" kms_client = KmsClient(target_bucket.AwsAccountId, target_bucket.region) kms_key_id = kms_client.get_key_id(key_alias) - if existing_policy: - s3_target_resources = [ - f"arn:aws:s3:::{target_bucket.S3BucketName}", - f"arn:aws:s3:::{target_bucket.S3BucketName}/*", + + s3_target_resources = [ + f"arn:aws:s3:::{target_bucket.S3BucketName}", + f"arn:aws:s3:::{target_bucket.S3BucketName}/*" + ] + share_policy_service.remove_resource_from_statement( + target_resources=s3_target_resources, + statement_sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", + policy_document=policy_document + ) + if kms_key_id: + kms_target_resources = [ + f"arn:aws:kms:{target_bucket.region}:{target_bucket.AwsAccountId}:key/{kms_key_id}" ] - share_manager = ShareManagerUtils( - self.session, - self.dataset, - self.share, - self.source_environment, - self.target_environment, - self.source_env_group, - self.env_group, + share_policy_service.remove_resource_from_statement( + target_resources=kms_target_resources, + statement_sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", + policy_document=policy_document ) - share_manager.remove_resource_from_statement(existing_policy["Statement"][0], s3_target_resources) - if kms_key_id: - kms_target_resources = [ - f"arn:aws:kms:{target_bucket.region}:{target_bucket.AwsAccountId}:key/{kms_key_id}" - ] - if len(existing_policy["Statement"]) > 1: - share_manager.remove_resource_from_statement(existing_policy["Statement"][1], kms_target_resources) - - policy_statements = [] - for statement in existing_policy["Statement"]: - if len(statement["Resource"]) != 0: - policy_statements.append(statement) - - existing_policy["Statement"] = policy_statements - if len(existing_policy["Statement"]) == 0: - IAM.delete_role_policy( - target_environment.AwsAccountId, share.principalIAMRoleName, IAM_S3BUCKET_ROLE_POLICY - ) - else: - IAM.update_role_policy( - target_environment.AwsAccountId, - share.principalIAMRoleName, - IAM_S3BUCKET_ROLE_POLICY, - json.dumps(existing_policy), - ) + + IAM.update_managed_policy_default_version( + self.target_account_id, + share_resource_policy_name, + version_id, + json.dumps(policy_document) + ) def delete_target_role_bucket_key_policy( self, diff --git a/backend/dataall/modules/dataset_sharing/services/share_managers/share_manager_utils.py b/backend/dataall/modules/dataset_sharing/services/share_managers/share_manager_utils.py index c655f0d1f..fed9fe91b 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_managers/share_manager_utils.py +++ b/backend/dataall/modules/dataset_sharing/services/share_managers/share_manager_utils.py @@ -1,11 +1,5 @@ -import abc import logging -from dataall.core.environment.db.environment_models import Environment, EnvironmentGroup -from dataall.modules.dataset_sharing.db.share_object_models import ShareObject -from dataall.modules.datasets_base.db.dataset_models import Dataset - - logger = logging.getLogger(__name__) @@ -25,70 +19,3 @@ def missing_permission_error_msg(requestor, permission_type, permissions, resour requestor = ShareErrorFormatter._stringify(requestor) permissions = ShareErrorFormatter._stringify(permissions) return f"Requestor {requestor} missing {permission_type} permissions: {permissions} for {resource_type} Target: {target_resource}" - - -class ShareManagerUtils: - def __init__( - self, - session, - dataset: Dataset, - share: ShareObject, - source_environment: Environment, - target_environment: Environment, - source_env_group: EnvironmentGroup, - env_group: EnvironmentGroup, - ): - self.target_requester_IAMRoleName = share.principalIAMRoleName - self.session = session - self.dataset = dataset - self.share = share - self.source_environment = source_environment - self.target_environment = target_environment - self.source_env_group = source_env_group - self.env_group = env_group - - def add_missing_resources_to_policy_statement( - self, resource_type, target_resources, existing_policy_statement, iam_role_policy_name - ): - """ - Checks if the resources are in the existing policy. Otherwise, it will add it. - :param resource_type: str - :param target_resources: list - :param existing_policy_statement: dict - :param iam_role_policy_name: str - :return - """ - for target_resource in target_resources: - if not self.check_resource_in_policy_statement([target_resource], existing_policy_statement): - logger.info( - f"{iam_role_policy_name} exists for IAM role {self.target_requester_IAMRoleName}, " - f"but {resource_type} is not included, updating..." - ) - existing_policy_statement["Resource"].extend([target_resource]) - else: - logger.info( - f"{iam_role_policy_name} exists for IAM role {self.target_requester_IAMRoleName} " - f"and {resource_type} is included, skipping..." - ) - - def check_resource_in_policy_statement( - self, - target_resources: list, - existing_policy_statement: dict, - ): - """ - Checks if the resources are in the existing policy - :param target_resources: list - :param existing_policy_statement: dict - :return True if all target_resources in the existing policy else False - """ - for target_resource in target_resources: - if target_resource not in existing_policy_statement["Resource"]: - return False - return True - - @staticmethod - def remove_resource_from_statement(policy_statement, target_resources): - for target_resource in target_resources: - if target_resource in policy_statement["Resource"]: - policy_statement["Resource"].remove(target_resource) diff --git a/backend/dataall/modules/dataset_sharing/services/share_object_service.py b/backend/dataall/modules/dataset_sharing/services/share_object_service.py index 36911c31f..a1a044655 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_object_service.py +++ b/backend/dataall/modules/dataset_sharing/services/share_object_service.py @@ -9,7 +9,6 @@ from dataall.core.permissions.permissions import GET_ENVIRONMENT from dataall.core.tasks.db.task_models import Task from dataall.base.db import utils -from dataall.base.utils.naming_convention import NamingConventionPattern from dataall.base.aws.quicksight import QuicksightClient from dataall.base.db.exceptions import UnauthorizedOperation from dataall.modules.dataset_sharing.services.dataset_sharing_enums import ( @@ -27,6 +26,7 @@ ) from dataall.modules.dataset_sharing.services.share_exceptions import ShareItemsFound from dataall.modules.dataset_sharing.services.share_notification_service import ShareNotificationService +from dataall.modules.dataset_sharing.services.managed_share_policy_service import SharePolicyService from dataall.modules.dataset_sharing.services.share_permissions import ( REJECT_SHARE_OBJECT, APPROVE_SHARE_OBJECT, @@ -41,6 +41,9 @@ from dataall.modules.datasets_base.db.dataset_repositories import DatasetRepository from dataall.modules.datasets_base.db.dataset_models import DatasetTable, Dataset from dataall.modules.datasets_base.services.permissions import DATASET_TABLE_READ +import logging + +log = logging.getLogger(__name__) class ShareObjectService: @@ -69,6 +72,7 @@ def create_share_object( principal_id, principal_type, requestPurpose, + attachMissingPolicies ): context = get_context() with context.db_engine.scoped_session() as session: @@ -87,11 +91,14 @@ def create_share_object( session, principal_id, environment.environmentUri ) principal_iam_role_name = consumption_role.IAMRoleName + managed = consumption_role.dataallManaged + else: env_group: EnvironmentGroup = EnvironmentService.get_environment_group( session, group_uri, environment.environmentUri ) principal_iam_role_name = env_group.environmentIAMRoleName + managed = True if ( (dataset.stewards == group_uri or dataset.SamlAdminGroupName == group_uri) @@ -105,6 +112,26 @@ def create_share_object( cls._validate_group_membership(session, group_uri, environment.environmentUri) + share_policy_service = SharePolicyService( + account=environment.AwsAccountId, + role_name=principal_iam_role_name, + environmentUri=environment.environmentUri, + resource_prefix=environment.resourcePrefix + ) + # Backwards compatibility + # we check if a managed share policy exists. If False, the role was introduced to data.all before this update + # We create the policy from the inline statements + # In this case it could also happen that the role is the Admin of the environment + if not share_policy_service.check_if_policy_exists(): + share_policy_service.create_managed_policy_from_inline_and_delete_inline() + # End of backwards compatibility + + attached = share_policy_service.check_if_policy_attached() + if not attached and not managed and not attachMissingPolicies: + raise Exception( + f"Required customer managed policy {share_policy_service.generate_policy_name()} is not attached to role {principal_iam_role_name}") + elif not attached: + share_policy_service.attach_policy() share = ShareObjectRepository.find_share(session, dataset, environment, principal_id, group_uri) if not share: share = ShareObject( diff --git a/backend/migrations/README.md b/backend/migrations/README.md index 3a775658a..92b8a8a0e 100644 --- a/backend/migrations/README.md +++ b/backend/migrations/README.md @@ -60,7 +60,7 @@ make generate-migrations or ```bash alembic -c backend/alembic.ini upgrade head -alembic -c backend/alembic.ini revision -m "_describe_changes_shortly" --autogenerate +alembic -c backend/alembic.ini revision -m "describe_changes_shortly" --autogenerate ``` Please, change the auto-generated filename postfix with the short description of the migration purpose. Also, rename this prefix in the file itself (first line) Always check autogenerated migration file to ensure that necessary changed are reflected there. diff --git a/backend/migrations/versions/194608b1ff7f_add_dataallmanaged_roles.py b/backend/migrations/versions/194608b1ff7f_add_dataallmanaged_roles.py new file mode 100644 index 000000000..3cb93c5a7 --- /dev/null +++ b/backend/migrations/versions/194608b1ff7f_add_dataallmanaged_roles.py @@ -0,0 +1,24 @@ +"""add_dataallManaged_roles + +Revision ID: 194608b1ff7f +Revises: af702716568f +Create Date: 2024-02-29 23:14:07.686581 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '194608b1ff7f' +down_revision = 'af702716568f' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column('consumptionrole', sa.Column('dataallManaged', sa.Boolean(), nullable=False, server_default=sa.sql.expression.true())) + + +def downgrade(): + op.drop_column('consumptionrole', 'dataallManaged') diff --git a/deploy/pivot_role/pivotRole.yaml b/deploy/pivot_role/pivotRole.yaml index 26435d897..56009a86a 100644 --- a/deploy/pivot_role/pivotRole.yaml +++ b/deploy/pivot_role/pivotRole.yaml @@ -439,12 +439,28 @@ Resources: - 'iam:ListRoles' Effect: Allow Resource: '*' - - Sid: IAMRolePolicy + - Sid: IAMRolePolicy1 Action: - 'iam:PutRolePolicy' - 'iam:DeleteRolePolicy' + - 'iam:AttachRolePolicy' + - 'iam:DetachRolePolicy' + - 'iam:ListAttachedRolePolicies' Effect: Allow - Resource: '*' + Resource: + - !Sub 'arn:aws:iam::${AWS::AccountId}:role/*' + - Sid: IAMRolePolicy2 + Action: + - 'iam:ListPolicyVersions' + - 'iam:CreatePolicy' + - 'iam:DeletePolicy' + - 'iam:CreatePolicyVersion' + - 'iam:DeletePolicyVersion' + Effect: Allow + Resource: + - !Sub 'arn:aws:iam::${AWS::AccountId}:policy/${EnvironmentResourcePrefix}*' + - !Sub 'arn:aws:iam::${AWS::AccountId}:policy/targetDatasetAccessControlPolicy' + - !Sub 'arn:aws:iam::${AWS::AccountId}:policy/dataall-targetDatasetS3Bucket-AccessControlPolicy' - Sid: IAMPassRole Action: - 'iam:PassRole' diff --git a/frontend/src/modules/Catalog/components/RequestAccessModal.js b/frontend/src/modules/Catalog/components/RequestAccessModal.js index 8657ad46b..2b1bc36fa 100644 --- a/frontend/src/modules/Catalog/components/RequestAccessModal.js +++ b/frontend/src/modules/Catalog/components/RequestAccessModal.js @@ -5,8 +5,10 @@ import { CardContent, CircularProgress, Dialog, + FormControlLabel, FormHelperText, MenuItem, + Switch, TextField, Typography } from '@mui/material'; @@ -108,7 +110,14 @@ export const RequestAccessModal = (props) => { setRoleOptions( response.data.listEnvironmentConsumptionRoles.nodes.map((g) => ({ value: g.consumptionRoleUri, - label: [g.consumptionRoleName, ' [', g.IAMRoleArn, ']'].join('') + label: [g.consumptionRoleName, ' [', g.IAMRoleArn, ']'].join(''), + dataallManaged: g.dataallManaged, + isSharePolicyAttached: g.managedPolicies.find( + (policy) => policy.policy_type === 'SharePolicy' + ).attached, + policyName: g.managedPolicies.find( + (policy) => policy.policy_type === 'SharePolicy' + ).policy_name })) ); } else { @@ -145,7 +154,8 @@ export const RequestAccessModal = (props) => { groupUri: values.groupUri, principalId: principal, principalType: type, - requestPurpose: values.comment + requestPurpose: values.comment, + attachMissingPolicies: values.attachMissingPolicies } }) ); @@ -161,7 +171,8 @@ export const RequestAccessModal = (props) => { groupUri: values.groupUri, principalId: principal, principalType: type, - requestPurpose: values.comment + requestPurpose: values.comment, + attachMissingPolicies: values.attachMissingPolicies } }) ); @@ -177,7 +188,8 @@ export const RequestAccessModal = (props) => { groupUri: values.groupUri, principalId: principal, principalType: type, - requestPurpose: values.comment + requestPurpose: values.comment, + attachMissingPolicies: values.attachMissingPolicies } }) ); @@ -237,7 +249,8 @@ export const RequestAccessModal = (props) => { { onChange={(event) => { setFieldValue( 'consumptionRole', + event.target.value.value + ); + setFieldValue( + 'consumptionRoleObj', event.target.value ); }} select - value={values.consumptionRole} + value={values.consumptionRoleObj} variant="outlined" > {roleOptions.map((role) => ( - + {role.label} ))} @@ -453,6 +470,56 @@ export const RequestAccessModal = (props) => { )} + + {!values.consumptionRole || + values.consumptionRoleObj.dataallManaged || + values.consumptionRoleObj.isSharePolicyAttached ? ( + + ) : ( + + + } + label={ +
+ Let Data.All attach policies to this role + + {values.consumptionRoleObj && + !( + values.consumptionRoleObj.dataallManaged || + values.consumptionRoleObj.isSharePolicyAttached || + values.attachMissingPolicies + ) ? ( + + Selected consumption role is managed by + customer, but the share policy{' '} + + {values.consumptionRoleObj.policyName} + {' '} + is not attached. +
+ Please attach it or let Data.all attach it for + you. + + ) : ( + '' + )} +
+ } + /> + + )} { fullWidth startIcon={} color="primary" - disabled={isSubmitting} + disabled={ + isSubmitting || + (values.consumptionRoleObj && + !( + values.consumptionRoleObj.dataallManaged || + values.consumptionRoleObj.isSharePolicyAttached || + values.attachMissingPolicies + )) + } type="submit" variant="contained" > diff --git a/frontend/src/modules/Environments/components/EnvironmentRoleAddForm.js b/frontend/src/modules/Environments/components/EnvironmentRoleAddForm.js index a25b21ef9..3e7bf4d25 100644 --- a/frontend/src/modules/Environments/components/EnvironmentRoleAddForm.js +++ b/frontend/src/modules/Environments/components/EnvironmentRoleAddForm.js @@ -5,7 +5,9 @@ import { CardContent, CircularProgress, Dialog, + FormControlLabel, MenuItem, + Switch, TextField, Typography } from '@mui/material'; @@ -31,7 +33,8 @@ export const EnvironmentRoleAddForm = (props) => { groupUri: values.groupUri, consumptionRoleName: values.consumptionRoleName, IAMRoleArn: values.IAMRoleArn, - environmentUri: environment.environmentUri + environmentUri: environment.environmentUri, + dataallManaged: values.dataallManaged }) ); if (!response.errors) { @@ -91,7 +94,8 @@ export const EnvironmentRoleAddForm = (props) => { { ))} + + + } + label={ +
+ Data.all managed + + Allow Data.all to attach IAM policies to this role + +
+ } + /> + { type: 'singleSelect', valueOptions: groupOptions.map((group) => group.label) }, + { + field: 'dataallManaged', + headerName: 'Policy Management', + valueGetter: (params) => { + return `${ + params.row.dataallManaged + ? 'Data.all managed' + : 'Customer managed' + }`; + }, + flex: 0.6 + }, + { + field: 'policiesNames', + headerName: 'IAM Policies', + flex: 0.5, + renderCell: (params: GridRenderCellParams) => ( + + + { + await navigator.clipboard.writeText( + params.row.managedPolicies.map( + (policy) => policy.policy_name + ) + ); + enqueueSnackbar( + 'Policy Name is copied to clipboard', + { + anchorOrigin: { + horizontal: 'right', + vertical: 'top' + }, + variant: 'success' + } + ); + }} + > + + + + ) + }, { field: 'actions', headerName: 'Actions', diff --git a/frontend/src/modules/Environments/services/listAllEnvironmentConsumptionRoles.js b/frontend/src/modules/Environments/services/listAllEnvironmentConsumptionRoles.js index 4024a9575..04cd522ea 100644 --- a/frontend/src/modules/Environments/services/listAllEnvironmentConsumptionRoles.js +++ b/frontend/src/modules/Environments/services/listAllEnvironmentConsumptionRoles.js @@ -28,6 +28,12 @@ export const listAllEnvironmentConsumptionRoles = ({ environmentUri groupUri IAMRoleArn + dataallManaged + managedPolicies { + policy_type + policy_name + attached + } } } } diff --git a/frontend/src/services/graphql/Environment/listEnvironmentConsumptionRoles.js b/frontend/src/services/graphql/Environment/listEnvironmentConsumptionRoles.js index 4d876ab88..a93028244 100644 --- a/frontend/src/services/graphql/Environment/listEnvironmentConsumptionRoles.js +++ b/frontend/src/services/graphql/Environment/listEnvironmentConsumptionRoles.js @@ -28,6 +28,12 @@ export const listEnvironmentConsumptionRoles = ({ environmentUri groupUri IAMRoleArn + dataallManaged + managedPolicies { + policy_type + policy_name + attached + } } } } diff --git a/tests/core/environments/conftest.py b/tests/core/environments/conftest.py index 0616f5a03..ebd41be72 100644 --- a/tests/core/environments/conftest.py +++ b/tests/core/environments/conftest.py @@ -32,7 +32,8 @@ def consumption_role(mock_aws_client, client, org_fixture, env_fixture, user, gr 'consumptionRoleName': str(uuid.uuid4())[:8], 'groupUri': str(uuid.uuid4())[:8], 'IAMRoleArn': test_arn, - 'environmentUri': env_fixture.environmentUri + 'environmentUri': env_fixture.environmentUri, + 'dataallManaged': False }, ) assert not response.errors diff --git a/tests/core/environments/test_environment.py b/tests/core/environments/test_environment.py index 9cb31b73a..1615d7d70 100644 --- a/tests/core/environments/test_environment.py +++ b/tests/core/environments/test_environment.py @@ -363,7 +363,7 @@ def test_paging(db, client, org_fixture, env_fixture, user, group): first_id = response.data.listEnvironments.nodes[0].environmentUri -def test_group_invitation(db, client, env_fixture, org_fixture, group2, user, group3, group): +def test_group_invitation(db, client, env_fixture, org_fixture, group2, user, group3, group, mocker): response = client.query( """ query listEnvironmentGroupInvitationPermissions($environmentUri:String){ @@ -382,6 +382,8 @@ def test_group_invitation(db, client, env_fixture, org_fixture, group2, user, gr env_permissions = [ p.name for p in response.data.listEnvironmentGroupInvitationPermissions ] + mocker.patch("dataall.core.environment.services.managed_iam_policies.PolicyManager.create_all_policies", + return_value=True) response = client.query( """ @@ -522,6 +524,8 @@ def test_group_invitation(db, client, env_fixture, org_fixture, group2, user, gr assert response.data.listAllEnvironmentGroups.count == 2 + mocker.patch("dataall.core.environment.services.managed_iam_policies.PolicyManager.delete_all_policies", + return_value=True) response = client.query( """ mutation removeGroupFromEnvironment($environmentUri: String!, $groupUri: String!){ @@ -620,8 +624,10 @@ def test_group_invitation(db, client, env_fixture, org_fixture, group2, user, gr ] -def test_archive_env(client, org_fixture, env, group, group2): +def test_archive_env(client, org_fixture, env, group, group2, mocker): env_fixture = env(org_fixture, 'dev-delete', 'alice', 'testadmins', '111111111111', 'eu-west-2') + mocker.patch("dataall.core.environment.services.managed_iam_policies.PolicyManager.delete_all_policies", + return_value=True) response = client.query( """ mutation deleteEnvironment($environmentUri:String!, $deleteFromAWS:Boolean!){ diff --git a/tests/modules/datasets/tasks/test_s3_access_point_share_manager.py b/tests/modules/datasets/tasks/test_s3_access_point_share_manager.py index 321a8bd75..97115e37e 100644 --- a/tests/modules/datasets/tasks/test_s3_access_point_share_manager.py +++ b/tests/modules/datasets/tasks/test_s3_access_point_share_manager.py @@ -6,11 +6,11 @@ from typing import Callable from dataall.core.groups.db.group_models import Group -from dataall.core.environment.db.environment_models import Environment, EnvironmentGroup +from dataall.core.environment.db.environment_models import Environment, EnvironmentGroup, ConsumptionRole from dataall.core.organizations.db.organization_models import Organization from dataall.modules.dataset_sharing.aws.s3_client import S3ControlClient from dataall.modules.dataset_sharing.db.share_object_models import ShareObject, ShareObjectItem - +from dataall.modules.dataset_sharing.services.managed_share_policy_service import SharePolicyService from dataall.modules.dataset_sharing.services.share_managers import S3AccessPointShareManager from dataall.modules.datasets_base.db.dataset_models import DatasetStorageLocation, Dataset @@ -25,6 +25,10 @@ DATAALL_ACCESS_POINT_KMS_DECRYPT_SID = "DataAll-Access-Point-KMS-Decrypt" DATAALL_KMS_PIVOT_ROLE_PERMISSIONS_SID = "KMSPivotRolePermissions" +IAM_S3_ACCESS_POINTS_STATEMENT_SID = "AccessPointsStatement" +IAM_S3_BUCKETS_STATEMENT_SID = "BucketStatement" +EMPTY_STATEMENT_SID = "EmptyStatement" + @pytest.fixture(scope="module") def source_environment(env: Callable, org_fixture: Organization, group: Group): @@ -214,6 +218,7 @@ def target_dataset_access_control_policy(request): "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", "Effect": "Allow", "Action": ["s3:*"], "Resource": [ @@ -224,6 +229,7 @@ def target_dataset_access_control_policy(request): ], }, { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -294,30 +300,68 @@ def test_manage_bucket_policy_existing_policy( # Then s3_client.create_bucket_policy.assert_not_called() - -@pytest.mark.parametrize("target_dataset_access_control_policy", - ([("bucketname", "aws_account_id", "access_point_name")]), - indirect=True) -def test_grant_target_role_access_policy_existing_policy_bucket_not_included( +def test_grant_target_role_access_policy_test_empty_policy( mocker, - dataset1, - location1, - target_dataset_access_control_policy, + dataset1: Dataset, + share1: ShareObject, + share_item_folder1: ShareObjectItem, + location1: DatasetStorageLocation, + target_environment: Environment, share_manager ): + initial_policy_document = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": EMPTY_STATEMENT_SID, + "Effect": "Allow", + "Action": "none:null", + "Resource": "*" + } + ] + } # Given - iam_policy = target_dataset_access_control_policy - mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=iam_policy, + "dataall.base.aws.iam.IAM.get_managed_policy_default_version", + return_value=('v1', initial_policy_document) ) iam_update_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.update_role_policy", + "dataall.base.aws.iam.IAM.update_managed_policy_default_version", return_value=None, ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True + ) + + expected_policy = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", + "Effect": "Allow", + "Action": ["s3:*"], + "Resource": [ + f"arn:aws:s3:::{location1.S3BucketName}", + f"arn:aws:s3:::{location1.S3BucketName}/*", + f"arn:aws:s3:{dataset1.region}:{dataset1.AwsAccountId}:accesspoint/{share_item_folder1.S3AccessPointName}", + f"arn:aws:s3:{dataset1.region}:{dataset1.AwsAccountId}:accesspoint/{share_item_folder1.S3AccessPointName}/*", + ], + }, + { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", + "Effect": "Allow", + "Action": [ + "kms:*" + ], + "Resource": [ + f"arn:aws:kms:{dataset1.region}:{dataset1.AwsAccountId}:key/kms-key" + ] + } + ], + } kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" @@ -325,22 +369,26 @@ def test_grant_target_role_access_policy_existing_policy_bucket_not_included( # When share_manager.grant_target_role_access_policy() + expected_policy_name = SharePolicyService( + environmentUri=target_environment.environmentUri, + role_name=share1.principalIAMRoleName, + account=target_environment.AwsAccountId, + resource_prefix=target_environment.resourcePrefix + ).generate_policy_name() # Then - iam_update_role_policy_mock.assert_called() - - # Iam function is called with str from object so we transform back to object - policy_object = json.loads(iam_update_role_policy_mock.call_args.args[3]) - - # Assert that bucket_name is inside the resource array of policy object - assert location1.S3BucketName in ",".join(policy_object["Statement"][0]["Resource"]) - assert f"arn:aws:kms:{dataset1.region}:{dataset1.AwsAccountId}:key/kms-key" in \ - iam_policy["Statement"][1]["Resource"] \ - and "kms:*" in iam_policy["Statement"][1]["Action"] + iam_update_role_policy_mock.assert_called_with( + target_environment.AwsAccountId, expected_policy_name, + "v1", json.dumps(expected_policy) + ) -@pytest.mark.parametrize("target_dataset_access_control_policy", ([("dataset1", SOURCE_ENV_ACCOUNT, "test")]), indirect=True) -def test_grant_target_role_access_policy_existing_policy_bucket_included( +@pytest.mark.parametrize("target_dataset_access_control_policy", + ([("bucketname", "aws_account_id", "access_point_name")]), + indirect=True) +def test_grant_target_role_access_policy_existing_policy_bucket_not_included( mocker, + dataset1, + location1, target_dataset_access_control_policy, share_manager ): @@ -349,12 +397,17 @@ def test_grant_target_role_access_policy_existing_policy_bucket_included( iam_policy = target_dataset_access_control_policy mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=iam_policy, + "dataall.base.aws.iam.IAM.get_managed_policy_default_version", + return_value=('v1', iam_policy) + + ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True ) iam_update_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.update_role_policy", + "dataall.base.aws.iam.IAM.update_managed_policy_default_version", return_value=None, ) @@ -367,53 +420,45 @@ def test_grant_target_role_access_policy_existing_policy_bucket_included( # Then iam_update_role_policy_mock.assert_called() + # Iam function is called with str from object so we transform back to object + policy_object = iam_policy + s3_index = SharePolicyService._get_statement_by_sid(policy=policy_object, sid=f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=policy_object, sid=f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS") + + # Assert that bucket_name is inside the resource array of policy object + assert location1.S3BucketName in ",".join(policy_object["Statement"][s3_index]["Resource"]) + assert f"arn:aws:kms:{dataset1.region}:{dataset1.AwsAccountId}:key/kms-key" in \ + iam_policy["Statement"][kms_index]["Resource"] \ + and "kms:*" in iam_policy["Statement"][kms_index]["Action"] -def test_grant_target_role_access_policy_test_no_policy( + # Assert that statements for S3 bucket sharing are unaffected + s3_index = SharePolicyService._get_statement_by_sid(policy=policy_object, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + +@pytest.mark.parametrize("target_dataset_access_control_policy", ([("dataset1", SOURCE_ENV_ACCOUNT, "test")]), indirect=True) +def test_grant_target_role_access_policy_existing_policy_bucket_included( mocker, - dataset1: Dataset, - share1: ShareObject, - share_item_folder1: ShareObjectItem, - location1: DatasetStorageLocation, - target_environment: Environment, + target_dataset_access_control_policy, share_manager ): # Given + iam_policy = target_dataset_access_control_policy + mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=None, + "dataall.base.aws.iam.IAM.get_managed_policy_default_version", + return_value=('v1', iam_policy) + ) + + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True ) iam_update_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.update_role_policy", + "dataall.base.aws.iam.IAM.update_managed_policy_default_version", return_value=None, ) - expected_policy = { - "Version": "2012-10-17", - "Statement": [ - { - "Effect": "Allow", - "Action": ["s3:*"], - "Resource": [ - f"arn:aws:s3:::{location1.S3BucketName}", - f"arn:aws:s3:::{location1.S3BucketName}/*", - f"arn:aws:s3:{dataset1.region}:{dataset1.AwsAccountId}:accesspoint/{share_item_folder1.S3AccessPointName}", - f"arn:aws:s3:{dataset1.region}:{dataset1.AwsAccountId}:accesspoint/{share_item_folder1.S3AccessPointName}/*", - ], - }, - { - "Effect": "Allow", - "Action": [ - "kms:*" - ], - "Resource": [ - f"arn:aws:kms:{dataset1.region}:{dataset1.AwsAccountId}:key/kms-key" - ] - } - ], - } - kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" @@ -421,11 +466,7 @@ def test_grant_target_role_access_policy_test_no_policy( share_manager.grant_target_role_access_policy() # Then - iam_update_role_policy_mock.assert_called_with( - target_environment.AwsAccountId, share1.principalIAMRoleName, - "targetDatasetAccessControlPolicy", json.dumps(expected_policy) - ) - + iam_update_role_policy_mock.assert_called() def test_update_dataset_bucket_key_policy_with_env_admin( mocker, @@ -855,13 +896,16 @@ def test_delete_target_role_access_policy_no_remaining_statement( target_environment: Environment, share_manager ): - # Given ap policy that only includes AllowAllToAdminStatement + # Given IAM policy with no bucket shares existing_target_role_policy = { "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", "Effect": "Allow", - "Action": ["s3:*"], + "Action": [ + "s3:*" + ], "Resource": [ f"arn:aws:s3:::{location1.S3BucketName}", f"arn:aws:s3:::{location1.S3BucketName}/*", @@ -870,6 +914,7 @@ def test_delete_target_role_access_policy_no_remaining_statement( ], }, { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -881,18 +926,30 @@ def test_delete_target_role_access_policy_no_remaining_statement( ], } + expected_remaining_target_role_policy = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": EMPTY_STATEMENT_SID, + "Effect": "Allow", + "Action": "none:null", + "Resource": "*" + } + ] + } + + # Given mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=existing_target_role_policy, + "dataall.base.aws.iam.IAM.get_managed_policy_default_version", + return_value=('v1', existing_target_role_policy) ) - - iam_delete_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.delete_role_policy", - return_value=None, + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True ) iam_update_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.update_role_policy", + "dataall.base.aws.iam.IAM.update_managed_policy_default_version", return_value=None, ) @@ -902,9 +959,17 @@ def test_delete_target_role_access_policy_no_remaining_statement( # When share_manager.delete_target_role_access_policy(share1, dataset1, target_environment) - # Then - iam_delete_role_policy_mock.assert_called() - iam_update_role_policy_mock.assert_not_called() + expected_policy_name = SharePolicyService( + environmentUri=target_environment.environmentUri, + role_name=share1.principalIAMRoleName, + account=target_environment.AwsAccountId, + resource_prefix = target_environment.resourcePrefix + ).generate_policy_name() + + iam_update_role_policy_mock.assert_called_with( + target_environment.AwsAccountId, expected_policy_name, + "v1", json.dumps(expected_remaining_target_role_policy) + ) def test_delete_target_role_access_policy_with_remaining_statement( @@ -921,8 +986,11 @@ def test_delete_target_role_access_policy_with_remaining_statement( "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", "Effect": "Allow", - "Action": ["s3:*"], + "Action": [ + "s3:*" + ], "Resource": [ "arn:aws:s3:::UNRELATED_BUCKET_ARN", f"arn:aws:s3:::{location1.S3BucketName}", @@ -932,6 +1000,7 @@ def test_delete_target_role_access_policy_with_remaining_statement( ], }, { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -948,11 +1017,15 @@ def test_delete_target_role_access_policy_with_remaining_statement( "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3", "Effect": "Allow", - "Action": ["s3:*"], + "Action": [ + "s3:*" + ], "Resource": ["arn:aws:s3:::UNRELATED_BUCKET_ARN"], }, { + "Sid": f"{IAM_S3_ACCESS_POINTS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -964,18 +1037,19 @@ def test_delete_target_role_access_policy_with_remaining_statement( ], } + # Given mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=existing_target_role_policy, + "dataall.base.aws.iam.IAM.get_managed_policy_default_version", + return_value=('v1', existing_target_role_policy) ) - iam_delete_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.delete_role_policy", - return_value=None, + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True ) iam_update_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.update_role_policy", + "dataall.base.aws.iam.IAM.update_managed_policy_default_version", return_value=None, ) @@ -986,13 +1060,16 @@ def test_delete_target_role_access_policy_with_remaining_statement( share_manager.delete_target_role_access_policy(share1, dataset1, target_environment) # Then - iam_delete_role_policy_mock.assert_not_called() + expected_policy_name = SharePolicyService( + environmentUri=target_environment.environmentUri, + role_name=share1.principalIAMRoleName, + account=target_environment.AwsAccountId, + resource_prefix=target_environment.resourcePrefix + ).generate_policy_name() iam_update_role_policy_mock.assert_called_with( - target_environment.AwsAccountId, - share1.principalIAMRoleName, - "targetDatasetAccessControlPolicy", - json.dumps(expected_remaining_target_role_policy), + target_environment.AwsAccountId, expected_policy_name, + "v1", json.dumps(expected_remaining_target_role_policy) ) @@ -1177,10 +1254,11 @@ def test_check_target_role_access_policy( share_manager ): # Given - iam_get_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=target_dataset_access_control_policy, - ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True) + iam_get_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", + return_value=('v1', target_dataset_access_control_policy)) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "some-key-2112" @@ -1188,7 +1266,7 @@ def test_check_target_role_access_policy( # When share_manager.check_target_role_access_policy() # Then - iam_get_role_policy_mock.assert_called() + iam_get_policy_mock.assert_called() kms_client().get_key_id.assert_called() assert len(share_manager.folder_errors) == 0 @@ -1202,10 +1280,11 @@ def test_check_target_role_access_policy_existing_policy_bucket_and_key_not_incl share_manager ): # Given - iam_get_role_policy_mock = mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=target_dataset_access_control_policy, - ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True) + # Gets policy with other S3 and KMS + iam_get_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1', target_dataset_access_control_policy)) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" @@ -1213,7 +1292,7 @@ def test_check_target_role_access_policy_existing_policy_bucket_and_key_not_incl # When share_manager.check_target_role_access_policy() # Then - iam_get_role_policy_mock.assert_called() + iam_get_policy_mock.assert_called() kms_client().get_key_id.assert_called() assert len(share_manager.folder_errors) == 2 @@ -1225,8 +1304,8 @@ def test_check_target_role_access_policy_test_no_policy( # Given mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=None, + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=False ) kms_client = mock_kms_client(mocker) @@ -1247,7 +1326,7 @@ def test_check_access_point_and_policy( access_point_arn = "existing-access-point-arn" s3_control_client = mock_s3_control_client(mocker) s3_control_client().get_bucket_access_point_arn.return_value = access_point_arn - + # target_env_admin is already in policy but current folder is NOT yet in prefix_list existing_ap_policy = _generate_ap_policy_object(access_point_arn, [[target_environment.SamlGroupName, [location1.S3Prefix]]]) # Existing access point policy diff --git a/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py b/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py index 84d74162b..28c45e1e8 100644 --- a/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py +++ b/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py @@ -9,6 +9,7 @@ from dataall.core.organizations.db.organization_models import Organization from dataall.modules.dataset_sharing.db.share_object_models import ShareObject from dataall.modules.dataset_sharing.services.share_managers import S3BucketShareManager +from dataall.modules.dataset_sharing.services.managed_share_policy_service import SharePolicyService from dataall.modules.datasets_base.db.dataset_models import Dataset, DatasetBucket SOURCE_ENV_ACCOUNT = "111111111111" @@ -23,6 +24,9 @@ DATAALL_BUCKET_KMS_DECRYPT_SID = "DataAll-Bucket-KMS-Decrypt" DATAALL_KMS_PIVOT_ROLE_PERMISSIONS_SID = "KMSPivotRolePermissions" +IAM_S3_ACCESS_POINTS_STATEMENT_SID = "AccessPointsStatement" +IAM_S3_BUCKETS_STATEMENT_SID = "BucketStatement" +EMPTY_STATEMENT_SID = "EmptyStatement" @pytest.fixture(scope="module") def source_environment(env: Callable, org_fixture: Organization, group: Group): @@ -454,37 +458,118 @@ def test_grant_role_bucket_policy_with_another_read_only_role( assert len(modified_bucket_policy["Statement"][1]["Principal"]["AWS"]) == 2 assert modified_bucket_policy["Statement"][1]["Principal"]["AWS"][0] == "SomeTargetResourceArn" - def test_grant_s3_iam_access_with_no_policy( mocker, dataset2, share2_manager ): # Given - # There is not existing IAM policy in the requesters account for the dataset's S3bucket - # Check if the update_role_policy func is called and policy statements are added + # The IAM Policy for sharing for the IAM role does not exist (check_if_policy_exists returns False) + # Backwards compatibility: check that the create_managed_policy_from_inline_and_delete_inline is called + # Check if the get and update_role_policy func are called and policy statements are added - mocker.patch("dataall.base.aws.iam.IAM.get_role_policy", return_value=None) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=False) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" - iam_update_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.update_role_policy", return_value=None) + empty_policy_document = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": EMPTY_STATEMENT_SID, + "Effect": "Allow", + "Action": "none:null", + "Resource": "*" + } + ] + } + share_policy_service_mock_1 = mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.create_managed_policy_from_inline_and_delete_inline", + return_value="arn:iam::someArn") + share_policy_service_mock_2 = mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.attach_policy", + return_value=True) + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1',empty_policy_document)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", + return_value=None) share2_manager.grant_s3_iam_access() - iam_update_role_policy_mock.assert_called() + # Assert IAM and service calls called + share_policy_service_mock_1.assert_called_once() + share_policy_service_mock_2.assert_called_once() + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() + + iam_policy = empty_policy_document + + s3_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") - iam_policy = json.loads(iam_update_role_policy_mock.call_args.args[3]) # Assert if the IAM role policy with S3 and KMS permissions was created assert len(iam_policy["Statement"]) == 2 - assert len(iam_policy["Statement"][0]["Resource"]) == 2 - assert len(iam_policy["Statement"][1]["Resource"]) == 1 - assert f"arn:aws:s3:::{dataset2.S3BucketName}" in iam_policy["Statement"][0]["Resource"] and "s3:*" in iam_policy["Statement"][0]["Action"] + assert len(iam_policy["Statement"][s3_index]["Resource"]) == 2 + assert len(iam_policy["Statement"][kms_index]["Resource"]) == 1 + assert f"arn:aws:s3:::{dataset2.S3BucketName}" in iam_policy["Statement"][s3_index]["Resource"] and "s3:*" in \ + iam_policy["Statement"][s3_index]["Action"] assert f"arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key" in \ - iam_policy["Statement"][1]["Resource"] \ - and "kms:*" in iam_policy["Statement"][1]["Action"] + iam_policy["Statement"][kms_index]["Resource"] \ + and "kms:*" in iam_policy["Statement"][kms_index]["Action"] + + +def test_grant_s3_iam_access_with_empty_policy( + mocker, + dataset2, + share2_manager +): + # Given + # The IAM Policy for sharing for the IAM role exists (check_if_policy_exists returns True) + # And the IAM Policy is empty (get_managed_policy_default_version returns initial_policy_document) + # Check if the get and update_role_policy func are called and policy statements are added + + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) + + kms_client = mock_kms_client(mocker) + kms_client().get_key_id.return_value = "kms-key" + + initial_policy_document = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": EMPTY_STATEMENT_SID, + "Effect": "Allow", + "Action": "none:null", + "Resource": "*" + } + ] + } + + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1',initial_policy_document)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", + return_value=None) + share2_manager.grant_s3_iam_access() + + # Assert IAM called + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() + + iam_policy = initial_policy_document + + s3_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") + + + # Assert if the IAM role policy with S3 and KMS permissions was created + assert len(iam_policy["Statement"]) == 2 + assert len(iam_policy["Statement"][s3_index]["Resource"]) == 2 + assert len(iam_policy["Statement"][kms_index]["Resource"]) == 1 + assert f"arn:aws:s3:::{dataset2.S3BucketName}" in iam_policy["Statement"][s3_index]["Resource"] and "s3:*" in \ + iam_policy["Statement"][s3_index]["Action"] + assert f"arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key" in \ + iam_policy["Statement"][kms_index]["Resource"] \ + and "kms:*" in iam_policy["Statement"][kms_index]["Action"] def test_grant_s3_iam_access_with_policy_and_target_resources_not_present( @@ -492,13 +577,16 @@ def test_grant_s3_iam_access_with_policy_and_target_resources_not_present( dataset2, share2_manager ): - # Given policy with some other bucket as resource - # Check if the correct resource is attached/appended + # Given + # The IAM Policy for sharing for the IAM role exists (check_if_policy_exists returns True) + # And the IAM Policy is NOT empty (get_managed_policy_default_version returns policy) + # Check if the get and update_role_policy func are called and policy statements are added to the existing ones policy = { "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "Effect": "Allow", "Action": [ "s3:*" @@ -509,6 +597,7 @@ def test_grant_s3_iam_access_with_policy_and_target_resources_not_present( ] }, { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -520,29 +609,35 @@ def test_grant_s3_iam_access_with_policy_and_target_resources_not_present( ] } - mocker.patch("dataall.base.aws.iam.IAM.get_role_policy", return_value=policy) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) + + s3_index = SharePolicyService._get_statement_by_sid(policy=policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") assert len(policy["Statement"]) == 2 - assert len(policy["Statement"][0]["Resource"]) == 2 - assert len(policy["Statement"][1]["Resource"]) == 1 + assert len(policy["Statement"][s3_index]["Resource"]) == 2 + assert len(policy["Statement"][kms_index]["Resource"]) == 1 kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" - iam_update_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.update_role_policy", return_value=None) + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1',policy)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", return_value=('v1',policy)) share2_manager.grant_s3_iam_access() - iam_update_role_policy_mock.assert_called() + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() - iam_policy = json.loads(iam_update_role_policy_mock.call_args.args[3]) + iam_policy = policy # Assert that new resources were appended assert len(policy["Statement"]) == 2 - assert len(iam_policy["Statement"][0]["Resource"]) == 4 - assert f'arn:aws:s3:::{dataset2.S3BucketName}' in iam_policy["Statement"][0]["Resource"] - assert len(iam_policy["Statement"][1]["Resource"]) == 2 - assert f"arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key" in iam_policy["Statement"][1]["Resource"] + assert len(iam_policy["Statement"][s3_index]["Resource"]) == 4 + assert f'arn:aws:s3:::{dataset2.S3BucketName}' in iam_policy["Statement"][s3_index]["Resource"] + assert len(iam_policy["Statement"][kms_index]["Resource"]) == 2 + assert f"arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key" in iam_policy["Statement"][kms_index][ + "Resource"] # Tests to check if @@ -551,23 +646,29 @@ def test_grant_s3_iam_access_with_complete_policy_present( dataset2, share2_manager ): - # Given complete policy present with required target resources - # Check if policy created after calling function and the existing Policy is same + # Given + # The IAM Policy for sharing for the IAM role exists (check_if_policy_exists returns True) + # And the IAM Policy is NOT empty and already contains all target resources (get_managed_policy_default_version returns policy) + # Check if policy created after calling function and the existing Policy are the same policy = { "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "Effect": "Allow", "Action": [ "s3:*" ], "Resource": [ f"arn:aws:s3:::{dataset2.S3BucketName}", - f"arn:aws:s3:::{dataset2.S3BucketName}/*" + f"arn:aws:s3:::{dataset2.S3BucketName}/*", + f"arn:aws:s3:::S3Bucket", + f"arn:aws:s3:::S3Bucket/*" ] }, { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -579,24 +680,31 @@ def test_grant_s3_iam_access_with_complete_policy_present( ] } - mocker.patch("dataall.base.aws.iam.IAM.get_role_policy", return_value=policy) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" - iam_update_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.update_role_policy", return_value=None) - + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1',policy)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", + return_value=None) share2_manager.grant_s3_iam_access() # Assert that the IAM Policy is the same as the existing complete policy - iam_update_role_policy_mock.assert_called() + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() - created_iam_policy = json.loads(iam_update_role_policy_mock.call_args.args[3]) + created_iam_policy = policy + + s3_index = SharePolicyService._get_statement_by_sid(policy=policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") assert len(created_iam_policy["Statement"]) == 2 - assert policy["Statement"][0]["Resource"] == created_iam_policy["Statement"][0]["Resource"] and policy["Statement"][0]["Action"] == created_iam_policy["Statement"][0]["Action"] - assert policy["Statement"][1]["Resource"] == created_iam_policy["Statement"][1]["Resource"] and policy["Statement"][1]["Action"] == \ - created_iam_policy["Statement"][1]["Action"] + assert policy["Statement"][s3_index]["Resource"] == created_iam_policy["Statement"][s3_index]["Resource"] and \ + policy["Statement"][s3_index]["Action"] == created_iam_policy["Statement"][s3_index]["Action"] + assert policy["Statement"][kms_index]["Resource"] == created_iam_policy["Statement"][kms_index]["Resource"] and \ + policy["Statement"][kms_index]["Action"] == \ + created_iam_policy["Statement"][kms_index]["Action"] def test_grant_dataset_bucket_key_policy_with_complete_policy_present( @@ -875,6 +983,71 @@ def test_delete_target_role_bucket_policy_with_one_principal_in_policy( assert f"{DATAALL_READ_ONLY_SID}" not in sid_list +def test_delete_target_role_access_no_policy_no_other_resources_shared( + mocker, + bucket2, + share2: ShareObject, + target_environment: Environment, + share2_manager +): + # Given + # IAM Policy does not exist (for backwards compatibility v2.3 or because it was manually deleted), check_if_policy_exists = False + # No existing resources were shared with the target role + # An empty IAM policy is created, revoked resources cannot be deleted, so the empty policy is the end result + + policy_document = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": EMPTY_STATEMENT_SID, + "Effect": "Allow", + "Action": "none:null", + "Resource": "*" + } + ] + } + + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=False) + + kms_client = mock_kms_client(mocker) + kms_client().get_key_id.return_value = "kms-key" + + share_policy_service_mock_1 = mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.create_managed_policy_from_inline_and_delete_inline", + return_value="arn:iam::someArn") + share_policy_service_mock_2 = mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.attach_policy", + return_value=True) + + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", + return_value=('v1', policy_document)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", + return_value=None) + + share2_manager.delete_target_role_access_policy( + share=share2, + target_bucket=bucket2, + target_environment=target_environment + ) + + share_policy_service_mock_1.assert_called_once() + share_policy_service_mock_2.assert_called_once() + + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() + + # Get the updated IAM policy and compare it with the existing one + updated_iam_policy = policy_document + s3_index = SharePolicyService._get_statement_by_sid(policy=updated_iam_policy, + sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=updated_iam_policy, + sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") + + assert len(updated_iam_policy["Statement"]) == 1 + assert "*" == updated_iam_policy["Statement"][0]["Resource"] + def test_delete_target_role_access_policy_no_resource_of_datasets_s3_bucket( mocker, bucket2, @@ -883,13 +1056,14 @@ def test_delete_target_role_access_policy_no_resource_of_datasets_s3_bucket( share2_manager ): # Given - # IAM Policy which doesn't contain target S3 bucket resources - # IAM.delete_role_policy & IAM.update_role_policy should not be called + # IAM Policy exists and doesn't contain target S3 bucket resources + # The IAM Policy remains the same iam_policy = { "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "Effect": "Allow", "Action": [ "s3:*" @@ -900,6 +1074,7 @@ def test_delete_target_role_access_policy_no_resource_of_datasets_s3_bucket( ] }, { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -911,17 +1086,16 @@ def test_delete_target_role_access_policy_no_resource_of_datasets_s3_bucket( ] } - mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=iam_policy, - ) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" - iam_update_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.update_role_policy", return_value=None) + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1', iam_policy)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", + return_value=None) + - iam_delete_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.delete_role_policy", return_value=None) share2_manager.delete_target_role_access_policy( share=share2, @@ -929,16 +1103,20 @@ def test_delete_target_role_access_policy_no_resource_of_datasets_s3_bucket( target_environment=target_environment ) - iam_update_role_policy_mock.assert_called() - iam_delete_role_policy_mock.assert_not_called() + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() # Get the updated IAM policy and compare it with the existing one - updated_iam_policy = json.loads(iam_update_role_policy_mock.call_args.args[3]) + updated_iam_policy = iam_policy + s3_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") + + assert len(updated_iam_policy["Statement"]) == 2 - assert "arn:aws:s3:::someOtherBucket,arn:aws:s3:::someOtherBucket/*" == ",".join(updated_iam_policy["Statement"][0]["Resource"]) + assert "arn:aws:s3:::someOtherBucket,arn:aws:s3:::someOtherBucket/*" == ",".join( + updated_iam_policy["Statement"][s3_index]["Resource"]) assert "arn:aws:kms:us-east-1:121231131212:key/some-key-2112" == ",".join( - updated_iam_policy["Statement"][1]["Resource"]) - + updated_iam_policy["Statement"][kms_index]["Resource"]) def test_delete_target_role_access_policy_with_multiple_s3_buckets_in_policy( mocker, @@ -956,6 +1134,7 @@ def test_delete_target_role_access_policy_with_multiple_s3_buckets_in_policy( "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "Effect": "Allow", "Action": [ "s3:*" @@ -968,6 +1147,7 @@ def test_delete_target_role_access_policy_with_multiple_s3_buckets_in_policy( ] }, { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -980,14 +1160,13 @@ def test_delete_target_role_access_policy_with_multiple_s3_buckets_in_policy( ] } - mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=iam_policy, - ) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) + + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1',iam_policy)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", + return_value=None) - iam_update_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.update_role_policy", return_value=None) - iam_delete_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.delete_role_policy", return_value=None) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" @@ -998,18 +1177,23 @@ def test_delete_target_role_access_policy_with_multiple_s3_buckets_in_policy( target_environment=target_environment ) - iam_update_role_policy_mock.assert_called() - iam_delete_role_policy_mock.assert_not_called() + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() + + updated_iam_policy = iam_policy - updated_iam_policy = json.loads(iam_update_role_policy_mock.call_args.args[3]) + s3_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}S3") + kms_index = SharePolicyService._get_statement_by_sid(policy=iam_policy, sid=f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS") - assert f"arn:aws:s3:::{dataset2.S3BucketName}" not in updated_iam_policy["Statement"][0]["Resource"] - assert f"arn:aws:s3:::{dataset2.S3BucketName}/*" not in updated_iam_policy["Statement"][0]["Resource"] - assert f"arn:aws:s3:::someOtherBucket" in updated_iam_policy["Statement"][0]["Resource"] - assert f"arn:aws:s3:::someOtherBucket/*" in updated_iam_policy["Statement"][0]["Resource"] - assert f"arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key" not in updated_iam_policy["Statement"][1]["Resource"] - assert f"arn:aws:kms:us-east-1:121231131212:key/some-key-2112" in updated_iam_policy["Statement"][1]["Resource"] + assert f"arn:aws:s3:::{dataset2.S3BucketName}" not in updated_iam_policy["Statement"][s3_index]["Resource"] + assert f"arn:aws:s3:::{dataset2.S3BucketName}/*" not in updated_iam_policy["Statement"][s3_index]["Resource"] + assert f"arn:aws:s3:::someOtherBucket" in updated_iam_policy["Statement"][s3_index]["Resource"] + assert f"arn:aws:s3:::someOtherBucket/*" in updated_iam_policy["Statement"][s3_index]["Resource"] + + assert f"arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key" not in \ + updated_iam_policy["Statement"][kms_index]["Resource"] + assert f"arn:aws:kms:us-east-1:121231131212:key/some-key-2112" in updated_iam_policy["Statement"][kms_index]["Resource"] def test_delete_target_role_access_policy_with_one_s3_bucket_and_one_kms_resource_in_policy( @@ -1028,6 +1212,7 @@ def test_delete_target_role_access_policy_with_one_s3_bucket_and_one_kms_resourc "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "Effect": "Allow", "Action": [ "s3:*" @@ -1038,6 +1223,7 @@ def test_delete_target_role_access_policy_with_one_s3_bucket_and_one_kms_resourc ] }, { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -1049,17 +1235,15 @@ def test_delete_target_role_access_policy_with_one_s3_bucket_and_one_kms_resourc ] } - mocker.patch( - "dataall.base.aws.iam.IAM.get_role_policy", - return_value=iam_policy, - ) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" - iam_update_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.update_role_policy", return_value=None) + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1',iam_policy)) + iam_update_role_policy_mock_2 = mocker.patch("dataall.base.aws.iam.IAM.update_managed_policy_default_version", + return_value=None) - iam_delete_role_policy_mock = mocker.patch("dataall.base.aws.iam.IAM.delete_role_policy", return_value=None) share2_manager.delete_target_role_access_policy( share=share2, @@ -1067,8 +1251,8 @@ def test_delete_target_role_access_policy_with_one_s3_bucket_and_one_kms_resourc target_environment=target_environment ) - iam_update_role_policy_mock.assert_not_called() - iam_delete_role_policy_mock.assert_called() + iam_update_role_policy_mock_1.assert_called_once() + iam_update_role_policy_mock_2.assert_called_once() def test_delete_target_role_bucket_key_policy_with_no_target_requester_id( @@ -1244,7 +1428,7 @@ def test_delete_target_role_bucket_key_policy_with_multiple_principals_in_policy new_kms_policy["Statement"][0]["Principal"]["AWS"] - + def test_check_role_bucket_policy( mocker, share2: ShareObject, @@ -1278,7 +1462,7 @@ def test_check_role_bucket_policy( s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) - + share2_manager.check_role_bucket_policy() assert(len(share2_manager.bucket_errors)) == 0 @@ -1317,7 +1501,7 @@ def test_check_role_bucket_policy_missing_role_principal( s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) - + share2_manager.check_role_bucket_policy() assert(len(share2_manager.bucket_errors)) == 1 @@ -1352,6 +1536,7 @@ def test_check_s3_iam_access( "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "Effect": "Allow", "Action": [ "s3:*" @@ -1362,6 +1547,7 @@ def test_check_s3_iam_access( ] }, { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -1372,13 +1558,16 @@ def test_check_s3_iam_access( } ] } - mocker.patch("dataall.base.aws.iam.IAM.get_role_policy", return_value=policy) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) + # Gets policy with S3 and KMS + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1', policy)) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" share2_manager.check_s3_iam_access() # Then + iam_update_role_policy_mock_1.assert_called_once() assert(len(share2_manager.bucket_errors)) == 0 @@ -1391,14 +1580,55 @@ def test_check_s3_iam_access_no_policy( # There is not existing IAM policy in the requesters account for the dataset's S3bucket # Check if the update_role_policy func is called and policy statements are added - mocker.patch("dataall.base.aws.iam.IAM.get_role_policy", return_value=None) + # When policy does not exist + iam_update_role_policy_mock_1 = mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=False) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "kms-key" # When share2_manager.check_s3_iam_access() # Then + iam_update_role_policy_mock_1.assert_called_once() assert(len(share2_manager.bucket_errors)) == 1 + assert "IAM Policy Target Resource" in share2_manager.bucket_errors[0] + +def test_check_s3_iam_access_missing_policy_statement( + mocker, + dataset2, + share2_manager +): + # Given policy with some other bucket as resource + # Check if the needed {IAM_S3_BUCKETS_STATEMENT_SID}S3 statement exists in the IAM policy + # KMS Statement exist but does not contain resources - triggers different error + + policy = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", + "Effect": "Allow", + "Action": [ + "kms:*" + ], + "Resource": [ + f"arn:aws:kms:us-east-1:12121121121:key/some-kms-key" + ] + } + ] + } + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) + # Gets policy with other S3 and KMS + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1', policy)) + + kms_client = mock_kms_client(mocker) + kms_client().get_key_id.return_value = "another-kms-key" + + share2_manager.check_s3_iam_access() + # Then + iam_update_role_policy_mock_1.assert_called_once() + assert f"missing IAM Policy Statement permissions: {IAM_S3_BUCKETS_STATEMENT_SID}S3" in share2_manager.bucket_errors[0] + assert f"missing IAM Policy Resource permissions: {IAM_S3_BUCKETS_STATEMENT_SID}KMS" in share2_manager.bucket_errors[1] + def test_check_s3_iam_access_missing_target_resource( mocker, @@ -1412,6 +1642,7 @@ def test_check_s3_iam_access_missing_target_resource( "Version": "2012-10-17", "Statement": [ { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}S3", "Effect": "Allow", "Action": [ "s3:*" @@ -1422,6 +1653,7 @@ def test_check_s3_iam_access_missing_target_resource( ] }, { + "Sid": f"{IAM_S3_BUCKETS_STATEMENT_SID}KMS", "Effect": "Allow", "Action": [ "kms:*" @@ -1432,15 +1664,20 @@ def test_check_s3_iam_access_missing_target_resource( } ] } - mocker.patch("dataall.base.aws.iam.IAM.get_role_policy", return_value=policy) + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", return_value=True) + # Gets policy with other S3 and KMS + iam_update_role_policy_mock_1 = mocker.patch("dataall.base.aws.iam.IAM.get_managed_policy_default_version", return_value=('v1', policy)) kms_client = mock_kms_client(mocker) kms_client().get_key_id.return_value = "another-kms-key" share2_manager.check_s3_iam_access() # Then + iam_update_role_policy_mock_1.assert_called_once() assert(len(share2_manager.bucket_errors)) == 2 - + assert f"missing IAM Policy Resource permissions: {IAM_S3_BUCKETS_STATEMENT_SID}S3" in share2_manager.bucket_errors[0] + assert f"missing IAM Policy Resource permissions: {IAM_S3_BUCKETS_STATEMENT_SID}KMS" in share2_manager.bucket_errors[1] + def test_check_dataset_bucket_key_policy( mocker, diff --git a/tests/modules/datasets/test_dataset_resource_found.py b/tests/modules/datasets/test_dataset_resource_found.py index 0c5294518..dfc50e9f6 100644 --- a/tests/modules/datasets/test_dataset_resource_found.py +++ b/tests/modules/datasets/test_dataset_resource_found.py @@ -33,7 +33,11 @@ def get_env(client, env_fixture, group): ) -def test_dataset_resource_found(db, client, env_fixture, org_fixture, group2, user, group3, group, dataset): +def test_dataset_resource_found(db, client, env_fixture, org_fixture, group2, user, group3, group, dataset, mocker): + mocker.patch("dataall.core.environment.services.managed_iam_policies.PolicyManager.create_all_policies", + return_value=True) + mocker.patch("dataall.core.environment.services.managed_iam_policies.PolicyManager.delete_all_policies", + return_value=True) response = client.query( """ query listEnvironmentGroupInvitationPermissions($environmentUri:String){ diff --git a/tests/modules/datasets/test_environment_stack_with_dataset.py b/tests/modules/datasets/test_environment_stack_with_dataset.py index 6448c8ee9..b93481e56 100644 --- a/tests/modules/datasets/test_environment_stack_with_dataset.py +++ b/tests/modules/datasets/test_environment_stack_with_dataset.py @@ -93,8 +93,9 @@ def patch_methods(mocker, db, env_fixture, another_group, permissions): ) -def test_resources_created(env_fixture, org_fixture): +def test_resources_created(env_fixture, org_fixture, mocker): app = App() + mocker.patch("dataall.core.environment.services.managed_iam_policies.PolicyManager.get_all_policies", return_value=[]) # Create the Stack stack = EnvironmentSetup(app, 'Environment', target_uri=env_fixture.environmentUri) diff --git a/tests/modules/datasets/test_share.py b/tests/modules/datasets/test_share.py index ba867f48a..687e9fbd8 100644 --- a/tests/modules/datasets/test_share.py +++ b/tests/modules/datasets/test_share.py @@ -470,7 +470,7 @@ def test_init(tables1, tables2): # Queries & mutations -def create_share_object(client, username, group, groupUri, environmentUri, datasetUri, itemUri=None): +def create_share_object(mocker, client, username, group, groupUri, environmentUri, datasetUri, itemUri=None, attachMissingPolicies=True, principalId=None, principalType=PrincipalType.Group.value): q = """ mutation CreateShareObject( $datasetUri: String! @@ -494,9 +494,11 @@ def create_share_object(client, username, group, groupUri, environmentUri, datas datasetUri datasetName } + } } """ + mocker.patch("dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.create_managed_policy_from_inline_and_delete_inline", return_value=True) response = client.query( q, @@ -508,9 +510,10 @@ def create_share_object(client, username, group, groupUri, environmentUri, datas input={ 'environmentUri': environmentUri, 'groupUri': groupUri, - 'principalId': groupUri, - 'principalType': PrincipalType.Group.value, + 'principalId': principalId if principalId else groupUri, + 'principalType': principalType, 'requestPurpose': 'testShare', + 'attachMissingPolicies': attachMissingPolicies }, ) @@ -987,11 +990,12 @@ def list_datasets_published_in_environment(client, user, group, environmentUri): # Tests -def test_create_share_object_unauthorized(client, group3, dataset1, env2, env2group): +def test_create_share_object_unauthorized(mocker, client, group3, dataset1, env2, env2group): # Given # Existing dataset, target environment and group # When a user that does not belong to environment and group creates request create_share_object_response = create_share_object( + mocker=mocker, client=client, username='anonymous', group=group3, @@ -1003,11 +1007,21 @@ def test_create_share_object_unauthorized(client, group3, dataset1, env2, env2gr assert 'Unauthorized' in create_share_object_response.errors[0].message -def test_create_share_object_as_requester(client, user2, group2, env2group, env2, dataset1): +def test_create_share_object_as_requester(mocker, client, user2, group2, env2group, env2, dataset1): # Given # Existing dataset, target environment and group + # SharePolicy exists and is attached # When a user that belongs to environment and group creates request + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True + ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_attached", + return_value=True + ) create_share_object_response = create_share_object( + mocker=mocker, client=client, username=user2.username, group=group2, @@ -1021,11 +1035,21 @@ def test_create_share_object_as_requester(client, user2, group2, env2group, env2 assert create_share_object_response.data.createShareObject.userRoleForShareObject == 'Requesters' assert create_share_object_response.data.createShareObject.requestPurpose == 'testShare' -def test_create_share_object_as_approver_and_requester(client, user, group2, env2group, env2, dataset1): +def test_create_share_object_as_approver_and_requester(mocker, client, user, group2, env2group, env2, dataset1): # Given # Existing dataset, target environment and group + # SharePolicy exists and is attached # When a user that belongs to environment and group creates request + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True + ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_attached", + return_value=True + ) create_share_object_response = create_share_object( + mocker=mocker, client=client, username=user.username, group=group2, @@ -1039,11 +1063,21 @@ def test_create_share_object_as_approver_and_requester(client, user, group2, env assert create_share_object_response.data.createShareObject.userRoleForShareObject == 'ApproversAndRequesters' assert create_share_object_response.data.createShareObject.requestPurpose == 'testShare' -def test_create_share_object_with_item_authorized(client, user2, group2, env2group, env2, dataset1, table1): +def test_create_share_object_with_item_authorized(mocker, client, user2, group2, env2group, env2, dataset1, table1): # Given # Existing dataset, table, target environment and group + # SharePolicy exists and is attached # When a user that belongs to environment and group creates request with table in the request + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True + ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_attached", + return_value=True + ) create_share_object_response = create_share_object( + mocker=mocker, client=client, username=user2.username, group=group2, @@ -1071,6 +1105,111 @@ def test_create_share_object_with_item_authorized(client, user2, group2, env2gro assert get_share_object_response.data.getShareObject.get('items').nodes[0].itemUri == table1.tableUri assert get_share_object_response.data.getShareObject.get('items').nodes[0].itemType == ShareableType.Table.name +def test_create_share_object_share_policy_not_attached_attachMissingPolicies_enabled(mocker, client, user2, group2, env2group, env2, dataset1): + # Given + # Existing dataset, target environment and group + # SharePolicy exists and is NOT attached, attachMissingPolicies=True + # When a correct user creates request, data.all attaches the policy and the share creates successfully + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True + ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_attached", + return_value=False + ) + attach_mocker = mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.attach_policy", + return_value=True + ) + create_share_object_response = create_share_object( + mocker=mocker, + client=client, + username=user2.username, + group=group2, + groupUri=env2group.groupUri, + environmentUri=env2.environmentUri, + datasetUri=dataset1.datasetUri, + attachMissingPolicies=True + ) + # Then share object created with status Draft and user is 'Requester' + attach_mocker.assert_called_once() + assert create_share_object_response.data.createShareObject.shareUri + assert create_share_object_response.data.createShareObject.status == ShareObjectStatus.Draft.value + assert create_share_object_response.data.createShareObject.userRoleForShareObject == 'Requesters' + assert create_share_object_response.data.createShareObject.requestPurpose == 'testShare' + +def test_create_share_object_share_policy_not_attached_attachMissingPolicies_disabled_dataallManaged(mocker, client, user2, group2, env2group, env2, dataset1): + # Given + # Existing dataset, target environment and group + # SharePolicy exists and is NOT attached, attachMissingPolicies=True but principal=Group so managed=Trye + # When a correct user creates request, data.all attaches the policy and the share creates successfully + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True + ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_attached", + return_value=False + ) + attach_mocker = mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.attach_policy", + return_value=True + ) + create_share_object_response = create_share_object( + mocker=mocker, + client=client, + username=user2.username, + group=group2, + groupUri=env2group.groupUri, + environmentUri=env2.environmentUri, + datasetUri=dataset1.datasetUri, + attachMissingPolicies=False + ) + # Then share object created with status Draft and user is 'Requester' + attach_mocker.assert_called_once() + assert create_share_object_response.data.createShareObject.shareUri + assert create_share_object_response.data.createShareObject.status == ShareObjectStatus.Draft.value + assert create_share_object_response.data.createShareObject.userRoleForShareObject == 'Requesters' + assert create_share_object_response.data.createShareObject.requestPurpose == 'testShare' + + +def test_create_share_object_share_policy_not_attached_attachMissingPolicies_disabled_dataallNotManaged(mocker, client, user2, group2, env2group, env2, dataset1): + # Given + # Existing dataset, target environment and group + # SharePolicy exists and is NOT attached, attachMissingPolicies=True + # When a correct user creates request, data.all attaches the policy and the share creates successfully + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_exists", + return_value=True + ) + mocker.patch( + "dataall.modules.dataset_sharing.services.managed_share_policy_service.SharePolicyService.check_if_policy_attached", + return_value=False + ) + consumption_role = type('consumption_role', (object,), {})() + consumption_role.IAMRoleName="randomName" + consumption_role.dataallManaged = False + mocker.patch( + "dataall.core.environment.services.environment_service.EnvironmentService.get_environment_consumption_role", + return_value=consumption_role + ) + create_share_object_response = create_share_object( + mocker=mocker, + client=client, + username=user2.username, + group=group2, + groupUri=env2group.groupUri, + environmentUri=env2.environmentUri, + datasetUri=dataset1.datasetUri, + attachMissingPolicies=False, + principalId=consumption_role.IAMRoleName, + principalType=PrincipalType.ConsumptionRole.value + ) + # Then share object is not created and an error appears + assert 'Required customer managed policy' in create_share_object_response.errors[0].message + assert 'is not attached to role randomName' in create_share_object_response.errors[0].message + def test_get_share_object(client, share1_draft, user, group): # Given