Skip to content

Commit

Permalink
Never delete multiple GroupMemberships
Browse files Browse the repository at this point in the history
`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:
#9080

Refactor the code to assume no more than one membership so that it can
never accidentally delete multiple memberships.
  • Loading branch information
seanh committed Nov 21, 2024
1 parent fb59a5b commit ee80ef6
Showing 1 changed file with 4 additions and 5 deletions.
9 changes: 4 additions & 5 deletions h/services/group_members.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down

0 comments on commit ee80ef6

Please sign in to comment.