Skip to content

Commit

Permalink
Address Review Feedback from OS
Browse files Browse the repository at this point in the history
  • Loading branch information
@datamel
datamel committed Oct 18, 2021
1 parent 94254b5 commit de5dd1f
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 59 deletions.
7 changes: 2 additions & 5 deletions cylc/uiserver/app.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@
)
from cylc.uiserver.authorise import (
Authorization,
AuthorizationMiddleware,
get_list_of_mutations
AuthorizationMiddleware
)
from cylc.uiserver.data_store_mgr import DataStoreMgr
from cylc.uiserver.handlers import (
Expand Down Expand Up @@ -469,9 +468,7 @@ def set_auth(self):
return Authorization(
getpass.getuser(),
self.config.CylcUIServer.user_authorization,
self.config.CylcUIServer.site_authorization,
get_list_of_mutations(control=True),
get_list_of_mutations()
self.config.CylcUIServer.site_authorization
)

def initialize_templates(self):
Expand Down
58 changes: 39 additions & 19 deletions cylc/uiserver/authorise.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@
LOG = logging.getLogger(__name__)


def constant(func):
"""Decorator preventing reassignment"""
def fset(self, value):
raise TypeError

def fget():
return func()
return property(fget, fset)


class Authorization:
"""Authorization Information Class
One instance for the life of the UI Server. If authorization settings
Expand Down Expand Up @@ -77,27 +87,36 @@ class Authorization:
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,
control_ops,
all_ops
site_auth_conf
) -> None:
self.control_ops = control_ops
self.all_ops = all_ops
self.owner = owner
self.owner_auth_conf = self.set_auth_conf(owner_auth_conf)
self.site_auth_config = self.set_auth_conf(site_auth_conf)
self.owner_user_info = {
'user': self.owner,
'user_groups': get_groups(self.owner)}
self.owner_dict = self.build_owner_site_auth_conf()
self.get_permitted_operations = (
lru_cache(maxsize=128)(self._get_permitted_operations))

def expand_and_process_access_groups(self, permission_set: set) -> set:
@staticmethod
def expand_and_process_access_groups(permission_set: set) -> set:
"""Process a permission set.
Takes a permission set, e.g. limits, defaults.
Expand All @@ -110,8 +129,8 @@ def expand_and_process_access_groups(self, permission_set: set) -> set:
permission_set: processed permission set.
"""
for action_group, expansion in {
Authorization.CONTROL: self.control_ops,
Authorization.ALL: self.all_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:
Expand All @@ -120,9 +139,11 @@ def expand_and_process_access_groups(self, permission_set: set) -> set:
# Expand negated permissions
for action_group, expansion in {
Authorization.NOT_CONTROL: list(
map((lambda x: '!' + x), self.control_ops)),
map((lambda x: '!' + x),
Authorization.CONTROL_OPS.fget())
),
Authorization.NOT_ALL: list(
map((lambda x: '!' + x), self.all_ops)),
map((lambda x: '!' + x), Authorization.ALL_OPS.fget())),
Authorization.NOT_READ: list(
map((lambda x: '!' + x), Authorization.READ_OPS))}.items():
if action_group in permission_set:
Expand Down Expand Up @@ -210,7 +231,8 @@ def get_access_user_permissions_from_owner_conf(
allowed_operations.discard('')
return allowed_operations

def _get_permitted_operations(self, access_user: str):
@lru_cache(maxsize=128)
def get_permitted_operations(self, access_user: str):
"""Return permitted operations for given access_user.
Cached for efficiency.
Expand All @@ -228,7 +250,7 @@ def _get_permitted_operations(self, access_user: str):
"""
# For use in the ui, owner permissions (ALL operations) are set
if access_user == self.owner:
return self.expand_and_process_access_groups(set(self.all_ops))
return set(Authorization.ALL_OPS.fget())
# Otherwise process permissions for (non-uiserver owner) access_user

access_user_dict = {'access_username': access_user,
Expand Down Expand Up @@ -272,7 +294,6 @@ def is_permitted(
"""
if access_user == self.owner_user_info['user']:
return True
LOG.info(f'{access_user}: requested {operation}')
if str(operation) in self.get_permitted_operations(access_user):
LOG.info(f'{access_user}: authorized to {operation}')
return True
Expand All @@ -295,8 +316,7 @@ def build_owner_site_auth_conf(self):
if uis_owner_conf in items_to_check:
# acc_user = access_user
for acc_user_conf, acc_user_perms in access_user_dict.items():
with suppress(KeyError):
existing_user_conf = owner_dict.get(acc_user_conf)
existing_user_conf = owner_dict.get(acc_user_conf)
if existing_user_conf:
# process limits and defaults and update dictionary
existing_default = existing_user_conf.get(
Expand Down Expand Up @@ -431,7 +451,7 @@ def get_op_name(self, field_name: str, operation: str) -> Union[None, str]:
return Authorization.READ_OPERATION
else:
# Check it is a mutation in our schema
if self.auth and field_name in self.auth.all_ops:
if self.auth and field_name in Authorization.ALL_OPS.fget():
return field_name
return None

Expand Down Expand Up @@ -479,7 +499,7 @@ def parse_group_ids(group_ids: List) -> List:
def get_list_of_mutations(control=False) -> List[str]:
"""Gets list of mutations"""
list_of_mutations = [
attr.replace('_', '') for attr in dir(UISMutations)
attr for attr in dir(UISMutations)
if isinstance(getattr(UISMutations, attr), graphene.Field)
]
if control:
Expand Down
64 changes: 29 additions & 35 deletions cylc/uiserver/tests/test_authorise.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,46 +24,46 @@
)

CONTROL_OPS = [
"exttrigger",
"pause",
"ext_trigger",
"hold",
"kill",
"message",
"hold",
"pause",
"play",
"poll",
"release",
"releaseholdpoint",
"release_hold_point",
"reload",
"remove",
"resume",
"setgraphwindowextent",
"setholdpoint",
"setoutputs",
"setverbosity",
"set_graph_window_extent",
"set_hold_point",
"set_outputs",
"set_verbosity",
"stop",
"trigger",
]

ALL_OPS = [
"ping",
"read",
"broadcast",
"exttrigger",
"pause",
"ext_trigger",
"hold",
"kill",
"message",
"hold",
"pause",
"ping",
"play",
"poll",
"release",
"releaseholdpoint",
"release_hold_point",
"reload",
"remove",
"resume",
"setgraphwindowextent",
"setholdpoint",
"setoutputs",
"setverbosity",
"set_graph_window_extent",
"set_hold_point",
"set_outputs",
"set_verbosity",
"stop",
"trigger",
]
Expand Down Expand Up @@ -120,19 +120,19 @@
"play",
"trigger",
"resume",
"setverbosity",
"setgraphwindowextent",
"set_verbosity",
"set_graph_window_extent",
"ping",
"read",
"poll",
"hold",
"remove",
"setholdpoint",
"releaseholdpoint",
"exttrigger",
"set_hold_point",
"release_hold_point",
"ext_trigger",
"pause",
"pause",
"setoutputs",
"set_outputs",
"release",
},
"server_owner_2",
Expand Down Expand Up @@ -186,7 +186,7 @@ def test_get_permitted_operations(
):
mocked_get_groups.side_effect = [owner_groups, user_groups]
auth_obj = Authorization(
owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, CONTROL_OPS, ALL_OPS
owner_name, FAKE_USER_CONF, FAKE_SITE_CONF
)
actual_operations = auth_obj.get_permitted_operations(
access_user=user_name
Expand Down Expand Up @@ -243,7 +243,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"}, CONTROL_OPS, ALL_OPS
"some_user", owner_auth_conf, {"fake": "config"}
)
permitted_operations = authobj.get_access_user_permissions_from_owner_conf(
access_user_dict
Expand Down Expand Up @@ -275,9 +275,7 @@ def test_expand_and_process_access_groups(permission_set, expected):
authobj = Authorization(
"some_user",
{"fake": "config"},
{"fake": "config"},
CONTROL_OPS,
ALL_OPS
{"fake": "config"}
)
actual = authobj.expand_and_process_access_groups(permission_set)
assert actual == expected
Expand Down Expand Up @@ -315,9 +313,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"},
CONTROL_OPS,
ALL_OPS
{"fake": "config"}
)
auth_middleware = AuthorizationMiddleware
auth_middleware.auth = mock_authobj
Expand Down Expand Up @@ -346,7 +342,7 @@ def test_get_op_name(mut_field_name, operation, expected_op_name):
),
],
)
@patch("cylc.uiserver.authorise.Authorization._get_permitted_operations")
@patch("cylc.uiserver.authorise.Authorization.get_permitted_operations")
@patch("cylc.uiserver.authorise.get_groups")
def test_is_permitted(
mocked_get_groups,
Expand All @@ -358,9 +354,7 @@ def test_is_permitted(
):
mocked_get_groups.side_effect = [[""], [""]]
mocked_get_permitted_operations.return_value = ["fake_operation"]
auth_obj = Authorization(
owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, CONTROL_OPS, ALL_OPS
)
auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF)
actual = auth_obj.is_permitted(
access_user=user_name, operation="fake_operation"
)
Expand Down

0 comments on commit de5dd1f

Please sign in to comment.