From ee80ef61a832f5f2d21ddc1ee40282c4010b6416 Mon Sep 17 00:00:00 2001
From: Sean Hammond <seanh@users.noreply.github.com>
Date: Tue, 19 Nov 2024 14:44:51 +0000
Subject: [PATCH] Never delete multiple GroupMemberships

`GroupMembersService.member_leave()` is written in a way that's intended
to work if the user has multiple memberships in the same group:

        matching_memberships = self.db.scalars(
            select(GroupMembership)
            .where(GroupMembership.group == group)
            .where(GroupMembership.user == user)
        ).all()

        if not matching_memberships:
            return

        for membership in matching_memberships:
            self.db.delete(membership)

In fact a user can't have more than one membership in the same group
(there's a unique constraint `(user_id,group_id)`) so this makes no
sense, and it's not possible for tests to cover the case of deleting
multiple memberships because tests can't add multiple memberships to the
DB.

The code was written this way in case we one day decide to remove the
constraint and allow a single user to have multiple memberships in the
same group. But it seems unlikely that we'd ever remove that constraint.
For example if in future we want a single user to be able to have
multiple roles in the same group we'd likely model that as a single
membership with multiple roles.

And this attempt to anticipate a possible future where multiple
memberships were possible enabled a bug to slip in, see:
https://github.com/hypothesis/h/pull/9080

Refactor the code to assume no more than one membership so that it can
never accidentally delete multiple memberships.
---
 h/services/group_members.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/h/services/group_members.py b/h/services/group_members.py
index f47c7e386c4..e0d30c000ff 100644
--- a/h/services/group_members.py
+++ b/h/services/group_members.py
@@ -78,17 +78,16 @@ def member_leave(self, group, userid):
         """Remove `userid` from the member list of `group`."""
         user = self.user_fetcher(userid)
 
-        matching_memberships = self.db.scalars(
+        membership = self.db.scalars(
             select(GroupMembership)
             .where(GroupMembership.group == group)
             .where(GroupMembership.user == user)
-        ).all()
+        ).one_or_none()
 
-        if not matching_memberships:
+        if not membership:
             return
 
-        for membership in matching_memberships:
-            self.db.delete(membership)
+        self.db.delete(membership)
 
         self.publish("group-leave", group.pubid, userid)