From 48fb6915f2c9f9a92f25b57f976e6d6641f00afb Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 2 Oct 2024 12:51:45 +0100 Subject: [PATCH] Remove unused group upsert API This API was added for our own LMS app: https://github.com/hypothesis/h/pull/5428 But the LMS app doesn't use this API anymore (and actually I'm not sure it ever did) and it doesn't look like anything else ever used it. In upcoming PRs we need to make changes to group-related APIs (for example instead of only a group's creator being able to edit/update a group, all of a group's owners and admins need to be able to). Let's first remove this unused API so we don't have to deal with updating it, especially since this API's code is entangled with other APIs that we do need to update. Slack thread: https://hypothes-is.slack.com/archives/C4K6M7P5E/p1724926065440579 --- docs/_extra/api-reference/hypothesis-v1.yaml | 28 -- docs/_extra/api-reference/hypothesis-v2.yaml | 28 -- h/routes.py | 7 - h/security/permission_map.py | 4 - h/security/permissions.py | 3 - h/security/policy/_auth_client.py | 1 - h/security/predicates.py | 4 - h/views/api/groups.py | 58 ---- tests/functional/api/groups/upsert_test.py | 291 ------------------- tests/functional/api/index_test.py | 4 +- tests/unit/h/routes_test.py | 7 - tests/unit/h/security/predicates_test.py | 7 - tests/unit/h/views/api/groups_test.py | 161 ---------- 13 files changed, 2 insertions(+), 601 deletions(-) delete mode 100644 tests/functional/api/groups/upsert_test.py 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]