diff --git a/invenio_rdm_records/config.py b/invenio_rdm_records/config.py index e7ebfc32c..53d634f22 100644 --- a/invenio_rdm_records/config.py +++ b/invenio_rdm_records/config.py @@ -134,7 +134,7 @@ def always_valid(identifier): # # Record communities # -RDM_RECORD_ALWAYS_IN_COMMUNITY = False +RDM_COMMUNITY_REQUIRED_TO_PUBLISH = False """Enforces at least one community per record.""" # diff --git a/invenio_rdm_records/resources/config.py b/invenio_rdm_records/resources/config.py index 6675de01c..d03f42e9b 100644 --- a/invenio_rdm_records/resources/config.py +++ b/invenio_rdm_records/resources/config.py @@ -36,7 +36,7 @@ from ..services.errors import ( AccessRequestExistsError, - CommunityNotSelectedError, + CommunityRequiredError, GrantExistsError, InvalidAccessRestrictions, RecordDeletedException, @@ -257,7 +257,7 @@ class RDMRecordResourceConfig(RecordResourceConfig, ConfiguratorMixin): description=e.description, ) ), - CommunityNotSelectedError: create_error_handler( + CommunityRequiredError: create_error_handler( HTTPJSONException( code=400, description="Cannot publish without selecting a community.", diff --git a/invenio_rdm_records/services/communities/service.py b/invenio_rdm_records/services/communities/service.py index d25282924..91c08e37a 100644 --- a/invenio_rdm_records/services/communities/service.py +++ b/invenio_rdm_records/services/communities/service.py @@ -203,26 +203,20 @@ def _remove(self, identity, community_id, record): if community_id not in record.parent.communities.ids: raise RecordCommunityMissing(record.id, community_id) - # Check for permission to remove a community from a record - self.require_permission( - identity, "remove_community", record=record, community_id=community_id - ) - is_community_required = current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] - is_last_community = len(record.parent.communities.ids) == 1 - # If community is required for a record and if it is the last community to remove - # Then, only users with special permissions can remove - can_remove_last_community = self.check_permission( - identity, - "remove_community_elevated", - record=record, - community_id=community_id, - ) - if ( - is_community_required - and is_last_community - and not can_remove_last_community - ): - raise CannotRemoveCommunityError() + try: + self.require_permission( + identity, "remove_community", record=record, community_id=community_id + ) + # By default, admin/superuser has permission to do everything, so PermissionDeniedError won't be raised for admin in any case + except PermissionDeniedError as exc: + # If user doesn't have permission, then check if community is required + if current_app.config["RDM_COMMUNITY_REQUIRED_TO_PUBLISH"]: + # If community is required for a record and if it is the last community to remove, raise error + if len(record.parent.communities.ids) == 1: + raise CannotRemoveCommunityError() + else: + # If the config wasn't enabled, then raise the PermissionDeniedError + raise exc # Default community is deleted when the exact same community is removed from the record record.parent.communities.remove(community_id) diff --git a/invenio_rdm_records/services/errors.py b/invenio_rdm_records/services/errors.py index 33c6e5d27..916cd25a0 100644 --- a/invenio_rdm_records/services/errors.py +++ b/invenio_rdm_records/services/errors.py @@ -210,13 +210,13 @@ class RecordSubmissionClosedCommunityError(PermissionDenied): ) -class CommunityNotSelectedError(Exception): +class CommunityRequiredError(Exception): """Error thrown when a record is being created/updated with less than 1 community.""" - description = _("Cannot publish without selecting a community.") + description = _("Cannot publish without a community.") class CannotRemoveCommunityError(Exception): """Error thrown when the last community is being removed from the record.""" - description = _("Cannot remove. A record should be part of at least 1 community.") + description = _("A record should be part of at least 1 community.") diff --git a/invenio_rdm_records/services/generators.py b/invenio_rdm_records/services/generators.py index 270476b27..f6c1dbd7e 100644 --- a/invenio_rdm_records/services/generators.py +++ b/invenio_rdm_records/services/generators.py @@ -414,3 +414,25 @@ 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 + + +class IfAtLeastOneCommunity(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) > 0 diff --git a/invenio_rdm_records/services/permissions.py b/invenio_rdm_records/services/permissions.py index de9681569..5cc3243cb 100644 --- a/invenio_rdm_records/services/permissions.py +++ b/invenio_rdm_records/services/permissions.py @@ -30,11 +30,13 @@ AccessGrant, CommunityInclusionReviewers, GuestAccessRequestToken, + IfAtLeastOneCommunity, IfCreate, IfDeleted, IfExternalDOIRecord, IfFileIsLocal, IfNewRecord, + IfOneCommunity, IfRecordDeleted, IfRequestType, IfRestricted, @@ -66,7 +68,6 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): RecordOwners(), RecordCommunitiesAction("curate"), AccessGrant("manage"), - Administration(), SystemProcess(), ] can_curate = can_manage + [AccessGrant("edit"), SecretLinks("edit")] @@ -200,9 +201,18 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): ), ] # Allow publishing a new record or changes to an existing record. - can_publish = can_review - # Permission to allow special users to publish a record in special cases - can_publish_elevated = [Administration(), SystemProcess()] + can_publish = [ + IfConfig( + "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", + then_=[ + IfAtLeastOneCommunity( + then_=can_review, + else_=[Administration(), SystemProcess()], + ), + ], + else_=can_review, + ) + ] # Allow lifting a record or draft. can_lift_embargo = can_manage @@ -212,16 +222,25 @@ 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(), - Administration(), SystemProcess(), ] - # Permission to allow special users to remove community in special cases - can_remove_community_elevated = [Administration(), SystemProcess()] + can_remove_community = [ + IfConfig( + "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", + then_=[ + IfOneCommunity( + then_=[Administration(), SystemProcess()], + else_=can_remove_community_, + ), + ], + else_=can_remove_community_, + ), + ] # Who can remove records from a community - can_remove_record = [CommunityCurators(), Administration()] + can_remove_record = [CommunityCurators(), Administration(), SystemProcess()] # Who can add records to a community in bulk can_bulk_add = [SystemProcess()] diff --git a/invenio_rdm_records/services/services.py b/invenio_rdm_records/services/services.py index eac5b4814..428cd5040 100644 --- a/invenio_rdm_records/services/services.py +++ b/invenio_rdm_records/services/services.py @@ -38,7 +38,7 @@ from ..records.systemfields.deletion_status import RecordDeletionStatusEnum from .errors import ( - CommunityNotSelectedError, + CommunityRequiredError, DeletionStatusException, EmbargoNotLiftedError, RecordDeletedException, @@ -412,22 +412,19 @@ def publish(self, identity, id_, uow=None, expand=False): Check for permissions to publish a draft and then call invenio_drafts_resourcs.services.records.services.publish() """ - # Get the draft - draft = self.draft_cls.pid.resolve(id_, registered_only=False) - - is_community_required = current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] - is_community_missing = len(draft.parent.communities.ids) == 0 - # If community is required for a record and there are no communities selected - # Then, check for permissions to publish without community - can_publish_without_community = self.check_permission( - identity, "publish_elevated", record=draft - ) - if ( - is_community_required - and is_community_missing - and not can_publish_without_community - ): - raise CommunityNotSelectedError() + try: + draft = self.draft_cls.pid.resolve(id_, registered_only=False) + self.require_permission(identity, "publish", record=draft) + # By default, admin/superuser has permission to do everything, so PermissionDeniedError won't be raised for admin in any case + except PermissionDeniedError as exc: + # If user doesn't have permission, then check if community is required + if current_app.config["RDM_COMMUNITY_REQUIRED_TO_PUBLISH"]: + # If there are no communities selected, raise custom error to inform user + if len(draft.parent.communities.ids) == 0: + raise CommunityRequiredError() + else: + # If the config wasn't enabled, then raise the PermissionDeniedError + raise exc return super().publish(identity, id_, uow=uow, expand=expand) diff --git a/tests/conftest.py b/tests/conftest.py index 73e1afe3a..3763a1fbe 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2014,20 +2014,22 @@ def create_record( """Creates new record that belongs to the same community.""" # create draft draft = current_rdm_records_service.create(uploader.identity, record_dict) - # publish and get record - result_item = current_rdm_records_service.publish( - uploader.identity, draft.id - ) - record = result_item._record + record = draft._record if community: # add the record to the community community_record = community._record record.parent.communities.add(community_record, default=False) record.parent.commit() db.session.commit() - current_rdm_records_service.indexer.index( - record, arguments={"refresh": True} - ) + + # publish and get record + result_item = current_rdm_records_service.publish( + uploader.identity, draft.id + ) + record = result_item._record + current_rdm_records_service.indexer.index( + record, arguments={"refresh": True} + ) return record @@ -2081,41 +2083,6 @@ def create_record( return RecordFactory() -@pytest.fixture() -def record_required_community(db, uploader, minimal_record, community): - """Creates a record that belongs to a community before publishing.""" - - class Record: - """Test record class.""" - - def create_record( - self, - record_dict=minimal_record, - uploader=uploader, - community=community, - ): - """Creates new record that belongs to the same community.""" - # create draft - draft = current_rdm_records_service.create(uploader.identity, record_dict) - record = draft._record - # add the record to the community - community_record = community._record - record.parent.communities.add(community_record, default=False) - record.parent.commit() - db.session.commit() - current_rdm_records_service.indexer.index( - record, arguments={"refresh": True} - ) - - # publish and get record - community_record = current_rdm_records_service.publish( - uploader.identity, draft.id - ) - return community_record - - return Record() - - @pytest.fixture(scope="session") def headers(): """Default headers for making requests.""" diff --git a/tests/resources/test_resources_communities.py b/tests/resources/test_resources_communities.py index 895479e15..db56027e2 100644 --- a/tests/resources/test_resources_communities.py +++ b/tests/resources/test_resources_communities.py @@ -20,7 +20,7 @@ from invenio_rdm_records.records.api import RDMDraft, RDMRecord from invenio_rdm_records.requests.community_inclusion import CommunityInclusion from invenio_rdm_records.services.errors import ( - CommunityNotSelectedError, + CommunityRequiredError, InvalidAccessRestrictions, ) @@ -899,24 +899,6 @@ def test_add_record_to_restricted_community_submission_open_member( # ------------------------------------- -@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, @@ -925,26 +907,28 @@ def test_restricted_record_creation( community_owner, test_user, superuser, + monkeypatch, ): - """Verify CommunityNotSelectedError is raised when direct publish a record""" + """Verify CommunityRequiredError is raised when direct publish a record""" # You can directly publish a record when the config is disabled - rec = record_community.create_record() + monkeypatch.setitem(app.config, "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", False) + rec = record_community.create_record(community=None) 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) + monkeypatch.setitem(app.config, "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", True) + # You can't directly publish + users = [ + curator, + test_user, + uploader, + community_owner, + ] + for user in users: + with pytest.raises(CommunityRequiredError): + record_community.create_record(uploader=user, community=None) - # Super user can! - super_user_rec = record_community.create_record(uploader=superuser) - assert super_user_rec.id + # Super user can! + super_user_rec = record_community.create_record(uploader=superuser, community=None) + assert super_user_rec.id def test_republish_with_mulitple_communities( @@ -954,45 +938,53 @@ def test_republish_with_mulitple_communities( client, minimal_record, open_review_community, - record_required_community, + record_community, community2, uploader, + monkeypatch, ): """Verify new version of record with multiple communities can be re-published""" + monkeypatch.setitem(app.config, "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", True) client = uploader.login(client) comm = [ community2, open_review_community, ] - with ensure_record_community_exists_config(app): - record = record_required_community.create_record() - record_pid = record._record.pid.pid_value - for com in comm: - _add_to_community(db, record._record, com) - assert len(record._record.parent.communities.ids) == 3 - response = client.post( - f"/records/{record_pid}/versions", - headers=headers, - ) - assert response.is_json - assert response.status_code == 201 - current_rdm_records_service.update_draft( - uploader.identity, response.json["id"], minimal_record - ) - result_item = current_rdm_records_service.publish( - uploader.identity, response.json["id"] - ) - new_record_pid = result_item._record.pid.pid_value + record = record_community.create_record() + record_pid = record.pid.pid_value + for com in comm: + _add_to_community(db, record, com) + assert len(record.parent.communities.ids) == 3 + response = client.post( + f"/records/{record_pid}/versions", + headers=headers, + ) + assert response.is_json + assert response.status_code == 201 + current_rdm_records_service.update_draft( + uploader.identity, response.json["id"], minimal_record + ) + result_item = current_rdm_records_service.publish( + uploader.identity, response.json["id"] + ) + new_record_pid = result_item._record.pid.pid_value - new_record = client.get(f"/records/{new_record_pid}", headers=headers) - assert len(new_record.json["parent"]["communities"]["ids"]) == 3 + new_record = client.get(f"/records/{new_record_pid}", headers=headers) + assert len(new_record.json["parent"]["communities"]["ids"]) == 3 def test_remove_last_existing_non_existing_community( - app, client, uploader, record_required_community, headers, community + app, + client, + uploader, + record_community, + headers, + community, + monkeypatch, ): """Test removal of an existing and non-existing community from the record, while ensuring at least one community exists.""" + monkeypatch.setitem(app.config, "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", True) data = { "communities": [ {"id": "wrong-id"}, @@ -1002,34 +994,35 @@ def test_remove_last_existing_non_existing_community( } client = uploader.login(client) - record = record_required_community.create_record() - record_pid = record._record.pid.pid_value - with ensure_record_community_exists_config(app): - response = client.delete( - f"/records/{record_pid}/communities", - headers=headers, - json=data, - ) - assert response.is_json - 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}", headers=headers) - assert record_saved.json["parent"]["communities"] + record = record_community.create_record() + record_pid = record.pid.pid_value + response = client.delete( + f"/records/{record_pid}/communities", + headers=headers, + json=data, + ) + assert response.is_json + 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}", headers=headers) + assert record_saved.json["parent"]["communities"] def test_remove_last_community_api_error_handling( - record_required_community, + record_community, community, uploader, headers, curator, client, app, + monkeypatch, ): """Testing error message when trying to remove last community.""" - record = record_required_community.create_record() - record_pid = record._record.pid.pid_value + monkeypatch.setitem(app.config, "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", True) + record = record_community.create_record() + record_pid = record.pid.pid_value data = {"communities": [{"id": community.id}]} for user in [uploader, curator]: client = user.login(client) @@ -1041,85 +1034,80 @@ def test_remove_last_community_api_error_handling( 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}/communities", - headers=headers, - json=data, - ) - assert response.is_json - assert response.status_code == 400 - - record_saved = client.get(f"/records/{record_pid}", headers=headers) - assert record_saved.json["parent"]["communities"] - assert len(response.json["errors"]) == 1 - - 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 - ) + response = client.delete( + f"/records/{record_pid}/communities", + headers=headers, + json=data, + ) + assert response.is_json + assert response.status_code == 400 + + record_saved = client.get(f"/records/{record_pid}", headers=headers) + assert record_saved.json["parent"]["communities"] + assert len(response.json["errors"]) == 1 + + 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_required_community, + record_community, community2, uploader, headers, client, app, db, + monkeypatch, ): """Testing correct removal of multiple communities""" + monkeypatch.setitem(app.config, "RDM_COMMUNITY_REQUIRED_TO_PUBLISH", True) client = uploader.login(client) - record = record_required_community.create_record() - record_pid = record._record.pid.pid_value + record = record_community.create_record() + record_pid = record.pid.pid_value 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._record, com) - assert len(record._record.parent.communities.ids) == 4 + _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._record.parent.communities.ids] - } + data = {"communities": [{"id": x} for x in record.parent.communities.ids]} - response = client.delete( - f"/records/{record_pid}/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) + response = client.delete( + f"/records/{record_pid}/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}", headers=headers) - assert len(rec_com_left.json["parent"]["communities"]["ids"]) == 1 + rec_com_left = client.get(f"/records/{record_pid}", 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}/communities", - headers=headers, - json={ - "communities": [{"id": str(record._record.parent.communities.ids[0])}] - }, - ) - assert response.status_code == 400 - assert "error" in str(response.data) + # You get res 400 with error msg if you Delete the last one only. + response = client.delete( + f"/records/{record_pid}/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}", headers=headers) - # check that only one community ID is associated with the record - assert len(record_saved.json["parent"]["communities"]["ids"]) == 1 + record_saved = client.get(f"/records/{record_pid}", headers=headers) + # check that only one community ID is associated with the record + assert len(record_saved.json["parent"]["communities"]["ids"]) == 1