Support setting role in add-member API requests

Add support for an optional JSON body in add-member API requests that
lets the user set the membership's role in the add-member API request.

The add-group-membership API will now return a 409 Conflict if a
conflicting membership (one with the same user and group but different
roles) already exists in the DB:

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["owner"]}
    -> 200 OK

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["admin"]}
    -> 409 Conflict

Previously this would have resulted in a 200 OK even though the
membership's role in the DB would *not* have been updated with the new
role from the second request.

For backwards-compatibility it will still respond 200 OK if the
subsequent request's role *does* match the existing role in the DB:

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["owner"]}
    -> 200 OK

    POST /api/groups/{groupid}/members/{userid}
    {"roles": ["owner"]}
    -> 200 OK

This also works if requests are made with no JSON body (which creates
memberships with the default role of "member"):

    POST /api/groups/{groupid}/members/{userid}
    (no body content)
    -> 200 OK

    POST /api/groups/{groupid}/members/{userid}
    (no body content)
    -> 200 OK

h/services/group_members.py

@@ -9,6 +9,10 @@
 log = logging.getLogger(__name__)
 
 
+class ConflictError(Exception):
+    """A conflicting group membership already exists in the DB."""
+
+
 class GroupMembersService:
     """A service for manipulating group membership."""
 
@@ -111,17 +115,34 @@ def update_members(self, group, userids):
         for userid in userids_for_removal:
             self.member_leave(group, userid)
 
-    def member_join(self, group, userid) -> GroupMembership:
-        """Add `userid` to the member list of `group`."""
+    def member_join(self, group, userid, roles=None) -> GroupMembership:
+        """
+        Add `userid` to `group` with `roles` and return the resulting membership.
+
+        If `roles=None` it will default to `[GroupMembershipRoles.MEMBER]`.
+
+        If a membership matching `group`, `userid` and `roles` already exists
+        in the DB it will just be returned.
+
+        :raise ConflictError: if a membership already exists with the given
+            group and userid but different roles
+        """
+        roles = roles or [GroupMembershipRoles.MEMBER]
+
         user = self.user_fetcher(userid)
 
-        existing_membership = self.get_membership(group, user)
+        kwargs = {"roles": roles}
+
+        if existing_membership := self.get_membership(group, user):
+            for key, value in kwargs.items():
+                if getattr(existing_membership, key) != value:
+                    raise ConflictError(
+                        "The user is already a member of the group, with conflicting membership attributes"
+                    )
 
-        if existing_membership:
-            # The user is already a member of the group.
             return existing_membership
 
-        membership = GroupMembership(group=group, user=user)
+        membership = GroupMembership(group=group, user=user, **kwargs)
         self.db.add(membership)
 
         # Flush the DB to generate SQL defaults for `membership` before logging it.

h/views/api/group_members.py

@@ -1,13 +1,14 @@
 import logging
 
 from pyramid.config import not_
-from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound
+from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound
 
 from h.presenters import GroupMembershipJSONPresenter
 from h.schemas.api.group_membership import EditGroupMembershipAPISchema
 from h.schemas.pagination import PaginationQueryParamsSchema
 from h.schemas.util import validate_query_params
 from h.security import Permission
+from h.services.group_members import ConflictError
 from h.traversal import EditGroupMembershipContext, GroupContext, GroupMembershipContext
 from h.views.api.config import api_config
 from h.views.api.helpers.json_payload import json_payload
@@ -99,9 +100,22 @@ def add_member(context: GroupMembershipContext, request):
     if context.user.authority != context.group.authority:
         raise HTTPNotFound()
 
+    if request.body:
+        appstruct = EditGroupMembershipAPISchema().validate(json_payload(request))
+        roles = appstruct["roles"]
+    else:
+        # This doesn't mean the membership will be created with no roles:
+        # default roles will be applied by the service.
+        roles = None
+
     group_members_service = request.find_service(name="group_members")
 
-    membership = group_members_service.member_join(context.group, context.user.userid)
+    try:
+        membership = group_members_service.member_join(
+            context.group, context.user.userid, roles
+        )
+    except ConflictError as err:
+        raise HTTPConflict(str(err)) from err
 
     return GroupMembershipJSONPresenter(request, membership).asdict()
 

tests/functional/api/groups/members_test.py

@@ -261,10 +261,22 @@ def test_it_returns_an_error_if_offset_and_limit_are_invalid(
 
 
 class TestAddMember:
-    def test_it(self, do_request, group, user):
-        do_request()
+    @pytest.mark.parametrize(
+        "json,expected_roles",
+        [
+            ({"roles": ["owner"]}, ["owner"]),
+            (None, ["member"]),
+        ],
+    )
+    def test_it(self, do_request, group, user, json, expected_roles):
+        do_request(json=json)
 
-        assert user in group.members
+        for membership in group.memberships:
+            if membership.user == user:
+                assert membership.roles == expected_roles
+                break
+        else:
+            assert False, "No membership was created"
 
     def test_it_does_nothing_if_the_user_is_already_a_member_of_the_group(
         self, do_request, group, user
@@ -275,6 +287,22 @@ def test_it_does_nothing_if_the_user_is_already_a_member_of_the_group(
 
         assert user in group.members
 
+    def test_it_when_a_conflicting_membership_already_exists(
+        self, do_request, group, user
+    ):
+        group.memberships.append(
+            GroupMembership(user=user, roles=[GroupMembershipRoles.MEMBER])
+        )
+
+        response = do_request(
+            json={"roles": [GroupMembershipRoles.MODERATOR]}, status=409
+        )
+
+        assert (
+            response.json["reason"]
+            == "The user is already a member of the group, with conflicting membership attributes"
+        )
+
     def test_it_errors_if_the_pubid_is_unknown(self, do_request):
         do_request(pubid="UNKNOWN_PUBID", status=404)
 
@@ -357,11 +385,14 @@ def test_me_alias_without_forwarded_user(self, do_request):
 
     @pytest.fixture
     def do_request(self, db_session, app, group, user, headers):
-        def do_request(pubid=group.pubid, userid=user.userid, status=200):
+        def do_request(pubid=group.pubid, userid=user.userid, status=200, json=None):
             db_session.commit()
-            return app.post_json(
-                f"/api/groups/{pubid}/members/{userid}", headers=headers, status=status
-            )
+            path = f"/api/groups/{pubid}/members/{userid}"
+
+            if json is None:
+                return app.post(path, headers=headers, status=status)
+            else:
+                return app.post_json(path, json, headers=headers, status=status)
 
         return do_request
 

tests/unit/h/services/group_members_test.py

@@ -7,7 +7,11 @@
 from sqlalchemy import select
 
 from h.models import GroupMembership, GroupMembershipRoles, User
-from h.services.group_members import GroupMembersService, group_members_factory
+from h.services.group_members import (
+    ConflictError,
+    GroupMembersService,
+    group_members_factory,
+)
 
 
 class TestGetMembership:
@@ -183,18 +187,36 @@ def test_it_adds_user_to_group(
         caplog.set_level(logging.INFO)
         user = factories.User()
         group = factories.Group()
-        returned = group_members_service.member_join(group, user.userid)
+
+        returned = group_members_service.member_join(
+            group, user.userid, roles=[GroupMembershipRoles.OWNER]
+        )
 
         membership = db_session.scalars(
             select(GroupMembership)
             .where(GroupMembership.user == user)
             .where(GroupMembership.group == group)
         ).one_or_none()
-        assert membership
+        assert membership.roles == [GroupMembershipRoles.OWNER]
         assert returned == membership
         assert caplog.messages == [f"Added group membership: {membership!r}"]
 
-    def test_it_is_idempotent(self, group_members_service, factories):
+    def test_it_with_no_role(self, group_members_service, factories, db_session):
+        user = factories.User()
+        group = factories.Group()
+
+        group_members_service.member_join(group, user.userid)
+
+        membership = db_session.scalars(
+            select(GroupMembership)
+            .where(GroupMembership.user == user)
+            .where(GroupMembership.group == group)
+        ).one_or_none()
+        assert membership.roles == [GroupMembershipRoles.MEMBER]
+
+    def test_it_when_a_matching_membership_already_exists(
+        self, group_members_service, factories
+    ):
         user = factories.User()
         group = factories.Group()
         existing_membership = group_members_service.member_join(group, user.userid)
@@ -204,6 +226,20 @@ def test_it_is_idempotent(self, group_members_service, factories):
         assert returned == existing_membership
         assert group.members.count(user) == 1
 
+    def test_it_when_a_conflicting_membership_already_exists(
+        self, group_members_service, factories
+    ):
+        user = factories.User()
+        group = factories.Group()
+        group_members_service.member_join(
+            group, user.userid, roles=[GroupMembershipRoles.MEMBER]
+        )
+
+        with pytest.raises(ConflictError):
+            group_members_service.member_join(
+                group, user.userid, roles=[GroupMembershipRoles.MODERATOR]
+            )
+
     def test_it_publishes_join_event(self, group_members_service, factories, publish):
         group = factories.Group()
         user = factories.User()

tests/unit/h/views/api/group_members_test.py

@@ -2,13 +2,14 @@
 from unittest.mock import PropertyMock, call, create_autospec, sentinel
 
 import pytest
-from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound
+from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound
 
 import h.views.api.group_members as views
 from h import presenters
 from h.models import GroupMembership
 from h.schemas.base import ValidationError
 from h.security.identity import Identity, LongLivedGroup, LongLivedMembership
+from h.services.group_members import ConflictError
 from h.traversal import GroupContext, GroupMembershipContext
 from h.views.api.exceptions import PayloadError
 
@@ -147,18 +148,65 @@ def test_it(
         group_members_service,
         context,
         GroupMembershipJSONPresenter,
+        EditGroupMembershipAPISchema,
     ):
         response = views.add_member(context, pyramid_request)
 
+        EditGroupMembershipAPISchema.assert_called_once_with()
+        EditGroupMembershipAPISchema.return_value.validate.assert_called_once_with(
+            sentinel.json_body
+        )
         group_members_service.member_join.assert_called_once_with(
-            context.group, context.user.userid
+            context.group, context.user.userid, roles=sentinel.roles
         )
         GroupMembershipJSONPresenter.assert_called_once_with(
             pyramid_request, group_members_service.member_join.return_value
         )
         GroupMembershipJSONPresenter.return_value.asdict.assert_called_once_with()
         assert response == GroupMembershipJSONPresenter.return_value.asdict.return_value
 
+    def test_it_with_no_request_body(
+        self,
+        pyramid_request,
+        group_members_service,
+        context,
+        EditGroupMembershipAPISchema,
+    ):
+        pyramid_request.body = b""
+
+        views.add_member(context, pyramid_request)
+
+        EditGroupMembershipAPISchema.assert_not_called()
+        group_members_service.member_join.assert_called_once_with(
+            context.group, context.user.userid, roles=None
+        )
+
+    def test_it_when_a_conflicting_membership_already_exists(
+        self, pyramid_request, group_members_service, context
+    ):
+        group_members_service.member_join.side_effect = ConflictError(
+            "test_error_message"
+        )
+
+        with pytest.raises(HTTPConflict, match="^test_error_message$"):
+            views.add_member(context, pyramid_request)
+
+    def test_it_errors_if_the_request_isnt_valid_JSON(
+        self, context, pyramid_request, mocker
+    ):
+        value_error = ValueError()
+        mocker.patch.object(
+            type(pyramid_request),
+            "json_body",
+            PropertyMock(side_effect=value_error),
+            create=True,
+        )
+
+        with pytest.raises(PayloadError) as exc_info:
+            views.add_member(context, pyramid_request)
+
+        assert exc_info.value.__cause__ == value_error
+
     def test_it_with_authority_mismatch(self, pyramid_request, context):
         context.group.authority = "other"
 
@@ -171,6 +219,19 @@ def context(self, factories):
         user = factories.User.build(authority=group.authority)
         return GroupMembershipContext(group=group, user=user, membership=None)
 
+    @pytest.fixture
+    def pyramid_request(self, pyramid_request):
+        pyramid_request.body = sentinel.body
+        pyramid_request.json_body = sentinel.json_body
+        return pyramid_request
+
+    @pytest.fixture
+    def EditGroupMembershipAPISchema(self, EditGroupMembershipAPISchema):
+        EditGroupMembershipAPISchema.return_value.validate.return_value = {
+            "roles": sentinel.roles
+        }
+        return EditGroupMembershipAPISchema
+
 
 class TestEditMember:
     def test_it(