diff --git a/docs/_extra/api-reference/hypothesis-v1.yaml b/docs/_extra/api-reference/hypothesis-v1.yaml index a9e4c6c31d9..245e76e540d 100644 --- a/docs/_extra/api-reference/hypothesis-v1.yaml +++ b/docs/_extra/api-reference/hypothesis-v1.yaml @@ -846,34 +846,6 @@ paths: schema: $ref: '#/components/schemas/Group' - # --------------------------------------------------------------------- - # PUT groups/{id} - Replace (Create or Update) a Group, a.k.a. "upsert" - # --------------------------------------------------------------------- - put: - tags: - - groups - summary: Create or Update a Group - description: | - Update the group with the indicated `id` or create one if it does - not exist. - security: - - ApiKey: [] - - AuthClientForwardedUser: [] - requestBody: - description: Full representation of Group resource - required: true - content: - application/*json: - schema: - $ref: '#/components/schemas/GroupCreate' - responses: - '200': - description: Success - content: - application/json: - schema: - $ref: '#/components/schemas/Group' - # --------------------------------------------------------------------------- # Operations on Group Membership # --------------------------------------------------------------------------- diff --git a/docs/_extra/api-reference/hypothesis-v2.yaml b/docs/_extra/api-reference/hypothesis-v2.yaml index dba43c85fdd..53bc22fe744 100644 --- a/docs/_extra/api-reference/hypothesis-v2.yaml +++ b/docs/_extra/api-reference/hypothesis-v2.yaml @@ -844,34 +844,6 @@ paths: schema: $ref: '#/components/schemas/Group' - # --------------------------------------------------------------------- - # PUT groups/{id} - Replace (Create or Update) a Group, a.k.a. "upsert" - # --------------------------------------------------------------------- - put: - tags: - - groups - summary: Create or Update a Group - description: | - Update the group with the indicated `id` or create one if it does - not exist. - security: - - ApiKey: [] - - AuthClientForwardedUser: [] - requestBody: - description: Full representation of Group resource - required: true - content: - application/*json: - schema: - $ref: '#/components/schemas/GroupCreate' - responses: - '200': - description: Success - content: - application/json: - schema: - $ref: '#/components/schemas/Group' - # --------------------------------------------------------------------------- # Operations on Group Membership # --------------------------------------------------------------------------- diff --git a/h/routes.py b/h/routes.py index bebe233c7b0..e05c0943f11 100644 --- a/h/routes.py +++ b/h/routes.py @@ -140,13 +140,6 @@ def includeme(config): # pylint: disable=too-many-statements ) config.add_route("api.groups", "/api/groups", factory="h.traversal.GroupRoot") - config.add_route( - "api.group_upsert", - "/api/groups/{id}", - request_method="PUT", - factory="h.traversal.GroupRoot", - traverse="/{id}", - ) config.add_route( "api.group", "/api/groups/{id}", diff --git a/h/security/permission_map.py b/h/security/permission_map.py index b8e80cbff08..f47decc217c 100644 --- a/h/security/permission_map.py +++ b/h/security/permission_map.py @@ -54,10 +54,6 @@ ], Permission.Group.MEMBER_ADD: [[p.group_matches_authenticated_client_authority]], Permission.Group.MODERATE: [[p.group_created_by_user]], - Permission.Group.UPSERT: [ - [p.group_created_by_user], - [p.group_not_found, p.authenticated_user], - ], # --------------------------------------------------------------------- # # Annotations Permission.Annotation.CREATE: [[p.authenticated]], diff --git a/h/security/permissions.py b/h/security/permissions.py index 7613d38daa0..14d3baead3b 100644 --- a/h/security/permissions.py +++ b/h/security/permissions.py @@ -28,9 +28,6 @@ class Group(Enum): EDIT = "group:edit" """Update the details of a group.""" - UPSERT = "group:upsert" - """Update the details of a group or create a new one.""" - FLAG = "group:flag" """Mark annotations in this group as inappropriate for moderators.""" diff --git a/h/security/policy/_auth_client.py b/h/security/policy/_auth_client.py index fa0951ca6cd..3c1f3debb4b 100644 --- a/h/security/policy/_auth_client.py +++ b/h/security/policy/_auth_client.py @@ -32,7 +32,6 @@ class AuthClientPolicy: ("api.group", "GET"), ("api.group_member", "POST"), ("api.group_members", "GET"), - ("api.group_upsert", "PUT"), ("api.users", "POST"), ("api.user_read", "GET"), ("api.user", "PATCH"), diff --git a/h/security/predicates.py b/h/security/predicates.py index 8105b61159d..f563c487e0b 100644 --- a/h/security/predicates.py +++ b/h/security/predicates.py @@ -112,10 +112,6 @@ def group_found(_identity, context): return hasattr(context, "group") and context.group -def group_not_found(_identity, context): - return not hasattr(context, "group") or not context.group - - @requires(group_found) def group_writable_by_members(_identity, context): return context.group.writeable_by == WriteableBy.members diff --git a/h/views/api/groups.py b/h/views/api/groups.py index 22848f37647..c1bdb3c4101 100644 --- a/h/views/api/groups.py +++ b/h/views/api/groups.py @@ -140,64 +140,6 @@ def update(context: GroupContext, request): return GroupJSONPresenter(group, request).asdict(expand=["organization", "scopes"]) -@api_config( - versions=["v1", "v2"], - route_name="api.group_upsert", - request_method="PUT", - permission=Permission.Group.UPSERT, - link_name="group.create_or_update", - description="Create or update a group", -) -def upsert(context: GroupContext, request): - """ - Create or update a group from a PUT payload. - - If no group model is present in the passed ``context`` (on ``context.group``), - treat this as a create action and delegate to ``create``. - - Otherwise, replace the existing group's resource properties entirely and update - the object. - """ - if context.group is None: - return create(request) - - group = context.group - - # Because this is a PUT endpoint and not a PATCH, a full replacement of the - # entire resource is expected. Thus, we're validating against the Create schema - # here as we want to make sure properties required for a fresh object are present - appstruct = CreateGroupAPISchema( - default_authority=request.default_authority, - group_authority=request.effective_authority, - ).validate(_json_payload(request)) - - group_update_service = request.find_service(name="group_update") - group_service = request.find_service(name="group") - - # Check for duplicate group - groupid = appstruct.get("groupid", None) - if groupid is not None: - duplicate_group = group_service.fetch(pubid_or_groupid=groupid) - if duplicate_group and (duplicate_group != group): - raise HTTPConflict( - _("group with groupid '{}' already exists").format(groupid) - ) - - # Need to make sure every resource-defined property is present, as this - # is meant as a full-resource-replace operation. - # TODO: This may be better handled in the schema at some point - update_properties = { - "name": appstruct["name"], - "description": appstruct.get("description", ""), - "groupid": appstruct.get("groupid", None), - "type": appstruct.get("type", DEFAULT_GROUP_TYPE), - } - - group = group_update_service.update(group, **update_properties) - - return GroupJSONPresenter(group, request).asdict(expand=["organization", "scopes"]) - - @api_config( versions=["v1", "v2"], route_name="api.group_members", diff --git a/tests/functional/api/groups/upsert_test.py b/tests/functional/api/groups/upsert_test.py deleted file mode 100644 index 4add397bb31..00000000000 --- a/tests/functional/api/groups/upsert_test.py +++ /dev/null @@ -1,291 +0,0 @@ -import base64 - -import pytest - -from h.models.auth_client import GrantType - - -class TestUpsertGroupUpdate: - def test_it_returns_http_404_if_no_authenticated_user(self, app, first_party_group): - group = {"name": "My Group"} - res = app.put_json( - "/api/groups/{id}".format(id=first_party_group.pubid), - group, - expect_errors=True, - ) - - assert res.status_code == 404 - - def test_it_returns_http_200_with_valid_payload_and_user_token( - self, app, token_auth_header, first_party_group - ): - group = {"name": "Rename My Group"} - res = app.put_json( - "/api/groups/{id}".format(id=first_party_group.pubid), - group, - headers=token_auth_header, - ) - - assert res.status_code == 200 - assert res.json_body["name"] == "Rename My Group" - assert res.json_body["groupid"] is None - - def test_it_ignores_non_whitelisted_fields_in_payload( - self, app, token_auth_header, first_party_group - ): - group = { - "name": "Rename Me", - "organization": "foobar", - "joinable_by": "whoever", - } - res = app.put_json( - "/api/groups/{id}".format(id=first_party_group.pubid), - group, - headers=token_auth_header, - ) - - assert res.status_code == 200 - assert res.json_body["organization"] is None - - def test_it_returns_http_400_with_invalid_payload( - self, app, token_auth_header, first_party_group - ): - group = {} - - res = app.put_json( - "/api/groups/{id}".format(id=first_party_group.pubid), - group, - headers=token_auth_header, - expect_errors=True, - ) - - assert res.status_code == 400 - - def test_it_returns_http_400_if_groupid_set_on_default_authority( - self, app, token_auth_header, first_party_group - ): - group = {"name": "Egghead", "groupid": "3434kjkjk"} - res = app.put_json( - "/api/groups/{id}".format(id=first_party_group.pubid), - group, - headers=token_auth_header, - expect_errors=True, - ) - - assert res.status_code == 400 - - def test_it_returns_http_404_if_token_user_unauthorized( - self, app, token_auth_header, factories, db_session - ): - # Not created by user represented by token_auth_header - group = factories.Group() - db_session.commit() - - group_payload = {"name": "My Group"} - res = app.put_json( - "/api/groups/{id}".format(id=group.pubid), - group_payload, - headers=token_auth_header, - expect_errors=True, - ) - - assert res.status_code == 404 - - def test_it_allows_auth_client_with_valid_forwarded_user( - self, app, auth_client_header, third_party_user, factories, db_session - ): - group = factories.Group( - creator=third_party_user, authority=third_party_user.authority - ) - db_session.commit() - - headers = auth_client_header - headers["X-Forwarded-User"] = third_party_user.userid - group_payload = {"name": "My Group"} - - path = "/api/groups/{id}".format(id=group.pubid) - res = app.put_json(path, group_payload, headers=headers) - - assert res.status_code == 200 - assert res.json_body["name"] == "My Group" - - def test_it_returns_404_if_forwarded_user_is_not_authorized_to_update( - self, app, auth_client_header, third_party_user, factories, db_session - ): - # Not created by `third_party_user` - group = factories.Group(authority=third_party_user.authority) - db_session.commit() - - headers = auth_client_header - headers["X-Forwarded-User"] = third_party_user.userid - group_payload = {"name": "My Group"} - - path = "/api/groups/{id}".format(id=group.pubid) - res = app.put_json(path, group_payload, headers=headers, expect_errors=True) - - assert res.status_code == 404 - - def test_it_allows_groupid_from_auth_client_with_forwarded_user( - self, app, auth_client_header, third_party_user, factories, db_session - ): - group = factories.Group( - creator=third_party_user, authority=third_party_user.authority - ) - db_session.commit() - - headers = auth_client_header - headers["X-Forwarded-User"] = third_party_user.userid - group_payload = { - "name": "My Group", - "groupid": "group:34567845@thirdparty.com", - } - - path = "/api/groups/{id}".format(id=group.pubid) - res = app.put_json(path, group_payload, headers=headers) - - assert res.status_code == 200 - assert "groupid" in res.json_body - assert res.json_body["groupid"] == "group:34567845@thirdparty.com" - - def test_it_supersedes_groupid_with_value_in_payload( - self, app, auth_client_header, third_party_user, factories, db_session - ): - # In this test, the ``groupid`` in the path param is different than that - # indicated in the payload. This allows a caller to change the ``groupid`` - group = factories.Group( - creator=third_party_user, - authority=third_party_user.authority, - authority_provided_id="doodad", - ) - db_session.commit() - - headers = auth_client_header - headers["X-Forwarded-User"] = third_party_user.userid - group_payload = { - "name": "My Group", - "groupid": "group:ice-cream@thirdparty.com", - } - - path = "/api/groups/{id}".format(id=group.groupid) - res = app.put_json(path, group_payload, headers=headers) - - assert res.status_code == 200 - assert "groupid" in res.json_body - assert res.json_body["groupid"] == "group:{groupid}@thirdparty.com".format( - groupid="ice-cream" - ) - - def test_it_returns_HTTP_Conflict_if_groupid_is_duplicate( - self, app, auth_client_header, third_party_user, factories, db_session - ): - group1 = factories.Group( - creator=third_party_user, - authority=third_party_user.authority, - groupid="group:upsert_one@thirdparty.com", - ) - group2 = factories.Group( - creator=third_party_user, - authority=third_party_user.authority, - groupid="group:upsert_two@thirdparty.com", - ) - db_session.commit() - - headers = auth_client_header - headers["X-Forwarded-User"] = third_party_user.userid - group_payload = { - "name": "Whatnot", - "groupid": "group:upsert_one@thirdparty.com", - } - - # Attempting to set group2's `groupid` to one already taken by group1 - path = "/api/groups/{id}".format(id=group2.pubid) - res = app.put_json(path, group_payload, headers=headers, expect_errors=True) - - assert group1.groupid in res.json_body["reason"] - assert res.status_code == 409 - - -class TestUpsertGroupCreate: - def test_it_allows_auth_client_with_forwarded_user( - self, app, auth_client_header, third_party_user - ): - headers = auth_client_header - headers["X-Forwarded-User"] = third_party_user.userid - group = { - "name": "My Group", - "groupid": "group:foothold@{auth}".format(auth=third_party_user.authority), - } - - res = app.put_json("/api/groups/somepubid", group, headers=headers) - - assert res.status_code == 200 - assert res.json_body["name"] == "My Group" - assert res.json_body["groupid"] == "group:foothold@{auth}".format( - auth=third_party_user.authority - ) - - def test_it_allows_user_with_token(self, app, token_auth_header): - group = {"name": "This is my group"} - res = app.put_json("/api/groups/randompubid", group, headers=token_auth_header) - - assert res.status_code == 200 - assert res.json_body["name"] == "This is my group" - assert res.json_body["groupid"] is None - - -@pytest.fixture -def first_party_user(db_session, factories): - user = factories.User() - db_session.commit() - return user - - -@pytest.fixture -def first_party_group(db_session, factories, first_party_user): - group = factories.Group( - name="My First Group", - description="Original description", - creator=first_party_user, - authority=first_party_user.authority, - ) - db_session.commit() - return group - - -@pytest.fixture -def user_with_token(db_session, factories, first_party_user): - token = factories.DeveloperToken(user=first_party_user) - db_session.add(token) - db_session.commit() - return (first_party_user, token) - - -@pytest.fixture -def token_auth_header(user_with_token): - user, token = user_with_token - return {"Authorization": "Bearer {}".format(token.value)} - - -@pytest.fixture -def third_party_user(factories, db_session): - user = factories.User(authority="thirdparty.com") - db_session.commit() - return user - - -@pytest.fixture -def auth_client(db_session, factories): - auth_client = factories.ConfidentialAuthClient( - authority="thirdparty.com", grant_type=GrantType.client_credentials - ) - db_session.commit() - return auth_client - - -@pytest.fixture -def auth_client_header(auth_client): - user_pass = "{client_id}:{secret}".format( - client_id=auth_client.id, secret=auth_client.secret - ) - encoded = base64.standard_b64encode(user_pass.encode("utf-8")) - return {"Authorization": "Basic {creds}".format(creds=encoded.decode("ascii"))} diff --git a/tests/functional/api/index_test.py b/tests/functional/api/index_test.py index ba77494534b..2168b631145 100644 --- a/tests/functional/api/index_test.py +++ b/tests/functional/api/index_test.py @@ -32,7 +32,7 @@ def test_it_returns_links_for_resources(self, app): "annotation", ["create", "read", "update", "delete", "flag", "hide", "unhide"], ), - ("group", ["create", "read", "update", "create_or_update"]), + ("group", ["create", "read", "update"]), ("profile", ["read", "update"]), ("user", ["create", "update"]), ], @@ -70,7 +70,7 @@ def test_it_returns_expected_nested_resource_service_links_for_v1( "annotation", ["create", "read", "update", "delete", "flag", "hide", "unhide"], ), - ("group", ["create", "read", "update", "create_or_update"]), + ("group", ["create", "read", "update"]), ("profile", ["read", "update"]), ("user", ["create", "update"]), ], diff --git a/tests/unit/h/routes_test.py b/tests/unit/h/routes_test.py index 328b604000a..d129d74b1cd 100644 --- a/tests/unit/h/routes_test.py +++ b/tests/unit/h/routes_test.py @@ -133,13 +133,6 @@ def test_includeme(): request_method="POST", ), call("api.groups", "/api/groups", factory="h.traversal.GroupRoot"), - call( - "api.group_upsert", - "/api/groups/{id}", - request_method="PUT", - factory="h.traversal.GroupRoot", - traverse="/{id}", - ), call( "api.group", "/api/groups/{id}", diff --git a/tests/unit/h/security/predicates_test.py b/tests/unit/h/security/predicates_test.py index 90946bdfc52..c81fdb75bc2 100644 --- a/tests/unit/h/security/predicates_test.py +++ b/tests/unit/h/security/predicates_test.py @@ -114,13 +114,6 @@ def test_group_found(self, annotation_context, group_context, user_context): assert predicates.group_found(sentinel.identity, annotation_context) assert predicates.group_found(sentinel.identity, group_context) - def test_group_not_found(self, annotation_context, group_context, user_context): - assert predicates.group_not_found(sentinel.identity, user_context) - assert predicates.group_not_found(sentinel.identity, GroupContext(group=None)) - # Annotations have a group - assert not predicates.group_not_found(sentinel.identity, annotation_context) - assert not predicates.group_not_found(sentinel.identity, group_context) - @pytest.mark.parametrize("writable_by", WriteableBy) def test_group_writable_by_members(self, group_context, writable_by): group_context.group.writeable_by = writable_by diff --git a/tests/unit/h/views/api/groups_test.py b/tests/unit/h/views/api/groups_test.py index 148e9e3e075..b7deeb76d74 100644 --- a/tests/unit/h/views/api/groups_test.py +++ b/tests/unit/h/views/api/groups_test.py @@ -408,167 +408,6 @@ def pyramid_request(self, pyramid_request): return pyramid_request -@pytest.mark.usefixtures("group_service", "group_update_service") -class TestUpsert: - def test_it_validates_the_request( - self, context, pyramid_request, CreateGroupAPISchema - ): - views.upsert(context, pyramid_request) - - CreateGroupAPISchema.assert_called_once_with( - default_authority=pyramid_request.default_authority, - group_authority=sentinel.effective_authority, - ) - CreateGroupAPISchema.return_value.validate.assert_called_once_with( - pyramid_request.json_body - ) - - def test_it_when_request_json_body_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.upsert(context, pyramid_request) - - assert exc_info.value.__cause__ == value_error - - def test_it_when_request_json_body_is_invalid( - self, context, pyramid_request, CreateGroupAPISchema - ): - CreateGroupAPISchema.return_value.validate.side_effect = ValidationError - - with pytest.raises(ValidationError): - views.upsert(context, pyramid_request) - - def test_if_the_group_doesnt_exist_yet_it_creates_it( - self, context, pyramid_request, create - ): - context.group = None - - response = views.upsert(context, pyramid_request) - - create.assert_called_once_with(pyramid_request) - assert response == create.return_value - - @pytest.mark.parametrize( - "appstruct,group_update_service_method_call", - [ - ( - {"name": sentinel.name}, - call.update( - sentinel.group, - name=sentinel.name, - description="", - groupid=None, - type="private", - ), - ), - ( - {"name": sentinel.name, "description": sentinel.description}, - call.update( - sentinel.group, - name=sentinel.name, - description=sentinel.description, - groupid=None, - type="private", - ), - ), - ( - {"name": sentinel.name, "type": "restricted"}, - call.update( - sentinel.group, - name=sentinel.name, - description="", - groupid=None, - type="restricted", - ), - ), - ( - {"name": sentinel.name, "type": "open"}, - call.update( - sentinel.group, - name=sentinel.name, - description="", - groupid=None, - type="open", - ), - ), - ], - ) - def test_it_updates_an_existing_group( - self, - context, - pyramid_request, - CreateGroupAPISchema, - group_update_service, - assert_it_returns_group_as_json, - appstruct, - group_update_service_method_call, - ): - CreateGroupAPISchema.return_value.validate.return_value = appstruct - - response = views.upsert(context, pyramid_request) - - assert group_update_service.method_calls == [group_update_service_method_call] - assert_it_returns_group_as_json( - response, group=group_update_service.update.return_value - ) - - def test_it_with_groupid_request_param( - self, - context, - pyramid_request, - CreateGroupAPISchema, - group_service, - group_update_service, - ): - CreateGroupAPISchema.return_value.validate.return_value["groupid"] = ( - sentinel.groupid - ) - group_service.fetch.return_value = context.group - - views.upsert(context, pyramid_request) - - group_service.fetch.assert_called_once_with(pubid_or_groupid=sentinel.groupid) - assert group_update_service.update.call_args[1]["groupid"] == sentinel.groupid - - def test_it_with_duplicate_groupid( - self, - context, - pyramid_request, - group_service, - group_update_service, - CreateGroupAPISchema, - ): - CreateGroupAPISchema.return_value.validate.return_value["groupid"] = ( - sentinel.groupid - ) - group_service.fetch.return_value = sentinel.duplicate_group - - with pytest.raises( - HTTPConflict, match="group with groupid 'sentinel.groupid' already exists" - ): - views.upsert(context, pyramid_request) - - group_update_service.update.assert_not_called() - - @pytest.fixture(autouse=True) - def create(self, mocker): - return mocker.patch("h.views.api.groups.create", autospec=True, spec_set=True) - - @pytest.fixture - def pyramid_request(self, pyramid_request): - pyramid_request.json_body = sentinel.json_body - return pyramid_request - - class TestReadMembers: def test_it(self, context, pyramid_request, UserJSONPresenter): context.group.members = [sentinel.member_1, sentinel.member_2]