Deduplicate some code

h/security/identity.py

@@ -3,7 +3,7 @@
 from dataclasses import dataclass, field
 from typing import List, Optional, Self
 
-from h.models import AuthClient, Group, User
+from h.models import AuthClient, Group, GroupMembershipRoles, User
 
 
 @dataclass
@@ -126,3 +126,14 @@ def authenticated_userid(identity: Self | None) -> str | None:
             return identity.user.userid
 
         return None
+
+    def get_roles(self, group) -> list[GroupMembershipRoles]:
+        """Return this identity's roles in `group`."""
+        if self.user is None:
+            return []
+
+        for membership in self.user.memberships:
+            if membership.group.id == group.id:
+                return membership.roles
+
+        return []

h/security/predicates.py

@@ -184,17 +184,9 @@ def group_matches_authenticated_client_authority(identity, context):
 
 @requires(authenticated_user, group_found)
 def group_member_remove(identity, context: GroupMembershipContext):
-    def get_authenticated_users_membership():
-        """Return the authenticated user's membership of the target group."""
-        for membership in identity.user.memberships:
-            if membership.group.id == context.group.id:
-                return membership
+    authenticated_users_roles = identity.get_roles(context.group)
 
-        return None
-
-    authenticated_users_membership = get_authenticated_users_membership()
-
-    if not authenticated_users_membership:
+    if not authenticated_users_roles:
         # You can't remove anyone from a group you're not a member of.
         return False
 
@@ -204,20 +196,19 @@ def get_authenticated_users_membership():
 
     if "owner" in context.membership.roles or "admin" in context.membership.roles:
         # Only owners can remove admins or other owners.
-        return "owner" in authenticated_users_membership.roles
+        return "owner" in authenticated_users_roles
 
     if "moderator" in context.membership.roles:
         # Owners and admins can remove moderators.
         return (
-            "owner" in authenticated_users_membership.roles
-            or "admin" in authenticated_users_membership.roles
+            "owner" in authenticated_users_roles or "admin" in authenticated_users_roles
         )
 
     # Owners, admins and moderators can remove plain members.
     return (
-        "owner" in authenticated_users_membership.roles
-        or "admin" in authenticated_users_membership.roles
-        or "moderator" in authenticated_users_membership.roles
+        "owner" in authenticated_users_roles
+        or "admin" in authenticated_users_roles
+        or "moderator" in authenticated_users_roles
     )
 
 
@@ -227,15 +218,7 @@ def group_member_add(identity, context: AddGroupMembershipContext):
         context.new_roles is not None
     ), "new_roles must be set before checking permissions"
 
-    def get_authenticated_users_roles():
-        """Return the authenticated users roles in the target group."""
-        for membership in identity.user.memberships:
-            if membership.group.id == context.group.id:
-                return membership.roles
-
-        return []
-
-    authenticated_users_roles = get_authenticated_users_roles()
+    authenticated_users_roles = identity.get_roles(context.group)
 
     if GroupMembershipRoles.OWNER in authenticated_users_roles:
         # Owners can add members with any roles.
@@ -254,23 +237,15 @@ def get_authenticated_users_roles():
 @requires(authenticated_user, group_found)
 def group_member_edit(
     identity, context: EditGroupMembershipContext
-):  # pylint:disable=too-many-return-statements,too-complex
+):  # pylint:disable=too-many-return-statements
     assert (
         context.new_roles is not None
     ), "new_roles must be set before checking permissions"
 
     old_roles = context.membership.roles
     new_roles = context.new_roles
 
-    def get_authenticated_users_roles():
-        """Return the authenticated users roles in the target group."""
-        for membership in identity.user.memberships:
-            if membership.group.id == context.group.id:
-                return membership.roles
-
-        return None
-
-    authenticated_users_roles = get_authenticated_users_roles()
+    authenticated_users_roles = identity.get_roles(context.group)
 
     if not authenticated_users_roles:
         return False

tests/unit/h/security/identity_test.py

@@ -136,3 +136,46 @@ def LongLivedUser(self, patch):
     @pytest.fixture(autouse=True)
     def LongLivedAuthClient(self, patch):
         return patch("h.security.identity.LongLivedAuthClient")
+
+
+class TestGetRoles:
+    def test_it(self, identity, group):
+        identity.user.memberships.append(
+            LongLivedMembership(
+                user=identity.user, group=group, roles=[GroupMembershipRoles.MODERATOR]
+            ),
+        )
+
+        assert identity.get_roles(group) == [GroupMembershipRoles.MODERATOR]
+
+    def test_when_no_membership(self, identity, group):
+        assert identity.get_roles(group) == []
+
+    def test_when_no_user(self, identity, group):
+        identity.user = None
+
+        assert identity.get_roles(group) == []
+
+    @pytest.fixture
+    def identity(self):
+        identity = Identity(
+            user=LongLivedUser(
+                id=sentinel.id,
+                userid=sentinel.userid,
+                authority=sentinel.authority,
+                staff=sentinel.staff,
+                admin=sentinel.admin,
+            )
+        )
+        identity.user.memberships = [
+            LongLivedMembership(
+                user=identity.user,
+                group=LongLivedGroup(id=24, pubid="other"),
+                roles=[GroupMembershipRoles.MEMBER],
+            )
+        ]
+        return identity
+
+    @pytest.fixture
+    def group(self):
+        return LongLivedGroup(id=42, pubid="pubid")