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

tenant-permission tests #1694

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions backend/dataall/modules/mlstudio/services/mlstudio_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
class SagemakerStudioCreationRequest:
"""A request dataclass for ml studio user profile creation. Adds default values for missed parameters"""

label: str
SamlAdminGroupName: str
label: str = None
SamlAdminGroupName: str = None
environment: Dict = field(default_factory=dict)
description: str = 'No description provided'
tags: List[str] = field(default_factory=list)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@
class NotebookCreationRequest:
"""A request dataclass for notebook creation. Adds default values for missed parameters"""

label: str
VpcId: str
SubnetId: str
SamlAdminGroupName: str
label: str = None
VpcId: str = None
SubnetId: str = None
SamlAdminGroupName: str = None
environment: Dict = field(default_factory=dict)
description: str = 'No description provided'
VolumeSizeInGB: int = 32
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
25 changes: 18 additions & 7 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,24 @@ def user3():
yield User('david')


def _create_group(db, tenant, name, user):
@pytest.fixture(scope='module', autouse=True)
def userNoTenantPermissions():
yield User('noPermissionsUser')


def _create_group(db, tenant, name, user, attach_permissions=True):
with db.scoped_session() as session:
group = Group(name=name, label=name, owner=user.username)
session.add(group)
session.commit()

TenantPolicyService.attach_group_tenant_policy(
session=session,
group=name,
permissions=TENANT_ALL,
tenant_name=tenant.name,
)
if attach_permissions:
TenantPolicyService.attach_group_tenant_policy(
session=session,
group=name,
permissions=TENANT_ALL,
tenant_name=tenant.name,
)
return group


Expand Down Expand Up @@ -133,6 +139,11 @@ def not_in_org_group(db, tenant, user):
yield _create_group(db, tenant, 'NotInOrgGroup', user)


@pytest.fixture(scope='module')
def groupNoTenantPermissions(db, tenant, userNoTenantPermissions):
yield _create_group(db, tenant, 'groupNoTenantPermissions', userNoTenantPermissions, attach_permissions=False)


@pytest.fixture(scope='module', autouse=True)
def tenant(db, permissions):
with db.scoped_session() as session:
Expand Down
155 changes: 155 additions & 0 deletions tests/test_tenant_unauthorized.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
from unittest.mock import MagicMock
import pytest
from assertpy import assert_that
from dataall.base.api import bootstrap
from dataall.base.loader import load_modules, ImportMode
from dataall.base.context import RequestContext
from dataall.base.db.exceptions import TenantUnauthorized
import inspect


load_modules(modes={ImportMode.API})

NO_CHECK_PERMS = [
'Mutation.updateGroupTenantPermissions',
'Mutation.updateSSMParameter',
'Mutation.createNetwork',
'Mutation.deleteNetwork',
Copy link
Contributor

Choose a reason for hiding this comment

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

is this to say Operations like createNetwork and deleteNetwork do not have MANAGE_XXXX checks on them? They do have MANAGE_ENVIRONMENTS ... and I think a handful of tohers on this list are simialr

Or are these mutations that would need some additional work on the backend app logic side to conform to this pattern?

Copy link
Contributor Author

@dlpzx dlpzx Nov 15, 2024

Choose a reason for hiding this comment

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

A couple of things,

  1. the list was very drafty. I have reviewed all Mutations and Queries and created a proper full list of all the operations that need to be considered.
  2. All the not commented operations have proper permissions.
    • I added missing permissions for a number of operations. I recommend you to go through the commit history
    • for the particular case of stacks I had to change a bit the registration of TargetType to add permissions checks based on stack type
    • for those resolvers that were decorated with @is_feature_enabled functools was used to pass the correct arguments using inspect
  3. All the commented ones have a comment explaining why they are commented:
    • admin actions - they do not need tests
    • operations with input objects that fail on input validation in the resolver - follow-up PR (this one is already big)
    • TO CONFIRM - vote, feed, notifications - personal actions that do not require tenant restrictions. The reason is that these actions cannot break shared resources, they are user-level objects.
    • TO DECIDE - should share objects have a MANAGE_SHARES permission? I am not strongly opinionated on either way. I would in any case implement it in a separate PR
  4. If the list of Mutations that do not require permissions is short enough we should change the logic so that it checks all mutations except a list of NON_TENANT_CHECK_MUTATIONS. Implementing an opt-out logic instead of opt-in. For Queries it will need to be opt-in no matter what

'Mutation.updateStack',
'Mutation.updateKeyValueTags',
'Mutation.createMetadataForm',
'Mutation.createMetadataFormVersion',
'Mutation.createAttachedMetadataForm',
'Mutation.deleteMetadataForm',
'Mutation.deleteMetadataFormVersion',
'Mutation.deleteAttachedMetadataForm',
'Mutation.createMetadataFormFields',
'Mutation.deleteMetadataFormField',
'Mutation.batchMetadataFormFieldUpdates',
'Mutation.startMaintenanceWindow',
'Mutation.stopMaintenanceWindow',
'Mutation.markNotificationAsRead',
'Mutation.deleteNotification',
'Mutation.postFeedMessage',
'Mutation.createGlossary',
'Mutation.updateGlossary',
'Mutation.deleteGlossary',
'Mutation.createCategory',
'Mutation.updateCategory',
'Mutation.deleteCategory',
'Mutation.createTerm',
'Mutation.updateTerm',
'Mutation.deleteTerm',
'Mutation.approveTermAssociation',
'Mutation.dismissTermAssociation',
'Mutation.startReindexCatalog',
'Mutation.createShareObject',
'Mutation.deleteShareObject',
'Mutation.cancelShareExtension',
'Mutation.addSharedItem',
'Mutation.removeSharedItem',
'Mutation.submitShareObject',
'Mutation.submitShareExtension',
'Mutation.approveShareObject',
'Mutation.approveShareExtension',
'Mutation.rejectShareObject',
'Mutation.revokeItemsShareObject',
'Mutation.verifyItemsShareObject',
'Mutation.reApplyItemsShareObject',
'Mutation.updateShareRejectReason',
'Mutation.updateShareExpirationPeriod',
'Mutation.updateShareExtensionReason',
'Mutation.updateShareRequestReason',
'Mutation.updateShareItemFilters',
'Mutation.removeShareItemFilter',
'Mutation.upVote',
]

CHECK_PERMS = [
'Mutation.createOrganization',
'Mutation.updateOrganization',
'Mutation.archiveOrganization',
'Mutation.inviteGroupToOrganization',
'Mutation.updateOrganizationGroup',
'Mutation.removeGroupFromOrganization',
'Mutation.createEnvironment',
'Mutation.updateEnvironment',
'Mutation.inviteGroupOnEnvironment',
'Mutation.addConsumptionRoleToEnvironment',
'Mutation.updateGroupEnvironmentPermissions',
'Mutation.removeGroupFromEnvironment',
'Mutation.removeConsumptionRoleFromEnvironment',
'Mutation.deleteEnvironment',
'Mutation.enableDataSubscriptions',
'Mutation.DisableDataSubscriptions',
'Mutation.updateConsumptionRole',
'Mutation.createSagemakerStudioUser',
'Mutation.deleteSagemakerStudioUser',
'Mutation.createSagemakerNotebook',
'Mutation.startSagemakerNotebook',
'Mutation.stopSagemakerNotebook',
'Mutation.deleteSagemakerNotebook',
# 'Mutation.syncDatasetTableColumns',
# 'Mutation.updateDatasetTableColumn',
# 'Mutation.startDatasetProfilingRun',
# 'Mutation.createDatasetStorageLocation',
# 'Mutation.updateDatasetStorageLocation',
# 'Mutation.deleteDatasetStorageLocation',
# 'Mutation.createDataset',
# 'Mutation.updateDataset',
# 'Mutation.generateDatasetAccessToken',
# 'Mutation.deleteDataset',
# 'Mutation.importDataset',
# 'Mutation.startGlueCrawler',
# 'Mutation.updateDatasetTable',
# 'Mutation.deleteDatasetTable',
# 'Mutation.syncTables',
# 'Mutation.createTableDataFilter',
# 'Mutation.deleteTableDataFilter',
# 'Mutation.createRedshiftConnection',
# 'Mutation.deleteRedshiftConnection',
# 'Mutation.addConnectionGroupPermission',
# 'Mutation.deleteConnectionGroupPermission',
# 'Mutation.importRedshiftDataset',
# 'Mutation.updateRedshiftDataset',
# 'Mutation.deleteRedshiftDataset',
# 'Mutation.addRedshiftDatasetTables',
# 'Mutation.deleteRedshiftDatasetTable',
# 'Mutation.updateRedshiftDatasetTable',
'Mutation.importDashboard',
'Mutation.updateDashboard',
'Mutation.deleteDashboard',
'Mutation.requestDashboardShare',
'Mutation.approveDashboardShare',
'Mutation.rejectDashboardShare',
# 'Mutation.createQuicksightDataSourceSet',
# 'Mutation.verifyDatasetShareObjects',
# 'Mutation.reApplyShareObjectItemsOnDataset',
'Mutation.createWorksheet',
'Mutation.updateWorksheet',
'Mutation.deleteWorksheet',
]

ALL_RESOLVERS = {
f'{_type.name}.{field.name}': field.resolver
for _type in bootstrap().types
for field in _type.fields
if field.resolver
}


@pytest.mark.parametrize('name,field_resolver', [(name, ALL_RESOLVERS.get(name, None)) for name in CHECK_PERMS])
def test_unauthorized_tenant_permissions(
name, field_resolver, mocker, db, userNoTenantPermissions, groupNoTenantPermissions
):
assert_that(field_resolver).is_not_none()
mock_local = MagicMock()
mock_local.context = RequestContext(
db, userNoTenantPermissions.username, [groupNoTenantPermissions.groupUri], userNoTenantPermissions
)
with mocker.patch('dataall.base.context._request_storage', mock_local):
print(inspect.signature(field_resolver).parameters.keys())
iargs = {arg: MagicMock() for arg in inspect.signature(field_resolver).parameters.keys()}
assert_that(field_resolver).raises(TenantUnauthorized).when_called_with(**iargs).contains(
'UnauthorizedOperation'
)
Loading