diff --git a/invenio_rdm_records/config.py b/invenio_rdm_records/config.py index ddc1d1f11..6e8710e8d 100644 --- a/invenio_rdm_records/config.py +++ b/invenio_rdm_records/config.py @@ -131,6 +131,12 @@ def always_valid(identifier): RDM_ALLOW_RESTRICTED_RECORDS = True """Allow users to set restricted/private records.""" +# +# Record communities +# +RDM_RECORD_ALWAYS_IN_COMMUNITY = True +"""Enforces at least one community per record on remove community function.""" + # # Search configuration # diff --git a/invenio_rdm_records/resources/config.py b/invenio_rdm_records/resources/config.py index 3ea7673d3..80ab769bf 100644 --- a/invenio_rdm_records/resources/config.py +++ b/invenio_rdm_records/resources/config.py @@ -187,6 +187,7 @@ class RDMRecordResourceConfig(RecordResourceConfig, ConfiguratorMixin): ) error_handlers = { + **ErrorHandlersMixin.error_handlers, DeserializerError: create_error_handler( lambda exc: HTTPJSONException( code=400, diff --git a/invenio_rdm_records/services/communities/service.py b/invenio_rdm_records/services/communities/service.py index d8d4d7747..c9265c0d2 100644 --- a/invenio_rdm_records/services/communities/service.py +++ b/invenio_rdm_records/services/communities/service.py @@ -13,7 +13,6 @@ from invenio_communities.proxies import current_communities from invenio_drafts_resources.services.records.uow import ( ParentRecordCommitOp, - RecordCommitOp, ) from invenio_i18n import lazy_gettext as _ from invenio_notifications.services.uow import NotificationOp @@ -23,7 +22,10 @@ Service, ServiceSchemaWrapper, ) -from invenio_records_resources.services.errors import PermissionDeniedError +from invenio_records_resources.services.errors import ( + PermissionDeniedError, + CannotRemoveCommunityError, +) from invenio_records_resources.services.uow import ( IndexRefreshOp, RecordIndexOp, @@ -205,6 +207,16 @@ def _remove(self, identity, community_id, record): if community_id not in record.parent.communities.ids: raise RecordCommunityMissing(record.id, community_id) + # If config is True and there is only 1 communities left to remove + # Then, check for permissions to remove last community + if ( + current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] + and len(record.parent.communities.ids) == 1 + ): + if not self.check_permission( + identity, "remove_community", record=record, community_id=community_id + ): + raise CannotRemoveCommunityError() # check permission here, per community: curator cannot remove another community self.require_permission( identity, "remove_community", record=record, community_id=community_id diff --git a/invenio_rdm_records/services/generators.py b/invenio_rdm_records/services/generators.py index 270476b27..b24959ba1 100644 --- a/invenio_rdm_records/services/generators.py +++ b/invenio_rdm_records/services/generators.py @@ -414,3 +414,14 @@ def needs(self, request=None, **kwargs): return [AccessRequestTokenNeed(request["payload"]["token"])] return [] + + +class IfOneCommunity(ConditionalGenerator): + """Conditional generator for records always in communities case.""" + + def _condition(self, record=None, **kwargs): + """Check if the record is associated with zero or one community.""" + if record is None: + return True + rec_communities = record.parent.communities.ids + return len(rec_communities) == 1 diff --git a/invenio_rdm_records/services/permissions.py b/invenio_rdm_records/services/permissions.py index 52b7f6417..486be998c 100644 --- a/invenio_rdm_records/services/permissions.py +++ b/invenio_rdm_records/services/permissions.py @@ -30,6 +30,7 @@ AccessGrant, CommunityInclusionReviewers, GuestAccessRequestToken, + IfOneCommunity, IfCreate, IfDeleted, IfExternalDOIRecord, @@ -199,7 +200,17 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): ), ] # Allow publishing a new record or changes to an existing record. - can_publish = can_review + can_publish = [ + IfConfig( + "RDM_RECORD_ALWAYS_IN_COMMUNITY", + then_=[ + IfOneCommunity( + then_=can_review, else_=[Administration(), SystemProcess()] + ) + ], + else_=can_review, + ) + ] # Allow lifting a record or draft. can_lift_embargo = can_manage @@ -209,11 +220,23 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): # Who can add record to a community can_add_community = can_manage # Who can remove a community from a record - can_remove_community = [ + can_remove_community_ = [ RecordOwners(), CommunityCurators(), SystemProcess(), ] + can_remove_community = [ + IfConfig( + "RDM_RECORD_ALWAYS_IN_COMMUNITY", + then_=[ + IfOneCommunity( + then_=[Administration(), SystemProcess()], + else_=can_remove_community_, + ) + ], + else_=can_remove_community_, + ) + ] # Who can remove records from a community can_remove_record = [CommunityCurators()] # Who can add records to a community in bulk diff --git a/tests/resources/test_resources_communities.py b/tests/resources/test_resources_communities.py index f4ad9c4dd..b3f63343f 100644 --- a/tests/resources/test_resources_communities.py +++ b/tests/resources/test_resources_communities.py @@ -7,6 +7,7 @@ """Tests record's communities resources.""" +from contextlib import contextmanager from copy import deepcopy import pytest @@ -889,3 +890,179 @@ def test_add_record_to_restricted_community_submission_open_member( assert not response.json.get("errors") processed = response.json["processed"] assert len(processed) == 1 + +# Assure Records community exists tests +# ------------------------------------- +from invenio_records_resources.services.errors import CommunityNotSelectedError, CannotRemoveCommunityError + + +@contextmanager +def ensure_record_community_exists_config(app): + """ + Context manager to ensure record community exists config + 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_RECORD_ALWAYS_IN_COMMUNITY"] = True + yield + finally: + app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] = False + +def test_restricted_record_creation( + app, record_community, uploader, curator, community_owner, test_user, superuser +): + """Verify CommunityNotSelectedError 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(CommunityNotSelectedError): + record_community.create_record(uploader=user) + + # Super user can! + 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): + with pytest.raises(CannotRemoveCommunityError): + 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 + + 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 fixture 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) + # check that only one community ID is associated with the record + assert len(record_saved.json["parent"]["communities"]["ids"]) == 1