Have authorise handle key errors when getting group names from ids (#272

)

* Have authorise handle key errors when getting group names from ids

* doc change to quash error in Cylc doc

CHANGES.md

@@ -32,6 +32,10 @@ Fix traceback which could appear when workflows are removed.
 [#241](https://github.com/cylc/cylc-uiserver/pull/241) -
 Update old, broken hold options for playing workflows.
 
+[#272](https://github.com/cylc/cylc-uiserver/pull/272) -
+Allowed broken entries in the group id database to be
+ignored and logged without causing total failure.
+
 -------------------------------------------------------------------------------
 ## __cylc-uiserver-0.5.0 (<span actions:bind='release-date'>Released 2021-07-28</span>)__
 

cylc/uiserver/app.py

@@ -473,7 +473,8 @@ def set_auth(self):
         return Authorization(
             getpass.getuser(),
             self.config.CylcUIServer.user_authorization,
-            self.config.CylcUIServer.site_authorization
+            self.config.CylcUIServer.site_authorization,
+            self.log
         )
 
     def initialize_templates(self):

cylc/uiserver/authorise.py

@@ -17,9 +17,8 @@
 from functools import lru_cache
 import graphene
 import grp
-from typing import List, Dict, Optional, Union, Any, Sequence, Set
+from typing import List, Dict, Optional, Union, Any, Sequence, Set, Tuple
 from inspect import iscoroutinefunction
-import logging
 import os
 import re
 from tornado import web
@@ -28,8 +27,6 @@
 
 from cylc.uiserver.schema import UISMutations
 
-LOG = logging.getLogger(__name__)
-
 
 def constant(func):
     """Decorator preventing reassignment"""
@@ -99,13 +96,14 @@ 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) -> None:
+    def __init__(self, owner, owner_auth_conf, site_auth_conf, log) -> None:
         self.owner = 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_user_info = {
             "user": self.owner,
-            "user_groups": get_groups(self.owner),
+            "user_groups": self._get_groups(self.owner),
         }
         self.owner_dict = self.build_owner_site_auth_conf()
 
@@ -251,7 +249,7 @@ def get_permitted_operations(self, access_user: str):
 
         access_user_dict = {
             "access_username": access_user,
-            "access_user_groups": get_groups(access_user),
+            "access_user_groups": self._get_groups(access_user),
         }
         limits_owner_can_give = self.get_owner_site_limits_for_access_user(
             access_user=access_user_dict)
@@ -276,7 +274,7 @@ def get_permitted_operations(self, access_user: str):
         allowed_operations = limits_owner_can_give.intersection(
             user_conf_permitted_ops
         )
-        LOG.info(
+        self.log.info(
             f"User {access_user} authorized permissions: "
             f"{sorted(allowed_operations)}"
         )
@@ -299,9 +297,9 @@ def is_permitted(self, access_user: str, operation: str) -> Bool:
         if re.sub(
             r'(?<!^)(?=[A-Z])', '_', operation
                 ).lower() in self.get_permitted_operations(access_user):
-            LOG.info(f"{access_user}: authorized to {operation}")
+            self.log.info(f"{access_user}: authorized to {operation}")
             return True
-        LOG.info(f"{access_user}: not authorized to {operation}")
+        self.log.info(f"{access_user}: not authorized to {operation}")
 
         return False
 
@@ -387,6 +385,20 @@ def return_site_auth_defaults_for_access_user(
         defaults.discard("")
         return defaults
 
+    def _get_groups(self, user: str) -> List:
+        """Allows get groups to use self.logger if something goes wrong.
+
+        Added to provide a single interface for get_groups to this class, to
+        avoid having to pass the logger to get_groups (and methods it calls).
+        """
+        good_groups, bad_groups = get_groups(user)
+        if bad_groups:
+            self.log.warning(
+                f'{user} has the following invalid groups in their profile '
+                f'{bad_groups} - these groups will be ignored.'
+            )
+        return good_groups
+
 
 # GraphQL middleware
 class AuthorizationMiddleware:
@@ -422,8 +434,7 @@ def resolve(self, next_, root, info, **args):
             return self.async_resolve(next_, root, info, **args)
         return next_(root, info, **args)
 
-    @staticmethod
-    def auth_failed(current_user: str, op_name: str,
+    def auth_failed(self, current_user: str, op_name: str,
                     http_code: int, message: Optional[str] = None):
         """
         Raise authorization error
@@ -440,7 +451,7 @@ def auth_failed(current_user: str, op_name: str,
                        f":requested to {op_name}.")
         if message:
             log_message = log_message + " " + message
-        LOG.warning(log_message)
+        Authorization.log.warning(log_message)
         raise web.HTTPError(http_code, reason=message)
 
     def get_op_name(self, field_name: str, operation: str) -> Union[None, str]:
@@ -469,11 +480,11 @@ async def async_resolve(self, next_, root, info, **args):
         return await next_(root, info, **args)
 
 
-def get_groups(username: str) -> List[str]:
+def get_groups(username: str) -> Tuple[List[str], List[str]]:
     """Return 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
+    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
     names.
 
     Args:
@@ -489,7 +500,7 @@ def get_groups(username: str) -> List[str]:
     return parse_group_ids(group_ids)
 
 
-def parse_group_ids(group_ids: List) -> List:
+def parse_group_ids(group_ids: List) -> Tuple[List[str], List[str]]:
     """Returns list of groups in the correct format for authorisation.
 
     Args:
@@ -500,14 +511,17 @@ def parse_group_ids(group_ids: List) -> List:
         prepended.
     """
     group_list = []
+    bad_group_list = []
     for x in group_ids:
         try:
             group_list.append(
                 f"{Authorization.GRP_IDENTIFIER}{grp.getgrgid(x).gr_name}"
             )
         except OverflowError:
             continue
-    return group_list
+        except KeyError:
+            bad_group_list.append(x)
+    return group_list, bad_group_list
 
 
 def get_list_of_mutations(control: bool = False) -> List[str]:

cylc/uiserver/tests/test_authorise.py

@@ -13,15 +13,21 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+# from logging import Logger as log
+from types import SimpleNamespace
+from jupyter_server.extension.application import ExtensionApp
 import pytest
 from unittest.mock import patch
 
 from cylc.uiserver.authorise import (
     Authorization,
     AuthorizationMiddleware,
     get_list_of_mutations,
+    parse_group_ids
 )
 
+log = ExtensionApp().log
+
 CONTROL_OPS = [
     "ext_trigger",
     "hold",
@@ -180,9 +186,9 @@ def test_get_permitted_operations(
     user_name,
     user_groups,
 ):
-    mocked_get_groups.side_effect = [owner_groups, user_groups]
+    mocked_get_groups.side_effect = [(owner_groups, []), (user_groups, [])]
     auth_obj = Authorization(
-        owner_name, FAKE_USER_CONF, FAKE_SITE_CONF
+        owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, log
     )
     actual_operations = auth_obj.get_permitted_operations(
         access_user=user_name
@@ -237,9 +243,9 @@ def test_get_access_user_permissions_from_owner_conf(
     mocked_get_groups, expected_operations, access_user_dict, owner_auth_conf
 ):
     """Test the un-processed permissions of owner conf."""
-    mocked_get_groups.return_value = ["group:blah"]
+    mocked_get_groups.return_value = (["group:blah"], [])
     authobj = Authorization(
-        "some_user", owner_auth_conf, {"fake": "config"}
+        "some_user", owner_auth_conf, {"fake": "config"}, log
     )
     permitted_operations = authobj.get_access_user_permissions_from_owner_conf(
         access_user_dict
@@ -271,7 +277,8 @@ def test_expand_and_process_access_groups(permission_set, expected):
     authobj = Authorization(
         "some_user",
         {"fake": "config"},
-        {"fake": "config"}
+        {"fake": "config"},
+        log
     )
     actual = authobj.expand_and_process_access_groups(permission_set)
     assert actual == expected
@@ -309,7 +316,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"}
+        {"fake": "config"}, log
     )
     auth_middleware = AuthorizationMiddleware
     auth_middleware.auth = mock_authobj
@@ -348,9 +355,9 @@ def test_is_permitted(
     get_permitted_operations_is_called,
     expected,
 ):
-    mocked_get_groups.side_effect = [[""], [""]]
+    mocked_get_groups.side_effect = [([""], []), ([""], [])]
     mocked_get_permitted_operations.return_value = ["fake_operation"]
-    auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF)
+    auth_obj = Authorization(owner_name, FAKE_USER_CONF, FAKE_SITE_CONF, log)
     actual = auth_obj.is_permitted(
         access_user=user_name, operation="fake_operation"
     )
@@ -370,3 +377,33 @@ def test_get_list_of_mutations(control, expected):
     """Test ALL_OPS are returned"""
     actual = get_list_of_mutations(control=control)
     assert set(actual) == set(expected)
+
+
+@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
+    """
+    mock_grid_db = {
+        123: 'foo',
+        456: 'bar',
+    }
+    monkeypatch.setattr(
+        'grp.getgrgid', lambda x: SimpleNamespace(gr_name=mock_grid_db[x])
+    )
+    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
+        ],
+        )