From 8c60aa3b752dee4bf556d7f88457b37bddc7d380 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 4 Jul 2024 10:03:54 +0100 Subject: [PATCH 1/8] tidy: cylc/uiserver/authorise.py * Blacken code. * Put permissions in order. * Add comments. * Use Google-style docstrings. --- cylc/uiserver/authorise.py | 233 ++++++++++++++++---------- cylc/uiserver/tests/test_authorise.py | 82 ++++----- 2 files changed, 174 insertions(+), 141 deletions(-) diff --git a/cylc/uiserver/authorise.py b/cylc/uiserver/authorise.py index 858ca369..aec858e2 100644 --- a/cylc/uiserver/authorise.py +++ b/cylc/uiserver/authorise.py @@ -17,10 +17,10 @@ from functools import lru_cache from getpass import getuser import grp -from typing import List, Dict, Optional, Union, Any, Sequence, Set, Tuple from inspect import iscoroutinefunction import os import re +from typing import List, Optional, Union, Any, Sequence, Set, Tuple import graphene from jupyter_server.auth import Authorizer @@ -118,13 +118,16 @@ def fget(): class Authorization: - """Authorization Information Class - One instance for the life of the UI Server. If authorization settings - change they will need to re-start the UI Server. + """Authorization configuration object. + + One instance of this class lives for the life of the UI Server. + + If authorization settings change the UI Server will need to be re-started + to pick them up. + Authorization has access groups: `READ`, `CONTROL`, `ALL` - along with their negations, `!READ`, `!CONTROL` and `!ALL` which indicate removal of the permission groups. - """ # config literals @@ -147,7 +150,6 @@ class Authorization: READ_OPERATION = "read" # Access group identifiers (used in config) - READ = "READ" CONTROL = "CONTROL" ALL = "ALL" @@ -156,23 +158,10 @@ class Authorization: NOT_ALL = "!ALL" # Access Groups - READ_OPS = {READ_OPERATION} ASYNC_OPS = {"query", "mutation"} READ_AUTH_OPS = {"query", "subscription"} - @staticmethod - @constant - def ALL_OPS() -> List[str]: - """ALL OPS constant, returns list of all mutations.""" - return get_list_of_mutations() - - @staticmethod - @constant - def CONTROL_OPS() -> List[str]: - """CONTROL OPS constant, returns list of all control mutations.""" - return get_list_of_mutations(control=True) - def __init__(self, owner, owner_auth_conf, site_auth_conf, log) -> None: self.owner = owner self.log = log @@ -189,6 +178,18 @@ def __init__(self, owner, owner_auth_conf, site_auth_conf, log) -> None: self._get_permitted_operations ) + @staticmethod + @constant + def ALL_OPS() -> List[str]: + """ALL OPS constant, returns list of all mutations.""" + return get_list_of_mutations() + + @staticmethod + @constant + def CONTROL_OPS() -> List[str]: + """CONTROL OPS constant, returns list of all control mutations.""" + return get_list_of_mutations(control=True) + @staticmethod def expand_and_process_access_groups(permission_set: set) -> set: """Process a permission set. @@ -200,36 +201,44 @@ def expand_and_process_access_groups(permission_set: set) -> set: permission_set: set of permissions Returns: - permission_set: processed permission set. + processed permission set. + """ + # Expand permission groups + # E.G. ALL -> ["read", "trigger", "broadcast", ...] for action_group, expansion in { + Authorization.READ: Authorization.READ_OPS, Authorization.CONTROL: Authorization.CONTROL_OPS.fget(), Authorization.ALL: Authorization.ALL_OPS.fget(), - Authorization.READ: Authorization.READ_OPS, }.items(): if action_group in permission_set: permission_set.remove(action_group) permission_set.update(expansion) - # Expand negated permissions + + # Expand negated permission groups + # E.G. !CONTROL -> ["!trigger", "!stop", "!pause", ...] for action_group, expansion in { - Authorization.NOT_CONTROL: [ - f"!{x}" for x in Authorization.CONTROL_OPS.fget()], - Authorization.NOT_ALL: [ - f"!{x}" for x in Authorization.ALL_OPS.fget()], - Authorization.NOT_READ: [ - f"!{x}" for x in Authorization.READ_OPS]}.items(): + Authorization.NOT_READ: [f"!{x}" for x in Authorization.READ_OPS], + Authorization.NOT_CONTROL: [ + f"!{x}" for x in Authorization.CONTROL_OPS.fget() + ], + Authorization.NOT_ALL: [ + f"!{x}" for x in Authorization.ALL_OPS.fget() + ], + }.items(): if action_group in permission_set: permission_set.remove(action_group) permission_set.update(expansion) + # Remove negated permissions remove = set() - for perm in permission_set: if perm.startswith("!"): remove.add(perm.lstrip("!")) remove.add(perm) permission_set.difference_update(remove) permission_set.discard("") + return permission_set @staticmethod @@ -241,29 +250,31 @@ def set_auth_conf(auth_conf: Union[LazyConfigValue, dict]) -> dict: Returns: Valid configuration dictionary + """ if isinstance(auth_conf, LazyConfigValue): return auth_conf.to_dict() return auth_conf def get_owner_site_limits_for_access_user( - self, access_user: Dict[str, Union[str, Sequence[Any]]] + self, access_user_name: str, access_user_groups: Sequence[Any] ) -> Set: """Returns limits owner can give to given access_user Args: - access_user: Dictionary containing info about access user and their - membership of system groups. + access_user_name: The username of the authenticated user. + access_user_groups: All groups the authenticated user belongs to. Returns: Set of limits that the uiserver owner is allowed to give away for given access user. + """ limits: Set[str] = set() if not self.owner_dict: return limits - items_to_check = ["*", access_user["access_username"]] - items_to_check.extend(access_user["access_user_groups"]) + items_to_check = ["*", access_user_name] + items_to_check.extend(access_user_groups) for item in items_to_check: permission: Union[str, List] = "" default = "" @@ -271,7 +282,8 @@ def get_owner_site_limits_for_access_user( default = self.owner_dict[item].get(Authorization.DEFAULT, "") with suppress(KeyError): permission = self.owner_dict[item].get( - Authorization.LIMIT, default) + Authorization.LIMIT, default + ) if permission == []: raise_auth_config_exception("site") if isinstance(permission, str): @@ -282,17 +294,18 @@ def get_owner_site_limits_for_access_user( return limits def get_access_user_permissions_from_owner_conf( - self, access_user: Dict[str, Union[str, Sequence[Any]]] + self, access_user_name: str, access_user_groups: Sequence[Any] ) -> set: """ Returns set of operations specific to access user from owner user conf. Args: - access_user: Dictionary containing info about access user and their - membership of system groups. Defaults to None. + access_user_name: The username of the authenticated user. + access_user_groups: All groups the authenticated user belongs to. + """ - items_to_check = ["*", access_user["access_username"]] - items_to_check.extend(access_user["access_user_groups"]) + items_to_check = ["*", access_user_name] + items_to_check.extend(access_user_groups) allowed_operations = set() for item in items_to_check: permission = self.owner_auth_conf.get(item, "") @@ -310,7 +323,8 @@ def get_access_user_permissions_from_owner_conf( def _get_permitted_operations(self, access_user: str): """Return permitted operations for given access_user. - Cached for efficiency. + This method is cached for efficiency. + Checks: - site config to ensure owner is permitted to give away permissions - user config for authorised operations related to access_user and @@ -322,39 +336,49 @@ def _get_permitted_operations(self, access_user: str): Returns: Set of operations permitted by given access user for this UI Server + """ - # For use in the ui, owner permissions (ALL operations) are set + # users have full access to their own server (ALL) if access_user == self.owner: return set(Authorization.ALL_OPS.fget()) - # Otherwise process permissions for (non-uiserver owner) access_user - access_user_dict = { - "access_username": access_user, - "access_user_groups": self._get_groups(access_user), - } + # all groups the authenticated user belongs to + access_user_groups = self._get_groups(access_user) + + # the maximum permissions the site permits the user to grant limits_owner_can_give = self.get_owner_site_limits_for_access_user( - access_user=access_user_dict) + access_user, access_user_groups + ) + + # the permissions the user wishes to grant user_conf_permitted_ops = ( self.get_access_user_permissions_from_owner_conf( - access_user=access_user_dict) + access_user, access_user_groups + ) ) - # If not explicit permissions for access user in owner conf then revert - # to site defaults + if len(user_conf_permitted_ops) == 0: + # the user has not specified the permissions they wish to grant + # -> fallback to the site defaults user_conf_permitted_ops = ( self.return_site_auth_defaults_for_access_user( - access_user=access_user_dict + access_user, access_user_groups ) ) + + # expand permission groups and remove negated permissions user_conf_permitted_ops = self.expand_and_process_access_groups( user_conf_permitted_ops ) limits_owner_can_give = self.expand_and_process_access_groups( limits_owner_can_give ) + + # subtract permissions that the site does not permit to be granted allowed_operations = limits_owner_can_give.intersection( user_conf_permitted_ops ) + self.log.info( f"User {access_user} authorized permissions: " f"{sorted(allowed_operations)}" @@ -371,6 +395,7 @@ def is_permitted(self, access_user: str, operation: str) -> bool: Returns: True if access_user permitted to action operation, otherwise, False. + """ if access_user == self.owner_user_info["user"]: return True @@ -386,8 +411,8 @@ def is_permitted(self, access_user: str, operation: str) -> bool: def build_owner_site_auth_conf(self): """Build UI Server owner permissions dictionary. - Creates a reduced site auth dictionary for the ui-server owner. + Creates a reduced site auth dictionary for the ui-server owner. """ owner_dict = {} items_to_check = ["*", self.owner_user_info["user"]] @@ -402,13 +427,17 @@ def build_owner_site_auth_conf(self): if existing_user_conf: # process limits and defaults and update dictionary existing_default = existing_user_conf.get( - Authorization.DEFAULT, '') + Authorization.DEFAULT, '' + ) existing_limit = existing_user_conf.get( - Authorization.LIMIT, existing_default) + Authorization.LIMIT, existing_default + ) new_default = acc_user_perms.get( - Authorization.DEFAULT, '') + Authorization.DEFAULT, '' + ) new_limit = acc_user_perms.get( - Authorization.LIMIT, new_default) + Authorization.LIMIT, new_default + ) set_defs = set() for conf in [existing_default, new_default]: if isinstance(conf, list): @@ -422,35 +451,35 @@ def build_owner_site_auth_conf(self): else: set_lims.add(conf) # update and continue - owner_dict[ - acc_user_conf][Authorization.LIMIT] = list( - set_lims) - owner_dict[ - acc_user_conf][Authorization.DEFAULT] = list( - set_defs) + owner_dict[acc_user_conf][Authorization.LIMIT] = list( + set_lims + ) + owner_dict[acc_user_conf][Authorization.DEFAULT] = ( + list(set_defs) + ) continue owner_dict.update(access_user_dict) # Now we have a reduced site auth dictionary for the current owner return owner_dict def return_site_auth_defaults_for_access_user( - self, access_user: Dict[str, Union[str, Sequence[Any]]] + self, access_user_name: str, access_user_groups: Sequence[Any] ) -> Set: """Return site authorization defaults for given access user. + Args: - access_user: access_user dictionary, in the form - {'access_username': username - 'access_user_group: [group1, group2,...]' - } + access_user_name: The username of the authenticated user. + access_user_groups: All groups the authenticated user belongs to. + Returns: - Set of default operations permitted + The set of default operations permitted. """ defaults: Set[str] = set() if not self.owner_dict: return defaults - items_to_check = ["*", access_user["access_username"]] - items_to_check.extend(access_user["access_user_groups"]) + items_to_check = ["*", access_user_name] + items_to_check.extend(access_user_groups) for item in items_to_check: permission: Union[str, List] = "" with suppress(KeyError): @@ -489,6 +518,7 @@ class AuthorizationMiddleware: Raises: web.HTTPError: Unauthorized requests. + """ auth = None @@ -502,8 +532,10 @@ def resolve(self, next_, root, info, **args): # It shouldn't get here but worth checking for zero trust if not op_name: self.auth_failed( - current_user, op_name, http_code=400, - msg="Operation not in schema." + current_user, + op_name, + http_code=400, + msg="Operation not in schema.", ) try: authorised = self.auth.is_permitted(current_user, op_name) @@ -512,15 +544,22 @@ def resolve(self, next_, root, info, **args): authorised = False if not authorised: self.auth_failed(current_user, op_name, http_code=403) - if (info.operation.operation in Authorization.ASYNC_OPS - or iscoroutinefunction(next_)): + if ( + info.operation.operation in Authorization.ASYNC_OPS + or iscoroutinefunction(next_) + ): return self.async_resolve(next_, root, info, **args) return next_(root, info, **args) - def auth_failed(self, current_user: str, op_name: str, - http_code: int, message: Optional[str] = None): - """ - Raise authorization error + def auth_failed( + self, + current_user: str, + op_name: str, + http_code: int, + message: Optional[str] = None, + ): + """Raise an authorization error. + Args: current_user: username accessing operation op_name: operation name @@ -529,31 +568,38 @@ def auth_failed(self, current_user: str, op_name: str, Raises: web.HTTPError + """ - log_message = (f"Authorization failed for {current_user}" - f":requested to {op_name}.") + log_message = ( + f"Authorization failed for {current_user}" + f":requested to {op_name}." + ) if message: log_message = log_message + " " + message raise web.HTTPError(http_code, reason=message) def get_op_name(self, field_name: str, operation: str) -> Optional[str]: - """ - Returns operation name required for authorization. + """Returns the operation name required for authorization. + Converts queries and subscriptions to read operations. + Args: field_name: Field name e.g. play operation: operation type Returns: - operation name + The operation name. + """ if operation in Authorization.READ_AUTH_OPS: return Authorization.READ_OPERATION else: # Check it is a mutation in our schema - if self.auth and re.sub( - r'(? Tuple[List[str], List[str]]: - """Return list of system groups for given user. + """Return a list of system groups for given user. Uses ``os.getgrouplist`` and ``os.NGROUPS_MAX`` to get system groups for a given user. ``grp.getgrgid`` then parses these to return a list of group @@ -573,7 +619,8 @@ def get_groups(username: str) -> Tuple[List[str], List[str]]: username: username used to check system groups. Returns: - list: system groups for username given + System groups for username given + """ groupmax = os.NGROUPS_MAX # type: ignore group_ids = os.getgrouplist(username, groupmax) @@ -589,8 +636,8 @@ def parse_group_ids(group_ids: List) -> Tuple[List[str], List[str]]: group_ids: List of users groups, in number format Returns: - List: List of users groups, in id format with group identifier - prepended. + List of users groups, in id format with group identifier prepended. + """ group_list = [] bad_group_list = [] @@ -609,7 +656,8 @@ def parse_group_ids(group_ids: List) -> Tuple[List[str], List[str]]: def get_list_of_mutations(control: bool = False) -> List[str]: """Gets list of mutations""" list_of_mutations = [ - attr for attr in dir(UISMutations) + attr + for attr in dir(UISMutations) if isinstance(getattr(UISMutations, attr), graphene.Field) ] if control: @@ -626,6 +674,7 @@ def raise_auth_config_exception(config_type: str): Args: config_type: Either site or user. + """ raise Exception( f'Error in {config_type} config: ' diff --git a/cylc/uiserver/tests/test_authorise.py b/cylc/uiserver/tests/test_authorise.py index 7122623d..df37331c 100644 --- a/cylc/uiserver/tests/test_authorise.py +++ b/cylc/uiserver/tests/test_authorise.py @@ -23,7 +23,7 @@ Authorization, AuthorizationMiddleware, get_list_of_mutations, - parse_group_ids + parse_group_ids, ) log = ExtensionApp().log @@ -86,9 +86,14 @@ }, }, "server_owner_1": { - "*": {"default": ["READ", "message"], - "limit": ["READ", "CONTROL"]}, - "user1": {"default": ["READ", "play", "pause"], "limit": ["ALL"]}, + "*": { + "default": ["READ", "message"], + "limit": ["READ", "CONTROL"], + }, + "user1": { + "default": ["READ", "play", "pause"], + "limit": ["ALL"], + }, }, "server_owner_2": { "user2": {"limit": "ALL"}, @@ -180,7 +185,7 @@ ["group:group2"], set(), id="owner only in group and *", - ) + ), ], ) @patch("cylc.uiserver.authorise.get_groups") @@ -193,9 +198,7 @@ def test_get_permitted_operations( user_groups, ): mocked_get_groups.side_effect = [(owner_groups, []), (user_groups, [])] - auth_obj = Authorization( - owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, log - ) + auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, log) actual_operations = auth_obj.get_permitted_operations( access_user=user_name ) @@ -203,14 +206,13 @@ def test_get_permitted_operations( @pytest.mark.parametrize( - "expected_operations, access_user_dict, owner_auth_conf,", + 'expected_operations, access_user_name,' + ' access_user_groups, owner_auth_conf,', [ pytest.param( {"!kill", "READ", "kill", "!stop", "pause", "play"}, - { - "access_username": "access_user_1", - "access_user_groups": ["group:group1", "group:group2"], - }, + "access_user_1", + ["group:group1", "group:group2"], { "*": ["READ", "!kill"], "access_user_1": ["READ", "pause", "kill", "play", "!stop"], @@ -219,10 +221,8 @@ def test_get_permitted_operations( ), pytest.param( {"READ"}, - { - "access_username": "access_user_2", - "access_user_groups": ["group:group1", "group:group2"], - }, + "access_user_2", + ["group:group1", "group:group2"], { "*": ["READ"], "access_user_1": ["READ", "pause", "kill", "play", "!stop"], @@ -231,10 +231,8 @@ def test_get_permitted_operations( ), pytest.param( {"pause", "kill", "!stop", "READ", "CONTROL"}, - { - "access_username": "access_user_1", - "access_user_groups": ["group:group1", "group:group2"], - }, + "access_user_1", + ["group:group1", "group:group2"], { "*": ["READ"], "access_user_1": ["READ", "pause", "kill", "!stop"], @@ -246,7 +244,11 @@ def test_get_permitted_operations( ) @patch("cylc.uiserver.authorise.get_groups") def test_get_access_user_permissions_from_owner_conf( - mocked_get_groups, expected_operations, access_user_dict, owner_auth_conf + mocked_get_groups, + expected_operations, + access_user_name, + access_user_groups, + owner_auth_conf, ): """Test the un-processed permissions of owner conf.""" mocked_get_groups.return_value = (["group:blah"], []) @@ -254,7 +256,7 @@ def test_get_access_user_permissions_from_owner_conf( "some_user", owner_auth_conf, {"fake": "config"}, log ) permitted_operations = authobj.get_access_user_permissions_from_owner_conf( - access_user_dict + access_user_name, access_user_groups ) assert permitted_operations == expected_operations @@ -284,7 +286,7 @@ def test_expand_and_process_access_groups(permission_set, expected): "some_user", {"fake": "config"}, {"fake": "config"}, - log + log, ) actual = authobj.expand_and_process_access_groups(permission_set) assert actual == expected @@ -321,8 +323,7 @@ def test_expand_and_process_access_groups(permission_set, expected): ) def test_get_op_name(mut_field_name, operation, expected_op_name): mock_authobj = Authorization( - "some_user", {"fake": "config"}, - {"fake": "config"}, log + "some_user", {"fake": "config"}, {"fake": "config"}, log ) auth_middleware = AuthorizationMiddleware auth_middleware.auth = mock_authobj @@ -336,11 +337,7 @@ def test_get_op_name(mut_field_name, operation, expected_op_name): "owner_name, user_name, get_permitted_operations_is_called, expected", [ pytest.param( - "mel", - "mel", - False, - True, - id="Owner user always permitted" + "mel", "mel", False, True, id="Owner user always permitted" ), pytest.param( "mel", @@ -383,17 +380,9 @@ def test_get_list_of_mutations(control, expected): assert set(actual) == set(expected) -@pytest.mark.parametrize( - 'input_', - ( - [123], - [123, 456], - [100, 123] - ) -) +@pytest.mark.parametrize('input_', ([123], [123, 456], [100, 123])) def test_parse_group_ids(monkeypatch, input_): - """Returns a list of group ids or groups where ID's haven't worked - """ + """Returns a list of group ids or groups where ID's haven't worked""" mock_grid_db = { 123: 'foo', 456: 'bar', @@ -403,11 +392,6 @@ def test_parse_group_ids(monkeypatch, input_): ) result = parse_group_ids(input_) assert result == ( - [ - f'group:{mock_grid_db[i]}' - for i in input_ if i in mock_grid_db - ], - [ - i for i in input_ if i not in mock_grid_db - ], - ) + [f'group:{mock_grid_db[i]}' for i in input_ if i in mock_grid_db], + [i for i in input_ if i not in mock_grid_db], + ) From 697f340df275e912faef539b87c682d5c54a55ea Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 4 Jul 2024 11:18:07 +0100 Subject: [PATCH 2/8] auth: remove constant decorator * This was doing what @property does by itself. * Because attrs were being accessed via the class rather than the instance, it wasn't offering any safety. * Switched to @property and accessing it via the instance. --- cylc/uiserver/authorise.py | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/cylc/uiserver/authorise.py b/cylc/uiserver/authorise.py index aec858e2..901d90f6 100644 --- a/cylc/uiserver/authorise.py +++ b/cylc/uiserver/authorise.py @@ -105,18 +105,6 @@ def is_authorized(self, handler, user, action, resource) -> bool: return False -def constant(func): - """Decorator preventing reassignment""" - - def fset(self, value): - raise TypeError - - def fget(): - return func() - - return property(fget, fset) - - class Authorization: """Authorization configuration object. @@ -178,20 +166,17 @@ def __init__(self, owner, owner_auth_conf, site_auth_conf, log) -> None: self._get_permitted_operations ) - @staticmethod - @constant - def ALL_OPS() -> List[str]: + @property + def ALL_OPS(self) -> List[str]: """ALL OPS constant, returns list of all mutations.""" return get_list_of_mutations() - @staticmethod - @constant - def CONTROL_OPS() -> List[str]: + @property + def CONTROL_OPS(self) -> List[str]: """CONTROL OPS constant, returns list of all control mutations.""" return get_list_of_mutations(control=True) - @staticmethod - def expand_and_process_access_groups(permission_set: set) -> set: + def expand_and_process_access_groups(self, permission_set: set) -> set: """Process a permission set. Takes a permission set, e.g. limits, defaults. @@ -208,8 +193,8 @@ def expand_and_process_access_groups(permission_set: set) -> set: # E.G. ALL -> ["read", "trigger", "broadcast", ...] for action_group, expansion in { Authorization.READ: Authorization.READ_OPS, - Authorization.CONTROL: Authorization.CONTROL_OPS.fget(), - Authorization.ALL: Authorization.ALL_OPS.fget(), + Authorization.CONTROL: self.CONTROL_OPS, + Authorization.ALL: self.ALL_OPS, }.items(): if action_group in permission_set: permission_set.remove(action_group) @@ -220,10 +205,10 @@ def expand_and_process_access_groups(permission_set: set) -> set: for action_group, expansion in { Authorization.NOT_READ: [f"!{x}" for x in Authorization.READ_OPS], Authorization.NOT_CONTROL: [ - f"!{x}" for x in Authorization.CONTROL_OPS.fget() + f"!{x}" for x in self.CONTROL_OPS ], Authorization.NOT_ALL: [ - f"!{x}" for x in Authorization.ALL_OPS.fget() + f"!{x}" for x in self.ALL_OPS ], }.items(): if action_group in permission_set: @@ -340,7 +325,7 @@ def _get_permitted_operations(self, access_user: str): """ # users have full access to their own server (ALL) if access_user == self.owner: - return set(Authorization.ALL_OPS.fget()) + return set(self.ALL_OPS) # all groups the authenticated user belongs to access_user_groups = self._get_groups(access_user) @@ -598,7 +583,7 @@ def get_op_name(self, field_name: str, operation: str) -> Optional[str]: if ( self.auth and re.sub(r'(? Date: Thu, 4 Jul 2024 11:34:23 +0100 Subject: [PATCH 3/8] auth: fix type hints --- cylc/uiserver/authorise.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cylc/uiserver/authorise.py b/cylc/uiserver/authorise.py index 901d90f6..511ff81d 100644 --- a/cylc/uiserver/authorise.py +++ b/cylc/uiserver/authorise.py @@ -20,7 +20,7 @@ from inspect import iscoroutinefunction import os import re -from typing import List, Optional, Union, Any, Sequence, Set, Tuple +from typing import List, Optional, Union, Set, Tuple import graphene from jupyter_server.auth import Authorizer @@ -242,7 +242,7 @@ def set_auth_conf(auth_conf: Union[LazyConfigValue, dict]) -> dict: return auth_conf def get_owner_site_limits_for_access_user( - self, access_user_name: str, access_user_groups: Sequence[Any] + self, access_user_name: str, access_user_groups: List[str] ) -> Set: """Returns limits owner can give to given access_user @@ -279,7 +279,7 @@ def get_owner_site_limits_for_access_user( return limits def get_access_user_permissions_from_owner_conf( - self, access_user_name: str, access_user_groups: Sequence[Any] + self, access_user_name: str, access_user_groups: List[str] ) -> set: """ Returns set of operations specific to access user from owner user conf. @@ -448,7 +448,7 @@ def build_owner_site_auth_conf(self): return owner_dict def return_site_auth_defaults_for_access_user( - self, access_user_name: str, access_user_groups: Sequence[Any] + self, access_user_name: str, access_user_groups: List[str] ) -> Set: """Return site authorization defaults for given access user. @@ -480,7 +480,7 @@ def return_site_auth_defaults_for_access_user( defaults.discard("") return defaults - def _get_groups(self, user: str) -> List: + def _get_groups(self, user: str) -> List[str]: """Allows get groups to use self.logger if something goes wrong. Added to provide a single interface for get_groups to this class, to From 2ca62e4decfd4b92c2806e75aa8a077bd2438602 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 4 Jul 2024 13:26:21 +0100 Subject: [PATCH 4/8] auth: convert config to dict on __init__ * Rationalise the configuration object type earlier. * This avoids managing multiple data types throughout the code. --- cylc/uiserver/app.py | 8 ++++---- cylc/uiserver/authorise.py | 37 +++++++++++++++++-------------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/cylc/uiserver/app.py b/cylc/uiserver/app.py index 8e9fb00c..a10665c1 100644 --- a/cylc/uiserver/app.py +++ b/cylc/uiserver/app.py @@ -542,15 +542,15 @@ def set_sub_server(self): auth=self.authobj, ) - def set_auth(self): + def set_auth(self) -> Authorization: """Create authorization object. One for the lifetime of the UIServer. """ return Authorization( getpass.getuser(), - self.config.CylcUIServer.user_authorization, - self.config.CylcUIServer.site_authorization, - self.log + self.config.CylcUIServer.user_authorization.to_dict(), + self.config.CylcUIServer.site_authorization.to_dict(), + self.log, ) def initialize_templates(self): diff --git a/cylc/uiserver/authorise.py b/cylc/uiserver/authorise.py index 511ff81d..af0b08d3 100644 --- a/cylc/uiserver/authorise.py +++ b/cylc/uiserver/authorise.py @@ -25,7 +25,6 @@ import graphene from jupyter_server.auth import Authorizer from tornado import web -from traitlets.config.loader import LazyConfigValue from cylc.uiserver.schema import UISMutations from cylc.uiserver.utils import is_bearer_token_authenticated @@ -116,6 +115,13 @@ class Authorization: Authorization has access groups: `READ`, `CONTROL`, `ALL` - along with their negations, `!READ`, `!CONTROL` and `!ALL` which indicate removal of the permission groups. + + Args: + owner: The server owner's user name. + owner_auth_conf: The server owner's authorization configuration. + site_auth_conf: The site's authorization configuration. + log: The application logger. + """ # config literals @@ -150,11 +156,17 @@ class Authorization: ASYNC_OPS = {"query", "mutation"} READ_AUTH_OPS = {"query", "subscription"} - def __init__(self, owner, owner_auth_conf, site_auth_conf, log) -> None: - self.owner = owner + def __init__( + self, + owner: str, + owner_auth_conf: dict, + site_auth_conf: dict, + log, + ): + self.owner: str = owner self.log = log - self.owner_auth_conf = self.set_auth_conf(owner_auth_conf) - self.site_auth_config = self.set_auth_conf(site_auth_conf) + self.owner_auth_conf: dict = owner_auth_conf + self.site_auth_config: dict = site_auth_conf self.owner_user_info = { "user": self.owner, "user_groups": self._get_groups(self.owner), @@ -226,21 +238,6 @@ def expand_and_process_access_groups(self, permission_set: set) -> set: return permission_set - @staticmethod - def set_auth_conf(auth_conf: Union[LazyConfigValue, dict]) -> dict: - """Resolve lazy config where empty - - Args: - auth_conf: Authorization configuration from a jupyter_config.py - - Returns: - Valid configuration dictionary - - """ - if isinstance(auth_conf, LazyConfigValue): - return auth_conf.to_dict() - return auth_conf - def get_owner_site_limits_for_access_user( self, access_user_name: str, access_user_groups: List[str] ) -> Set: From 26677dfa3d9acaa7e8a2d821fbb3735bced36c8e Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 4 Jul 2024 13:32:18 +0100 Subject: [PATCH 5/8] auth: turn a dictionary into attributes * Replace the `owner_user_info` dictionary (which contained two keys, one of which was duplicated) into variables. * This removes unnecessary dictionary lookups, the potential for key errors and makes typing easier. --- cylc/uiserver/authorise.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/cylc/uiserver/authorise.py b/cylc/uiserver/authorise.py index af0b08d3..cfcf74e8 100644 --- a/cylc/uiserver/authorise.py +++ b/cylc/uiserver/authorise.py @@ -158,19 +158,18 @@ class Authorization: def __init__( self, - owner: str, + owner_user_name: str, owner_auth_conf: dict, site_auth_conf: dict, log, ): - self.owner: str = owner + self.owner_user_name: str = owner_user_name + self.owner_user_groups: List[str] = self._get_groups( + self.owner_user_name + ) self.log = log self.owner_auth_conf: dict = owner_auth_conf self.site_auth_config: dict = site_auth_conf - self.owner_user_info = { - "user": self.owner, - "user_groups": self._get_groups(self.owner), - } self.owner_dict = self.build_owner_site_auth_conf() # lru_cache this method - see flake8-bugbear B019 @@ -321,7 +320,7 @@ def _get_permitted_operations(self, access_user: str): """ # users have full access to their own server (ALL) - if access_user == self.owner: + if access_user == self.owner_user_name: return set(self.ALL_OPS) # all groups the authenticated user belongs to @@ -379,7 +378,7 @@ def is_permitted(self, access_user: str, operation: str) -> bool: False. """ - if access_user == self.owner_user_info["user"]: + if access_user == self.owner_user_name: return True # re.sub needed for snake/camel case if re.sub( @@ -397,8 +396,8 @@ def build_owner_site_auth_conf(self): Creates a reduced site auth dictionary for the ui-server owner. """ owner_dict = {} - items_to_check = ["*", self.owner_user_info["user"]] - items_to_check.extend(self.owner_user_info["user_groups"]) + items_to_check = ["*", self.owner_user_name] + items_to_check.extend(self.owner_user_groups) # dict containing user info applying to the current ui_server owner for uis_owner_conf, access_user_dict in self.site_auth_config.items(): From 7b6455e1f17d8b343fa41dfce21563577feb5634 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 4 Jul 2024 14:06:40 +0100 Subject: [PATCH 6/8] auth: fill coverage holes --- cylc/uiserver/authorise.py | 2 +- cylc/uiserver/tests/test_authorise.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/cylc/uiserver/authorise.py b/cylc/uiserver/authorise.py index cfcf74e8..bebcbd87 100644 --- a/cylc/uiserver/authorise.py +++ b/cylc/uiserver/authorise.py @@ -239,7 +239,7 @@ def expand_and_process_access_groups(self, permission_set: set) -> set: def get_owner_site_limits_for_access_user( self, access_user_name: str, access_user_groups: List[str] - ) -> Set: + ) -> Set[str]: """Returns limits owner can give to given access_user Args: diff --git a/cylc/uiserver/tests/test_authorise.py b/cylc/uiserver/tests/test_authorise.py index df37331c..43e5e484 100644 --- a/cylc/uiserver/tests/test_authorise.py +++ b/cylc/uiserver/tests/test_authorise.py @@ -395,3 +395,17 @@ def test_parse_group_ids(monkeypatch, input_): [f'group:{mock_grid_db[i]}' for i in input_ if i in mock_grid_db], [i for i in input_ if i not in mock_grid_db], ) + + +def test_empty_configs(): + """Test the default permissions when no auth config is provided.""" + # blank site & user configs + auth_obj = Authorization('me', {}, {}, log) + # the owner_dict should be empty + assert auth_obj.owner_dict == {} + + # other users should not have any permissions + assert auth_obj._get_permitted_operations('someone-else') == set() + + # the owner should always have full permissions + assert auth_obj._get_permitted_operations('me') == set(ALL_OPS) From b4dce64445d70b2f762aba6389a22901afda28bd Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 4 Jul 2024 14:29:29 +0100 Subject: [PATCH 7/8] auth: switch to existing snake case conversion function * Also add tests covering the snake/camel case calling of the is_permitted interface. --- cylc/uiserver/authorise.py | 30 +++++++------- cylc/uiserver/tests/test_authorise.py | 56 +++++++++++++++++++++++---- 2 files changed, 64 insertions(+), 22 deletions(-) diff --git a/cylc/uiserver/authorise.py b/cylc/uiserver/authorise.py index bebcbd87..c45a1df5 100644 --- a/cylc/uiserver/authorise.py +++ b/cylc/uiserver/authorise.py @@ -19,7 +19,6 @@ import grp from inspect import iscoroutinefunction import os -import re from typing import List, Optional, Union, Set, Tuple import graphene @@ -29,6 +28,8 @@ from cylc.uiserver.schema import UISMutations from cylc.uiserver.utils import is_bearer_token_authenticated +from graphene.utils.str_converters import to_snake_case + class CylcAuthorizer(Authorizer): """Defines a safe default authorization policy for Jupyter Server. @@ -380,14 +381,15 @@ def is_permitted(self, access_user: str, operation: str) -> bool: """ if access_user == self.owner_user_name: return True - # re.sub needed for snake/camel case - if re.sub( - r'(? Optional[str]: """ if operation in Authorization.READ_AUTH_OPS: return Authorization.READ_OPERATION - else: - # Check it is a mutation in our schema - if ( - self.auth - and re.sub(r'(?. -from jupyter_server.extension.application import ExtensionApp +import logging from types import SimpleNamespace from unittest.mock import Mock, patch +from jupyter_server.extension.application import ExtensionApp + import pytest from cylc.uiserver.authorise import ( @@ -26,7 +28,7 @@ parse_group_ids, ) -log = ExtensionApp().log +LOG = ExtensionApp().log CONTROL_OPS = [ "clean", @@ -198,7 +200,7 @@ def test_get_permitted_operations( user_groups, ): mocked_get_groups.side_effect = [(owner_groups, []), (user_groups, [])] - auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, log) + auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, LOG) actual_operations = auth_obj.get_permitted_operations( access_user=user_name ) @@ -253,7 +255,7 @@ def test_get_access_user_permissions_from_owner_conf( """Test the un-processed permissions of owner conf.""" mocked_get_groups.return_value = (["group:blah"], []) authobj = Authorization( - "some_user", owner_auth_conf, {"fake": "config"}, log + "some_user", owner_auth_conf, {"fake": "config"}, LOG ) permitted_operations = authobj.get_access_user_permissions_from_owner_conf( access_user_name, access_user_groups @@ -286,7 +288,7 @@ def test_expand_and_process_access_groups(permission_set, expected): "some_user", {"fake": "config"}, {"fake": "config"}, - log, + LOG, ) actual = authobj.expand_and_process_access_groups(permission_set) assert actual == expected @@ -323,7 +325,7 @@ def test_expand_and_process_access_groups(permission_set, expected): ) def test_get_op_name(mut_field_name, operation, expected_op_name): mock_authobj = Authorization( - "some_user", {"fake": "config"}, {"fake": "config"}, log + "some_user", {"fake": "config"}, {"fake": "config"}, LOG ) auth_middleware = AuthorizationMiddleware auth_middleware.auth = mock_authobj @@ -357,7 +359,7 @@ def test_is_permitted( expected, ): mocked_get_groups.side_effect = [([""], []), ([""], [])] - auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, log) + auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, LOG) auth_obj.get_permitted_operations = Mock(return_value=["fake_operation"]) actual = auth_obj.is_permitted( access_user=user_name, operation="fake_operation" @@ -400,7 +402,7 @@ def test_parse_group_ids(monkeypatch, input_): def test_empty_configs(): """Test the default permissions when no auth config is provided.""" # blank site & user configs - auth_obj = Authorization('me', {}, {}, log) + auth_obj = Authorization('me', {}, {}, LOG) # the owner_dict should be empty assert auth_obj.owner_dict == {} @@ -409,3 +411,41 @@ def test_empty_configs(): # the owner should always have full permissions assert auth_obj._get_permitted_operations('me') == set(ALL_OPS) + + +def test_case_conversion(caplog): + """Test camel to snake case conversion of permission names.""" + my_log = logging.getLogger('test_case_conversion') + caplog.set_level(logging.INFO, logger=my_log.name) + + auth_obj = Authorization( + 'me', + {'other': ['CONTROL']}, + {'*': {'*': {'limit': ['ALL']}}}, + my_log, + ) + + # internal use case (perm provided in Python syntax) + assert auth_obj.is_permitted('other', 'release_hold_point') + assert auth_obj.is_permitted('other', 'set_graph_window_extent') + + # external use case (perm provided in GraphQL syntax) + assert auth_obj.is_permitted('other', 'ReleaseHoldPoint') + assert auth_obj.is_permitted('other', 'SetGraphWindowExtent') + + # invalid permission names + assert not auth_obj.is_permitted('other', 'no_such_permission') + assert not auth_obj.is_permitted('other', 'RELeaseHOLdPoint') + assert not auth_obj.is_permitted('other', 'SEtGRAPhWINDOwEXTENt') + assert not auth_obj.is_permitted('other', 'Release_Hold_Point') + + # calls should be logged + # (successfull call) + caplog.clear() + assert auth_obj.is_permitted('other', 'release_hold_point') + assert caplog.messages[-1] == 'other: authorized to release_hold_point' + + # (rejected call) + caplog.clear() + assert not auth_obj.is_permitted('other', 'broadcast') + assert caplog.messages[-1] == 'other: not authorized to broadcast' From 042f42e64101e674362e8d25ae038547f50b0de5 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 4 Jul 2024 15:14:33 +0100 Subject: [PATCH 8/8] auth: fill coverage hole --- cylc/uiserver/tests/test_authorise.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/cylc/uiserver/tests/test_authorise.py b/cylc/uiserver/tests/test_authorise.py index 6ed25dab..bb1da22b 100644 --- a/cylc/uiserver/tests/test_authorise.py +++ b/cylc/uiserver/tests/test_authorise.py @@ -449,3 +449,30 @@ def test_case_conversion(caplog): caplog.clear() assert not auth_obj.is_permitted('other', 'broadcast') assert caplog.messages[-1] == 'other: not authorized to broadcast' + + +def test_empty_list_in_config(): + """Test empty lists cause exceptions to be raised. + + Empty lists have an unclear meaning due to the inheritance of default + permissions so are not permitted. + + Note: + It would be nicer to validate this out at startup than forcing the + error at runtime. + """ + auth_obj = Authorization('me', {'other': []}, {}, LOG) + with pytest.raises(Exception, match='Error in user config'): + auth_obj.is_permitted('other', 'read') + + auth_obj = Authorization('me', {}, {'*': {'other': {'limit': []}}}, LOG) + with pytest.raises(Exception, match='Error in site config'): + auth_obj.is_permitted('other', 'read') + + auth_obj = Authorization('me', {}, {'*': {'other': {'default': []}}}, LOG) + with pytest.raises(Exception, match='Error in site config'): + auth_obj.is_permitted('other', 'read') + + auth_obj = Authorization('me', {}, {'*': {'me': {'default': []}}}, LOG) + with pytest.raises(Exception, match='Error in site config'): + auth_obj.return_site_auth_defaults_for_access_user('me', ['x'])