From 385c788a0c4d4ef7871279aca84bfb87e25f3f6f Mon Sep 17 00:00:00 2001 From: Saksham Date: Mon, 16 Sep 2024 10:26:29 +0200 Subject: [PATCH 1/6] Services: Communities: Add feature to require community based on config --- invenio_rdm_records/config.py | 6 + invenio_rdm_records/resources/config.py | 1 + .../services/communities/service.py | 16 +- invenio_rdm_records/services/generators.py | 11 ++ invenio_rdm_records/services/permissions.py | 27 ++- tests/resources/test_resources_communities.py | 177 ++++++++++++++++++ 6 files changed, 234 insertions(+), 4 deletions(-) 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 From cd57bfd021b1902c616ffe9885e2c4b80bdabd1f Mon Sep 17 00:00:00 2001 From: Saksham Date: Fri, 20 Sep 2024 17:28:51 +0200 Subject: [PATCH 2/6] services: Override publish and remove APIs to check for new config --- invenio_rdm_records/resources/config.py | 7 +++ .../services/communities/service.py | 38 ++++++++------- invenio_rdm_records/services/errors.py | 12 +++++ invenio_rdm_records/services/permissions.py | 2 +- invenio_rdm_records/services/services.py | 24 ++++++++++ tests/resources/test_resources_communities.py | 47 ++++++++++--------- 6 files changed, 90 insertions(+), 40 deletions(-) diff --git a/invenio_rdm_records/resources/config.py b/invenio_rdm_records/resources/config.py index 80ab769bf..6675de01c 100644 --- a/invenio_rdm_records/resources/config.py +++ b/invenio_rdm_records/resources/config.py @@ -36,6 +36,7 @@ from ..services.errors import ( AccessRequestExistsError, + CommunityNotSelectedError, GrantExistsError, InvalidAccessRestrictions, RecordDeletedException, @@ -256,6 +257,12 @@ class RDMRecordResourceConfig(RecordResourceConfig, ConfiguratorMixin): description=e.description, ) ), + CommunityNotSelectedError: 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 c9265c0d2..5d84b2998 100644 --- a/invenio_rdm_records/services/communities/service.py +++ b/invenio_rdm_records/services/communities/service.py @@ -11,9 +11,7 @@ from flask import current_app from invenio_access.permissions import system_identity from invenio_communities.proxies import current_communities -from invenio_drafts_resources.services.records.uow import ( - ParentRecordCommitOp, -) +from invenio_drafts_resources.services.records.uow import ParentRecordCommitOp from invenio_i18n import lazy_gettext as _ from invenio_notifications.services.uow import NotificationOp from invenio_pidstore.errors import PIDDoesNotExistError @@ -22,10 +20,7 @@ Service, ServiceSchemaWrapper, ) -from invenio_records_resources.services.errors import ( - PermissionDeniedError, - CannotRemoveCommunityError, -) +from invenio_records_resources.services.errors import PermissionDeniedError from invenio_records_resources.services.uow import ( IndexRefreshOp, RecordIndexOp, @@ -40,6 +35,7 @@ from ...proxies import current_rdm_records, current_rdm_records_service from ...requests import CommunityInclusion from ..errors import ( + CannotRemoveCommunityError, CommunityAlreadyExists, InvalidAccessRestrictions, OpenRequestAlreadyExists, @@ -207,20 +203,22 @@ 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 + # If config is true and there is only 1 communities left to remove + is_community_required = current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] + is_last_community = len(record.parent.communities.ids) == 1 # Then, check for permissions to remove last community + can_remove_last_community = self.check_permission( + identity, "remove_community", record=record, community_id=community_id + ) if ( - current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] - and len(record.parent.communities.ids) == 1 + is_community_required + and is_last_community + and not can_remove_last_community ): - if not self.check_permission( - identity, "remove_community", record=record, community_id=community_id - ): - raise CannotRemoveCommunityError() + raise CannotRemoveCommunityError() # check permission here, per community: curator cannot remove another community - self.require_permission( - identity, "remove_community", record=record, community_id=community_id - ) + elif not can_remove_last_community: + raise PermissionDeniedError("remove_community") # Default community is deleted when the exact same community is removed from the record record.parent.communities.remove(community_id) @@ -245,7 +243,11 @@ def remove(self, identity, id_, data, uow): try: self._remove(identity, community_id, record) processed.append({"community": community_id}) - except (RecordCommunityMissing, PermissionDeniedError) as ex: + except ( + RecordCommunityMissing, + PermissionDeniedError, + CannotRemoveCommunityError, + ) as ex: errors.append( { "community": community_id, diff --git a/invenio_rdm_records/services/errors.py b/invenio_rdm_records/services/errors.py index 024ab39ee..5757c5d55 100644 --- a/invenio_rdm_records/services/errors.py +++ b/invenio_rdm_records/services/errors.py @@ -206,3 +206,15 @@ class RecordSubmissionClosedCommunityError(PermissionDenied): """Record submission policy forbids non-members from submitting records to community.""" description = "Submission to this community is only allowed to community members." + + +class CommunityNotSelectedError(Exception): + """Error thrown when a record is being created/updated with less than 1 community.""" + + description = "Cannot publish without selecting a community." + + +class CannotRemoveCommunityError(PermissionDenied): + """Error thrown when the last community is being removed from the record.""" + + description = "Cannot remove. A record should be part of atleast 1 community." diff --git a/invenio_rdm_records/services/permissions.py b/invenio_rdm_records/services/permissions.py index 486be998c..ea5efac14 100644 --- a/invenio_rdm_records/services/permissions.py +++ b/invenio_rdm_records/services/permissions.py @@ -30,12 +30,12 @@ AccessGrant, CommunityInclusionReviewers, GuestAccessRequestToken, - IfOneCommunity, IfCreate, IfDeleted, IfExternalDOIRecord, IfFileIsLocal, IfNewRecord, + IfOneCommunity, IfRecordDeleted, IfRequestType, IfRestricted, diff --git a/invenio_rdm_records/services/services.py b/invenio_rdm_records/services/services.py index 3841fdb29..25c10b159 100644 --- a/invenio_rdm_records/services/services.py +++ b/invenio_rdm_records/services/services.py @@ -20,6 +20,7 @@ from invenio_drafts_resources.services.records import RecordService from invenio_drafts_resources.services.records.uow import ParentRecordCommitOp from invenio_records_resources.services import LinksTemplate, ServiceSchemaWrapper +from invenio_records_resources.services.errors import PermissionDeniedError from invenio_records_resources.services.uow import ( RecordCommitOp, RecordIndexDeleteOp, @@ -37,6 +38,7 @@ from ..records.systemfields.deletion_status import RecordDeletionStatusEnum from .errors import ( + CommunityNotSelectedError, DeletionStatusException, EmbargoNotLiftedError, RecordDeletedException, @@ -404,6 +406,28 @@ def purge_record(self, identity, id_, uow=None): raise NotImplementedError() + def publish(self, identity, id_, uow=None, expand=False): + """Publish a draft. + + 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) + + # If config is true and there are no communities selected + is_community_required = current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] + is_community_missing = len(draft.parent.communities.ids) == 0 + # Then, check for permissions to upload without community + can_publish_draft = self.check_permission(identity, "publish", record=draft) + + if is_community_required and is_community_missing and not can_publish_draft: + raise CommunityNotSelectedError() + elif not can_publish_draft: + # If community is not missing or the community is not required, raise error if user doesn't have permissions + raise PermissionDeniedError("publish") + + return super().publish(identity, id_, uow=uow, expand=expand) + # # Search functions # diff --git a/tests/resources/test_resources_communities.py b/tests/resources/test_resources_communities.py index b3f63343f..2aff90fee 100644 --- a/tests/resources/test_resources_communities.py +++ b/tests/resources/test_resources_communities.py @@ -19,7 +19,10 @@ ) 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 InvalidAccessRestrictions +from invenio_rdm_records.services.errors import ( + CommunityNotSelectedError, + InvalidAccessRestrictions, +) def _add_to_community(db, record, community): @@ -891,21 +894,21 @@ def test_add_record_to_restricted_community_submission_open_member( 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 + 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 @@ -913,6 +916,7 @@ def ensure_record_community_exists_config(app): finally: app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] = False + def test_restricted_record_creation( app, record_community, uploader, curator, community_owner, test_user, superuser ): @@ -953,17 +957,17 @@ def test_remove_last_existing_non_existing_community( 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"] + response = client.delete( + f"/records/{record.pid.pid_value}/communities", + headers=headers, + json=data, + ) + assert response.is_json + assert response.status_code == 200 + # 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( @@ -995,12 +999,13 @@ def test_remove_last_community_api_error_handling( json=data, ) assert response.is_json - assert response.status_code == 400 + assert response.status_code == 200 record_saved = client.get( f"/records/{record.pid.pid_value}", headers=headers ) assert record_saved.json["parent"]["communities"] + assert len(response.json["errors"]) == 1 client = user.logout(client) # check communities number From daa9e94d7bb762610b22a446ecb2fb93a7865a3a Mon Sep 17 00:00:00 2001 From: Saksham Date: Mon, 23 Sep 2024 15:25:55 +0200 Subject: [PATCH 3/6] tests: Select community before publishing --- invenio_rdm_records/config.py | 4 +- invenio_rdm_records/services/errors.py | 2 +- invenio_rdm_records/services/services.py | 1 + tests/conftest.py | 36 ++++++++++++ tests/resources/test_resources_communities.py | 57 +++++++++++-------- 5 files changed, 74 insertions(+), 26 deletions(-) diff --git a/invenio_rdm_records/config.py b/invenio_rdm_records/config.py index 6e8710e8d..e7ebfc32c 100644 --- a/invenio_rdm_records/config.py +++ b/invenio_rdm_records/config.py @@ -134,8 +134,8 @@ def always_valid(identifier): # # Record communities # -RDM_RECORD_ALWAYS_IN_COMMUNITY = True -"""Enforces at least one community per record on remove community function.""" +RDM_RECORD_ALWAYS_IN_COMMUNITY = False +"""Enforces at least one community per record.""" # # Search configuration diff --git a/invenio_rdm_records/services/errors.py b/invenio_rdm_records/services/errors.py index 5757c5d55..011460fcc 100644 --- a/invenio_rdm_records/services/errors.py +++ b/invenio_rdm_records/services/errors.py @@ -214,7 +214,7 @@ class CommunityNotSelectedError(Exception): description = "Cannot publish without selecting a community." -class CannotRemoveCommunityError(PermissionDenied): +class CannotRemoveCommunityError(Exception): """Error thrown when the last community is being removed from the record.""" description = "Cannot remove. A record should be part of atleast 1 community." diff --git a/invenio_rdm_records/services/services.py b/invenio_rdm_records/services/services.py index 25c10b159..66ea2697b 100644 --- a/invenio_rdm_records/services/services.py +++ b/invenio_rdm_records/services/services.py @@ -406,6 +406,7 @@ def purge_record(self, identity, id_, uow=None): raise NotImplementedError() + @unit_of_work() def publish(self, identity, id_, uow=None, expand=False): """Publish a draft. diff --git a/tests/conftest.py b/tests/conftest.py index 29580de56..73e1afe3a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -67,6 +67,7 @@ from invenio_records_resources.proxies import current_service_registry from invenio_records_resources.references.entity_resolvers import ServiceResultResolver from invenio_records_resources.services.custom_fields import TextCF +from invenio_records_resources.services.uow import UnitOfWork from invenio_requests.notifications.builders import ( CommentRequestEventCreateNotificationBuilder, ) @@ -2080,6 +2081,41 @@ 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 2aff90fee..cb6ca4ee9 100644 --- a/tests/resources/test_resources_communities.py +++ b/tests/resources/test_resources_communities.py @@ -918,7 +918,13 @@ def ensure_record_community_exists_config(app): def test_restricted_record_creation( - app, record_community, uploader, curator, community_owner, test_user, superuser + 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 @@ -942,7 +948,7 @@ def test_restricted_record_creation( def test_remove_last_existing_non_existing_community( - app, client, uploader, record_community, headers, community + app, client, uploader, record_required_community, headers, community ): """Test removal of an existing and non-existing community from the record, while ensuring at least one community exists.""" @@ -955,23 +961,24 @@ def test_remove_last_existing_non_existing_community( } client = uploader.login(client) - record = record_community.create_record() + 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.pid_value}/communities", + f"/records/{record_pid}/communities", headers=headers, json=data, ) assert response.is_json - assert response.status_code == 200 + 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) + 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_community, + record_required_community, community, uploader, headers, @@ -980,7 +987,8 @@ def test_remove_last_community_api_error_handling( app, ): """Testing error message when trying to remove last community.""" - record = record_community.create_record() + record = record_required_community.create_record() + record_pid = record._record.pid.pid_value data = {"communities": [{"id": community.id}]} for user in [uploader, curator]: client = user.login(client) @@ -994,16 +1002,14 @@ def test_remove_last_community_api_error_handling( ) with ensure_record_community_exists_config(app): response = client.delete( - f"/records/{record.pid.pid_value}/communities", + f"/records/{record_pid}/communities", headers=headers, json=data, ) assert response.is_json - assert response.status_code == 200 + assert response.status_code == 400 - record_saved = client.get( - f"/records/{record.pid.pid_value}", headers=headers - ) + record_saved = client.get(f"/records/{record_pid}", headers=headers) assert record_saved.json["parent"]["communities"] assert len(response.json["errors"]) == 1 @@ -1023,7 +1029,7 @@ def test_remove_last_community_api_error_handling( def test_remove_record_last_community_with_multiple_communities( closed_review_community, open_review_community, - record_community, + record_required_community, community2, uploader, headers, @@ -1034,21 +1040,24 @@ def test_remove_record_last_community_with_multiple_communities( """Testing correct removal of multiple communities""" client = uploader.login(client) - record = record_community.create_record() + record = record_required_community.create_record() + record_pid = record._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, com) - assert len(record.parent.communities.ids) == 4 + _add_to_community(db, record._record, com) + assert len(record._record.parent.communities.ids) == 4 with ensure_record_community_exists_config(app): - data = {"communities": [{"id": x} for x in record.parent.communities.ids]} + data = { + "communities": [{"id": x} for x in record._record.parent.communities.ids] + } response = client.delete( - f"/records/{record.pid.pid_value}/communities", + f"/records/{record_pid}/communities", headers=headers, json=data, ) @@ -1056,18 +1065,20 @@ def test_remove_record_last_community_with_multiple_communities( assert response.status_code == 200 assert "error" in str(response.data) - rec_com_left = client.get(f"/records/{record.pid.pid_value}", headers=headers) + 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.pid_value}/communities", + f"/records/{record_pid}/communities", headers=headers, - json={"communities": [{"id": str(record.parent.communities.ids[0])}]}, + json={ + "communities": [{"id": str(record._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) + 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 From c983d91ab5f5de70e44d5ea7a30085561d2a1f50 Mon Sep 17 00:00:00 2001 From: Saksham Date: Thu, 26 Sep 2024 11:25:28 +0200 Subject: [PATCH 4/6] services: Handle case for draft republish with multiple communities --- invenio_rdm_records/services/generators.py | 11 +++++ invenio_rdm_records/services/permissions.py | 3 +- invenio_rdm_records/services/services.py | 11 +++-- tests/resources/test_resources_communities.py | 41 +++++++++++++++++++ 4 files changed, 59 insertions(+), 7 deletions(-) diff --git a/invenio_rdm_records/services/generators.py b/invenio_rdm_records/services/generators.py index b24959ba1..3754254a1 100644 --- a/invenio_rdm_records/services/generators.py +++ b/invenio_rdm_records/services/generators.py @@ -425,3 +425,14 @@ def _condition(self, record=None, **kwargs): 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 ea5efac14..7abc37db3 100644 --- a/invenio_rdm_records/services/permissions.py +++ b/invenio_rdm_records/services/permissions.py @@ -30,6 +30,7 @@ AccessGrant, CommunityInclusionReviewers, GuestAccessRequestToken, + IfAtleastOneCommunity, IfCreate, IfDeleted, IfExternalDOIRecord, @@ -204,7 +205,7 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): IfConfig( "RDM_RECORD_ALWAYS_IN_COMMUNITY", then_=[ - IfOneCommunity( + IfAtleastOneCommunity( then_=can_review, else_=[Administration(), SystemProcess()] ) ], diff --git a/invenio_rdm_records/services/services.py b/invenio_rdm_records/services/services.py index 66ea2697b..616cb9493 100644 --- a/invenio_rdm_records/services/services.py +++ b/invenio_rdm_records/services/services.py @@ -419,13 +419,12 @@ def publish(self, identity, id_, uow=None, expand=False): is_community_required = current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] is_community_missing = len(draft.parent.communities.ids) == 0 # Then, check for permissions to upload without community - can_publish_draft = self.check_permission(identity, "publish", record=draft) - - if is_community_required and is_community_missing and not can_publish_draft: + if ( + is_community_required + and is_community_missing + and not self.check_permission(identity, "publish", record=draft) + ): raise CommunityNotSelectedError() - elif not can_publish_draft: - # If community is not missing or the community is not required, raise error if user doesn't have permissions - raise PermissionDeniedError("publish") return super().publish(identity, id_, uow=uow, expand=expand) diff --git a/tests/resources/test_resources_communities.py b/tests/resources/test_resources_communities.py index cb6ca4ee9..895479e15 100644 --- a/tests/resources/test_resources_communities.py +++ b/tests/resources/test_resources_communities.py @@ -947,6 +947,47 @@ def test_restricted_record_creation( assert super_user_rec.id +def test_republish_with_mulitple_communities( + app, + db, + headers, + client, + minimal_record, + open_review_community, + record_required_community, + community2, + uploader, +): + """Verify new version of record with multiple communities can be re-published""" + 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 + + 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 ): From f857b4af593976c68d1f3c967d6641e95a92d3b2 Mon Sep 17 00:00:00 2001 From: Saksham Date: Wed, 2 Oct 2024 14:16:48 +0200 Subject: [PATCH 5/6] services: Add elevated permissions check for publish and community removal --- .../services/communities/service.py | 16 +++++---- invenio_rdm_records/services/errors.py | 8 +++-- invenio_rdm_records/services/generators.py | 22 ------------ invenio_rdm_records/services/permissions.py | 36 +++++-------------- invenio_rdm_records/services/services.py | 9 +++-- 5 files changed, 30 insertions(+), 61 deletions(-) diff --git a/invenio_rdm_records/services/communities/service.py b/invenio_rdm_records/services/communities/service.py index 5d84b2998..d25282924 100644 --- a/invenio_rdm_records/services/communities/service.py +++ b/invenio_rdm_records/services/communities/service.py @@ -203,12 +203,19 @@ 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 + # 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 - # Then, check for permissions to remove last community + # 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", record=record, community_id=community_id + identity, + "remove_community_elevated", + record=record, + community_id=community_id, ) if ( is_community_required @@ -216,9 +223,6 @@ def _remove(self, identity, community_id, record): and not can_remove_last_community ): raise CannotRemoveCommunityError() - # check permission here, per community: curator cannot remove another community - elif not can_remove_last_community: - raise PermissionDeniedError("remove_community") # 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 011460fcc..33c6e5d27 100644 --- a/invenio_rdm_records/services/errors.py +++ b/invenio_rdm_records/services/errors.py @@ -205,16 +205,18 @@ def description(self): class RecordSubmissionClosedCommunityError(PermissionDenied): """Record submission policy forbids non-members from submitting records to community.""" - description = "Submission to this community is only allowed to community members." + description = _( + "Submission to this community is only allowed to community members." + ) class CommunityNotSelectedError(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 selecting 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 atleast 1 community." + description = _("Cannot remove. 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 3754254a1..270476b27 100644 --- a/invenio_rdm_records/services/generators.py +++ b/invenio_rdm_records/services/generators.py @@ -414,25 +414,3 @@ 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 7abc37db3..de9681569 100644 --- a/invenio_rdm_records/services/permissions.py +++ b/invenio_rdm_records/services/permissions.py @@ -30,13 +30,11 @@ AccessGrant, CommunityInclusionReviewers, GuestAccessRequestToken, - IfAtleastOneCommunity, IfCreate, IfDeleted, IfExternalDOIRecord, IfFileIsLocal, IfNewRecord, - IfOneCommunity, IfRecordDeleted, IfRequestType, IfRestricted, @@ -68,6 +66,7 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): RecordOwners(), RecordCommunitiesAction("curate"), AccessGrant("manage"), + Administration(), SystemProcess(), ] can_curate = can_manage + [AccessGrant("edit"), SecretLinks("edit")] @@ -201,17 +200,9 @@ class RDMRecordPermissionPolicy(RecordPermissionPolicy): ), ] # Allow publishing a new record or changes to an existing record. - can_publish = [ - IfConfig( - "RDM_RECORD_ALWAYS_IN_COMMUNITY", - then_=[ - IfAtleastOneCommunity( - then_=can_review, else_=[Administration(), SystemProcess()] - ) - ], - else_=can_review, - ) - ] + can_publish = can_review + # Permission to allow special users to publish a record in special cases + can_publish_elevated = [Administration(), SystemProcess()] # Allow lifting a record or draft. can_lift_embargo = can_manage @@ -221,25 +212,16 @@ 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(), ] - can_remove_community = [ - IfConfig( - "RDM_RECORD_ALWAYS_IN_COMMUNITY", - then_=[ - IfOneCommunity( - then_=[Administration(), SystemProcess()], - else_=can_remove_community_, - ) - ], - else_=can_remove_community_, - ) - ] + # Permission to allow special users to remove community in special cases + can_remove_community_elevated = [Administration(), SystemProcess()] # Who can remove records from a community - can_remove_record = [CommunityCurators()] + can_remove_record = [CommunityCurators(), Administration()] # 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 616cb9493..eac5b4814 100644 --- a/invenio_rdm_records/services/services.py +++ b/invenio_rdm_records/services/services.py @@ -415,14 +415,17 @@ def publish(self, identity, id_, uow=None, expand=False): # Get the draft draft = self.draft_cls.pid.resolve(id_, registered_only=False) - # If config is true and there are no communities selected is_community_required = current_app.config["RDM_RECORD_ALWAYS_IN_COMMUNITY"] is_community_missing = len(draft.parent.communities.ids) == 0 - # Then, check for permissions to upload without community + # 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 self.check_permission(identity, "publish", record=draft) + and not can_publish_without_community ): raise CommunityNotSelectedError() From 5a993554b75d22657d783ada31054a252fba23e0 Mon Sep 17 00:00:00 2001 From: Saksham Date: Wed, 9 Oct 2024 09:50:18 +0200 Subject: [PATCH 6/6] services: Allow only admin to bypass feature flag --- invenio_rdm_records/config.py | 2 +- invenio_rdm_records/resources/config.py | 6 +- .../services/communities/service.py | 34 +-- invenio_rdm_records/services/errors.py | 6 +- invenio_rdm_records/services/generators.py | 16 ++ invenio_rdm_records/services/permissions.py | 37 ++- invenio_rdm_records/services/services.py | 31 +-- tests/conftest.py | 53 +--- tests/resources/test_resources_communities.py | 250 +++++++++--------- 9 files changed, 208 insertions(+), 227 deletions(-) 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..192ab61ad 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,10 +257,10 @@ 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.", + 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..ef4f69444 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 permission is denied, determine which error to raise, based on config + community_required = current_app.config["RDM_COMMUNITY_REQUIRED_TO_PUBLISH"] + is_last_community = len(record.parent.communities.ids) <= 1 + if community_required and is_last_community: + 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..154fddc09 100644 --- a/invenio_rdm_records/services/generators.py +++ b/invenio_rdm_records/services/generators.py @@ -414,3 +414,19 @@ 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 one community.""" + return bool(record and len(record.parent.communities.ids) == 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 at least one community.""" + return bool(record and record.parent.communities.ids) 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..e99d0761a 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 to publish, determine which error to raise, based on config + community_required = current_app.config["RDM_COMMUNITY_REQUIRED_TO_PUBLISH"] + is_community_missing = len(draft.parent.communities.ids) < 1 + if community_required and is_community_missing: + 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