From e5f5ab89b6cbab70e966bffd0df42b357ceacd4e Mon Sep 17 00:00:00 2001 From: Sam Al Arbid Date: Thu, 4 May 2023 10:41:45 +0200 Subject: [PATCH] services: Require record community --- invenio_rdm_records/config.py | 7 + .../services/communities/service.py | 25 +- invenio_rdm_records/services/errors.py | 10 + invenio_rdm_records/services/permissions.py | 15 +- tests/resources/test_resources_communities.py | 225 ++++++++++++++++++ 5 files changed, 274 insertions(+), 8 deletions(-) diff --git a/invenio_rdm_records/config.py b/invenio_rdm_records/config.py index ca7082dbfa..445011437b 100644 --- a/invenio_rdm_records/config.py +++ b/invenio_rdm_records/config.py @@ -4,6 +4,7 @@ # Copyright (C) 2019 Northwestern University. # Copyright (C) 2021-2023 Graz University of Technology. # Copyright (C) 2023 TU Wien. +# Copyright (C) 2023 KTH Royal Institute of Technology. # # Invenio-RDM-Records is free software; you can redistribute it and/or modify # it under the terms of the MIT License; see LICENSE file for more details. @@ -125,6 +126,12 @@ def always_valid(identifier): RDM_ALLOW_RESTRICTED_RECORDS = True """Allow users to set restricted/private records.""" +# +# Record communities +# +RDM_ENSURE_RECORD_COMMUNITY_EXISTS = False +"""Enforces at least one community per record on remove community function.""" + # # Search configuration # diff --git a/invenio_rdm_records/services/communities/service.py b/invenio_rdm_records/services/communities/service.py index 26ab36b993..448c103549 100644 --- a/invenio_rdm_records/services/communities/service.py +++ b/invenio_rdm_records/services/communities/service.py @@ -1,12 +1,13 @@ # -*- coding: utf-8 -*- # # Copyright (C) 2023 CERN. +# Copyright (C) 2023 KTH Royal Institute of Technology. # # Invenio-RDM-Records is free software; you can redistribute it and/or modify # it under the terms of the MIT License; see LICENSE file for more details. """RDM Record Communities Service.""" - +from flask import current_app from invenio_communities.proxies import current_communities from invenio_i18n import lazy_gettext as _ from invenio_pidstore.errors import PIDDoesNotExistError @@ -34,6 +35,7 @@ InvalidAccessRestrictions, OpenRequestAlreadyExists, RecordCommunityMissing, + RecordCommunityRequired, ) @@ -166,6 +168,10 @@ def _remove(self, identity, community_id, record): self.require_permission( identity, "remove_community", record=record, community_id=community_id ) + if current_app.config.get("RDM_ENSURE_RECORD_COMMUNITY_EXISTS"): + rec_communities = record.parent.communities.ids + if len(rec_communities) <= 1: + raise RecordCommunityRequired() # Default community is deleted when the exact same community is removed from the record record.parent.communities.remove(community_id) @@ -185,12 +191,17 @@ def remove(self, identity, id_, data, uow): ) communities = valid_data["communities"] processed = [] + for community in communities: community_id = community["id"] try: self._remove(identity, community_id, record) processed.append({"community": community_id}) - except (RecordCommunityMissing, PermissionDeniedError) as ex: + except ( + RecordCommunityMissing, + PermissionDeniedError, + RecordCommunityRequired, + ) as ex: errors.append( { "community": community_id, @@ -213,7 +224,7 @@ def search( search_preference=None, expand=False, extra_filter=None, - **kwargs + **kwargs, ): """Search for record's communities.""" record = self.record_cls.pid.resolve(id_) @@ -230,7 +241,7 @@ def search( search_preference=search_preference, expand=expand, extra_filter=communities_filter, - **kwargs + **kwargs, ) @staticmethod @@ -274,7 +285,7 @@ def search_suggested_communities( expand=False, by_membership=False, extra_filter=None, - **kwargs + **kwargs, ): """Search for communities that can be added to a record.""" record = self.record_cls.pid.resolve(id_) @@ -294,7 +305,7 @@ def search_suggested_communities( params=params, search_preference=search_preference, extra_filter=communities_filter, - **kwargs + **kwargs, ) return current_communities.service.search( @@ -303,5 +314,5 @@ def search_suggested_communities( search_preference=search_preference, expand=expand, extra_filter=communities_filter, - **kwargs + **kwargs, ) diff --git a/invenio_rdm_records/services/errors.py b/invenio_rdm_records/services/errors.py index 4280ed59bf..e03c7c3801 100644 --- a/invenio_rdm_records/services/errors.py +++ b/invenio_rdm_records/services/errors.py @@ -2,6 +2,7 @@ # # Copyright (C) 2021 CERN. # Copyright (C) 2023 Graz University of Technology. +# Copyright (C) 2023 KTH Royal Institute of Technology. # # Invenio-RDM-Records is free software; you can redistribute it and/or modify # it under the terms of the MIT License; see LICENSE file for more details. @@ -142,6 +143,15 @@ def description(self): ) +class RecordCommunityRequired(Exception): + """Record associated community required.""" + + @property + def description(self): + """Exception description.""" + return _("Last community on record cannot be removed.") + + class InvalidCommunityVisibility(Exception): """Community visibility does not match the content.""" diff --git a/invenio_rdm_records/services/permissions.py b/invenio_rdm_records/services/permissions.py index 865f15bdc0..9ec7f6542d 100644 --- a/invenio_rdm_records/services/permissions.py +++ b/invenio_rdm_records/services/permissions.py @@ -10,6 +10,7 @@ """Permissions for Invenio RDM Records.""" from invenio_communities.generators import CommunityCurators +from invenio_communities.permissions import CommunityMembers from invenio_records import Record from invenio_records_permissions.generators import ( AnyUser, @@ -147,8 +148,20 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): can_delete_draft = can_curate # Allow creating a new version of an existing published record. can_new_version = can_curate + can_publish_restricted = ( + can_manage[1:] + + [CommunityMembers()] + + [SecretLinks("edit")] + + [SubmissionReviewer()] + ) # Allow publishing a new record or changes to an existing record. - can_publish = can_review + can_publish = [ + IfConfig( + "RDM_ENSURE_RECORD_COMMUNITY_EXISTS", + then_=can_publish_restricted, + else_=can_review, + ), + ] # Allow lifting a record or draft. can_lift_embargo = can_manage diff --git a/tests/resources/test_resources_communities.py b/tests/resources/test_resources_communities.py index 2a0f59aa97..08c444ce4a 100644 --- a/tests/resources/test_resources_communities.py +++ b/tests/resources/test_resources_communities.py @@ -1,12 +1,14 @@ # -*- coding: utf-8 -*- # # Copyright (C) 2023 CERN. +# Copyright (C) 2023 KTH Royal Institute of Technology. # # Invenio-RDM-Records is free software; you can redistribute it and/or modify # it under the terms of the MIT License; see LICENSE file for more details. """Tests record's communities resources.""" +from contextlib import contextmanager from copy import deepcopy import pytest @@ -713,3 +715,226 @@ def test_search_communities( headers=headers, ) assert response.status_code == 403 + + +# Assure Records community exists tests +# ------------------------------------- +from invenio_records_resources.services.errors import PermissionDeniedError + + +@contextmanager +def ensure_record_community_exists_config(app): + """Context manager to ensure the configuration flag for ensuring record community exists + is set to True during a specific code block. + + Parameters: + app: app fixture + + Usage: + with ensure_record_community_exists_config(app): + # code block that requires the flag to be set""" + try: + app.config["RDM_ENSURE_RECORD_COMMUNITY_EXISTS"] = True + yield + finally: + app.config["RDM_ENSURE_RECORD_COMMUNITY_EXISTS"] = False + + +def test_restricted_record_creation( + app, record_community, uploader, curator, community_owner, test_user, superuser +): + """Verify PermissionDeniedError is raised when direct publish a record""" + # You can directly publish a record when the config is disabled + rec = record_community.create_record() + assert rec.id + with ensure_record_community_exists_config(app): + # You can't directly publish + users = [ + curator, + test_user, + uploader, + community_owner, + ] + for user in users: + with pytest.raises(PermissionDeniedError): + record_community.create_record(uploader=user) + + # Super user: creating records like a boss, doing whatever they like! + super_user_rec = record_community.create_record(uploader=superuser) + assert super_user_rec.id + + +def test_remove_last_existing_non_existing_community( + app, client, uploader, record_community, headers, community +): + """Test removal of an existing and non-existing community from the record, + while ensuring at least one community exists.""" + data = { + "communities": [ + {"id": "wrong-id"}, + {"id": community.id}, + {"id": "wrong-id2"}, + ] + } + + client = uploader.login(client) + record = record_community.create_record() + with ensure_record_community_exists_config(app): + response = client.delete( + f"/records/{record.pid.pid_value}/communities", + headers=headers, + json=data, + ) + assert response.status_code == 400 + # Should get 3 errors: Can't remove community, 2 bad IDs + assert len(response.json["errors"]) == 3 + record_saved = client.get(f"/records/{record.pid.pid_value}", headers=headers) + assert record_saved.json["parent"]["communities"] + + +def test_remove_last_community_api_error_handling( + record_community, + community, + uploader, + headers, + curator, + client, + app, +): + """Testing error message when trying to remove last community.""" + record = record_community.create_record() + data = {"communities": [{"id": community.id}]} + for user in [uploader, curator]: + client = user.login(client) + response = client.get( + f"/communities/{community.id}/records", + headers=headers, + json=data, + ) + assert ( + len(response.json["hits"]["hits"][0]["parent"]["communities"]["ids"]) == 1 + ) + with ensure_record_community_exists_config(app): + response = client.delete( + f"/records/{record.pid.pid_value}/communities", + headers=headers, + json=data, + ) + assert response.is_json + assert response.status_code == 400 + res_data = response.get_json() + assert len(res_data["errors"]) == 1 + assert len(res_data["errors"][0]["community"]) > 1 + assert len(res_data["errors"][0]["message"]) > 1 + record_saved = client.get( + f"/records/{record.pid.pid_value}", headers=headers + ) + assert record_saved.json["parent"]["communities"] + + client = user.logout(client) + # check communities number + response = client.get( + f"/communities/{community.id}/records", + headers=headers, + json=data, + ) + assert ( + len(response.json["hits"]["hits"][0]["parent"]["communities"]["ids"]) + == 1 + ) + + +def test_remove_record_last_community_with_multiple_communities( + closed_review_community, + open_review_community, + record_community, + community2, + uploader, + headers, + client, + app, + db, +): + """Testing correct removal of multiple communities""" + client = uploader.login(client) + + record = record_community.create_record() + comm = [ + community2, + open_review_community, + closed_review_community, + ] # one more in the rec fixuture so it's 4 + for com in comm: + _add_to_community(db, record, com) + assert len(record.parent.communities.ids) == 4 + + with ensure_record_community_exists_config(app): + data = {"communities": [{"id": x} for x in record.parent.communities.ids]} + + response = client.delete( + f"/records/{record.pid.pid_value}/communities", + headers=headers, + json=data, + ) + # You get res 200 with error msg if all communities you are deleting + assert response.status_code == 200 + assert "error" in str(response.data) + + rec_com_left = client.get(f"/records/{record.pid.pid_value}", headers=headers) + assert len(rec_com_left.json["parent"]["communities"]["ids"]) == 1 + + # You get res 400 with error msg if you Delete the last one only. + response = client.delete( + f"/records/{record.pid.pid_value}/communities", + headers=headers, + json={"communities": [{"id": str(record.parent.communities.ids[0])}]}, + ) + assert response.status_code == 400 + assert "error" in str(response.data) + + record_saved = client.get(f"/records/{record.pid.pid_value}", headers=headers) + assert ( + len(record_saved.json["parent"]["communities"]["ids"]) == 1 + ) # check that only one community ID is associated with the record + + +# TODO +# def test_remove_public_community_with_restricted_community_attached_error_handling( +# restricted_community, +# record_community, +# community, +# uploader, +# headers, +# client, +# app, +# db, +# ): +# """Testing removal of public community with restricted community attached""" +# client = uploader.login(client) + +# record = record_community.create_record() + +# # attach the restricted community to the public community +# _add_to_community(db, record, restricted_community) + +# with ensure_record_community_exists_config(app): +# # try to remove the public community +# data = {"communities": [{"id": record.parent.communities.ids[0]}]} +# response = client.delete( +# f"/records/{record.pid.pid_value}/communities", +# headers=headers, +# json=data, +# ) + +# # check that the response is a 400 error +# assert response.status_code == 400 + +# # check that the error message is correct +# # assert ( +# # response.json["errors"][0]["message"] +# # == "Cannot remove public community with restricted community attached." +# # ) + +# # check that the record is still attached to the public community +# record_saved = client.get(f"/records/{record.pid.pid_value}", headers=headers) +# assert record_saved.json["parent"]["communities"]["ids"][0] == community.id