Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add resource permission checks #1711

Open
wants to merge 47 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
bed7d27
add resource permission checks
petrkalos Nov 21, 2024
15b19b8
force enable all modules
petrkalos Nov 21, 2024
646c7eb
test_id
petrkalos Nov 21, 2024
cf5a0be
DRY
petrkalos Nov 21, 2024
4c599ff
add more tests
petrkalos Nov 25, 2024
77a8b82
query.filter.all mock returns list
petrkalos Nov 26, 2024
1fa5c08
assert called_once only without expecting to raise as some resolvers …
petrkalos Nov 26, 2024
5dad8dc
filter out networks
petrkalos Nov 26, 2024
f11e3f5
ignore reason
petrkalos Nov 26, 2024
0f12c31
fix check order
petrkalos Nov 26, 2024
377bef8
fix find_environment_by_uri invocation
petrkalos Nov 27, 2024
32a85cc
remove get_dataset check
petrkalos Nov 27, 2024
a5ffea9
fix test client to allow errors
petrkalos Nov 27, 2024
46d134b
fix test client
petrkalos Nov 28, 2024
c2e4949
fix datasetlist
petrkalos Nov 29, 2024
49fd527
fix dataset view
petrkalos Nov 29, 2024
c22fea3
add pipeline exceptions for checkov
petrkalos Nov 29, 2024
4671a4e
fix tests
petrkalos Nov 29, 2024
5e83ea6
fix folder ui
petrkalos Nov 29, 2024
3c54f3c
use explicit tenant perms
petrkalos Nov 29, 2024
6a15647
make use of testdata
petrkalos Nov 29, 2024
ec6beb0
generalise resource/tenant tests
petrkalos Nov 29, 2024
8681d7e
add test
petrkalos Dec 2, 2024
9646835
add postinit
petrkalos Dec 2, 2024
64ac83a
rename to tenat
petrkalos Dec 2, 2024
b557d81
appsupport
petrkalos Dec 2, 2024
b9cc73b
improve IgnoreReason
petrkalos Dec 2, 2024
f60b2ba
dispatch all
petrkalos Dec 2, 2024
921b830
explicit tests
petrkalos Dec 2, 2024
6163197
mock tenant
petrkalos Dec 2, 2024
c3b221d
indirect check support using TargetType
petrkalos Dec 2, 2024
c7395b7
indirect check support using TargetType
petrkalos Dec 2, 2024
89e27a9
add checks
petrkalos Dec 2, 2024
dcb42f1
add checks
petrkalos Dec 2, 2024
eb770bc
add checks
petrkalos Dec 2, 2024
724dd95
merge tests
petrkalos Dec 2, 2024
0bd0a90
mock aws calls for speed
petrkalos Dec 2, 2024
6b316c0
add deleteDataPipelineEnvironment perm
petrkalos Dec 2, 2024
afec8eb
workaround inf loop
petrkalos Dec 2, 2024
754f51a
call get_organization
petrkalos Dec 3, 2024
e9469f9
introduce dataset_service.py::find_dataset
petrkalos Dec 3, 2024
fd355b6
split into 2 tests
petrkalos Dec 3, 2024
28006c0
add unchecked test and parametrize based on type
petrkalos Dec 3, 2024
e2e26b5
fix semgrep
petrkalos Dec 3, 2024
d88540a
simplify
petrkalos Dec 3, 2024
d9fcfa2
add tenant_admin checks
petrkalos Dec 3, 2024
e65674c
remove inf loop workaround
petrkalos Dec 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -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:
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 @@ -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
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 @@ -255,6 +255,7 @@ 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')
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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
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 @@ -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):
Expand Down Expand Up @@ -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,
)

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 @@ -38,6 +38,7 @@
DATASET_ALL,
DATASET_READ,
IMPORT_DATASET,
GET_DATASET,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used, (we could use it by find_dataset_by_uri if implemented)

DATASET_TABLE_ALL,
)
from dataall.modules.datasets_base.services.dataset_list_permissions import LIST_ENVIRONMENT_DATASETS
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 4 additions & 2 deletions frontend/src/modules/DatasetsBase/views/DatasetList.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,12 @@ const DatasetList = () => {
const fetchItems = useCallback(async () => {
setLoading(true);
const response = await client.query(listDatasets({ filter }));
if (!response.errors) {
if (response.data.listDatasets !== null) {
setItems(response.data.listDatasets);
} else {
dispatch({ type: SET_ERROR, error: response.errors[0].message });
response.errors.forEach((err) =>
dispatch({ type: SET_ERROR, error: err.message })
);
}
setLoading(false);
}, [client, dispatch, filter]);
Expand Down
8 changes: 4 additions & 4 deletions frontend/src/modules/Folders/components/FolderOverview.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ export const FolderOverview = (props) => {
</Grid>
<Grid item lg={4} xl={3} xs={12}>
<ObjectMetadata
environment={folder.dataset.environment}
region={folder.dataset.region}
organization={folder.dataset.environment.organization}
environment={folder.dataset?.environment}
region={folder.dataset?.region}
organization={folder.dataset?.environment.organization}
owner={folder.owner}
admins={folder.dataset.SamlAdminGroupName || '-'}
admins={folder.dataset?.SamlAdminGroupName || '-'}
created={folder.created}
/>
</Grid>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {

export const FolderS3Properties = (props) => {
const { folder } = props;

if (folder.dataset === null) return null;
return (
<Card {...folder}>
<CardHeader title="S3 Properties" />
Expand Down
9 changes: 5 additions & 4 deletions frontend/src/modules/Folders/views/FolderView.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,17 +233,18 @@ const FolderView = () => {
const fetchItem = useCallback(async () => {
setLoading(true);
const response = await client.query(getDatasetStorageLocation(params.uri));
if (!response.errors && response.data.getDatasetStorageLocation !== null) {
if (response.data.getDatasetStorageLocation !== null) {
setFolder(response.data.getDatasetStorageLocation);
setIsAdmin(
['Creator', 'Admin', 'Owner'].indexOf(
response.data.getDatasetStorageLocation.dataset.userRoleForDataset
response.data.getDatasetStorageLocation.dataset?.userRoleForDataset
petrkalos marked this conversation as resolved.
Show resolved Hide resolved
) !== -1
);
} else {
setFolder(null);
const error = response.errors[0].message;
dispatch({ type: SET_ERROR, error });
response.errors.forEach((err) =>
dispatch({ type: SET_ERROR, error: err.message })
);
}
setLoading(false);
}, [client, dispatch, params.uri]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const DatasetOverview = (props) => {
<ObjectMetadata
environment={dataset.environment}
region={dataset.region}
organization={dataset.environment.organization}
organization={dataset.environment?.organization}
owner={dataset.owner}
created={dataset.created}
status={dataset.stack?.status}
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/modules/S3_Datasets/views/DatasetView.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ const DatasetView = () => {
const fetchItem = useCallback(async () => {
setLoading(true);
const response = await client.query(getDataset(params.uri));
if (!response.errors && response.data.getDataset !== null) {
if (response.data.getDataset !== null) {
setDataset(response.data.getDataset);
setIsAdmin(
['BusinessOwner', 'Admin', 'DataSteward', 'Creator'].indexOf(
Expand Down
9 changes: 6 additions & 3 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
import os
from dataclasses import dataclass
from glob import glob
from unittest.mock import MagicMock

import pytest
from starlette.testclient import TestClient

from dataall.base.config import config
from dataall.base.db import get_engine, create_schema_and_tables, Engine
from dataall.base.loader import load_modules, ImportMode, list_loaded_modules
from glob import glob

from dataall.core.groups.db.group_models import Group
from dataall.core.permissions.services.permission_service import PermissionService
from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
from dataall.core.permissions.services.tenant_permissions import TENANT_ALL
from dataall.core.permissions.services.tenant_policy_service import TenantPolicyService
from tests.client import create_app, ClientWrapper

for module in config.get_property('modules'):
config.set_property(f'modules.{module}.active', True)

load_modules(modes=ImportMode.all())
ENVNAME = os.environ.get('envname', 'pytest')

Expand Down
Loading
Loading