From 2088c40f5e7ce38a19f856d7eb2a4727654684cb Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Mon, 17 Aug 2020 13:27:43 -0700 Subject: [PATCH 01/25] Add databricks permissions command This is a public preview api --- databricks_cli/cli.py | 2 + databricks_cli/permissions/__init__.py | 0 databricks_cli/permissions/api.py | 224 ++++++++++++++++++++++ databricks_cli/permissions/cli.py | 188 ++++++++++++++++++ databricks_cli/permissions/exceptions.py | 26 +++ databricks_cli/sdk/permissions_service.py | 36 ++++ databricks_cli/sdk/preview_service.py | 11 ++ tests/__init__.py | 22 --- tests/permissions/__init__.py | 0 tests/permissions/test_cli.py | 84 ++++++++ tests/test_cli.py | 84 ++++++++ 11 files changed, 655 insertions(+), 22 deletions(-) create mode 100644 databricks_cli/permissions/__init__.py create mode 100644 databricks_cli/permissions/api.py create mode 100644 databricks_cli/permissions/cli.py create mode 100644 databricks_cli/permissions/exceptions.py create mode 100644 databricks_cli/sdk/permissions_service.py create mode 100644 databricks_cli/sdk/preview_service.py create mode 100644 tests/permissions/__init__.py create mode 100644 tests/permissions/test_cli.py create mode 100644 tests/test_cli.py diff --git a/databricks_cli/cli.py b/databricks_cli/cli.py index 9c527ffd..4d1b011b 100644 --- a/databricks_cli/cli.py +++ b/databricks_cli/cli.py @@ -25,6 +25,7 @@ from databricks_cli.configure.config import profile_option, debug_option from databricks_cli.libraries.cli import libraries_group +from databricks_cli.permissions.cli import permissions_group from databricks_cli.version import print_version_callback, version from databricks_cli.utils import CONTEXT_SETTINGS from databricks_cli.configure.cli import configure_cli @@ -63,6 +64,7 @@ def cli(): cli.add_command(groups_group, name='groups') cli.add_command(instance_pools_group, name="instance-pools") cli.add_command(pipelines_group, name='pipelines') +cli.add_command(permissions_group, name='permissions') if __name__ == "__main__": cli() \ No newline at end of file diff --git a/databricks_cli/permissions/__init__.py b/databricks_cli/permissions/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py new file mode 100644 index 00000000..6beabeaf --- /dev/null +++ b/databricks_cli/permissions/api.py @@ -0,0 +1,224 @@ +# Databricks CLI +# Copyright 2017 Databricks, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"), except +# that the use of services to which certain application programming +# interfaces (each, an "API") connect requires that the user first obtain +# a license for the use of the APIs from Databricks, Inc. ("Databricks"), +# by creating an account at www.databricks.com and agreeing to either (a) +# the Community Edition Terms of Service, (b) the Databricks Terms of +# Service, or (c) another written agreement between Licensee and Databricks +# for the use of the APIs. +# +# You may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from enum import Enum + +from databricks_cli.sdk.permissions_service import PermissionsService + +from .exceptions import PermissionsError +from ..workspace.api import WorkspaceApi + + +class PermissionTargets(Enum): + clusters = 'clusters' + cluster = clusters + directories = 'directories' + directory = directories + instance_pools = 'instance-pools' + instance_pool = instance_pools + jobs = 'jobs' + job = jobs + notebooks = 'notebooks' + notebook = notebooks + registered_models = 'registered-models' + registered_model = registered_models + model = registered_models + models = registered_models + + def to_name(self): + return self[self.value] + + @classmethod + def values(cls): + return [e.value for e in PermissionTargets] + + @classmethod + def help_values(cls): + return ', '.join([e.value for e in PermissionTargets]) + + +class PermissionLevel(Enum): + manage = 'CAN_MANAGE' + restart = 'CAN_RESTART' + attach = 'CAN_ATTACH_TO' + manage_run = 'CAN_MANAGE_RUN' + owner = 'IS_OWNER' + view = 'CAN_VIEW' + read = 'CAN_READ' + run = 'CAN_RUN' + edit = 'CAN_EDIT' + + def to_name(self): + return self[self.value] + + +class PermissionType(Enum): + user = 'user_name' + group = 'group_name' + service = 'service_principal_name' + + def to_name(self): + return self[self.value] + + +class Lookups(object): + items = { + 'CAN_MANAGE': PermissionLevel.manage, + 'CAN_RESTART': PermissionLevel.restart, + 'CAN_ATTACH_TO': PermissionLevel.attach, + 'CAN_MANAGE_RUN': PermissionLevel.manage_run, + 'IS_OWNER': PermissionLevel.owner, + 'CAN_VIEW': PermissionLevel.view, + 'CAN_READ': PermissionLevel.read, + 'CAN_RUN': PermissionLevel.run, + 'CAN_EDIT': PermissionLevel.edit, + 'user_name': PermissionType.user, + 'group_name': PermissionType.group, + 'service_principal_name': PermissionType.service, + + 'clusters': PermissionTargets.clusters, + 'cluster': PermissionTargets.cluster, + 'directories': PermissionTargets.directories, + 'directory': PermissionTargets.directory, + 'instance-pools': PermissionTargets.instance_pools, + 'instance_pools': PermissionTargets.instance_pool, + 'jobs': PermissionTargets.jobs, + 'job': PermissionTargets.job, + 'notebooks': PermissionTargets.notebooks, + 'notebook': PermissionTargets.notebook, + 'registered-models': PermissionTargets.registered_models, + 'registered_models': PermissionTargets.registered_model, + 'model': PermissionTargets.model, + 'models': PermissionTargets.models, + + } + + @classmethod + def from_name(cls, name): + return cls.items[name] + + +class Permission(object): + def __init__(self, permission_type, value, permission_level): + # type: (PermissionType, str, PermissionLevel) -> None + self.permission_type = permission_type + self.permission_level = permission_level + if value is None: + value = '' + + self.value = value + + def to_dict(self): + # type: () -> dict + if not self.permission_type or not self.permission_level: + return {} + + return { + self.permission_type.value: self.value, + 'permission_level': self.permission_level.value + } + + +class PermissionsObject(object): + def __init__(self): + self.permissions = [] + + def add(self, permission): + # type: (Permission) -> None + self.permissions.append(permission) + + def user(self, name, level): + # type: (str, PermissionLevel) -> None + self.add(Permission(PermissionType.user, value=name, permission_level=level)) + + def group(self, name, level): + # type: (str, PermissionLevel) -> None + self.add(Permission(PermissionType.group, value=name, permission_level=level)) + + def service(self, name, level): + # type: (str, PermissionLevel) -> None + self.add(Permission(PermissionType.service, value=name, permission_level=level)) + + def to_dict(self): + # type: () -> dict + if not self.permissions: + return {} + + return { + 'access_control_list': [entry.to_dict() for entry in self.permissions] + } + + +class PermissionsApi(object): + def __init__(self, api_client): + self.api_client = api_client + self.client = PermissionsService(api_client) + + def get_permissions(self, object_type, object_id): + # type: (str, str) -> dict + if not object_type: + raise PermissionsError('object_type is invalid') + if not object_id: + object_id = '' + # raise PermissionsError('object_id is invalid') + + return self.client.get_permissions(object_type=PermissionTargets[object_type].value, + object_id=object_id) + + def get_possible_permissions(self, object_type, object_id): + # type: (str, str) -> dict + if not object_type: + raise PermissionsError('object_type is invalid') + if not object_id: + raise PermissionsError('object_id is invalid') + + return self.client.get_possible_permissions( + object_type=PermissionTargets[object_type].value, + object_id=object_id) + + def add_permissions(self, object_type, object_id, permissions): + # type: (str, str, PermissionsObject) -> dict + if not object_type: + raise PermissionsError('object_type is invalid') + if not object_id: + raise PermissionsError('object_id is invalid') + + return self.client.add_permissions(object_type=PermissionTargets[object_type].value, + object_id=object_id, data=permissions.to_dict()) + + def get_id_for_directory(self, path): + # type: (str) -> list[str] + """ + Given a path, use the workspaces API to look up the object id. + :param path: path to a directory + :return: object id, [] if not found + """ + + if not path: + return [] + + objects = WorkspaceApi(self.api_client).list_directory_info(path) + if not objects: + return [] + + # ls -d already filtered for us + return [workspace_object.object_id for workspace_object in objects] diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py new file mode 100644 index 00000000..c49e2009 --- /dev/null +++ b/databricks_cli/permissions/cli.py @@ -0,0 +1,188 @@ +# Databricks CLI +# Copyright 2017 Databricks, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"), except +# that the use of services to which certain application programming +# interfaces (each, an "API") connect requires that the user first obtain +# a license for the use of the APIs from Databricks, Inc. ("Databricks"), +# by creating an account at www.databricks.com and agreeing to either (a) +# the Community Edition Terms of Service, (b) the Databricks Terms of +# Service, or (c) another written agreement between Licensee and Databricks +# for the use of the APIs. +# +# You may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import click + +from databricks_cli.click_types import OneOfOption +from databricks_cli.configure.config import provide_api_client, profile_option, debug_option +from databricks_cli.permissions.api import PermissionsApi, PermissionTargets, PermissionLevel, \ + PermissionType, Permission, PermissionsObject, Lookups +from databricks_cli.utils import eat_exceptions, CONTEXT_SETTINGS +from databricks_cli.utils import pretty_format +from databricks_cli.version import print_version_callback, version + +FILTERS_HELP = 'Filters for filtering the list of users: ' + \ + 'https://docs.databricks.com/api/latest/scim.html#filters' +USER_OPTIONS = ['user-id', 'user-name'] +JSON_FILE_OPTIONS = ['json-file', 'json'] +CREATE_USER_OPTIONS = JSON_FILE_OPTIONS + ['user-name'] + +PERMISSIONS_OPTION = ['group-name', 'user-name', 'service-name'] + + +@click.command(context_settings=CONTEXT_SETTINGS, + short_help='Get permissions for an item. ' + + 'Possible object types are: \n\t{}\n' + .format(PermissionTargets.help_values()) + ) +@click.argument('object-type') +@click.argument('object-id', required=False) +@debug_option +@profile_option +@eat_exceptions +@provide_api_client +def get_cli(api_client, object_type, object_id): + perms_api = PermissionsApi(api_client) + if not object_type: + click.echo('Possible object types are: {}'.format(PermissionTargets)) + click.echo(pretty_format(perms_api.get_permissions(object_type, object_id))) + + +@click.command(context_settings=CONTEXT_SETTINGS, + short_help='List permission types') +@click.argument('object_type') +@click.argument('object_id') +@debug_option +@profile_option +@eat_exceptions +@provide_api_client +def list_permissions_types_cli(api_client, object_type, object_id): + perms_api = PermissionsApi(api_client) + click.echo(pretty_format(perms_api.get_possible_permissions(object_type, object_id))) + + +@click.command(context_settings=CONTEXT_SETTINGS, + short_help='Add or modify permission types') +@click.argument('object_type') +@click.argument('object_id') +@click.option('--group-name', metavar='', cls=OneOfOption, default=None, + one_of=PERMISSIONS_OPTION) +@click.option('--user-name', metavar='', cls=OneOfOption, default=None, + one_of=PERMISSIONS_OPTION) +@click.option('--service-name', metavar='', cls=OneOfOption, default=None, + one_of=PERMISSIONS_OPTION) +@click.option('--permission-level', metavar='', default=None, + required=True) +@debug_option +@profile_option +@eat_exceptions +@provide_api_client +# pylint:disable=unused-argument +def add_cli(api_client, object_type, object_id, user_name, group_name, service_name, + permission_level): + perms_api = PermissionsApi(api_client) + + if not user_name and not group_name and not service_name: + click.echo('Need --user-name, --service-name or --group-name') + return + + if not permission_level: + click.echo('Need permission-level: {}'.format([e.value for e in PermissionLevel])) + + # build the json for the use + perm_type = None + value = None + if user_name: + perm_type = PermissionType.user + value = user_name + elif group_name: + perm_type = PermissionType.group + value = group_name + elif service_name: + perm_type = PermissionType.service + value = service_name + else: + click.echo( + 'Invalid permission-level must be one of {}'.format([e.value for e in PermissionLevel])) + return + + permission = Permission(perm_type, value, Lookups.from_name(permission_level.upper())) + all_permissions = PermissionsObject() + all_permissions.add(permission) + + if not all_permissions.to_dict(): + click.echo('invalid permissions') + return + + click.echo(pretty_format(perms_api.add_permissions(object_type, object_id, all_permissions))) + + +@click.command(context_settings=CONTEXT_SETTINGS) +@debug_option +@profile_option +@eat_exceptions +def list_permissions_targets_cli(): + click.echo('Possible object types are: {}'.format([e.value for e in PermissionTargets])) + + +@click.command(context_settings=CONTEXT_SETTINGS) +@debug_option +@profile_option +@eat_exceptions +def list_permissions_level_cli(): + click.echo('Possible permission levels are: {}'.format([e.value for e in PermissionLevel])) + + +@click.command(context_settings=CONTEXT_SETTINGS, + short_help='Get permissions for a directory') +@click.argument('path') +@debug_option +@profile_option +@eat_exceptions +@provide_api_client +def directory_cli(api_client, path): + perms_api = PermissionsApi(api_client) + object_type = 'directories' + object_ids = perms_api.get_id_for_directory(path) + + if not object_ids: + click.echo('Failed to find id for {}'.format(path)) + return + + # if len(object_ids) > 1: + # click.echo('Too many objects ({}) returned for {}'.format(len(object_ids), path)) + + for object_id in object_ids: + click.echo(pretty_format(perms_api.get_permissions(object_type, object_id))) + + +@click.group(context_settings=CONTEXT_SETTINGS, + short_help='xUtility to interact with Databricks permissions api.\n' + + 'Possible object types are: {}\n'.format(PermissionTargets)) +@click.option('--version', '-v', is_flag=True, callback=print_version_callback, + expose_value=False, is_eager=True, help=version) +@debug_option +@profile_option +@eat_exceptions +def permissions_group(): + """Provide utility to interact with Databricks permissions api.""" + pass + + +permissions_group.add_command(list_permissions_types_cli, name='list-types') +permissions_group.add_command(get_cli, name='get') +permissions_group.add_command(add_cli, name='add') +permissions_group.add_command(list_permissions_targets_cli, name='targets') +permissions_group.add_command(list_permissions_level_cli, name='levels') + +permissions_group.add_command(directory_cli, name='ls') diff --git a/databricks_cli/permissions/exceptions.py b/databricks_cli/permissions/exceptions.py new file mode 100644 index 00000000..c986acb6 --- /dev/null +++ b/databricks_cli/permissions/exceptions.py @@ -0,0 +1,26 @@ +# Databricks CLI +# Copyright 2018 Databricks, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"), except +# that the use of services to which certain application programming +# interfaces (each, an "API") connect requires that the user first obtain +# a license for the use of the APIs from Databricks, Inc. ("Databricks"), +# by creating an account at www.databricks.com and agreeing to either (a) +# the Community Edition Terms of Service, (b) the Databricks Terms of +# Service, or (c) another written agreement between Licensee and Databricks +# for the use of the APIs. +# +# You may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +class PermissionsError(Exception): + pass diff --git a/databricks_cli/sdk/permissions_service.py b/databricks_cli/sdk/permissions_service.py new file mode 100644 index 00000000..c9cbf0d9 --- /dev/null +++ b/databricks_cli/sdk/permissions_service.py @@ -0,0 +1,36 @@ +from typing import Optional + +from databricks_cli.sdk.preview_service import PreviewService + + +class PermissionsService(PreviewService): + def __init__(self, client): + super(PermissionsService, self).__init__('permissions') + self.client = client + + def create_url(self, object_type, object_id, suffix=''): + # type: (str, str, str) -> str + return '{}/{}/{}{}'.format(self.url_base, object_type, object_id, suffix) + + def get_permissions(self, object_type, object_id, headers=None): + # type: (str, str, Optional[dict]) -> dict + return self.client.perform_query('GET', self.create_url(object_type, object_id), + headers=headers) + + def get_possible_permissions(self, object_type, object_id, headers=None): + # type: (str, str, Optional[dict]) -> dict + return self.client.perform_query('GET', self.create_url(object_type, object_id, + '/permissionLevels'), + headers=headers) + + def add_permissions(self, object_type, object_id, data, headers=None): + # type: (str, str, dict, Optional[dict]) -> dict + return self.client.perform_query('PATCH', self.create_url(object_type, object_id), + data=data, + headers=headers) + + def update_permissions(self, object_type, object_id, data, headers=None): + # type: (str, str, dict, Optional[dict]) -> dict + return self.client.perform_query('PUT', self.create_url(object_type, object_id), + data=data, + headers=headers) diff --git a/databricks_cli/sdk/preview_service.py b/databricks_cli/sdk/preview_service.py new file mode 100644 index 00000000..8ec41849 --- /dev/null +++ b/databricks_cli/sdk/preview_service.py @@ -0,0 +1,11 @@ +class PreviewService(object): + """ + This class makes it easier to create preview endpoints. + """ + PREVIEW_BASE = '/preview/' + + def __init__(self, base_url): + self.url_base = self.create_preview_url(base_url) + + def create_preview_url(self, base_url): + return self.PREVIEW_BASE + base_url diff --git a/tests/__init__.py b/tests/__init__.py index b0c9feac..e69de29b 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,22 +0,0 @@ -# Databricks CLI -# Copyright 2017 Databricks, Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"), except -# that the use of services to which certain application programming -# interfaces (each, an "API") connect requires that the user first obtain -# a license for the use of the APIs from Databricks, Inc. ("Databricks"), -# by creating an account at www.databricks.com and agreeing to either (a) -# the Community Edition Terms of Service, (b) the Databricks Terms of -# Service, or (c) another written agreement between Licensee and Databricks -# for the use of the APIs. -# -# You may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. diff --git a/tests/permissions/__init__.py b/tests/permissions/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py new file mode 100644 index 00000000..139f6085 --- /dev/null +++ b/tests/permissions/test_cli.py @@ -0,0 +1,84 @@ +# Databricks CLI +# Copyright 2017 Databricks, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"), except +# that the use of services to which certain application programming +# interfaces (each, an "API") connect requires that the user first obtain +# a license for the use of the APIs from Databricks, Inc. ("Databricks"), +# by creating an account at www.databricks.com and agreeing to either (a) +# the Community Edition Terms of Service, (b) the Databricks Terms of +# Service, or (c) another written agreement between Licensee and Databricks +# for the use of the APIs. +# +# You may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# pylint:disable=redefined-outer-name + +import re +import mock +import pytest +from click.testing import CliRunner +from tests.utils import provide_conf + +import databricks_cli.permissions.cli as cli +from databricks_cli.utils import pretty_format + + +def strip_margin(text): + # type: (str) -> str + return re.sub('\n[ \t]*\\|', '\n', text) + + +PERMISSIONS_RETURNS = { + 'get': { + 'clusters': { + '1234-567890-kens4': { + 'object_id': '/clusters/1234-567890-kens4', + 'object_type': 'cluster', + 'access_control_list': [ + { + 'group_name': 'admins', + 'all_permissions': [ + { + 'permission_level': 'CAN_MANAGE', + 'inherited': True, + 'inherited_from_object': [ + '/clusters/' + ] + } + ] + } + ] + } + } + } +} + + +@pytest.fixture() +def perms_api_mock(): + with mock.patch('databricks_cli.permissions.cli.PermissionsApi') as PermissionsApiMock: + _perms_api_mock = mock.MagicMock() + PermissionsApiMock.return_value = _perms_api_mock + yield _perms_api_mock + + +@provide_conf +def test_get_cli(perms_api_mock): + with mock.patch('databricks_cli.permissions.cli.click.echo') as echo_mock: + return_value = PERMISSIONS_RETURNS['get']['clusters'] + perms_api_mock.get_permissions.return_value = return_value + runner = CliRunner() + runner.invoke(cli.get_cli, ['clusters', '1234-567890-kens4']) + assert perms_api_mock.get_permissions.call_args[0][0] == 'clusters' + assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' + assert echo_mock.call_args[0][0] == pretty_format(return_value) diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 00000000..139f6085 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,84 @@ +# Databricks CLI +# Copyright 2017 Databricks, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"), except +# that the use of services to which certain application programming +# interfaces (each, an "API") connect requires that the user first obtain +# a license for the use of the APIs from Databricks, Inc. ("Databricks"), +# by creating an account at www.databricks.com and agreeing to either (a) +# the Community Edition Terms of Service, (b) the Databricks Terms of +# Service, or (c) another written agreement between Licensee and Databricks +# for the use of the APIs. +# +# You may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# pylint:disable=redefined-outer-name + +import re +import mock +import pytest +from click.testing import CliRunner +from tests.utils import provide_conf + +import databricks_cli.permissions.cli as cli +from databricks_cli.utils import pretty_format + + +def strip_margin(text): + # type: (str) -> str + return re.sub('\n[ \t]*\\|', '\n', text) + + +PERMISSIONS_RETURNS = { + 'get': { + 'clusters': { + '1234-567890-kens4': { + 'object_id': '/clusters/1234-567890-kens4', + 'object_type': 'cluster', + 'access_control_list': [ + { + 'group_name': 'admins', + 'all_permissions': [ + { + 'permission_level': 'CAN_MANAGE', + 'inherited': True, + 'inherited_from_object': [ + '/clusters/' + ] + } + ] + } + ] + } + } + } +} + + +@pytest.fixture() +def perms_api_mock(): + with mock.patch('databricks_cli.permissions.cli.PermissionsApi') as PermissionsApiMock: + _perms_api_mock = mock.MagicMock() + PermissionsApiMock.return_value = _perms_api_mock + yield _perms_api_mock + + +@provide_conf +def test_get_cli(perms_api_mock): + with mock.patch('databricks_cli.permissions.cli.click.echo') as echo_mock: + return_value = PERMISSIONS_RETURNS['get']['clusters'] + perms_api_mock.get_permissions.return_value = return_value + runner = CliRunner() + runner.invoke(cli.get_cli, ['clusters', '1234-567890-kens4']) + assert perms_api_mock.get_permissions.call_args[0][0] == 'clusters' + assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' + assert echo_mock.call_args[0][0] == pretty_format(return_value) From 407abe7e7772e0a249f8284b4834900e661bc208 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Mon, 17 Aug 2020 13:32:21 -0700 Subject: [PATCH 02/25] Remove extra file --- tests/test_cli.py | 84 ----------------------------------------------- 1 file changed, 84 deletions(-) delete mode 100644 tests/test_cli.py diff --git a/tests/test_cli.py b/tests/test_cli.py deleted file mode 100644 index 139f6085..00000000 --- a/tests/test_cli.py +++ /dev/null @@ -1,84 +0,0 @@ -# Databricks CLI -# Copyright 2017 Databricks, Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"), except -# that the use of services to which certain application programming -# interfaces (each, an "API") connect requires that the user first obtain -# a license for the use of the APIs from Databricks, Inc. ("Databricks"), -# by creating an account at www.databricks.com and agreeing to either (a) -# the Community Edition Terms of Service, (b) the Databricks Terms of -# Service, or (c) another written agreement between Licensee and Databricks -# for the use of the APIs. -# -# You may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -# pylint:disable=redefined-outer-name - -import re -import mock -import pytest -from click.testing import CliRunner -from tests.utils import provide_conf - -import databricks_cli.permissions.cli as cli -from databricks_cli.utils import pretty_format - - -def strip_margin(text): - # type: (str) -> str - return re.sub('\n[ \t]*\\|', '\n', text) - - -PERMISSIONS_RETURNS = { - 'get': { - 'clusters': { - '1234-567890-kens4': { - 'object_id': '/clusters/1234-567890-kens4', - 'object_type': 'cluster', - 'access_control_list': [ - { - 'group_name': 'admins', - 'all_permissions': [ - { - 'permission_level': 'CAN_MANAGE', - 'inherited': True, - 'inherited_from_object': [ - '/clusters/' - ] - } - ] - } - ] - } - } - } -} - - -@pytest.fixture() -def perms_api_mock(): - with mock.patch('databricks_cli.permissions.cli.PermissionsApi') as PermissionsApiMock: - _perms_api_mock = mock.MagicMock() - PermissionsApiMock.return_value = _perms_api_mock - yield _perms_api_mock - - -@provide_conf -def test_get_cli(perms_api_mock): - with mock.patch('databricks_cli.permissions.cli.click.echo') as echo_mock: - return_value = PERMISSIONS_RETURNS['get']['clusters'] - perms_api_mock.get_permissions.return_value = return_value - runner = CliRunner() - runner.invoke(cli.get_cli, ['clusters', '1234-567890-kens4']) - assert perms_api_mock.get_permissions.call_args[0][0] == 'clusters' - assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' - assert echo_mock.call_args[0][0] == pretty_format(return_value) From 631f20b1bcf7553524480cf63d4281c163175177 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Mon, 17 Aug 2020 13:33:50 -0700 Subject: [PATCH 03/25] Fix acidental removal of tests/__init__.py --- tests/__init__.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/__init__.py b/tests/__init__.py index e69de29b..b0c9feac 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -0,0 +1,22 @@ +# Databricks CLI +# Copyright 2017 Databricks, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"), except +# that the use of services to which certain application programming +# interfaces (each, an "API") connect requires that the user first obtain +# a license for the use of the APIs from Databricks, Inc. ("Databricks"), +# by creating an account at www.databricks.com and agreeing to either (a) +# the Community Edition Terms of Service, (b) the Databricks Terms of +# Service, or (c) another written agreement between Licensee and Databricks +# for the use of the APIs. +# +# You may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. From c5075e1c3fafabbdc6ca3922ec96bf5fc670384d Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Mon, 17 Aug 2020 13:34:24 -0700 Subject: [PATCH 04/25] tests/permissions/__init__.py was missing the license --- tests/permissions/__init__.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/permissions/__init__.py b/tests/permissions/__init__.py index e69de29b..b0c9feac 100644 --- a/tests/permissions/__init__.py +++ b/tests/permissions/__init__.py @@ -0,0 +1,22 @@ +# Databricks CLI +# Copyright 2017 Databricks, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"), except +# that the use of services to which certain application programming +# interfaces (each, an "API") connect requires that the user first obtain +# a license for the use of the APIs from Databricks, Inc. ("Databricks"), +# by creating an account at www.databricks.com and agreeing to either (a) +# the Community Edition Terms of Service, (b) the Databricks Terms of +# Service, or (c) another written agreement between Licensee and Databricks +# for the use of the APIs. +# +# You may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. From 6f08afd27a371fee4091e154106b787d7a959a2a Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Fri, 18 Sep 2020 09:37:42 -0700 Subject: [PATCH 05/25] WIP: Add missing bits in the workspace API Move get_id_for_directory from PermissionsApi to WorkspaceApi --- databricks_cli/permissions/api.py | 19 +---------- databricks_cli/workspace/api.py | 57 ++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index 6beabeaf..0146dad4 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -168,6 +168,7 @@ def to_dict(self): } +# FIXME: add set/update permissions, right now this is read only. class PermissionsApi(object): def __init__(self, api_client): self.api_client = api_client @@ -204,21 +205,3 @@ def add_permissions(self, object_type, object_id, permissions): return self.client.add_permissions(object_type=PermissionTargets[object_type].value, object_id=object_id, data=permissions.to_dict()) - - def get_id_for_directory(self, path): - # type: (str) -> list[str] - """ - Given a path, use the workspaces API to look up the object id. - :param path: path to a directory - :return: object id, [] if not found - """ - - if not path: - return [] - - objects = WorkspaceApi(self.api_client).list_directory_info(path) - if not objects: - return [] - - # ls -d already filtered for us - return [workspace_object.object_id for workspace_object in objects] diff --git a/databricks_cli/workspace/api.py b/databricks_cli/workspace/api.py index a1813459..54ef2914 100644 --- a/databricks_cli/workspace/api.py +++ b/databricks_cli/workspace/api.py @@ -23,6 +23,7 @@ import os from base64 import b64encode, b64decode +from os.path import dirname import click from requests.exceptions import HTTPError @@ -37,12 +38,13 @@ class WorkspaceFileInfo(object): - def __init__(self, path, object_type, language=None, **kwargs): # noqa + def __init__(self, path, object_type, object_id=None, language=None, **kwargs): # noqa self.path = path self.object_type = object_type self.language = language + self.object_id = object_id - def to_row(self, is_long_form, is_absolute): + def to_row(self, is_long_form, is_absolute, with_object_id=False): path = self.path if is_absolute else self.basename if self.is_dir: stylized_path = click.style(path, 'cyan') @@ -50,10 +52,24 @@ def to_row(self, is_long_form, is_absolute): stylized_path = click.style(path, 'green') else: stylized_path = path + + result = [stylized_path] + if is_long_form: - return [self.object_type, stylized_path, self.language] - else: - return [stylized_path] + result = [self.object_type, stylized_path, self.language] + + if with_object_id: + result.append(self.object_id) + + return result + + def to_json(self): + return { + 'path': self.path, + 'object_type': self.object_type, + 'language': self.language, + 'object_id': self.object_id, + } @property def is_dir(self): @@ -179,3 +195,34 @@ def export_workspace_dir(self, source_path, target_path, overwrite, headers=None click.echo('{} already exists locally as {}. Skip.'.format(cur_src, cur_dst)) else: click.echo('{} is neither a dir or a notebook. Skip.'.format(cur_src)) + + def list_directory_info(self, workspace_path, headers=None): + # first, we need to trim the workspace_path + # then we need the parent + workspace_path = workspace_path.rstrip('/') + parent_path = dirname(workspace_path) + if len(parent_path) == 0: + parent_path = '/' + + last_entry = workspace_path.split('/')[-1] + + return [object_value for object_value in self.list_objects(parent_path, headers) if + last_entry in object_value.path] + + def get_id_for_directory(self, path): + # type: (str) -> list[str] + """ + Given a path, use the workspaces API to look up the object id. + :param path: path to a directory + :return: object id, [] if not found + """ + + if not path: + return [] + + objects = self.list_directory_info(path) + if not objects: + return [] + + # ls -d already filtered for us + return [workspace_object.object_id for workspace_object in objects] From 4e17f06f66c30d0a17e64753932193bcd4c6c944 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Fri, 18 Sep 2020 09:41:22 -0700 Subject: [PATCH 06/25] Fix an issue where the if statements were checking type but raising an error that said level. :( --- databricks_cli/permissions/cli.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index c49e2009..f44554a4 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -99,9 +99,7 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n if not permission_level: click.echo('Need permission-level: {}'.format([e.value for e in PermissionLevel])) - # build the json for the use - perm_type = None - value = None + # Determine the type of permissions we're adding. if user_name: perm_type = PermissionType.user value = user_name @@ -113,7 +111,7 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n value = service_name else: click.echo( - 'Invalid permission-level must be one of {}'.format([e.value for e in PermissionLevel])) + 'Invalid permission-type must be one of {}'.format([e.value for e in PermissionType])) return permission = Permission(perm_type, value, Lookups.from_name(permission_level.upper())) From 123a8054553fc0c4fcea80ff1b1eabc1001c63c3 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Fri, 18 Sep 2020 09:43:27 -0700 Subject: [PATCH 07/25] Remove WorkspaceApi import --- databricks_cli/permissions/api.py | 1 - databricks_cli/permissions/cli.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index 0146dad4..8c08e9a2 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -25,7 +25,6 @@ from databricks_cli.sdk.permissions_service import PermissionsService from .exceptions import PermissionsError -from ..workspace.api import WorkspaceApi class PermissionTargets(Enum): diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index f44554a4..1f75ab58 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -99,7 +99,7 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n if not permission_level: click.echo('Need permission-level: {}'.format([e.value for e in PermissionLevel])) - # Determine the type of permissions we're adding. + # Determine the type of permissions we're adding. if user_name: perm_type = PermissionType.user value = user_name From c00458b0c0da6182f2f5ed1d00d93d8e854be81d Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Fri, 18 Sep 2020 09:50:42 -0700 Subject: [PATCH 08/25] Fix a typo in WorkspaceFileInfo. We cannot change the order of arguments, as that breaks existing code --- databricks_cli/workspace/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/databricks_cli/workspace/api.py b/databricks_cli/workspace/api.py index 54ef2914..b78b5f44 100644 --- a/databricks_cli/workspace/api.py +++ b/databricks_cli/workspace/api.py @@ -38,7 +38,7 @@ class WorkspaceFileInfo(object): - def __init__(self, path, object_type, object_id=None, language=None, **kwargs): # noqa + def __init__(self, path, object_type, language=None, object_id=None, **kwargs): # noqa self.path = path self.object_type = object_type self.language = language From 4b7302cb08ed2179874c06aab70b15fc1d5150cb Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Fri, 18 Sep 2020 10:03:31 -0700 Subject: [PATCH 09/25] get_id_for_directory was moved from the permissions api to the workspace api --- databricks_cli/permissions/cli.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index 1f75ab58..ed4d2a75 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -30,6 +30,7 @@ from databricks_cli.utils import eat_exceptions, CONTEXT_SETTINGS from databricks_cli.utils import pretty_format from databricks_cli.version import print_version_callback, version +from databricks_cli.workspace.api import WorkspaceApi FILTERS_HELP = 'Filters for filtering the list of users: ' + \ 'https://docs.databricks.com/api/latest/scim.html#filters' @@ -150,8 +151,10 @@ def list_permissions_level_cli(): @provide_api_client def directory_cli(api_client, path): perms_api = PermissionsApi(api_client) + workspace_api = WorkspaceApi(api_client) + object_type = 'directories' - object_ids = perms_api.get_id_for_directory(path) + object_ids = workspace_api.get_id_for_directory(path) if not object_ids: click.echo('Failed to find id for {}'.format(path)) From e10f6f6b775abb1df3b14456d2889a7ea36b0e7a Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Tue, 22 Sep 2020 09:47:59 -0700 Subject: [PATCH 10/25] Fix lint errors about un-grouped imports --- databricks_cli/permissions/api.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index 8c08e9a2..638989ae 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -23,7 +23,6 @@ from enum import Enum from databricks_cli.sdk.permissions_service import PermissionsService - from .exceptions import PermissionsError @@ -204,3 +203,13 @@ def add_permissions(self, object_type, object_id, permissions): return self.client.add_permissions(object_type=PermissionTargets[object_type].value, object_id=object_id, data=permissions.to_dict()) + + def update_permissions(self, object_type, object_id, permissions): + # type: (str, str, PermissionsObject) -> dict + if not object_type: + raise PermissionsError('object_type is invalid') + if not object_id: + raise PermissionsError('object_id is invalid') + + return self.client.update_permissions(object_type=PermissionTargets[object_type].value, + object_id=object_id, data=permissions.to_dict()) From e8d55c054cf9805ea9d8bd6a4e84652fc066bb9c Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Tue, 22 Sep 2020 10:08:32 -0700 Subject: [PATCH 11/25] Add some documentation showing all of the various urls that the permissions api calls to make life easier --- databricks_cli/sdk/permissions_service.py | 52 +++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/databricks_cli/sdk/permissions_service.py b/databricks_cli/sdk/permissions_service.py index c9cbf0d9..862d93a0 100644 --- a/databricks_cli/sdk/permissions_service.py +++ b/databricks_cli/sdk/permissions_service.py @@ -13,17 +13,54 @@ def create_url(self, object_type, object_id, suffix=''): return '{}/{}/{}{}'.format(self.url_base, object_type, object_id, suffix) def get_permissions(self, object_type, object_id, headers=None): + """ + Get the permissions for an object type and id + + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-tokens-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-passwords-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-cluster-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-instance-pool-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-job-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-notebook-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-directory-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-registered-model-permissions + """ # type: (str, str, Optional[dict]) -> dict return self.client.perform_query('GET', self.create_url(object_type, object_id), headers=headers) def get_possible_permissions(self, object_type, object_id, headers=None): + """ + Get the permission levels for an object type. + + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-tokens-permission-levels + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-password-permission-levels + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-clusters-permission-levels + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-instance-pools-permission-levels + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-jobs-permission-levels + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-notebooks-permission-levels + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-directories-permission-levels + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-registered-models-permission-levels + """ # type: (str, str, Optional[dict]) -> dict return self.client.perform_query('GET', self.create_url(object_type, object_id, '/permissionLevels'), headers=headers) def add_permissions(self, object_type, object_id, data, headers=None): + """ + Add permissions, this does not REMOVE. + A remove requires an update_permissions call to complete replacement. + + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-tokens-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-password-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-cluster-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-instance-pool-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-job-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-notebook-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-directory-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-registered-model-permissions + """ # type: (str, str, dict, Optional[dict]) -> dict return self.client.perform_query('PATCH', self.create_url(object_type, object_id), data=data, @@ -31,6 +68,21 @@ def add_permissions(self, object_type, object_id, data, headers=None): def update_permissions(self, object_type, object_id, data, headers=None): # type: (str, str, dict, Optional[dict]) -> dict + """ + Update/Overwrite all permissions + This overwrites all of the permissions for an object. + This is how you remove permissions, you call update with a complete set of permissions. + + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-tokens-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-all-password-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-all-cluster-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-all-instance-pool-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-all-job-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-all-notebook-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-all-directory-permissions + https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-registered-model-permissions + """ + return self.client.perform_query('PUT', self.create_url(object_type, object_id), data=data, headers=headers) From 849944cc729d5926b1aaf73470ff97326c832dfa Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Tue, 22 Sep 2020 10:35:47 -0700 Subject: [PATCH 12/25] databricks permissions: Move from argument to option so help can be provided. Share the same help text across all of the databricks permissions functions --- databricks_cli/permissions/api.py | 16 ++++++++++++++++ databricks_cli/permissions/cli.py | 32 +++++++++++++++++++------------ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index 638989ae..5cc701c0 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -68,6 +68,14 @@ class PermissionLevel(Enum): def to_name(self): return self[self.value] + @classmethod + def values(cls): + return [e.value for e in PermissionLevel] + + @classmethod + def help_values(cls): + return ', '.join([e.value for e in PermissionLevel]) + class PermissionType(Enum): user = 'user_name' @@ -77,6 +85,14 @@ class PermissionType(Enum): def to_name(self): return self[self.value] + @classmethod + def values(cls): + return [e.value for e in PermissionType] + + @classmethod + def help_values(cls): + return ', '.join([e.value for e in PermissionType]) + class Lookups(object): items = { diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index ed4d2a75..d04a445c 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -40,14 +40,17 @@ PERMISSIONS_OPTION = ['group-name', 'user-name', 'service-name'] +POSSIBLE_OBJECT_TYPES = 'Possible object types are: \n\t{}\n'.format( + PermissionTargets.help_values()) + +POSSIBLE_PERMISSION_LEVELS = 'Possible permission levels are: \n\t{}\n'.format( + PermissionLevel.help_values()) + @click.command(context_settings=CONTEXT_SETTINGS, - short_help='Get permissions for an item. ' + - 'Possible object types are: \n\t{}\n' - .format(PermissionTargets.help_values()) - ) -@click.argument('object-type') -@click.argument('object-id', required=False) + short_help='Get permissions for an item. ' + POSSIBLE_OBJECT_TYPES) +@click.option('--object_type', help='Possible object types are: {}'.format(PermissionTargets)) +@click.option('--object-id', required=False) @debug_option @profile_option @eat_exceptions @@ -61,21 +64,23 @@ def get_cli(api_client, object_type, object_id): @click.command(context_settings=CONTEXT_SETTINGS, short_help='List permission types') -@click.argument('object_type') -@click.argument('object_id') +@click.option('--object_type', help=POSSIBLE_OBJECT_TYPES) +@click.option('--object_id') @debug_option @profile_option @eat_exceptions @provide_api_client def list_permissions_types_cli(api_client, object_type, object_id): perms_api = PermissionsApi(api_client) + if not object_type: + click.echo('Possible object types are: {}'.format(PermissionTargets)) click.echo(pretty_format(perms_api.get_possible_permissions(object_type, object_id))) @click.command(context_settings=CONTEXT_SETTINGS, short_help='Add or modify permission types') -@click.argument('object_type') -@click.argument('object_id') +@click.option('--object_type', help=POSSIBLE_OBJECT_TYPES) +@click.option('--object_id') @click.option('--group-name', metavar='', cls=OneOfOption, default=None, one_of=PERMISSIONS_OPTION) @click.option('--user-name', metavar='', cls=OneOfOption, default=None, @@ -97,6 +102,9 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n click.echo('Need --user-name, --service-name or --group-name') return + if not object_type: + click.echo('Possible object types are: {}'.format(PermissionTargets)) + if not permission_level: click.echo('Need permission-level: {}'.format([e.value for e in PermissionLevel])) @@ -131,7 +139,7 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n @profile_option @eat_exceptions def list_permissions_targets_cli(): - click.echo('Possible object types are: {}'.format([e.value for e in PermissionTargets])) + click.echo(POSSIBLE_OBJECT_TYPES) @click.command(context_settings=CONTEXT_SETTINGS) @@ -139,7 +147,7 @@ def list_permissions_targets_cli(): @profile_option @eat_exceptions def list_permissions_level_cli(): - click.echo('Possible permission levels are: {}'.format([e.value for e in PermissionLevel])) + click.echo(POSSIBLE_PERMISSION_LEVELS) @click.command(context_settings=CONTEXT_SETTINGS, From 5b598550d9cacc76e60100169e9631badc39a65d Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Tue, 22 Sep 2020 15:07:04 -0700 Subject: [PATCH 13/25] databricks cli permissions Move from --object_type to --object-id to be inline with other commands Fix tests to validate the test output the same way clusters does --- databricks_cli/permissions/cli.py | 10 +++---- tests/permissions/test_cli.py | 43 +++++++++++++++++++++++-------- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index d04a445c..ab7f13af 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -49,7 +49,7 @@ @click.command(context_settings=CONTEXT_SETTINGS, short_help='Get permissions for an item. ' + POSSIBLE_OBJECT_TYPES) -@click.option('--object_type', help='Possible object types are: {}'.format(PermissionTargets)) +@click.option('--object-type', help='Possible object types are: {}'.format(PermissionTargets)) @click.option('--object-id', required=False) @debug_option @profile_option @@ -64,8 +64,8 @@ def get_cli(api_client, object_type, object_id): @click.command(context_settings=CONTEXT_SETTINGS, short_help='List permission types') -@click.option('--object_type', help=POSSIBLE_OBJECT_TYPES) -@click.option('--object_id') +@click.option('--object-type', help=POSSIBLE_OBJECT_TYPES) +@click.option('--object-id') @debug_option @profile_option @eat_exceptions @@ -79,8 +79,8 @@ def list_permissions_types_cli(api_client, object_type, object_id): @click.command(context_settings=CONTEXT_SETTINGS, short_help='Add or modify permission types') -@click.option('--object_type', help=POSSIBLE_OBJECT_TYPES) -@click.option('--object_id') +@click.option('--object-type', help=POSSIBLE_OBJECT_TYPES) +@click.option('--object-id') @click.option('--group-name', metavar='', cls=OneOfOption, default=None, one_of=PERMISSIONS_OPTION) @click.option('--user-name', metavar='', cls=OneOfOption, default=None, diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 139f6085..673d83cb 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -24,13 +24,15 @@ # pylint:disable=redefined-outer-name import re + import mock import pytest from click.testing import CliRunner -from tests.utils import provide_conf import databricks_cli.permissions.cli as cli from databricks_cli.utils import pretty_format +from tests.test_data import TEST_CLUSTER_ID +from tests.utils import provide_conf def strip_margin(text): @@ -41,8 +43,8 @@ def strip_margin(text): PERMISSIONS_RETURNS = { 'get': { 'clusters': { - '1234-567890-kens4': { - 'object_id': '/clusters/1234-567890-kens4', + TEST_CLUSTER_ID: { + 'object_id': '/clusters/{}'.format(TEST_CLUSTER_ID), 'object_type': 'cluster', 'access_control_list': [ { @@ -64,6 +66,22 @@ def strip_margin(text): } +def help_test(cli_function, service_function=None, rv=None, args=None): + """ + This function makes testing the cli functions that just pass data through simpler + """ + + if args is None: + args = [] + + with mock.patch('databricks_cli.permissions.cli.click.echo') as echo_mock: + if service_function: + service_function.return_value = rv + runner = CliRunner() + result = runner.invoke(cli_function, args) + assert echo_mock.call_args[0][0] == pretty_format(rv) + + @pytest.fixture() def perms_api_mock(): with mock.patch('databricks_cli.permissions.cli.PermissionsApi') as PermissionsApiMock: @@ -74,11 +92,14 @@ def perms_api_mock(): @provide_conf def test_get_cli(perms_api_mock): - with mock.patch('databricks_cli.permissions.cli.click.echo') as echo_mock: - return_value = PERMISSIONS_RETURNS['get']['clusters'] - perms_api_mock.get_permissions.return_value = return_value - runner = CliRunner() - runner.invoke(cli.get_cli, ['clusters', '1234-567890-kens4']) - assert perms_api_mock.get_permissions.call_args[0][0] == 'clusters' - assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' - assert echo_mock.call_args[0][0] == pretty_format(return_value) + return_value = PERMISSIONS_RETURNS['get']['clusters'] + perms_api_mock.get_permissions.return_value = return_value + help_test(cli.get_cli, args=[ + '--object-type', + 'clusters', + '--object-id', + '1234-567890-kens4' + ], rv=return_value) + + assert perms_api_mock.get_permissions.call_args[0][0] == 'clusters' + assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' From 12da9756dff81c396a6b3560a8cf24e6d2242989 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Tue, 22 Sep 2020 15:13:01 -0700 Subject: [PATCH 14/25] Fix a lint error in databricks_cli/sdk/permissions.py caused by the type hint not being the first line of the functions Remove an unused variable from tests/permissions/test_cli.py --- databricks_cli/sdk/permissions_service.py | 10 +++++++--- tests/permissions/test_cli.py | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/databricks_cli/sdk/permissions_service.py b/databricks_cli/sdk/permissions_service.py index 862d93a0..10b31e03 100644 --- a/databricks_cli/sdk/permissions_service.py +++ b/databricks_cli/sdk/permissions_service.py @@ -10,9 +10,11 @@ def __init__(self, client): def create_url(self, object_type, object_id, suffix=''): # type: (str, str, str) -> str + return '{}/{}/{}{}'.format(self.url_base, object_type, object_id, suffix) def get_permissions(self, object_type, object_id, headers=None): + # type: (str, str, Optional[dict]) -> dict """ Get the permissions for an object type and id @@ -25,11 +27,12 @@ def get_permissions(self, object_type, object_id, headers=None): https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-directory-permissions https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-registered-model-permissions """ - # type: (str, str, Optional[dict]) -> dict + return self.client.perform_query('GET', self.create_url(object_type, object_id), headers=headers) def get_possible_permissions(self, object_type, object_id, headers=None): + # type: (str, str, Optional[dict]) -> dict """ Get the permission levels for an object type. @@ -42,12 +45,13 @@ def get_possible_permissions(self, object_type, object_id, headers=None): https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-directories-permission-levels https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-registered-models-permission-levels """ - # type: (str, str, Optional[dict]) -> dict + return self.client.perform_query('GET', self.create_url(object_type, object_id, '/permissionLevels'), headers=headers) def add_permissions(self, object_type, object_id, data, headers=None): + # type: (str, str, dict, Optional[dict]) -> dict """ Add permissions, this does not REMOVE. A remove requires an update_permissions call to complete replacement. @@ -61,7 +65,7 @@ def add_permissions(self, object_type, object_id, data, headers=None): https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-directory-permissions https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/set-registered-model-permissions """ - # type: (str, str, dict, Optional[dict]) -> dict + return self.client.perform_query('PATCH', self.create_url(object_type, object_id), data=data, headers=headers) diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 673d83cb..96472cf3 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -78,7 +78,7 @@ def help_test(cli_function, service_function=None, rv=None, args=None): if service_function: service_function.return_value = rv runner = CliRunner() - result = runner.invoke(cli_function, args) + runner.invoke(cli_function, args) assert echo_mock.call_args[0][0] == pretty_format(rv) From 2c9af5ee085ed8aabd4a8a1680070b4c6bb12cf7 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Tue, 22 Sep 2020 16:50:31 -0700 Subject: [PATCH 15/25] databricks permissions: 1. Reuse the same help string for all object types 2. Update the group help to display the object types, it could be hard coded or displayed a lot cleaner. 3. object-id is always required, even on list, need to find out why 4. Remove if not object_type checks, as object type is a required option 5. Add mostly complete tests for list-permission-types, I do not have registered-models or instance-pools data available --- databricks_cli/permissions/cli.py | 34 ++++--- databricks_cli/sdk/permissions_service.py | 13 ++- tests/permissions/test_cli.py | 112 +++++++++++++++++++++- 3 files changed, 135 insertions(+), 24 deletions(-) diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index ab7f13af..7cac7142 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -20,6 +20,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import json import click @@ -49,38 +50,34 @@ @click.command(context_settings=CONTEXT_SETTINGS, short_help='Get permissions for an item. ' + POSSIBLE_OBJECT_TYPES) -@click.option('--object-type', help='Possible object types are: {}'.format(PermissionTargets)) -@click.option('--object-id', required=False) +@click.option('--object-type', required=True, help=POSSIBLE_OBJECT_TYPES) +@click.option('--object-id', required=True, help='object id to require permission about') @debug_option @profile_option @eat_exceptions @provide_api_client def get_cli(api_client, object_type, object_id): perms_api = PermissionsApi(api_client) - if not object_type: - click.echo('Possible object types are: {}'.format(PermissionTargets)) click.echo(pretty_format(perms_api.get_permissions(object_type, object_id))) @click.command(context_settings=CONTEXT_SETTINGS, short_help='List permission types') -@click.option('--object-type', help=POSSIBLE_OBJECT_TYPES) -@click.option('--object-id') +@click.option('--object-type', required=True, help=POSSIBLE_OBJECT_TYPES) +@click.option('--object-id', required=True, help='object id to require permission about') @debug_option @profile_option @eat_exceptions @provide_api_client def list_permissions_types_cli(api_client, object_type, object_id): perms_api = PermissionsApi(api_client) - if not object_type: - click.echo('Possible object types are: {}'.format(PermissionTargets)) click.echo(pretty_format(perms_api.get_possible_permissions(object_type, object_id))) @click.command(context_settings=CONTEXT_SETTINGS, short_help='Add or modify permission types') -@click.option('--object-type', help=POSSIBLE_OBJECT_TYPES) -@click.option('--object-id') +@click.option('--object-type', required=True, help=POSSIBLE_OBJECT_TYPES) +@click.option('--object-id', required=True, help='object id to require permission about') @click.option('--group-name', metavar='', cls=OneOfOption, default=None, one_of=PERMISSIONS_OPTION) @click.option('--user-name', metavar='', cls=OneOfOption, default=None, @@ -103,10 +100,10 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n return if not object_type: - click.echo('Possible object types are: {}'.format(PermissionTargets)) + click.echo(POSSIBLE_OBJECT_TYPES) if not permission_level: - click.echo('Need permission-level: {}'.format([e.value for e in PermissionLevel])) + click.echo(POSSIBLE_PERMISSION_LEVELS) # Determine the type of permissions we're adding. if user_name: @@ -119,8 +116,7 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n perm_type = PermissionType.service value = service_name else: - click.echo( - 'Invalid permission-type must be one of {}'.format([e.value for e in PermissionType])) + click.echo(POSSIBLE_PERMISSION_LEVELS) return permission = Permission(perm_type, value, Lookups.from_name(permission_level.upper())) @@ -152,7 +148,7 @@ def list_permissions_level_cli(): @click.command(context_settings=CONTEXT_SETTINGS, short_help='Get permissions for a directory') -@click.argument('path') +@click.option('--path') @debug_option @profile_option @eat_exceptions @@ -176,15 +172,17 @@ def directory_cli(api_client, path): @click.group(context_settings=CONTEXT_SETTINGS, - short_help='xUtility to interact with Databricks permissions api.\n' + - 'Possible object types are: {}\n'.format(PermissionTargets)) + help='Utility to interact with Databricks permissions api.\n\n' + + 'Valid object types for --object-type are:\n\n\t' + + json.dumps(PermissionTargets.values()) + ) @click.option('--version', '-v', is_flag=True, callback=print_version_callback, expose_value=False, is_eager=True, help=version) @debug_option @profile_option @eat_exceptions def permissions_group(): - """Provide utility to interact with Databricks permissions api.""" + # A python doc comment here will override the hand coded help above. pass diff --git a/databricks_cli/sdk/permissions_service.py b/databricks_cli/sdk/permissions_service.py index 10b31e03..a95fbb3c 100644 --- a/databricks_cli/sdk/permissions_service.py +++ b/databricks_cli/sdk/permissions_service.py @@ -10,8 +10,9 @@ def __init__(self, client): def create_url(self, object_type, object_id, suffix=''): # type: (str, str, str) -> str - - return '{}/{}/{}{}'.format(self.url_base, object_type, object_id, suffix) + return '{base}/{object_type}/{object_id}{suffix}'.format(base=self.url_base, + object_type=object_type, + object_id=object_id, suffix=suffix) def get_permissions(self, object_type, object_id, headers=None): # type: (str, str, Optional[dict]) -> dict @@ -28,7 +29,8 @@ def get_permissions(self, object_type, object_id, headers=None): https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-registered-model-permissions """ - return self.client.perform_query('GET', self.create_url(object_type, object_id), + return self.client.perform_query('GET', self.create_url(object_type=object_type, + object_id=object_id), headers=headers) def get_possible_permissions(self, object_type, object_id, headers=None): @@ -46,8 +48,9 @@ def get_possible_permissions(self, object_type, object_id, headers=None): https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/get-registered-models-permission-levels """ - return self.client.perform_query('GET', self.create_url(object_type, object_id, - '/permissionLevels'), + return self.client.perform_query('GET', self.create_url(object_type=object_type, + object_id=object_id, + suffix='/permissionLevels'), headers=headers) def add_permissions(self, object_type, object_id, data, headers=None): diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 96472cf3..4c004a0b 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -30,6 +30,7 @@ from click.testing import CliRunner import databricks_cli.permissions.cli as cli +from databricks_cli.permissions.api import PermissionTargets from databricks_cli.utils import pretty_format from tests.test_data import TEST_CLUSTER_ID from tests.utils import provide_conf @@ -62,10 +63,99 @@ def strip_margin(text): ] } } + }, + 'list-permissions': { + 'clusters': { + 'permission_levels': [ + { + 'permission_level': 'CAN_MANAGE', + 'description': 'Can Manage permission on cluster' + }, + { + 'permission_level': 'CAN_RESTART', + 'description': 'Can Restart permission on cluster' + }, + { + 'permission_level': 'CAN_ATTACH_TO', + 'description': 'Can Attach To permission on cluster' + } + ] + }, + + 'directories': + { + 'permission_levels': [ + { + 'permission_level': 'CAN_READ', + 'description': 'Can view and comment on notebooks in the directory' + }, + { + 'permission_level': 'CAN_RUN', + 'description': 'Can view, comment, attach/detach, and run commands in notebooks in the directory' + }, + { + 'permission_level': 'CAN_EDIT', + 'description': 'Can view, comment, attach/detach, run commands, and edit notebooks in the directory' + }, + { + 'permission_level': 'CAN_MANAGE', + 'description': 'Can view, comment, attach/detach, run commands, and edit notebooks in the folder, and can create, delete, and change permissions of items in the directory' + } + ] + }, + 'jobs': + { + 'permission_levels': [ + { + 'permission_level': 'IS_OWNER', + 'description': 'Is Owner permission on a job' + }, + { + 'permission_level': 'CAN_MANAGE_RUN', + 'description': 'Can Manage Run permission to trigger or cancel job runs' + }, + { + 'permission_level': 'CAN_VIEW', + 'description': 'Can View permission to view job run results' + } + ] + }, + 'notebooks': + { + 'permission_levels': [ + { + 'permission_level': 'CAN_READ', + 'description': 'Can view and comment on the notebook' + }, + { + 'permission_level': 'CAN_RUN', + 'description': 'Can view, comment, attach/detach, and run commands in the notebook' + }, + { + 'permission_level': 'CAN_EDIT', + 'description': 'Can view, comment, attach/detach, run commands, and edit the notebook' + }, + { + 'permission_level': 'CAN_MANAGE', + 'description': 'Can view, comment, attach/detach, run commands, edit, and change permissions of the notebook' + } + ] + } + } } +@pytest.fixture() +def permissions_sdk_mock(): + with mock.patch('databricks_cli.permissions.api.PermissionsService') as SdkMock: + _permissions_sdk_mock = mock.MagicMock() + SdkMock.return_value = _permissions_sdk_mock + # _permissions_sdk_mock.get_cluster = mock.MagicMock(return_value={}) + + yield _permissions_sdk_mock + + def help_test(cli_function, service_function=None, rv=None, args=None): """ This function makes testing the cli functions that just pass data through simpler @@ -78,7 +168,8 @@ def help_test(cli_function, service_function=None, rv=None, args=None): if service_function: service_function.return_value = rv runner = CliRunner() - runner.invoke(cli_function, args) + output = runner.invoke(cli_function, args) + print(output) assert echo_mock.call_args[0][0] == pretty_format(rv) @@ -103,3 +194,22 @@ def test_get_cli(perms_api_mock): assert perms_api_mock.get_permissions.call_args[0][0] == 'clusters' assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' + + +def filtered_perm_types(): + # FIXME: I do not have test data for instance-pools or registered data + return [e for e in PermissionTargets.values() if + e != 'instance-pools' and e != 'registered-models'] + + +@provide_conf +def test_list_permissions_types_cli(permissions_sdk_mock): + for perm_type in filtered_perm_types(): + return_value = PERMISSIONS_RETURNS['list-permissions'][perm_type] + permissions_sdk_mock.get_possible_permissions.return_value = return_value + help_test(cli.list_permissions_types_cli, args=[ + '--object-type', + perm_type, + '--object-id', + '1234-567890-kens4' + ], rv=return_value) From eef1210180d17cd58eef4bcae5f5c0fc589283a1 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 08:44:33 -0700 Subject: [PATCH 16/25] databricks permssions Fix line too long lint errors. Fix errors looking up instance-pools in the enum --- databricks_cli/permissions/api.py | 14 ++- tests/permissions/test_cli.py | 181 ++++++++++++++++++------------ 2 files changed, 120 insertions(+), 75 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index 5cc701c0..7d8403ea 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -53,6 +53,12 @@ def values(cls): def help_values(cls): return ', '.join([e.value for e in PermissionTargets]) + @classmethod + def get(cls, item): + if '-' in item: + item = item.replace('-', '_') + return PermissionTargets[item] + class PermissionLevel(Enum): manage = 'CAN_MANAGE' @@ -196,7 +202,7 @@ def get_permissions(self, object_type, object_id): object_id = '' # raise PermissionsError('object_id is invalid') - return self.client.get_permissions(object_type=PermissionTargets[object_type].value, + return self.client.get_permissions(object_type=PermissionTargets.get(object_type).value, object_id=object_id) def get_possible_permissions(self, object_type, object_id): @@ -207,7 +213,7 @@ def get_possible_permissions(self, object_type, object_id): raise PermissionsError('object_id is invalid') return self.client.get_possible_permissions( - object_type=PermissionTargets[object_type].value, + object_type=PermissionTargets.get(object_type).value, object_id=object_id) def add_permissions(self, object_type, object_id, permissions): @@ -217,7 +223,7 @@ def add_permissions(self, object_type, object_id, permissions): if not object_id: raise PermissionsError('object_id is invalid') - return self.client.add_permissions(object_type=PermissionTargets[object_type].value, + return self.client.add_permissions(object_type=PermissionTargets.get(object_type).value, object_id=object_id, data=permissions.to_dict()) def update_permissions(self, object_type, object_id, permissions): @@ -227,5 +233,5 @@ def update_permissions(self, object_type, object_id, permissions): if not object_id: raise PermissionsError('object_id is invalid') - return self.client.update_permissions(object_type=PermissionTargets[object_type].value, + return self.client.update_permissions(object_type=PermissionTargets.get(object_type).value, object_id=object_id, data=permissions.to_dict()) diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 4c004a0b..4d156931 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -21,7 +21,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -# pylint:disable=redefined-outer-name +# pylint: disable=redefined-outer-name import re @@ -81,67 +81,113 @@ def strip_margin(text): } ] }, - - 'directories': - { - 'permission_levels': [ - { - 'permission_level': 'CAN_READ', - 'description': 'Can view and comment on notebooks in the directory' - }, - { - 'permission_level': 'CAN_RUN', - 'description': 'Can view, comment, attach/detach, and run commands in notebooks in the directory' - }, - { - 'permission_level': 'CAN_EDIT', - 'description': 'Can view, comment, attach/detach, run commands, and edit notebooks in the directory' - }, - { - 'permission_level': 'CAN_MANAGE', - 'description': 'Can view, comment, attach/detach, run commands, and edit notebooks in the folder, and can create, delete, and change permissions of items in the directory' - } - ] - }, - 'jobs': - { - 'permission_levels': [ - { - 'permission_level': 'IS_OWNER', - 'description': 'Is Owner permission on a job' - }, - { - 'permission_level': 'CAN_MANAGE_RUN', - 'description': 'Can Manage Run permission to trigger or cancel job runs' - }, - { - 'permission_level': 'CAN_VIEW', - 'description': 'Can View permission to view job run results' - } - ] - }, - 'notebooks': - { - 'permission_levels': [ - { - 'permission_level': 'CAN_READ', - 'description': 'Can view and comment on the notebook' - }, - { - 'permission_level': 'CAN_RUN', - 'description': 'Can view, comment, attach/detach, and run commands in the notebook' - }, - { - 'permission_level': 'CAN_EDIT', - 'description': 'Can view, comment, attach/detach, run commands, and edit the notebook' - }, - { - 'permission_level': 'CAN_MANAGE', - 'description': 'Can view, comment, attach/detach, run commands, edit, and change permissions of the notebook' - } - ] - } - + 'directories': { + 'permission_levels': [ + { + 'permission_level': 'CAN_READ', + 'description': 'Can view and comment on notebooks in the directory' + }, + { + 'permission_level': 'CAN_RUN', + 'description': 'Can view, comment, attach/detach, and run commands in ' + + 'notebooks in the directory' + }, + { + 'permission_level': 'CAN_EDIT', + 'description': 'Can view, comment, attach/detach, run commands, and edit ' + + 'notebooks in the directory' + }, + { + 'permission_level': 'CAN_MANAGE', + 'description': 'Can view, comment, attach/detach, run commands, and edit ' + + 'notebooks in the folder, and can create, delete, and change ' + + 'permissions of items in the directory' + } + ] + }, + 'instance-pools': { + 'permission_levels': [ + { + "permission_level": "CAN_MANAGE", + "description": "Can Manage permission on a pool" + }, + { + "permission_level": "CAN_ATTACH_TO", + "description": "Can Attach To permission on a pool" + } + ] + }, + 'jobs': { + 'permission_levels': [ + { + 'permission_level': 'IS_OWNER', + 'description': 'Is Owner permission on a job' + }, + { + 'permission_level': 'CAN_MANAGE_RUN', + 'description': 'Can Manage Run permission to trigger or cancel job runs' + }, + { + 'permission_level': 'CAN_VIEW', + 'description': 'Can View permission to view job run results' + } + ] + }, + 'notebooks': { + 'permission_levels': [ + { + 'permission_level': 'CAN_READ', + 'description': 'Can view and comment on the notebook' + }, + { + 'permission_level': 'CAN_RUN', + 'description': 'Can view, comment, attach/detach, and run commands in the' + + 'notebook' + }, + { + 'permission_level': 'CAN_EDIT', + 'description': 'Can view, comment, attach/detach, run commands, and edit ' + + 'the notebook' + }, + { + 'permission_level': 'CAN_MANAGE', + 'description': 'Can view, comment, attach/detach, run commands, edit, and ' + + 'change permissions of the notebook' + } + ] + }, + 'registered-models': { + 'permission_levels': [ + { + 'permission_level': 'CAN_READ', + 'description': 'Can view the details of the registered model and its model ' + + 'versions, and use the model versions.' + }, + { + 'permission_level': 'CAN_EDIT', + 'description': 'Can view and edit the details of a registered model and ' + + 'its model versions (except stage changes), and add ' + + 'new model versions.' + }, + { + 'permission_level': 'CAN_MANAGE_STAGING_VERSIONS', + 'description': 'Can view and edit the details of a registered model and its ' + + 'model versions, add new model versions, and manage stage ' + + 'transitions between non-Production stages.' + }, + { + 'permission_level': 'CAN_MANAGE_PRODUCTION_VERSIONS', + 'description': 'Can view and edit the details of a registered model and its ' + + 'model versions, add new model versions, and manage stage ' + + 'transitions between any stages.' + }, + { + 'permission_level': 'CAN_MANAGE', + 'description': 'Can manage permissions on, view all details of, and perform ' + + 'all actions on the registered model and its model versions.' + } + ] + } } } @@ -168,8 +214,7 @@ def help_test(cli_function, service_function=None, rv=None, args=None): if service_function: service_function.return_value = rv runner = CliRunner() - output = runner.invoke(cli_function, args) - print(output) + runner.invoke(cli_function, args) assert echo_mock.call_args[0][0] == pretty_format(rv) @@ -196,15 +241,9 @@ def test_get_cli(perms_api_mock): assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' -def filtered_perm_types(): - # FIXME: I do not have test data for instance-pools or registered data - return [e for e in PermissionTargets.values() if - e != 'instance-pools' and e != 'registered-models'] - - @provide_conf def test_list_permissions_types_cli(permissions_sdk_mock): - for perm_type in filtered_perm_types(): + for perm_type in PermissionTargets.values(): return_value = PERMISSIONS_RETURNS['list-permissions'][perm_type] permissions_sdk_mock.get_possible_permissions.return_value = return_value help_test(cli.list_permissions_types_cli, args=[ From 0d9e5ebfa8e71a615aea6c7ea2ad5964be9d087a Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 08:53:09 -0700 Subject: [PATCH 17/25] Fix an issue with OneOfOption where the error would say Missing None. $ databricks permissions add --object-type clusters --object-id foo Error: Missing None. One of ['group-name', 'user-name', 'service-name'] must be provided. $ databricks permissions add --object-type clusters --object-id foo Error: Missing option '--group-name'. One of ['group-name', 'user-name', 'service-name'] must be provided. --- databricks_cli/click_types.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/databricks_cli/click_types.py b/databricks_cli/click_types.py index ff508a74..074f803d 100644 --- a/databricks_cli/click_types.py +++ b/databricks_cli/click_types.py @@ -114,7 +114,7 @@ def __init__(self, *args, **kwargs): def handle_parse_result(self, ctx, opts, args): cleaned_opts = set([o.replace('_', '-') for o in opts.keys()]) if len(cleaned_opts.intersection(set(self.one_of))) == 0: - raise MissingParameter('One of {} must be provided.'.format(self.one_of)) + raise MissingParameter('One of {} must be provided.'.format(self.one_of), param=self) if len(cleaned_opts.intersection(set(self.one_of))) > 1: raise UsageError('Only one of {} should be provided.'.format(self.one_of)) return super(OneOfOption, self).handle_parse_result(ctx, opts, args) @@ -124,6 +124,7 @@ class OptionalOneOfOption(Option): def __init__(self, *args, **kwargs): self.one_of = kwargs.pop('one_of') super(OptionalOneOfOption, self).__init__(*args, **kwargs) + self.param_hint = self.one_of def handle_parse_result(self, ctx, opts, args): cleaned_opts = set([o.replace('_', '-') for o in opts.keys()]) From d49cd78c8eff982b1ae30b51fb85d42f31884854 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 08:57:44 -0700 Subject: [PATCH 18/25] databricks permssions Cleanup ordering of commands added to the group Remove checks that are done by click and are not required in databricks permissions add --- databricks_cli/permissions/cli.py | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index 7cac7142..05a56e5d 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -78,19 +78,14 @@ def list_permissions_types_cli(api_client, object_type, object_id): short_help='Add or modify permission types') @click.option('--object-type', required=True, help=POSSIBLE_OBJECT_TYPES) @click.option('--object-id', required=True, help='object id to require permission about') -@click.option('--group-name', metavar='', cls=OneOfOption, default=None, - one_of=PERMISSIONS_OPTION) -@click.option('--user-name', metavar='', cls=OneOfOption, default=None, - one_of=PERMISSIONS_OPTION) -@click.option('--service-name', metavar='', cls=OneOfOption, default=None, - one_of=PERMISSIONS_OPTION) -@click.option('--permission-level', metavar='', default=None, - required=True) +@click.option('--group-name', metavar='', cls=OneOfOption, one_of=PERMISSIONS_OPTION) +@click.option('--user-name', metavar='', cls=OneOfOption, one_of=PERMISSIONS_OPTION) +@click.option('--service-name', metavar='', cls=OneOfOption, one_of=PERMISSIONS_OPTION) +@click.option('--permission-level', metavar='', required=True) @debug_option @profile_option @eat_exceptions @provide_api_client -# pylint:disable=unused-argument def add_cli(api_client, object_type, object_id, user_name, group_name, service_name, permission_level): perms_api = PermissionsApi(api_client) @@ -99,12 +94,6 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n click.echo('Need --user-name, --service-name or --group-name') return - if not object_type: - click.echo(POSSIBLE_OBJECT_TYPES) - - if not permission_level: - click.echo(POSSIBLE_PERMISSION_LEVELS) - # Determine the type of permissions we're adding. if user_name: perm_type = PermissionType.user @@ -164,9 +153,6 @@ def directory_cli(api_client, path): click.echo('Failed to find id for {}'.format(path)) return - # if len(object_ids) > 1: - # click.echo('Too many objects ({}) returned for {}'.format(len(object_ids), path)) - for object_id in object_ids: click.echo(pretty_format(perms_api.get_permissions(object_type, object_id))) @@ -186,10 +172,9 @@ def permissions_group(): pass -permissions_group.add_command(list_permissions_types_cli, name='list-types') -permissions_group.add_command(get_cli, name='get') permissions_group.add_command(add_cli, name='add') -permissions_group.add_command(list_permissions_targets_cli, name='targets') -permissions_group.add_command(list_permissions_level_cli, name='levels') - permissions_group.add_command(directory_cli, name='ls') +permissions_group.add_command(get_cli, name='get') +permissions_group.add_command(list_permissions_level_cli, name='levels') +permissions_group.add_command(list_permissions_targets_cli, name='targets') +permissions_group.add_command(list_permissions_types_cli, name='list-types') From cf37d756cc7c315aa2bdd79233bd9c12042c2c4f Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 09:18:35 -0700 Subject: [PATCH 19/25] databricks permissions Rename databricks_cli/permissions/api.py:Lookups to PermissionsLookup PermissionsLookup.from_name should raise a useful error with a message instead of KeyError Remove more error checking that is done by click. --permission-level should be checked by click to provide a useful error --- databricks_cli/permissions/api.py | 7 +++++-- databricks_cli/permissions/cli.py | 33 ++++++++++++++++--------------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index 7d8403ea..d2b80df7 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -100,7 +100,7 @@ def help_values(cls): return ', '.join([e.value for e in PermissionType]) -class Lookups(object): +class PermissionsLookup(object): items = { 'CAN_MANAGE': PermissionLevel.manage, 'CAN_RESTART': PermissionLevel.restart, @@ -134,7 +134,10 @@ class Lookups(object): @classmethod def from_name(cls, name): - return cls.items[name] + rv = cls.items.get(name) + if not rv: + raise PermissionsError('Unable to find value for name {}'.format(name)) + return rv class Permission(object): diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index 05a56e5d..7a42ffc9 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -23,11 +23,12 @@ import json import click +from click import UsageError from databricks_cli.click_types import OneOfOption from databricks_cli.configure.config import provide_api_client, profile_option, debug_option from databricks_cli.permissions.api import PermissionsApi, PermissionTargets, PermissionLevel, \ - PermissionType, Permission, PermissionsObject, Lookups + PermissionType, Permission, PermissionsObject, PermissionsLookup from databricks_cli.utils import eat_exceptions, CONTEXT_SETTINGS from databricks_cli.utils import pretty_format from databricks_cli.version import print_version_callback, version @@ -39,8 +40,8 @@ JSON_FILE_OPTIONS = ['json-file', 'json'] CREATE_USER_OPTIONS = JSON_FILE_OPTIONS + ['user-name'] -PERMISSIONS_OPTION = ['group-name', 'user-name', 'service-name'] - +GROUP_USER_SERVICE_OPTIONS = ['group-name', 'user-name', 'service-name'] +PERMISSION_LEVEL_OPTIONS = [e.name for e in PermissionLevel] POSSIBLE_OBJECT_TYPES = 'Possible object types are: \n\t{}\n'.format( PermissionTargets.help_values()) @@ -78,10 +79,13 @@ def list_permissions_types_cli(api_client, object_type, object_id): short_help='Add or modify permission types') @click.option('--object-type', required=True, help=POSSIBLE_OBJECT_TYPES) @click.option('--object-id', required=True, help='object id to require permission about') -@click.option('--group-name', metavar='', cls=OneOfOption, one_of=PERMISSIONS_OPTION) -@click.option('--user-name', metavar='', cls=OneOfOption, one_of=PERMISSIONS_OPTION) -@click.option('--service-name', metavar='', cls=OneOfOption, one_of=PERMISSIONS_OPTION) -@click.option('--permission-level', metavar='', required=True) +@click.option('--group-name', metavar='', cls=OneOfOption, + one_of=GROUP_USER_SERVICE_OPTIONS) +@click.option('--user-name', metavar='', cls=OneOfOption, one_of=GROUP_USER_SERVICE_OPTIONS) +@click.option('--service-name', metavar='', cls=OneOfOption, + one_of=GROUP_USER_SERVICE_OPTIONS) +@click.option('--permission-level', metavar='', cls=OneOfOption, + one_of=PERMISSION_LEVEL_OPTIONS, required=True, help=POSSIBLE_PERMISSION_LEVELS) @debug_option @profile_option @eat_exceptions @@ -90,10 +94,6 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n permission_level): perms_api = PermissionsApi(api_client) - if not user_name and not group_name and not service_name: - click.echo('Need --user-name, --service-name or --group-name') - return - # Determine the type of permissions we're adding. if user_name: perm_type = PermissionType.user @@ -105,16 +105,17 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n perm_type = PermissionType.service value = service_name else: - click.echo(POSSIBLE_PERMISSION_LEVELS) - return + # hanging if. This shouldn't be hit, because OneOfOption should prevent it. + # this else/raise is for readability when doing a review. + raise UsageError('Invalid argument') - permission = Permission(perm_type, value, Lookups.from_name(permission_level.upper())) + permission = Permission(perm_type, value, PermissionsLookup.from_name(permission_level.upper())) all_permissions = PermissionsObject() all_permissions.add(permission) if not all_permissions.to_dict(): - click.echo('invalid permissions') - return + raise UsageError( + 'Unable to parse permissions from permission level: {}'.format(permission_level)) click.echo(pretty_format(perms_api.add_permissions(object_type, object_id, all_permissions))) From cccf174e9c716456311230986944f97c38ce04e3 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 09:58:51 -0700 Subject: [PATCH 20/25] databricks permissions Use click.Choice to validate permission levels Add test for databricks permissions add clusters manage --- databricks_cli/permissions/api.py | 5 ++- databricks_cli/permissions/cli.py | 6 ++-- tests/permissions/test_cli.py | 58 ++++++++++++++++++++++++++++--- 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index d2b80df7..bb956a19 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -74,6 +74,10 @@ class PermissionLevel(Enum): def to_name(self): return self[self.value] + @classmethod + def names(cls): + return [e.name for e in PermissionLevel] + @classmethod def values(cls): return [e.value for e in PermissionLevel] @@ -129,7 +133,6 @@ class PermissionsLookup(object): 'registered_models': PermissionTargets.registered_model, 'model': PermissionTargets.model, 'models': PermissionTargets.models, - } @classmethod diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index 7a42ffc9..79f67a89 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -84,8 +84,8 @@ def list_permissions_types_cli(api_client, object_type, object_id): @click.option('--user-name', metavar='', cls=OneOfOption, one_of=GROUP_USER_SERVICE_OPTIONS) @click.option('--service-name', metavar='', cls=OneOfOption, one_of=GROUP_USER_SERVICE_OPTIONS) -@click.option('--permission-level', metavar='', cls=OneOfOption, - one_of=PERMISSION_LEVEL_OPTIONS, required=True, help=POSSIBLE_PERMISSION_LEVELS) +@click.option('--permission-level', metavar='', type=click.Choice(PermissionLevel.names()), + required=True, help=POSSIBLE_PERMISSION_LEVELS) @debug_option @profile_option @eat_exceptions @@ -109,7 +109,7 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n # this else/raise is for readability when doing a review. raise UsageError('Invalid argument') - permission = Permission(perm_type, value, PermissionsLookup.from_name(permission_level.upper())) + permission = Permission(perm_type, value, PermissionLevel[permission_level]) all_permissions = PermissionsObject() all_permissions.add(permission) diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 4d156931..3687ae52 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -188,6 +188,37 @@ def strip_margin(text): } ] } + }, + 'add': { + 'clusters': { + 'add-manage': { + 'object_id': TEST_CLUSTER_ID, + 'object_type': 'cluster', + 'access_control_list': [ + { + 'group_name': 'admins', + 'all_permissions': [ + { + 'permission_level': 'CAN_MANAGE', + 'inherited': True, + 'inherited_from_object': [ + '/clusters/' + ] + } + ] + }, + { + 'group_name': 'foo', + 'all_permissions': [ + { + 'permission_level': 'CAN_MANAGE', + 'inherited': False + } + ] + } + ] + } + } } } @@ -214,7 +245,9 @@ def help_test(cli_function, service_function=None, rv=None, args=None): if service_function: service_function.return_value = rv runner = CliRunner() - runner.invoke(cli_function, args) + result = runner.invoke(cli_function, args) + if result.exit_code != 0: + print(result.output) assert echo_mock.call_args[0][0] == pretty_format(rv) @@ -234,11 +267,11 @@ def test_get_cli(perms_api_mock): '--object-type', 'clusters', '--object-id', - '1234-567890-kens4' + TEST_CLUSTER_ID ], rv=return_value) assert perms_api_mock.get_permissions.call_args[0][0] == 'clusters' - assert perms_api_mock.get_permissions.call_args[0][1] == '1234-567890-kens4' + assert perms_api_mock.get_permissions.call_args[0][1] == TEST_CLUSTER_ID @provide_conf @@ -250,5 +283,22 @@ def test_list_permissions_types_cli(permissions_sdk_mock): '--object-type', perm_type, '--object-id', - '1234-567890-kens4' + TEST_CLUSTER_ID ], rv=return_value) + + +@provide_conf +def test_add_cluster_manage(permissions_sdk_mock): + perm_type = 'clusters' + return_value = PERMISSIONS_RETURNS['add'][perm_type]['add-manage'] + permissions_sdk_mock.add_permissions.return_value = return_value + help_test(cli.add_cli, args=[ + '--object-type', + perm_type, + '--object-id', + TEST_CLUSTER_ID, + '--group-name', + 'foo', + '--permission-level', + 'manage' + ], rv=return_value) From 25e710ff2ae955a38b8310f4cabe36de70c707a4 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 10:09:12 -0700 Subject: [PATCH 21/25] Add more tests of the permissions cli, still need to test the permissions api and service better --- tests/permissions/test_cli.py | 77 +++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 16 deletions(-) diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 3687ae52..9a0737e6 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -191,7 +191,7 @@ def strip_margin(text): }, 'add': { 'clusters': { - 'add-manage': { + 'add-manage-group-name': { 'object_id': TEST_CLUSTER_ID, 'object_type': 'cluster', 'access_control_list': [ @@ -208,7 +208,34 @@ def strip_margin(text): ] }, { - 'group_name': 'foo', + 'group_name': 'sam', + 'all_permissions': [ + { + 'permission_level': 'CAN_MANAGE', + 'inherited': False + } + ] + } + ] + }, + 'add-manage-user-name': { + 'object_id': TEST_CLUSTER_ID, + 'object_type': 'cluster', + 'access_control_list': [ + { + 'group_name': 'admins', + 'all_permissions': [ + { + 'permission_level': 'CAN_MANAGE', + 'inherited': True, + 'inherited_from_object': [ + '/clusters/' + ] + } + ] + }, + { + 'user_name': 'sam', 'all_permissions': [ { 'permission_level': 'CAN_MANAGE', @@ -233,7 +260,7 @@ def permissions_sdk_mock(): yield _permissions_sdk_mock -def help_test(cli_function, service_function=None, rv=None, args=None): +def help_test(cli_function, service_function=None, rv=None, args=None, format=True): """ This function makes testing the cli functions that just pass data through simpler """ @@ -248,7 +275,11 @@ def help_test(cli_function, service_function=None, rv=None, args=None): result = runner.invoke(cli_function, args) if result.exit_code != 0: print(result.output) - assert echo_mock.call_args[0][0] == pretty_format(rv) + if format: + expected = pretty_format(rv) + else: + expected = rv + assert echo_mock.call_args[0][0] == expected @pytest.fixture() @@ -290,15 +321,29 @@ def test_list_permissions_types_cli(permissions_sdk_mock): @provide_conf def test_add_cluster_manage(permissions_sdk_mock): perm_type = 'clusters' - return_value = PERMISSIONS_RETURNS['add'][perm_type]['add-manage'] - permissions_sdk_mock.add_permissions.return_value = return_value - help_test(cli.add_cli, args=[ - '--object-type', - perm_type, - '--object-id', - TEST_CLUSTER_ID, - '--group-name', - 'foo', - '--permission-level', - 'manage' - ], rv=return_value) + + for add_type in ['user-name', 'group-name']: + return_value = PERMISSIONS_RETURNS['add'][perm_type]['add-manage-{}'.format(add_type)] + permissions_sdk_mock.add_permissions.return_value = return_value + help_test(cli.add_cli, args=[ + '--object-type', + perm_type, + '--object-id', + TEST_CLUSTER_ID, + '--{}'.format(add_type), + 'sam', + '--permission-level', + 'manage' + ], rv=return_value) + + +@provide_conf +def test_list_permissions_targets_cli(): + help_test(cli.list_permissions_targets_cli, args=None, rv=cli.POSSIBLE_OBJECT_TYPES, + format=False) + + +@provide_conf +def test_list_permissions_level_cli(): + help_test(cli.list_permissions_level_cli, args=None, rv=cli.POSSIBLE_PERMISSION_LEVELS, + format=False) From 6ca0a006e0a5290c3dea73982bcb7baedd33b2b6 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 10:59:29 -0700 Subject: [PATCH 22/25] Fix lint errors --- databricks_cli/permissions/cli.py | 2 +- tests/permissions/test_cli.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index 79f67a89..bbf45999 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -28,7 +28,7 @@ from databricks_cli.click_types import OneOfOption from databricks_cli.configure.config import provide_api_client, profile_option, debug_option from databricks_cli.permissions.api import PermissionsApi, PermissionTargets, PermissionLevel, \ - PermissionType, Permission, PermissionsObject, PermissionsLookup + PermissionType, Permission, PermissionsObject from databricks_cli.utils import eat_exceptions, CONTEXT_SETTINGS from databricks_cli.utils import pretty_format from databricks_cli.version import print_version_callback, version diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 9a0737e6..08f0e374 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -260,7 +260,7 @@ def permissions_sdk_mock(): yield _permissions_sdk_mock -def help_test(cli_function, service_function=None, rv=None, args=None, format=True): +def help_test(cli_function, service_function=None, rv=None, args=None, format_result=True): """ This function makes testing the cli functions that just pass data through simpler """ @@ -275,7 +275,7 @@ def help_test(cli_function, service_function=None, rv=None, args=None, format=Tr result = runner.invoke(cli_function, args) if result.exit_code != 0: print(result.output) - if format: + if format_result: expected = pretty_format(rv) else: expected = rv @@ -340,10 +340,10 @@ def test_add_cluster_manage(permissions_sdk_mock): @provide_conf def test_list_permissions_targets_cli(): help_test(cli.list_permissions_targets_cli, args=None, rv=cli.POSSIBLE_OBJECT_TYPES, - format=False) + format_result=False) @provide_conf def test_list_permissions_level_cli(): help_test(cli.list_permissions_level_cli, args=None, rv=cli.POSSIBLE_PERMISSION_LEVELS, - format=False) + format_result=False) From d9ba966a9fd8b63c20be4046d1077768ac4c0165 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 15:22:19 -0700 Subject: [PATCH 23/25] databricks permissions Allow PermissionsObject to init with constructor databricks permissions ls should require path, and path should have help Add more tests --- databricks_cli/permissions/api.py | 7 +++++-- databricks_cli/permissions/cli.py | 9 ++------- tests/permissions/test_cli.py | 23 +++++++++++++++++++++-- 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index bb956a19..cc6d9042 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -23,6 +23,7 @@ from enum import Enum from databricks_cli.sdk.permissions_service import PermissionsService + from .exceptions import PermissionsError @@ -165,8 +166,10 @@ def to_dict(self): class PermissionsObject(object): - def __init__(self): - self.permissions = [] + def __init__(self, permissions=None): + if not permissions: + permissions = [] + self.permissions = permissions def add(self, permission): # type: (Permission) -> None diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index bbf45999..d6dd13c6 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -110,12 +110,7 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n raise UsageError('Invalid argument') permission = Permission(perm_type, value, PermissionLevel[permission_level]) - all_permissions = PermissionsObject() - all_permissions.add(permission) - - if not all_permissions.to_dict(): - raise UsageError( - 'Unable to parse permissions from permission level: {}'.format(permission_level)) + all_permissions = PermissionsObject([permission]) click.echo(pretty_format(perms_api.add_permissions(object_type, object_id, all_permissions))) @@ -138,7 +133,7 @@ def list_permissions_level_cli(): @click.command(context_settings=CONTEXT_SETTINGS, short_help='Get permissions for a directory') -@click.option('--path') +@click.option('--path', required=True, help='Path in the workspace for to get permissions for.') @debug_option @profile_option @eat_exceptions diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 08f0e374..5006cef1 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -25,11 +25,10 @@ import re +import databricks_cli.permissions.cli as cli import mock import pytest from click.testing import CliRunner - -import databricks_cli.permissions.cli as cli from databricks_cli.permissions.api import PermissionTargets from databricks_cli.utils import pretty_format from tests.test_data import TEST_CLUSTER_ID @@ -347,3 +346,23 @@ def test_list_permissions_targets_cli(): def test_list_permissions_level_cli(): help_test(cli.list_permissions_level_cli, args=None, rv=cli.POSSIBLE_PERMISSION_LEVELS, format_result=False) + + +@pytest.fixture() +def workspace_api_mock(): + with mock.patch('databricks_cli.permissions.cli.WorkspaceApi') as WorkspaceApiMock: + workspace_api_mock = mock.MagicMock() + WorkspaceApiMock.return_value = workspace_api_mock + yield workspace_api_mock + + +@provide_conf +def test_directory_cli_missing_path(permissions_sdk_mock, workspace_api_mock): + workspace_api_mock.get_id_for_directory.return_value = None + help_test(cli.directory_cli, + args=[ + '--path', + '/workspace/missing.txt' + ], + rv='Failed to find id for /workspace/missing.txt', + format_result=False) From fb5f3541dddb354d876f6e3c446a3c711c73632f Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Wed, 23 Sep 2020 15:49:07 -0700 Subject: [PATCH 24/25] databricks permissions: Remove dead code that wasn't called Update tests to cover all of the commands --- databricks_cli/permissions/api.py | 25 +++------------ databricks_cli/permissions/cli.py | 4 +-- databricks_cli/workspace/api.py | 1 + tests/permissions/test_cli.py | 53 ++++++++++++++++++++++++++++++- 4 files changed, 59 insertions(+), 24 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index cc6d9042..12551521 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -23,7 +23,6 @@ from enum import Enum from databricks_cli.sdk.permissions_service import PermissionsService - from .exceptions import PermissionsError @@ -43,9 +42,6 @@ class PermissionTargets(Enum): model = registered_models models = registered_models - def to_name(self): - return self[self.value] - @classmethod def values(cls): return [e.value for e in PermissionTargets] @@ -72,9 +68,6 @@ class PermissionLevel(Enum): run = 'CAN_RUN' edit = 'CAN_EDIT' - def to_name(self): - return self[self.value] - @classmethod def names(cls): return [e.name for e in PermissionLevel] @@ -93,17 +86,10 @@ class PermissionType(Enum): group = 'group_name' service = 'service_principal_name' - def to_name(self): - return self[self.value] - @classmethod def values(cls): return [e.value for e in PermissionType] - @classmethod - def help_values(cls): - return ', '.join([e.value for e in PermissionType]) - class PermissionsLookup(object): items = { @@ -136,13 +122,6 @@ class PermissionsLookup(object): 'models': PermissionTargets.models, } - @classmethod - def from_name(cls, name): - rv = cls.items.get(name) - if not rv: - raise PermissionsError('Unable to find value for name {}'.format(name)) - return rv - class Permission(object): def __init__(self, permission_type, value, permission_level): @@ -207,6 +186,7 @@ def get_permissions(self, object_type, object_id): # type: (str, str) -> dict if not object_type: raise PermissionsError('object_type is invalid') + if not object_id: object_id = '' # raise PermissionsError('object_id is invalid') @@ -218,6 +198,7 @@ def get_possible_permissions(self, object_type, object_id): # type: (str, str) -> dict if not object_type: raise PermissionsError('object_type is invalid') + if not object_id: raise PermissionsError('object_id is invalid') @@ -229,6 +210,7 @@ def add_permissions(self, object_type, object_id, permissions): # type: (str, str, PermissionsObject) -> dict if not object_type: raise PermissionsError('object_type is invalid') + if not object_id: raise PermissionsError('object_id is invalid') @@ -239,6 +221,7 @@ def update_permissions(self, object_type, object_id, permissions): # type: (str, str, PermissionsObject) -> dict if not object_type: raise PermissionsError('object_type is invalid') + if not object_id: raise PermissionsError('object_id is invalid') diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index d6dd13c6..11109948 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -41,7 +41,7 @@ CREATE_USER_OPTIONS = JSON_FILE_OPTIONS + ['user-name'] GROUP_USER_SERVICE_OPTIONS = ['group-name', 'user-name', 'service-name'] -PERMISSION_LEVEL_OPTIONS = [e.name for e in PermissionLevel] +PERMISSION_LEVEL_OPTIONS = PermissionLevel.names() POSSIBLE_OBJECT_TYPES = 'Possible object types are: \n\t{}\n'.format( PermissionTargets.help_values()) @@ -163,7 +163,7 @@ def directory_cli(api_client, path): @debug_option @profile_option @eat_exceptions -def permissions_group(): +def permissions_group(): # NOQA # A python doc comment here will override the hand coded help above. pass diff --git a/databricks_cli/workspace/api.py b/databricks_cli/workspace/api.py index b78b5f44..83ac5dec 100644 --- a/databricks_cli/workspace/api.py +++ b/databricks_cli/workspace/api.py @@ -107,6 +107,7 @@ def list_objects(self, workspace_path, headers=None): if 'objects' not in response: return [] objects = response['objects'] + return [WorkspaceFileInfo.from_json(f) for f in objects] def mkdirs(self, workspace_path, headers=None): diff --git a/tests/permissions/test_cli.py b/tests/permissions/test_cli.py index 5006cef1..c68dc203 100644 --- a/tests/permissions/test_cli.py +++ b/tests/permissions/test_cli.py @@ -25,10 +25,11 @@ import re -import databricks_cli.permissions.cli as cli import mock import pytest from click.testing import CliRunner + +import databricks_cli.permissions.cli as cli from databricks_cli.permissions.api import PermissionTargets from databricks_cli.utils import pretty_format from tests.test_data import TEST_CLUSTER_ID @@ -245,6 +246,26 @@ def strip_margin(text): ] } } + }, + 'ls': { + '/': { + "object_id": "/directories/1", + "object_type": "directory", + "access_control_list": [ + { + "group_name": "admins", + "all_permissions": [ + { + "permission_level": "CAN_MANAGE", + "inherited": True, + "inherited_from_object": [ + "/directories/" + ] + } + ] + } + ] + } } } @@ -366,3 +387,33 @@ def test_directory_cli_missing_path(permissions_sdk_mock, workspace_api_mock): ], rv='Failed to find id for /workspace/missing.txt', format_result=False) + + +@provide_conf +def test_directory_cli(permissions_sdk_mock, workspace_api_mock): + workspace_api_mock.get_id_for_directory.return_value = ['1'] + permissions_sdk_mock.get_permissions.return_value = { + 'object_id': '/directories/1', + 'object_type': 'directory', + 'access_control_list': [ + { + 'group_name': 'admins', + 'all_permissions': [ + { + 'permission_level': 'CAN_MANAGE', + 'inherited': True, + 'inherited_from_object': [ + '/directories/' + ] + } + ] + } + ] + } + + help_test(cli.directory_cli, + args=[ + '--path', + '/' + ], + rv=PERMISSIONS_RETURNS['ls']['/']) From 83c668805ff25a4ea99d7b8a68d14c3fa1df3684 Mon Sep 17 00:00:00 2001 From: Allen Reese Date: Thu, 24 Sep 2020 08:18:32 -0700 Subject: [PATCH 25/25] Checkpoint trying to cleanup how the permissions are managed to make detecting bad combinations easier --- databricks_cli/permissions/api.py | 156 ++++++++++++++++++++++++++---- databricks_cli/permissions/cli.py | 4 +- 2 files changed, 138 insertions(+), 22 deletions(-) diff --git a/databricks_cli/permissions/api.py b/databricks_cli/permissions/api.py index 12551521..fe437781 100644 --- a/databricks_cli/permissions/api.py +++ b/databricks_cli/permissions/api.py @@ -58,7 +58,10 @@ def get(cls, item): class PermissionLevel(Enum): + no_permissions = 'NONE' manage = 'CAN_MANAGE' + manage_staging_versions = 'CAN_MANAGE_STAGING_VERSIONS' + manage_production_versions = 'CAN_MANAGE_PRODUCTION_VERSIONS' restart = 'CAN_RESTART' attach = 'CAN_ATTACH_TO' manage_run = 'CAN_MANAGE_RUN' @@ -67,6 +70,7 @@ class PermissionLevel(Enum): read = 'CAN_READ' run = 'CAN_RUN' edit = 'CAN_EDIT' + use = 'CAN_USE' @classmethod def names(cls): @@ -81,6 +85,100 @@ def help_values(cls): return ', '.join([e.value for e in PermissionLevel]) +class BasicPermissions(object): + def __init__(self, object_type, valid_permissions): + self.object_type = object_type + self.valid_permissions = valid_permissions + + def is_valid_target(self, permission): + # type: (str) -> bool + return permission in self.valid_permissions + + def valid_targets(self): + return [s.name for s in self.valid_permissions] + + +class TokenPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.token, { + PermissionLevel.no_permissions, + PermissionLevel.use, + PermissionLevel.manage, + }) + + +class PasswordPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.password, { + PermissionLevel.no_permissions, + PermissionLevel.use, + }) + + +class ClusterPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.clusters, { + PermissionLevel.no_permissions, + PermissionLevel.attach, + PermissionLevel.restart, + PermissionLevel.manage, + }) + + +class InstancePoolPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.instance_pools, { + PermissionLevel.no_permissions, + PermissionLevel.attach, + PermissionLevel.manage, + }) + + +class JobPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.jobs, { + PermissionLevel.no_permissions, + PermissionLevel.view, + PermissionLevel.manage_run, + PermissionLevel.owner, + PermissionLevel.manage, + }) + + +class NotebookPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.notebook, { + PermissionLevel.no_permissions, + PermissionLevel.read, + PermissionLevel.run, + PermissionLevel.edit, + PermissionLevel.manage, + }) + + +class DirectoryPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.directory, { + PermissionLevel.no_permissions, + PermissionLevel.read, + PermissionLevel.run, + PermissionLevel.edit, + PermissionLevel.manage, + }) + + +class MlFlowPermissions(BasicPermissions): + def __init__(self): + super().__init__(PermissionTargets.models, { + PermissionLevel.no_permissions, + PermissionLevel.read, + PermissionLevel.edit, + PermissionLevel.manage_staging_versions, + PermissionLevel.manage_production_versions, + PermissionLevel.manage, + }) + + class PermissionType(Enum): user = 'user_name' group = 'group_name' @@ -92,6 +190,10 @@ def values(cls): class PermissionsLookup(object): + """ + static lookup table for permissions + """ + items = { 'CAN_MANAGE': PermissionLevel.manage, 'CAN_RESTART': PermissionLevel.restart, @@ -106,32 +208,38 @@ class PermissionsLookup(object): 'group_name': PermissionType.group, 'service_principal_name': PermissionType.service, - 'clusters': PermissionTargets.clusters, - 'cluster': PermissionTargets.cluster, - 'directories': PermissionTargets.directories, - 'directory': PermissionTargets.directory, - 'instance-pools': PermissionTargets.instance_pools, - 'instance_pools': PermissionTargets.instance_pool, - 'jobs': PermissionTargets.jobs, - 'job': PermissionTargets.job, - 'notebooks': PermissionTargets.notebooks, - 'notebook': PermissionTargets.notebook, - 'registered-models': PermissionTargets.registered_models, - 'registered_models': PermissionTargets.registered_model, - 'model': PermissionTargets.model, - 'models': PermissionTargets.models, + 'clusters': ClusterPermissions(), + 'cluster': ClusterPermissions(), + 'directories': DirectoryPermissions(), + 'directory': DirectoryPermissions(), + 'instance-pools': InstancePoolPermissions(), + 'instance_pools': InstancePoolPermissions(), + 'jobs': JobPermissions(), + 'job': JobPermissions(), + 'notebooks': NotebookPermissions(), + 'notebook': NotebookPermissions(), + 'registered-models': MlFlowPermissions(), + 'registered_models': MlFlowPermissions(), + 'model': MlFlowPermissions(), + 'models': MlFlowPermissions(), } class Permission(object): - def __init__(self, permission_type, value, permission_level): - # type: (PermissionType, str, PermissionLevel) -> None - self.permission_type = permission_type - self.permission_level = permission_level - if value is None: - value = '' + def __init__(self, object_type, permission_type, permission_level, permission_value): + # type: (str, PermissionType, str, str) -> None + self.validator = PermissionsLookup.items[object_type] - self.value = value + if not self.validator.is_valid_target(permission_level): + raise PermissionsError( + '{} is not a valid target for {}\n'.format(permission_level, + self.validator.object_type) + + + 'Valid values are {}'.format(self.validator.valid_targets())) + + self.permission_type = permission_type + self.permission_level = PermissionLevel[permission_level] + self.value = permission_value def to_dict(self): # type: () -> dict @@ -175,6 +283,12 @@ def to_dict(self): 'access_control_list': [entry.to_dict() for entry in self.permissions] } + def check_if_valid_for(self, object_type): + """ + Check if the permissions are valid for this object type. + """ + pass + # FIXME: add set/update permissions, right now this is read only. class PermissionsApi(object): diff --git a/databricks_cli/permissions/cli.py b/databricks_cli/permissions/cli.py index 11109948..5601cf2b 100644 --- a/databricks_cli/permissions/cli.py +++ b/databricks_cli/permissions/cli.py @@ -109,9 +109,11 @@ def add_cli(api_client, object_type, object_id, user_name, group_name, service_n # this else/raise is for readability when doing a review. raise UsageError('Invalid argument') - permission = Permission(perm_type, value, PermissionLevel[permission_level]) + permission = Permission(object_type, perm_type, permission_level, value) all_permissions = PermissionsObject([permission]) + all_permissions.check_if_valid_for(object_type) + click.echo(pretty_format(perms_api.add_permissions(object_type, object_id, all_permissions)))