Skip to content

Commit

Permalink
tenant-permission tests (#1694)
Browse files Browse the repository at this point in the history
### Feature or Bugfix
- Feature

### Detail
Add unit tests that verify that MANAGE_X permissions are applied to all
Mutations except for an OPT_OUT list of Mutations and to a subset of
OPT_IN queries.

The OPT_OUT mutations are either:
- admin actions that can only be performed by the tenants. Applying
permissions in this case does not make sense.
- platform "support" features such as feed, notification, votes. No
object needs to be protected in this case.
- v2.7.0 features which will be addressed in a separate PR

The OPT_IN queries are operations that retrieve credentials or redirect
URLs that allow the user to effectively create/update data.all objects.

### Relates

### 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.
  • Loading branch information
dlpzx authored Nov 20, 2024
1 parent 882dfca commit 5c0b208
Show file tree
Hide file tree
Showing 18 changed files with 201 additions and 31 deletions.
3 changes: 3 additions & 0 deletions backend/dataall/base/feature_toggle_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Contains decorators that check if a feature has been enabled or not
"""

import functools
from typing import List, Any, Optional, Callable

from dataall.base.config import config
Expand All @@ -12,6 +13,7 @@ def is_feature_enabled(config_property: str):
def decorator(f):
fn, fn_decorator = process_func(f)

@functools.wraps(fn)
def decorated(*args, **kwargs):
value = config.get_property(config_property)
if not value:
Expand All @@ -33,6 +35,7 @@ def is_feature_enabled_for_allowed_values(
def decorator(f):
fn, fn_decorator = process_func(f)

@functools.wraps(fn)
def decorated(*args, **kwargs):
config_property_value = None
if config_property is None and resolve_property is None:
Expand Down
11 changes: 9 additions & 2 deletions backend/dataall/core/stacks/db/target_type_repositories.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
GET_ENVIRONMENT,
UPDATE_ENVIRONMENT,
)
from dataall.core.permissions.services.tenant_permissions import MANAGE_ENVIRONMENTS

logger = logging.getLogger(__name__)

Expand All @@ -14,10 +15,11 @@ class TargetType:

_TARGET_TYPES = {}

def __init__(self, name, read_permission, write_permission):
def __init__(self, name, read_permission, write_permission, tenant_permission):
self.name = name
self.read_permission = read_permission
self.write_permission = write_permission
self.tenant_permission = tenant_permission

TargetType._TARGET_TYPES[name] = self

Expand All @@ -31,6 +33,11 @@ def get_resource_read_permission_name(target_type):
TargetType.is_supported_target_type(target_type)
return TargetType._TARGET_TYPES[target_type].read_permission

@staticmethod
def get_resource_tenant_permission_name(target_type):
TargetType.is_supported_target_type(target_type)
return TargetType._TARGET_TYPES[target_type].tenant_permission

@staticmethod
def is_supported_target_type(target_type):
if target_type not in TargetType._TARGET_TYPES:
Expand All @@ -41,4 +48,4 @@ def is_supported_target_type(target_type):
)


TargetType('environment', GET_ENVIRONMENT, UPDATE_ENVIRONMENT)
TargetType('environment', GET_ENVIRONMENT, UPDATE_ENVIRONMENT, MANAGE_ENVIRONMENTS)
25 changes: 25 additions & 0 deletions backend/dataall/core/stacks/services/stack_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from dataall.base.db import exceptions
from dataall.base.feature_toggle_checker import is_feature_enabled_for_allowed_values
from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
from dataall.core.stacks.aws.cloudformation import CloudFormation
from dataall.core.stacks.services.keyvaluetag_service import KeyValueTagService
from dataall.core.tasks.service_handlers import Worker
Expand Down Expand Up @@ -181,6 +182,13 @@ def update_stack_by_target_uri(target_uri, target_type):
StackRequestVerifier.verify_target_type_and_uri(target_uri, target_type)
context = get_context()
with context.db_engine.scoped_session() as session:
TenantPolicyService.check_user_tenant_permission(
session=session,
username=context.username,
groups=context.groups,
permission_name=TargetType.get_resource_tenant_permission_name(target_type),
tenant_name=TenantPolicyService.TENANT_NAME,
)
ResourcePolicyService.check_user_resource_permission(
session=session,
username=context.username,
Expand All @@ -196,6 +204,23 @@ def update_stack_by_target_uri(target_uri, target_type):
def update_stack_tags(input):
StackRequestVerifier.validate_update_tag_input(input)
target_uri = input.get('targetUri')
target_type = input.get('targetType')
context = get_context()
with context.db_engine.scoped_session() as session:
TenantPolicyService.check_user_tenant_permission(
session=session,
username=context.username,
groups=context.groups,
permission_name=TargetType.get_resource_tenant_permission_name(target_type),
tenant_name=TenantPolicyService.TENANT_NAME,
)
ResourcePolicyService.check_user_resource_permission(
session=session,
username=context.username,
groups=context.groups,
resource_uri=target_uri,
permission_name=TargetType.get_resource_update_permission_name(target_type),
)
kv_tags = KeyValueTagService.update_key_value_tags(
uri=target_uri,
data=input,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@
from dataall.base.db.exceptions import UnauthorizedOperation, TenantUnauthorized, AWSResourceNotFound
from dataall.core.permissions.services.tenant_permissions import TENANT_ALL
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.modules.dashboards.aws.dashboard_quicksight_client import DashboardQuicksightClient
from dataall.modules.dashboards.services.dashboard_permissions import GET_DASHBOARD, CREATE_DASHBOARD
from dataall.modules.dashboards.services.dashboard_permissions import GET_DASHBOARD, CREATE_DASHBOARD, MANAGE_DASHBOARDS
from dataall.base.utils import Parameter


Expand Down Expand Up @@ -58,6 +59,7 @@ def get_quicksight_reader_url(cls, uri):
return client.get_anonymous_session(dashboard_id=dash.DashboardId)

@classmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DASHBOARDS)
@ResourcePolicyService.has_resource_permission(CREATE_DASHBOARD)
def get_quicksight_designer_url(cls, uri: str):
context = get_context()
Expand Down
11 changes: 7 additions & 4 deletions backend/dataall/modules/datapipelines/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,17 @@ def __init__(self):
from dataall.modules.feed.api.registry import FeedRegistry, FeedDefinition
from dataall.modules.datapipelines.db.datapipelines_models import DataPipeline
from dataall.modules.datapipelines.db.datapipelines_repositories import DatapipelinesRepository
from dataall.modules.datapipelines.services.datapipelines_permissions import GET_PIPELINE, UPDATE_PIPELINE

from dataall.modules.datapipelines.services.datapipelines_permissions import (
GET_PIPELINE,
UPDATE_PIPELINE,
MANAGE_PIPELINES,
)
import dataall.modules.datapipelines.api

FeedRegistry.register(FeedDefinition('DataPipeline', DataPipeline))

TargetType('pipeline', GET_PIPELINE, UPDATE_PIPELINE)
TargetType('cdkpipeline', GET_PIPELINE, UPDATE_PIPELINE)
TargetType('pipeline', GET_PIPELINE, UPDATE_PIPELINE, MANAGE_PIPELINES)
TargetType('cdkpipeline', GET_PIPELINE, UPDATE_PIPELINE, MANAGE_PIPELINES)

EnvironmentResourceManager.register(DatapipelinesRepository())

Expand Down
8 changes: 6 additions & 2 deletions backend/dataall/modules/mlstudio/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,13 @@ def __init__(self):
from dataall.core.stacks.db.target_type_repositories import TargetType
import dataall.modules.mlstudio.api
from dataall.modules.mlstudio.services.mlstudio_service import SagemakerStudioEnvironmentResource
from dataall.modules.mlstudio.services.mlstudio_permissions import GET_SGMSTUDIO_USER, UPDATE_SGMSTUDIO_USER
from dataall.modules.mlstudio.services.mlstudio_permissions import (
GET_SGMSTUDIO_USER,
UPDATE_SGMSTUDIO_USER,
MANAGE_SGMSTUDIO_USERS,
)

TargetType('mlstudio', GET_SGMSTUDIO_USER, UPDATE_SGMSTUDIO_USER)
TargetType('mlstudio', GET_SGMSTUDIO_USER, UPDATE_SGMSTUDIO_USER, MANAGE_SGMSTUDIO_USERS)

EnvironmentResourceManager.register(SagemakerStudioEnvironmentResource())

Expand Down
8 changes: 6 additions & 2 deletions backend/dataall/modules/notebooks/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@ def is_supported(modes):
def __init__(self):
import dataall.modules.notebooks.api
from dataall.core.stacks.db.target_type_repositories import TargetType
from dataall.modules.notebooks.services.notebook_permissions import GET_NOTEBOOK, UPDATE_NOTEBOOK
from dataall.modules.notebooks.services.notebook_permissions import (
GET_NOTEBOOK,
UPDATE_NOTEBOOK,
MANAGE_NOTEBOOKS,
)

TargetType('notebook', GET_NOTEBOOK, UPDATE_NOTEBOOK)
TargetType('notebook', GET_NOTEBOOK, UPDATE_NOTEBOOK, MANAGE_NOTEBOOKS)

log.info('API of sagemaker notebooks has been imported')

Expand Down
8 changes: 6 additions & 2 deletions backend/dataall/modules/s3_datasets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ def __init__(self):
from dataall.modules.s3_datasets.indexers.table_indexer import DatasetTableIndexer

import dataall.modules.s3_datasets.api
from dataall.modules.s3_datasets.services.dataset_permissions import GET_DATASET, UPDATE_DATASET
from dataall.modules.s3_datasets.services.dataset_permissions import (
GET_DATASET,
UPDATE_DATASET,
MANAGE_DATASETS,
)
from dataall.modules.s3_datasets.db.dataset_repositories import DatasetRepository
from dataall.modules.s3_datasets.db.dataset_models import DatasetStorageLocation, DatasetTable, S3Dataset

Expand Down Expand Up @@ -73,7 +77,7 @@ def __init__(self):

add_vote_type('dataset', DatasetIndexer)

TargetType('dataset', GET_DATASET, UPDATE_DATASET)
TargetType('dataset', GET_DATASET, UPDATE_DATASET, MANAGE_DATASETS)

EnvironmentResourceManager.register(DatasetRepository())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,19 @@
log = logging.getLogger(__name__)


def _validate_uri(uri):
if not uri:
raise RequiredParameter('URI')


def resolve_dataset(context, source: DatasetProfilingRun):
if not source:
return None
return DatasetService.get_dataset(uri=source.datasetUri)


def start_profiling_run(context: Context, source, input: dict = None):
if 'datasetUri' not in input:
raise RequiredParameter('datasetUri')
_validate_uri(input.get('datasetUri'))

return DatasetProfilingService.start_profiling_run(
uri=input['datasetUri'], table_uri=input.get('tableUri'), glue_table_name=input.get('GlueTableName')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@
from dataall.modules.s3_datasets.db.dataset_models import DatasetStorageLocation, S3Dataset


@is_feature_enabled('modules.s3_datasets.features.file_actions')
def create_storage_location(context, source, datasetUri: str = None, input: dict = None):
if 'prefix' not in input:
raise RequiredParameter('prefix')
def _validate_input(input: dict):
if 'label' not in input:
raise RequiredParameter('label')
if 'prefix' not in input:
raise RequiredParameter('prefix')


@is_feature_enabled('modules.s3_datasets.features.file_actions')
def create_storage_location(context, source, datasetUri: str = None, input: dict = None):
_validate_input(input)
return DatasetLocationService.create_storage_location(uri=datasetUri, data=input)


Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
from dataall.core.tasks.service_handlers import Worker
from dataall.base.aws.sts import SessionHelper
from dataall.base.context import get_context
Expand All @@ -7,11 +8,10 @@
from dataall.modules.s3_datasets.aws.glue_table_client import GlueTableClient
from dataall.modules.s3_datasets.db.dataset_column_repositories import DatasetColumnRepository
from dataall.modules.s3_datasets.db.dataset_table_repositories import DatasetTableRepository
from dataall.modules.s3_datasets.services.dataset_permissions import UPDATE_DATASET_TABLE
from dataall.modules.s3_datasets.services.dataset_permissions import UPDATE_DATASET_TABLE, MANAGE_DATASETS
from dataall.modules.s3_datasets.db.dataset_models import DatasetTable, DatasetTableColumn
from dataall.modules.s3_datasets.db.dataset_repositories import DatasetRepository
from dataall.modules.datasets_base.services.datasets_enums import ConfidentialityClassification
from dataall.modules.s3_datasets.services.dataset_permissions import PREVIEW_DATASET_TABLE


class DatasetColumnService:
Expand Down Expand Up @@ -42,6 +42,7 @@ def paginate_active_columns_for_table(uri: str, filter=None):
return DatasetColumnRepository.paginate_active_columns_for_table(session, uri, filter)

@classmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DATASETS)
@ResourcePolicyService.has_resource_permission(
UPDATE_DATASET_TABLE, parent_resource=_get_dataset_uri, param_name='table_uri'
)
Expand All @@ -56,6 +57,7 @@ def sync_table_columns(cls, table_uri: str):
return cls.paginate_active_columns_for_table(uri=table_uri, filter={})

@staticmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DATASETS)
@ResourcePolicyService.has_resource_permission(
UPDATE_DATASET_TABLE, parent_resource=_get_dataset_uri_for_column, param_name='column_uri'
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from dataall.base.feature_toggle_checker import is_feature_enabled
from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
from dataall.core.tasks.service_handlers import Worker
from dataall.base.context import get_context
from dataall.base.db import exceptions
Expand All @@ -13,7 +14,7 @@
from dataall.modules.s3_datasets.aws.s3_profiler_client import S3ProfilerClient
from dataall.modules.s3_datasets.db.dataset_profiling_repositories import DatasetProfilingRepository
from dataall.modules.s3_datasets.db.dataset_table_repositories import DatasetTableRepository
from dataall.modules.s3_datasets.services.dataset_permissions import PROFILE_DATASET_TABLE, GET_DATASET
from dataall.modules.s3_datasets.services.dataset_permissions import PROFILE_DATASET_TABLE, GET_DATASET, MANAGE_DATASETS
from dataall.modules.s3_datasets.db.dataset_repositories import DatasetRepository
from dataall.modules.datasets_base.services.datasets_enums import ConfidentialityClassification
from dataall.modules.s3_datasets.db.dataset_models import DatasetProfilingRun, DatasetTable
Expand All @@ -22,6 +23,7 @@

class DatasetProfilingService:
@staticmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DATASETS)
@ResourcePolicyService.has_resource_permission(PROFILE_DATASET_TABLE)
@is_feature_enabled('modules.s3_datasets.features.metrics_data')
def start_profiling_run(uri, table_uri, glue_table_name):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import re
from dataall.base.context import get_context
from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
from dataall.modules.s3_datasets.db.dataset_table_data_filter_repositories import DatasetTableDataFilterRepository
from dataall.modules.s3_datasets.db.dataset_table_repositories import DatasetTableRepository
from dataall.modules.s3_datasets.db.dataset_repositories import DatasetRepository
Expand All @@ -11,6 +12,7 @@
CREATE_TABLE_DATA_FILTER,
DELETE_TABLE_DATA_FILTER,
LIST_TABLE_DATA_FILTERS,
MANAGE_DATASETS,
)
from dataall.base.db import exceptions
from dataall.modules.s3_datasets.aws.lf_data_filter_client import LakeFormationDataFilterClient
Expand Down Expand Up @@ -70,6 +72,7 @@ def _get_table_uri_from_filter(session, uri):
return data_filter.tableUri

@staticmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DATASETS)
@ResourcePolicyService.has_resource_permission(CREATE_TABLE_DATA_FILTER)
def create_table_data_filter(uri: str, data: dict):
DatasetTableDataFilterRequestValidationService.validate_creation_data_filter_params(uri, data)
Expand All @@ -93,6 +96,7 @@ def create_table_data_filter(uri: str, data: dict):
return data_filter

@staticmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DATASETS)
@ResourcePolicyService.has_resource_permission(DELETE_TABLE_DATA_FILTER, parent_resource=_get_table_uri_from_filter)
def delete_table_data_filter(uri: str):
with get_context().db_engine.scoped_session() as session:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ def list_shared_tables_by_env_dataset(dataset_uri: str, env_uri: str):
]

@staticmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DATASETS)
@ResourcePolicyService.has_resource_permission(CREDENTIALS_DATASET)
def get_dataset_shared_assume_role_url(uri):
context = get_context()
Expand Down
2 changes: 0 additions & 2 deletions backend/dataall/modules/worksheets/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ def create_worksheet(context: Context, source, input: dict = None):
raise exceptions.RequiredParameter(input)
if not input.get('SamlAdminGroupName'):
raise exceptions.RequiredParameter('groupUri')
if input.get('SamlAdminGroupName') not in context.groups:
raise exceptions.InvalidInput('groupUri', input.get('SamlAdminGroupName'), " a user's groups")
if not input.get('label'):
raise exceptions.RequiredParameter('label')

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ def _get_worksheet_by_uri(session, uri: str) -> Worksheet:
@TenantPolicyService.has_tenant_permission(MANAGE_WORKSHEETS)
def create_worksheet(data=None) -> Worksheet:
context = get_context()
if data['SamlAdminGroupName'] not in context.groups:
raise exceptions.UnauthorizedOperation(
'CREATE_WORKSHEET', f"user {context.username} does not belong to group {data['SamlAdminGroupName']}"
)
with context.db_engine.scoped_session() as session:
worksheet = Worksheet(
owner=context.username,
Expand Down Expand Up @@ -126,6 +130,7 @@ def delete_worksheet(uri) -> bool:
return True

@staticmethod
@TenantPolicyService.has_tenant_permission(MANAGE_WORKSHEETS)
@ResourcePolicyService.has_resource_permission(RUN_ATHENA_QUERY)
def run_sql_query(uri, worksheetUri, sqlQuery):
with get_context().db_engine.scoped_session() as session:
Expand Down
Loading

0 comments on commit 5c0b208

Please sign in to comment.