Skip to content

Commit

Permalink
add resource permission checks (#1711)
Browse files Browse the repository at this point in the history
Feature

* 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

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.
  • Loading branch information
petrkalos authored and dlpzx committed Dec 9, 2024
1 parent 283fadc commit 6ee449b
Show file tree
Hide file tree
Showing 29 changed files with 1,426 additions and 158 deletions.
19 changes: 19 additions & 0 deletions .checkov.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down
5 changes: 2 additions & 3 deletions backend/dataall/core/environment/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

from dataall.core.organizations.api.resolvers import Context, exceptions, get_organization_simplified


log = logging.getLogger()


Expand Down Expand Up @@ -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,
)

Expand Down Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 8 additions & 1 deletion backend/dataall/core/stacks/services/stack_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,16 @@ def verify_target_type_and_uri(target_type, target_uri):

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:
Expand Down
24 changes: 22 additions & 2 deletions backend/dataall/core/vpc/services/vpc_service.py
Original file line number Diff line number Diff line change
@@ -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()
Expand Down Expand Up @@ -89,4 +94,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
40 changes: 21 additions & 19 deletions backend/dataall/modules/catalog/services/glossaries_service.py
Original file line number Diff line number Diff line change
@@ -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__)

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions backend/dataall/modules/datapipelines/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -25,7 +25,6 @@
UPDATE_PIPELINE,
)


logger = logging.getLogger(__name__)


Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions backend/dataall/modules/datasets_base/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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,
)
1 change: 1 addition & 0 deletions backend/dataall/modules/mlstudio/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand Down
1 change: 1 addition & 0 deletions backend/dataall/modules/notebooks/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand Down
Empty file.
4 changes: 2 additions & 2 deletions backend/dataall/modules/s3_datasets/api/dataset/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,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):
Expand Down Expand Up @@ -130,6 +129,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,
)

Expand Down
Original file line number Diff line number Diff line change
@@ -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):
Expand Down Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
DATASET_ALL,
DATASET_READ,
IMPORT_DATASET,
GET_DATASET,
)
from dataall.modules.datasets_base.services.dataset_list_permissions import LIST_ENVIRONMENT_DATASETS
from dataall.modules.s3_datasets.db.dataset_repositories import DatasetRepository
Expand Down Expand Up @@ -218,6 +219,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)
Expand Down
3 changes: 1 addition & 2 deletions backend/dataall/modules/s3_datasets_shares/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)


Expand Down Expand Up @@ -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')
Expand Down
4 changes: 0 additions & 4 deletions backend/dataall/modules/vote/api/resolvers.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
from typing import Dict, Type
from dataall.base.db import exceptions
from dataall.modules.vote.services.vote_service import VoteService
from dataall.modules.catalog.indexers.base_indexer import BaseIndexer

_VOTE_TYPES: Dict[str, Type[BaseIndexer]] = {}


def _required_param(param, name):
Expand Down
Loading

0 comments on commit 6ee449b

Please sign in to comment.