add resource permission checks (#1711)

### Feature or Bugfix Feature ### Detail * introducing a test that is going through all the nested SQL queries and assert that `ResourcePolicyService.check_user_resource_permission` have been called with the expected permission name OR explicitly ignore the test. * New subqueries will be tested automatically and fail if the expected permission is missing * Removed queries will make the test suite fail to avoid keeping stale permissions * UI: Make handling responses (i.e ListDatasets, GetDataset) more tolerant to missing information (i.e missing Stack) by doing conditional rendering. Example usecase: A dataset is being shared by a user but only owners have permissions to see stack and environment info. * Override config.json and enable all modules when running the tests. As a result checkov now synths the pipeline module that throws some errors (added in the baseline). @noah-paige * Make TestClient more tolerant to GQLErrors previously it would always throw if errors, now it will throw if there are only erros (and no data) allowing for partial information to be returned to the caller ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
data-dot-all · Dec 6, 2024 · 15eae8f · 15eae8f
1 parent 9c5714b
commit 15eae8f
Show file tree

Hide file tree

Showing 30 changed files with 1,442 additions and 168 deletions.
diff --git a/.checkov.baseline b/.checkov.baseline
@@ -613,6 +613,25 @@
                 }
             ]
         },
+        {
+            "file": "/checkov_pipeline_synth.json",
+            "findings": [
+                {
+                    "resource": "AWS::IAM::Role.PipelineRoleDCFDBB91",
+                    "check_ids": [
+                        "CKV_AWS_107",
+                        "CKV_AWS_108",
+                        "CKV_AWS_111"
+                    ]
+                },
+                {
+                    "resource": "AWS::S3::Bucket.thistableartifactsbucketDB1C8C64",
+                    "check_ids": [
+                        "CKV_AWS_18"
+                    ]
+                }
+            ]
+        },
         {
             "file": "/frontend/docker/prod/Dockerfile",
             "findings": [

diff --git a/backend/dataall/core/environment/api/resolvers.py b/backend/dataall/core/environment/api/resolvers.py
@@ -14,7 +14,6 @@
 
 from dataall.core.organizations.api.resolvers import Context, exceptions, get_organization_simplified
 
-
 log = logging.getLogger()
 
 
@@ -223,6 +222,7 @@ def generate_environment_access_token(context, source, environmentUri: str = Non
 def get_environment_stack(context: Context, source: Environment, **kwargs):
     return StackService.resolve_parent_obj_stack(
         targetUri=source.environmentUri,
+        targetType='environment',
         environmentUri=source.environmentUri,
     )
 
@@ -275,8 +275,7 @@ def resolve_environment(context, source, **kwargs):
     """Resolves the environment for a environmental resource"""
     if not source:
         return None
-    with context.engine.scoped_session() as session:
-        return EnvironmentService.get_environment_by_uri(session, source.environmentUri)
+    return EnvironmentService.find_environment_by_uri(uri=source.environmentUri)
 
 
 def resolve_parameters(context, source: Environment, **kwargs):

diff --git a/backend/dataall/core/organizations/services/organization_service.py b/backend/dataall/core/organizations/services/organization_service.py
@@ -305,7 +305,7 @@ def resolve_organization_by_env(uri):
         context = get_context()
         with context.db_engine.scoped_session() as session:
             env = EnvironmentRepository.get_environment_by_uri(session, uri)
-            return OrganizationRepository.find_organization_by_uri(session, env.organizationUri)
+            return OrganizationService.get_organization(uri=env.organizationUri)
 
     @staticmethod
     @ResourcePolicyService.has_resource_permission(GET_ORGANIZATION)

diff --git a/backend/dataall/core/stacks/services/stack_service.py b/backend/dataall/core/stacks/services/stack_service.py
@@ -65,9 +65,16 @@ def map_target_type_to_log_config_path(**kwargs):
 
 class StackService:
     @staticmethod
-    def resolve_parent_obj_stack(targetUri: str, environmentUri: str):
+    def resolve_parent_obj_stack(targetUri: str, targetType: str, environmentUri: str):
         context = get_context()
         with context.db_engine.scoped_session() as session:
+            ResourcePolicyService.check_user_resource_permission(
+                session=session,
+                username=context.username,
+                groups=context.groups,
+                resource_uri=targetUri,
+                permission_name=TargetType.get_resource_read_permission_name(targetType),
+            )
             env: Environment = EnvironmentRepository.get_environment_by_uri(session, environmentUri)
             stack: Stack = StackRepository.find_stack_by_target_uri(session, target_uri=targetUri)
             if not stack:

diff --git a/backend/dataall/core/vpc/services/vpc_service.py b/backend/dataall/core/vpc/services/vpc_service.py
@@ -1,16 +1,21 @@
+import logging
+
 from dataall.base.context import get_context
 from dataall.base.db import exceptions
+from dataall.base.db.exceptions import ResourceUnauthorized
 from dataall.core.permissions.services.group_policy_service import GroupPolicyService
 from dataall.core.environment.db.environment_repositories import EnvironmentRepository
 from dataall.core.activity.db.activity_models import Activity
 from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
 from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
 from dataall.core.vpc.db.vpc_repositories import VpcRepository
 from dataall.core.vpc.db.vpc_models import Vpc
-from dataall.core.permissions.services.network_permissions import NETWORK_ALL, DELETE_NETWORK
+from dataall.core.permissions.services.network_permissions import NETWORK_ALL, DELETE_NETWORK, GET_NETWORK
 from dataall.core.permissions.services.environment_permissions import CREATE_NETWORK
 from dataall.core.permissions.services.tenant_permissions import MANAGE_ENVIRONMENTS
 
+log = logging.getLogger(__name__)
+
 
 def _session():
     return get_context().db_engine.scoped_session()
@@ -90,4 +95,19 @@ def delete_network(uri):
     @staticmethod
     def get_environment_networks(environment_uri):
         with _session() as session:
-            return VpcRepository.get_environment_networks(session=session, environment_uri=environment_uri)
+            nets = []
+            all_nets = VpcRepository.get_environment_networks(session=session, environment_uri=environment_uri)
+            for net in all_nets:
+                try:
+                    ResourcePolicyService.check_user_resource_permission(
+                        session=session,
+                        username=get_context().username,
+                        groups=get_context().groups,
+                        resource_uri=net.vpcUri,
+                        permission_name=GET_NETWORK,
+                    )
+                except ResourceUnauthorized as exc:
+                    log.info(exc)
+                else:
+                    nets += net
+            return nets
diff --git a/backend/dataall/modules/catalog/services/glossaries_service.py b/backend/dataall/modules/catalog/services/glossaries_service.py
@@ -1,14 +1,13 @@
-from functools import wraps
 import logging
+from functools import wraps
 
 from dataall.base.context import get_context
 from dataall.base.db import exceptions
 from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
-
-from dataall.modules.catalog.db.glossary_repositories import GlossaryRepository
 from dataall.modules.catalog.db.glossary_models import GlossaryNode
-from dataall.modules.catalog.services.glossaries_permissions import MANAGE_GLOSSARIES
+from dataall.modules.catalog.db.glossary_repositories import GlossaryRepository
 from dataall.modules.catalog.indexers.registry import GlossaryRegistry
+from dataall.modules.catalog.services.glossaries_permissions import MANAGE_GLOSSARIES
 
 logger = logging.getLogger(__name__)
 
@@ -26,26 +25,29 @@ def wrapper(*args, **kwargs):
                 uri = kwargs.get('uri')
                 if not uri:
                     raise KeyError(f"{f.__name__} doesn't have parameter uri.")
-                context = get_context()
-                with context.db_engine.scoped_session() as session:
-                    node = GlossaryRepository.get_node(session=session, uri=uri)
-                    MAX_GLOSSARY_DEPTH = 10
-                    depth = 0
-                    while node.nodeType != 'G' and depth <= MAX_GLOSSARY_DEPTH:
-                        node = GlossaryRepository.get_node(session=session, uri=node.parentUri)
-                        depth += 1
-                    if node and (node.admin in context.groups):
-                        return f(*args, **kwargs)
-                    else:
-                        raise exceptions.UnauthorizedOperation(
-                            action='GLOSSARY MUTATION',
-                            message=f'User {context.username} is not the admin of the glossary {node.label}.',
-                        )
+                GlossariesResourceAccess.check_owner(uri)
+                return f(*args, **kwargs)
 
             return wrapper
 
         return decorator
 
+    @staticmethod
+    def check_owner(uri):
+        context = get_context()
+        with context.db_engine.scoped_session() as session:
+            node = GlossaryRepository.get_node(session=session, uri=uri)
+            MAX_GLOSSARY_DEPTH = 10
+            depth = 0
+            while node.nodeType != 'G' and depth <= MAX_GLOSSARY_DEPTH:
+                node = GlossaryRepository.get_node(session=session, uri=node.parentUri)
+                depth += 1
+            if not node or node.admin not in context.groups:
+                raise exceptions.UnauthorizedOperation(
+                    action='GLOSSARY MUTATION',
+                    message=f'User {context.username} is not the admin of the glossary {node.label}.',
+                )
+
 
 class GlossariesService:
     @staticmethod

diff --git a/backend/dataall/modules/dashboards/services/dashboard_quicksight_service.py b/backend/dataall/modules/dashboards/services/dashboard_quicksight_service.py
@@ -3,17 +3,16 @@
 from dataall.base.aws.parameter_store import ParameterStoreManager
 from dataall.base.aws.sts import SessionHelper
 from dataall.base.context import get_context
-from dataall.core.environment.services.environment_service import EnvironmentService
-from dataall.core.permissions.db.tenant.tenant_policy_repositories import TenantPolicyRepository
 from dataall.base.db.exceptions import UnauthorizedOperation, TenantUnauthorized, AWSResourceNotFound
-from dataall.core.permissions.services.tenant_permissions import TENANT_ALL
+from dataall.base.utils import Parameter
+from dataall.core.environment.services.environment_service import EnvironmentService
 from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
-from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
-from dataall.modules.dashboards.db.dashboard_repositories import DashboardRepository
-from dataall.modules.dashboards.db.dashboard_models import Dashboard
+from dataall.core.permissions.services.tenant_permissions import TENANT_ALL
+from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService, TenantPolicyValidationService
 from dataall.modules.dashboards.aws.dashboard_quicksight_client import DashboardQuicksightClient
+from dataall.modules.dashboards.db.dashboard_models import Dashboard
+from dataall.modules.dashboards.db.dashboard_repositories import DashboardRepository
 from dataall.modules.dashboards.services.dashboard_permissions import GET_DASHBOARD, CREATE_DASHBOARD, MANAGE_DASHBOARDS
-from dataall.base.utils import Parameter
 
 
 class DashboardQuicksightService:
@@ -128,7 +127,7 @@ def get_quicksight_reader_session(cls, dashboard_uri):
     @staticmethod
     def _check_user_must_be_admin():
         context = get_context()
-        admin = TenantPolicyRepository.is_tenant_admin(context.groups)
+        admin = TenantPolicyValidationService.is_tenant_admin(context.groups)
 
         if not admin:
             raise TenantUnauthorized(

diff --git a/backend/dataall/modules/datapipelines/api/resolvers.py b/backend/dataall/modules/datapipelines/api/resolvers.py
@@ -105,5 +105,6 @@ def resolve_stack(context, source: DataPipeline, **kwargs):
         return None
     return StackService.resolve_parent_obj_stack(
         targetUri=source.DataPipelineUri,
+        targetType='pipeline',
         environmentUri=source.environmentUri,
     )
diff --git a/backend/dataall/modules/datapipelines/services/datapipelines_service.py b/backend/dataall/modules/datapipelines/services/datapipelines_service.py
@@ -3,16 +3,16 @@
 
 from dataall.base.aws.sts import SessionHelper
 from dataall.base.context import get_context
-from dataall.core.permissions.services.group_policy_service import GroupPolicyService
+from dataall.base.db import exceptions
 from dataall.core.environment.services.environment_service import EnvironmentService
+from dataall.core.permissions.services.group_policy_service import GroupPolicyService
 from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
 from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
 from dataall.core.stacks.db.keyvaluetag_repositories import KeyValueTagRepository
 from dataall.core.stacks.db.stack_repositories import StackRepository
 from dataall.core.stacks.services.stack_service import StackService
 from dataall.core.tasks.db.task_models import Task
 from dataall.core.tasks.service_handlers import Worker
-from dataall.base.db import exceptions
 from dataall.modules.datapipelines.db.datapipelines_models import DataPipeline, DataPipelineEnvironment
 from dataall.modules.datapipelines.db.datapipelines_repositories import DatapipelinesRepository
 from dataall.modules.datapipelines.services.datapipelines_permissions import (
@@ -25,7 +25,6 @@
     UPDATE_PIPELINE,
 )
 
-
 logger = logging.getLogger(__name__)
 
 
@@ -34,6 +33,10 @@ def _session():
 
 
 class DataPipelineService:
+    @staticmethod
+    def _get_pipeline_uri_from_env_uri(session, envPipelineUri):
+        return DatapipelinesRepository.get_pipeline_environment_by_uri(session, envPipelineUri).pipelineUri
+
     @staticmethod
     @TenantPolicyService.has_tenant_permission(MANAGE_PIPELINES)
     @ResourcePolicyService.has_resource_permission(CREATE_PIPELINE)
@@ -255,6 +258,9 @@ def _delete_repository(target_uri, accountid, cdk_role_arn, region, repo_name):
 
     @staticmethod
     @TenantPolicyService.has_tenant_permission(MANAGE_PIPELINES)
+    @ResourcePolicyService.has_resource_permission(
+        UPDATE_PIPELINE, param_name='envPipelineUri', parent_resource=_get_pipeline_uri_from_env_uri
+    )
     def delete_pipeline_environment(envPipelineUri: str):
         with _session() as session:
             DatapipelinesRepository.delete_pipeline_environment(session=session, envPipelineUri=envPipelineUri)

diff --git a/backend/dataall/modules/datasets_base/api/resolvers.py b/backend/dataall/modules/datasets_base/api/resolvers.py
@@ -58,8 +58,7 @@ def get_dataset_organization(context, source: DatasetBase, **kwargs):
 def get_dataset_environment(context, source: DatasetBase, **kwargs):
     if not source:
         return None
-    with context.engine.scoped_session() as session:
-        return EnvironmentService.get_environment_by_uri(session, source.environmentUri)
+    return EnvironmentService.find_environment_by_uri(uri=source.environmentUri)
 
 
 def get_dataset_owners_group(context, source: DatasetBase, **kwargs):
@@ -79,5 +78,6 @@ def resolve_dataset_stack(context: Context, source: DatasetBase, **kwargs):
         return None
     return StackService.resolve_parent_obj_stack(
         targetUri=source.datasetUri,
+        targetType='dataset',
         environmentUri=source.environmentUri,
     )
diff --git a/backend/dataall/modules/mlstudio/api/resolvers.py b/backend/dataall/modules/mlstudio/api/resolvers.py
@@ -122,6 +122,7 @@ def resolve_sagemaker_studio_user_stack(context: Context, source: SagemakerStudi
         return None
     return StackService.resolve_parent_obj_stack(
         targetUri=source.sagemakerStudioUserUri,
+        targetType='mlstudio',
         environmentUri=source.environmentUri,
     )
 

diff --git a/backend/dataall/modules/notebooks/api/resolvers.py b/backend/dataall/modules/notebooks/api/resolvers.py
@@ -90,6 +90,7 @@ def resolve_notebook_stack(context: Context, source: SagemakerNotebook, **kwargs
         return None
     return StackService.resolve_parent_obj_stack(
         targetUri=source.notebookUri,
+        targetType='notebook',
         environmentUri=source.environmentUri,
     )
 

diff --git a/backend/dataall/modules/redshift_datasets/api/datasets/resolvers.py b/backend/dataall/modules/redshift_datasets/api/datasets/resolvers.py
@@ -87,8 +87,7 @@ def resolve_dataset_environment(
 ):  # TODO- duplicated with S3 datasets - follow-up PR
     if not source:
         return None
-    with context.engine.scoped_session() as session:
-        return EnvironmentService.get_environment_by_uri(session, source.environmentUri)
+    return EnvironmentService.find_environment_by_uri(uri=source.environmentUri)
 
 
 def resolve_dataset_owners_group(

diff --git a/backend/dataall/modules/s3_datasets/api/dataset/resolvers.py b/backend/dataall/modules/s3_datasets/api/dataset/resolvers.py
@@ -85,8 +85,7 @@ def get_dataset_organization(context, source: S3Dataset, **kwargs):
 def get_dataset_environment(context, source: S3Dataset, **kwargs):
     if not source:
         return None
-    with context.engine.scoped_session() as session:
-        return EnvironmentService.get_environment_by_uri(session, source.environmentUri)
+    return EnvironmentService.find_environment_by_uri(uri=source.environmentUri)
 
 
 def get_dataset_owners_group(context, source: S3Dataset, **kwargs):
@@ -133,6 +132,7 @@ def resolve_dataset_stack(context: Context, source: S3Dataset, **kwargs):
         return None
     return StackService.resolve_parent_obj_stack(
         targetUri=source.datasetUri,
+        targetType='dataset',
         environmentUri=source.environmentUri,
     )
 

diff --git a/backend/dataall/modules/s3_datasets/api/storage_location/resolvers.py b/backend/dataall/modules/s3_datasets/api/storage_location/resolvers.py
@@ -1,9 +1,10 @@
 from dataall.base.api.context import Context
-from dataall.modules.catalog.db.glossary_repositories import GlossaryRepository
 from dataall.base.db.exceptions import RequiredParameter
 from dataall.base.feature_toggle_checker import is_feature_enabled
+from dataall.modules.catalog.db.glossary_repositories import GlossaryRepository
+from dataall.modules.s3_datasets.db.dataset_models import DatasetStorageLocation
 from dataall.modules.s3_datasets.services.dataset_location_service import DatasetLocationService
-from dataall.modules.s3_datasets.db.dataset_models import DatasetStorageLocation, S3Dataset
+from dataall.modules.s3_datasets.services.dataset_service import DatasetService
 
 
 def _validate_input(input: dict):
@@ -46,9 +47,7 @@ def remove_storage_location(context, source, locationUri: str = None):
 def resolve_dataset(context, source: DatasetStorageLocation, **kwargs):
     if not source:
         return None
-    with context.engine.scoped_session() as session:
-        d = session.query(S3Dataset).get(source.datasetUri)
-    return d
+    return DatasetService.find_dataset(uri=source.datasetUri)
 
 
 def resolve_glossary_terms(context: Context, source: DatasetStorageLocation, **kwargs):

diff --git a/backend/dataall/modules/s3_datasets/services/dataset_service.py b/backend/dataall/modules/s3_datasets/services/dataset_service.py
@@ -38,6 +38,7 @@
     DATASET_ALL,
     DATASET_READ,
     IMPORT_DATASET,
+    GET_DATASET,
     DATASET_TABLE_ALL,
 )
 from dataall.modules.datasets_base.services.dataset_list_permissions import LIST_ENVIRONMENT_DATASETS
@@ -242,6 +243,11 @@ def get_dataset(uri):
                 dataset.userRoleForDataset = DatasetRole.Admin.value
             return dataset
 
+    @classmethod
+    @ResourcePolicyService.has_resource_permission(GET_DATASET)
+    def find_dataset(cls, uri):
+        return DatasetService.get_dataset(uri)
+
     @staticmethod
     @TenantPolicyService.has_tenant_permission(MANAGE_DATASETS)
     @ResourcePolicyService.has_resource_permission(CREDENTIALS_DATASET)

diff --git a/backend/dataall/modules/s3_datasets_shares/api/resolvers.py b/backend/dataall/modules/s3_datasets_shares/api/resolvers.py
@@ -5,7 +5,6 @@
 from dataall.base.feature_toggle_checker import is_feature_enabled
 from dataall.modules.s3_datasets_shares.services.s3_share_service import S3ShareService
 
-
 log = logging.getLogger(__name__)
 
 
@@ -41,7 +40,7 @@ def validate_dataset_share_selector_input(data):
 
 
 def list_shared_tables_by_env_dataset(context: Context, source, datasetUri: str, envUri: str):
-    return S3ShareService.list_shared_tables_by_env_dataset(datasetUri, envUri)
+    return S3ShareService.list_shared_tables_by_env_dataset(uri=envUri, dataset_uri=datasetUri)
 
 
 @is_feature_enabled('modules.s3_datasets.features.aws_actions')