Allow owners, admins and moderators to edit groups #9064

seanh · 2024-11-01T16:24:42Z

Permit owners, admins and moderators, rather than the group's creator, to edit a group. The creator can't edit the group unless they happen to also be an owner, admin or moderator (creators of groups will be owners unless they've subsequently left the group or changed roles).

This allows owners, admins and moderators to call the edit-group API, to see the "edit group" link on the group's page, and to view the edit-group page.

Testing

Create a group and visit the group's page. As the owner of the group you should see the Edit group link. Click on the link and check that you can edit the group.
Log out and visit the group's edit page to test that an unauthenticated user can't edit a group: you should get a 404.
Log in as another user and visit the group's edit page to check that a non-member can't edit a group.
Join the group and visit the group's edit page to check that a plain member can't edit a group.
Promote the user to moderator or admin and check that now they can edit the group.

This has to be done in SQL, for example:
```
update user_group set roles = '["admin"]'::jsonb where id = 42;
```

Change `factories.Group()` to *not* automatically add the group's creator as a member of the group. Future commits need to replace the `group.members` relation with a `group.memberships` relation (which is a list of `GroupMembership`'s rather than a list of `User`'s). See <#9047>. This is necessary because `GroupMembership`'s will in future have additional attributes (e.g. `roles`) and to add a user to a group with a particular role it'll be necessary to append a `GroupMembership` with that role to `group.memberships`, it's not enough to append a `User` to `group.members` because the role is an attribute of the membership not an attribute of the user, so we need to actually create a `GroupMembership` with the desired role and append that. With this change it'll no longer be possible for `factories.Group`'s `add_creator_as_member()` to add the creator as a member. For example this kind of thing won't work: @factory.post_generation def add_creator_as_member( # pylint:disable=no-self-argument obj, _create, _extracted, **_kwargs ): if ( obj.creator and obj.creator not in obj.members ): obj.memberships.append( models.GroupMembership( group=obj, user=obj.creator, role="owner ) ) The problem is that the `GroupMembership` that's been appended will not have been added to the DB session, which causes this SQLAlchemy error: https://docs.sqlalchemy.org/en/20/errors.html#object-is-being-merged-into-a-session-along-the-backref-cascade Or alternatively you get a `NotNullViolation`, depending. Nor can `factories.Group.add_creator_as_member()` simply add the `GroupMembership` to the DB session: it doesn't have access to the DB session (and this wouldn't necessarily get around `NotNullViolation`'s anyway). Removing this feels like a good direction to me because `add_creator_as_member()` seems too clever for a test factory, and my experience with test factories is that having them do extra things like this automatically usually ends up creating problems and it's better to keep the factories simpler and just make certain tests do more work. There looks to have been a bunch of tests that were implicitly or explicitly relying on the fact that the factory adds the group's creator as a member, even when this concept is irrelevant to the test at hand. So I think removing this is a good thing. The current behavior is also potentially confusing when you do something like `factories.Group(members=[...])` and then it auto-generates a user to be the group's `creator` and adds them to the group's members even though that user wasn't in the members list that was passed in.

seanh · 2024-11-01T16:48:20Z

h/models/group.py

+    def get_members(self, role: GroupMembershipRoles | None = None) -> tuple[User, ...]:
+        """Return a tuple of this group's members."""
+        if role:
+            memberships = [
+                membership
+                for membership in self.memberships
+                if role in membership.roles
+            ]
+        else:
+            memberships = self.memberships
+
+        return tuple(membership.user for membership in memberships)


I considered having three separate @property's moderators, admins and owners. These would be easier to use (just group.owners instead of group.get_members(GroupMembershipRoles.OWNER)). But you'd have to keep adding more @property's if we add more roles, and you can't add a property for the GroupMembershipRoles.MEMBER role because there's already a property called members (which returns all members regardless of role). With this method you can do group.get_members(GroupMembershipRoles.MEMBER) to get only the plain members, or group.get_members() to get all members regardless of role (or just access the group.members convenience property, which is the same as group.get_members()).

seanh · 2024-11-01T16:48:56Z

h/security/permission_map.py

+        [p.group_has_user_as_owner],
+        [p.group_has_user_as_admin],
+        [p.group_has_user_as_moderator],


This means that the group's creator is no longer permitted to edit the group, and instead the group's owners, admins and moderators are.

seanh · 2024-11-01T17:01:34Z

h/security/predicates.py

+def group_has_user_as_owner(identity, context):
+    return any(
+        owner.id == identity.user.id
+        for owner in context.group.get_members(GroupMembershipRoles.OWNER)
+    )
+
+
+@requires(authenticated_user, group_found)
+def group_has_user_as_admin(identity, context):
+    return any(
+        admin.id == identity.user.id
+        for admin in context.group.get_members(GroupMembershipRoles.ADMIN)
+    )
+

-    return any(user_group.id == context.group.id for user_group in identity.user.groups)
+@requires(authenticated_user, group_found)
+def group_has_user_as_moderator(identity, context):
+    return any(
+        moderator.id == identity.user.id
+        for moderator in context.group.get_members(GroupMembershipRoles.MODERATOR)
+    )


Hard-coding the three roles owner, admin and moderator here, but given how this "predicates" system works I don't see a choice without refactoring it (I don't think the permission map can pass arguments to predicates).

This could read better as a service method

has_role_in_group(user, group, role) or is_member(user, group, role=None) or similar. It could also be written with less iteration and avoiding any N+1 query issues.

seanh · 2024-11-01T17:02:49Z

h/security/predicates.py

+@requires(authenticated_user, group_found)
+def group_has_user_as_member(identity, context):
+    return any(member.id == identity.user.id for member in context.group.members)


I rewrote the existing group_has_user_as_member() predicate to work by iterating over the group's members the same as the three new group_has_user_as_*() predicates that I added above, instead of iterating over the user's groups (which doesn't work now that we have to care about membership roles).

It's tempting to do for member in context.group.get_members(GroupMembershipRoles.MEMBER) here, following the same pattern as the other predicates, but that would change the behavior of this predicate to return only those members with the role "member" rather than returning all members regardless of role. (Unfortunately there are two different meanings of the word "member" in play here.)

seanh · 2024-11-01T17:03:46Z

h/security/predicates.py

-    # With detached groups like we have with the websocket, this doesn't work
-    # as SQLAlchemy does not consider them equal:
-    # return context.group in identity.user.groups


This was a distracting comment giving a commented-out alternative implementation of the line below it, plus a comment explaining why the commented-out version wouldn't work. I just removed the comment.

seanh · 2024-11-01T17:21:05Z

tests/unit/h/security/predicates_test.py

-        # Note we don't use the same literal objects here. It's important
-        # that we test based on the values not Python equality as the objects
-        # are detached in the WS and don't evaluate as equal
-        identity.user.groups = [factories.Group.build(id=i) for i in range(3)]
-        group_context.group = factories.Group.build(id=1 if matching else 100)


This test was written in a weird way that was coupled to the way the predicate was implemented (based on user.groups instead of group.members). Rewrote the test to work the same as the new tests that I added for the other predicates.

Permit owners, admins and moderators, rather than the group's creator, to edit a group. The creator can't edit the group unless they happen to also be an owner, admin or moderator (creators of groups will be owners unless they've subsequently left the group or changed roles). This allows owners, admins and moderators to call the edit-group API, to see the "edit group" link on the group's page, and to view the edit-group page.

It's not enough to just test the status code, you also have to test _why_ that status code was returned. Otherwise the test could be passing even though the code isn't returning the expected response.

marcospri

Looks good, added a suggestion on how to potentially avoid N+1 query issues.

marcospri · 2024-11-06T09:23:03Z

h/models/group.py

-        return tuple(membership.user for membership in self.memberships)
+        return self.get_members()
+
+    def get_members(self, role: GroupMembershipRoles | None = None) -> tuple[User, ...]:


This method could potentially make many queries to the DB without any explicit load strategies declared here.

Usually our go to solution here would be to have this in the service writing as one query returning the same tuple.

We might get away with it as this is scoped for one group only

marcospri · 2024-11-06T09:33:02Z

h/security/predicates.py

+def group_has_user_as_owner(identity, context):
+    return any(
+        owner.id == identity.user.id
+        for owner in context.group.get_members(GroupMembershipRoles.OWNER)
+    )
+
+
+@requires(authenticated_user, group_found)
+def group_has_user_as_admin(identity, context):
+    return any(
+        admin.id == identity.user.id
+        for admin in context.group.get_members(GroupMembershipRoles.ADMIN)
+    )
+

-    return any(user_group.id == context.group.id for user_group in identity.user.groups)
+@requires(authenticated_user, group_found)
+def group_has_user_as_moderator(identity, context):
+    return any(
+        moderator.id == identity.user.id
+        for moderator in context.group.get_members(GroupMembershipRoles.MODERATOR)
+    )


This could read better as a service method

has_role_in_group(user, group, role) or is_member(user, group, role=None) or similar. It could also be written with less iteration and avoiding any N+1 query issues.

seanh added 4 commits October 18, 2024 14:47

Add GroupMembership.roles column to model

795eafc

Add creators of new groups as owners

3f9bf26

Replace Group.members and User.members with memberships

b3e3cfd

seanh requested a review from marcospri November 1, 2024 16:24

seanh force-pushed the new-permissions branch 2 times, most recently from 541aa42 to da675cf Compare November 1, 2024 16:47

seanh commented Nov 1, 2024

View reviewed changes

seanh force-pushed the new-permissions branch from da675cf to 0307a7f Compare November 1, 2024 17:00

seanh commented Nov 1, 2024

View reviewed changes

seanh force-pushed the new-permissions branch 2 times, most recently from 728ee44 to b953ba4 Compare November 1, 2024 17:17

seanh commented Nov 1, 2024

View reviewed changes

seanh force-pushed the new-permissions branch from b953ba4 to 9f7cc2c Compare November 1, 2024 17:35

seanh added 2 commits November 1, 2024 17:36

Make a couple of test assertions more specific

3d02964

It's not enough to just test the status code, you also have to test _why_ that status code was returned. Otherwise the test could be passing even though the code isn't returning the expected response.

seanh force-pushed the new-permissions branch from 9f7cc2c to 3d02964 Compare November 1, 2024 17:36

marcospri approved these changes Nov 6, 2024

View reviewed changes

seanh force-pushed the memberships-relationship branch from b3e3cfd to c0fffb9 Compare November 7, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow owners, admins and moderators to edit groups #9064

Allow owners, admins and moderators to edit groups #9064

seanh commented Nov 1, 2024 •

edited

Loading

seanh Nov 1, 2024 •

edited

Loading

seanh Nov 1, 2024

seanh Nov 1, 2024

marcospri Nov 6, 2024

seanh Nov 1, 2024 •

edited

Loading

seanh Nov 1, 2024

seanh Nov 1, 2024

marcospri left a comment

marcospri Nov 6, 2024

marcospri Nov 6, 2024

Allow owners, admins and moderators to edit groups #9064

Are you sure you want to change the base?

Allow owners, admins and moderators to edit groups #9064

Conversation

seanh commented Nov 1, 2024 • edited Loading

Testing

seanh Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Nov 1, 2024

Choose a reason for hiding this comment

seanh Nov 1, 2024

Choose a reason for hiding this comment

marcospri Nov 6, 2024

Choose a reason for hiding this comment

seanh Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Nov 1, 2024

Choose a reason for hiding this comment

seanh Nov 1, 2024

Choose a reason for hiding this comment

marcospri left a comment

Choose a reason for hiding this comment

marcospri Nov 6, 2024

Choose a reason for hiding this comment

marcospri Nov 6, 2024

Choose a reason for hiding this comment

seanh commented Nov 1, 2024 •

edited

Loading

seanh Nov 1, 2024 •

edited

Loading

seanh Nov 1, 2024 •

edited

Loading