From 801c29380827135595605d44f0ad5d8e7c06c415 Mon Sep 17 00:00:00 2001
From: Javier Romero Castro <jrcastro9515@gmail.com>
Date: Fri, 8 Sep 2023 10:18:32 +0200
Subject: [PATCH] service: prevent creating a request if invalid restrictions

* Community selection modal checks if the community is
  restricted and disables it if the record is public
* closes https://github.com/inveniosoftware/invenio-app-rdm/issues/2384
---
 .../CommunityHeader/CommunityHeader.js        |  3 ++
 .../CommunityListItem.js                      | 40 ++++++++++++++-----
 .../CommunitySelectionModal.js                |  4 +-
 .../CommunitySelectionSearch.js               |  8 +++-
 .../SubmitReviewOrPublishButton.js            |  3 ++
 .../requests/community_inclusion.py           |  4 +-
 .../requests/community_submission.py          |  6 +++
 .../services/community_inclusion/service.py   |  4 --
 tests/resources/test_resources_communities.py | 39 +++++++++++-------
 tests/services/test_service_review.py         | 14 ++++---
 10 files changed, 85 insertions(+), 40 deletions(-)

diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunityHeader/CommunityHeader.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunityHeader/CommunityHeader.js
index 6dc9f0a0c..7b863e294 100644
--- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunityHeader/CommunityHeader.js
+++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunityHeader/CommunityHeader.js
@@ -30,6 +30,7 @@ class CommunityHeaderComponent extends Component {
       showCommunitySelectionButton,
       disableCommunitySelectionButton,
       showCommunityHeader,
+      record,
     } = this.props;
     const { modalOpen } = this.state;
 
@@ -73,6 +74,7 @@ class CommunityHeaderComponent extends Component {
                     modalOpen={modalOpen}
                     chosenCommunity={community}
                     displaySelected
+                    record={record}
                     trigger={
                       <Button
                         className="community-header-button"
@@ -119,6 +121,7 @@ CommunityHeaderComponent.propTypes = {
   showCommunitySelectionButton: PropTypes.bool.isRequired,
   showCommunityHeader: PropTypes.bool.isRequired,
   changeSelectedCommunity: PropTypes.func.isRequired,
+  record: PropTypes.object.isRequired,
 };
 
 CommunityHeaderComponent.defaultProps = {
diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunityListItem.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunityListItem.js
index 0fede3819..80a51b7a2 100644
--- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunityListItem.js
+++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunityListItem.js
@@ -11,8 +11,9 @@ import React, { useContext } from "react";
 import { Button, Icon, Label } from "semantic-ui-react";
 import { CommunityCompactItem } from "@js/invenio_communities/community";
 import { CommunityContext } from "./CommunityContext";
+import { InvenioPopup } from "react-invenio-forms";
 
-export const CommunityListItem = ({ result }) => {
+export const CommunityListItem = ({ result, record }) => {
   const {
     setLocalCommunity,
     getChosenCommunity,
@@ -23,17 +24,35 @@ export const CommunityListItem = ({ result }) => {
   const { metadata } = result;
   const itemSelected = getChosenCommunity()?.id === result.id;
   const userMembership = userCommunitiesMemberships[result["id"]];
+  const invalidPermissionLevel =
+    record.access.record === "public" && result.access.visibility === "restricted";
 
   const actions = (
-    <Button
-      content={
-        displaySelected && itemSelected ? i18next.t("Selected") : i18next.t("Select")
-      }
-      size="tiny"
-      positive={displaySelected && itemSelected}
-      onClick={() => setLocalCommunity(result)}
-      aria-label={i18next.t("Select {{title}}", { title: metadata.title })}
-    />
+    <>
+      {invalidPermissionLevel && (
+        <InvenioPopup
+          popupId="community-inclusion-info-popup"
+          size="small"
+          trigger={
+            <Icon className="mb-5" color="grey" name="question circle outline" />
+          }
+          ariaLabel={i18next.t("Community inclusion information")}
+          content={i18next.t(
+            "Submission to this community is only allowed if the record is restricted."
+          )}
+        />
+      )}
+      <Button
+        content={
+          displaySelected && itemSelected ? i18next.t("Selected") : i18next.t("Select")
+        }
+        size="tiny"
+        positive={displaySelected && itemSelected}
+        onClick={() => setLocalCommunity(result)}
+        disabled={invalidPermissionLevel}
+        aria-label={i18next.t("Select {{title}}", { title: metadata.title })}
+      />
+    </>
   );
 
   const extraLabels = userMembership && (
@@ -55,4 +74,5 @@ export const CommunityListItem = ({ result }) => {
 
 CommunityListItem.propTypes = {
   result: PropTypes.object.isRequired,
+  record: PropTypes.object.isRequired,
 };
diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionModal.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionModal.js
index 42ae087d2..4e7e302ed 100644
--- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionModal.js
+++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionModal.js
@@ -64,6 +64,7 @@ export class CommunitySelectionModalComponent extends Component {
       modalOpen,
       apiConfigs,
       handleClose,
+      record,
     } = this.props;
 
     return (
@@ -93,7 +94,7 @@ export class CommunitySelectionModalComponent extends Component {
             </Header>
           </Modal.Header>
 
-          <CommunitySelectionSearch apiConfigs={apiConfigs} />
+          <CommunitySelectionSearch apiConfigs={apiConfigs} record={record} />
 
           {extraContentComponents && (
             <Modal.Content>{extraContentComponents}</Modal.Content>
@@ -120,6 +121,7 @@ CommunitySelectionModalComponent.propTypes = {
   modalOpen: PropTypes.bool,
   apiConfigs: PropTypes.object,
   handleClose: PropTypes.func.isRequired,
+  record: PropTypes.object.isRequired,
 };
 
 CommunitySelectionModalComponent.defaultProps = {
diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionSearch.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionSearch.js
index 8cbb00d43..08ea7a749 100644
--- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionSearch.js
+++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/components/CommunitySelectionModal/CommunitySelectionSearch.js
@@ -7,7 +7,7 @@
 
 import { i18next } from "@translations/invenio_rdm_records/i18next";
 import React, { Component } from "react";
-import { OverridableContext } from "react-overridable";
+import { OverridableContext, parametrize } from "react-overridable";
 import {
   EmptyResults,
   Error,
@@ -47,10 +47,13 @@ export class CommunitySelectionSearch extends Component {
     } = this.state;
     const {
       apiConfigs: { allCommunities, myCommunities },
+      record,
     } = this.props;
     const searchApi = new InvenioSearchApi(selectedsearchApi);
     const overriddenComponents = {
-      [`${selectedAppId}.ResultsList.item`]: CommunityListItem,
+      [`${selectedAppId}.ResultsList.item`]: parametrize(CommunityListItem, {
+        record: record,
+      }),
     };
     return (
       <OverridableContext.Provider value={overriddenComponents}>
@@ -163,6 +166,7 @@ CommunitySelectionSearch.propTypes = {
       searchApi: PropTypes.object.isRequired,
     }),
   }),
+  record: PropTypes.object.isRequired,
 };
 
 CommunitySelectionSearch.defaultProps = {
diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/SubmitReviewOrPublishButton.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/SubmitReviewOrPublishButton.js
index bb920f220..6d7a0bfdb 100644
--- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/SubmitReviewOrPublishButton.js
+++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/SubmitReviewOrPublishButton.js
@@ -28,6 +28,7 @@ class SubmitReviewOrPublishComponent extends Component {
       showChangeCommunityButton,
       showDirectPublishButton,
       showSubmitForReviewButton,
+      record,
       ...ui
     } = this.props;
     const { modalOpen } = this.state;
@@ -52,6 +53,7 @@ class SubmitReviewOrPublishComponent extends Component {
             onModalChange={(value) => this.setState({ modalOpen: value })}
             modalOpen={modalOpen}
             displaySelected
+            record={record}
             chosenCommunity={community}
             trigger={
               <Button content={i18next.t("Change community")} fluid className="mb-10" />
@@ -77,6 +79,7 @@ SubmitReviewOrPublishComponent.propTypes = {
   showChangeCommunityButton: PropTypes.bool.isRequired,
   showDirectPublishButton: PropTypes.bool.isRequired,
   showSubmitForReviewButton: PropTypes.bool.isRequired,
+  record: PropTypes.object.isRequired,
 };
 
 SubmitReviewOrPublishComponent.defaultProps = {
diff --git a/invenio_rdm_records/requests/community_inclusion.py b/invenio_rdm_records/requests/community_inclusion.py
index 4bc0b89d7..19fa6937d 100644
--- a/invenio_rdm_records/requests/community_inclusion.py
+++ b/invenio_rdm_records/requests/community_inclusion.py
@@ -11,7 +11,6 @@
 from invenio_i18n import lazy_gettext as _
 from invenio_records_resources.services.uow import RecordIndexOp
 from invenio_requests.customizations import RequestType, actions
-from invenio_requests.errors import CannotExecuteActionError
 
 from ..proxies import current_rdm_records_service as service
 from ..services.errors import InvalidAccessRestrictions
@@ -53,8 +52,7 @@ def execute(self, identity, uow):
         assert not record.parent.review
 
         if not is_access_restriction_valid(record, community):
-            description = InvalidAccessRestrictions.description
-            raise CannotExecuteActionError(description)
+            raise InvalidAccessRestrictions()
 
         # set the community to `default` if it is the first
         default = not record.parent.communities
diff --git a/invenio_rdm_records/requests/community_submission.py b/invenio_rdm_records/requests/community_submission.py
index 619f393ca..1ab6ad513 100644
--- a/invenio_rdm_records/requests/community_submission.py
+++ b/invenio_rdm_records/requests/community_submission.py
@@ -13,7 +13,9 @@
 from invenio_requests.customizations import actions
 
 from ..proxies import current_rdm_records_service as service
+from ..services.errors import InvalidAccessRestrictions
 from .base import ReviewRequest
+from .community_inclusion import is_access_restriction_valid
 
 
 #
@@ -42,6 +44,10 @@ def execute(self, identity, uow):
         community = self.request.receiver.resolve()
         service._validate_draft(identity, draft)
 
+        # validate record and community access
+        if not is_access_restriction_valid(draft, community):
+            raise InvalidAccessRestrictions()
+
         # Unset review from record (still accessible from request)
         # The curator (receiver) should still have access, via the community
         # The creator (uploader) should also still have access, because
diff --git a/invenio_rdm_records/services/community_inclusion/service.py b/invenio_rdm_records/services/community_inclusion/service.py
index a27cea596..35ed3cb7e 100644
--- a/invenio_rdm_records/services/community_inclusion/service.py
+++ b/invenio_rdm_records/services/community_inclusion/service.py
@@ -44,10 +44,6 @@ def submit(self, identity, record, community, request, data, uow):
         if request.type.type_id not in self.supported_types:
             raise ValueError("Invalid request type.")
 
-        # validate record and community access
-        if not is_access_restriction_valid(record, community):
-            raise InvalidAccessRestrictions()
-
         # All other preconditions can be checked by the action itself which can
         # raise appropriate exceptions.
         return current_requests_service.execute_action(
diff --git a/tests/resources/test_resources_communities.py b/tests/resources/test_resources_communities.py
index 3761f17fd..1dee85b84 100644
--- a/tests/resources/test_resources_communities.py
+++ b/tests/resources/test_resources_communities.py
@@ -18,6 +18,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 InvalidAccessRestrictions
 
 
 def _add_to_community(db, record, community):
@@ -313,12 +314,13 @@ def test_restrict_community_before_accepting_inclusion(
     assert response.status_code == 200
 
     # accept request should fail
-    response = client.post(
-        f"/requests/{request_id}/actions/accept",
-        headers=headers,
-        json={},
-    )
-    assert response.status_code == 400
+    # The error handlers for this action are defined in invenio-app-rdm, therefore we catch the exception here
+    with pytest.raises(InvalidAccessRestrictions):
+        client.post(
+            f"/requests/{request_id}/actions/accept",
+            headers=headers,
+            json={},
+        )
 
 
 def test_create_new_version_after_inclusion_request(
@@ -417,14 +419,15 @@ def test_create_new_version_after_inclusion_request(
     assert hit["metadata"]["title"] == second_version_record["metadata"]["title"]
 
 
-def test_include_public_record_in_restricted_community(
+def test_accept_public_record_in_restricted_community(
     client,
     uploader,
     record_community,
     headers,
     restricted_community,
+    community_owner,
 ):
-    """Test include public record in restricted community."""
+    """Test accept public record in restricted community."""
     client = uploader.login(client)
 
     data = {
@@ -438,12 +441,20 @@ def test_include_public_record_in_restricted_community(
         headers=headers,
         json=data,
     )
-    assert response.status_code == 400
-    assert response.json["errors"]
-    assert (
-        "cannot be included in a restricted community"
-        in response.json["errors"][0]["message"]
-    )
+    assert response.status_code == 200
+    assert response.json["processed"]
+    assert len(response.json["processed"]) == 1
+    request_id = response.json["processed"][0]["request_id"]
+    client = uploader.logout(client)
+    client = community_owner.login(client)
+
+    # The error handlers for this action are defined in invenio-app-rdm, therefore we catch the exception here
+    with pytest.raises(InvalidAccessRestrictions):
+        client.post(
+            f"/requests/{request_id}/actions/accept",
+            headers=headers,
+            json={},
+        )
 
 
 def test_include_community_already_in(
diff --git a/tests/services/test_service_review.py b/tests/services/test_service_review.py
index 7d592bf0f..32892eb6f 100644
--- a/tests/services/test_service_review.py
+++ b/tests/services/test_service_review.py
@@ -377,14 +377,16 @@ def test_create_review_after_draft(running_app, community, service, minimal_reco
     assert draft["status"] == DraftStatus.review_to_draft_statuses[req["status"]]
 
 
-def test_submit_public_record_review_to_restricted_community(
-    running_app, public_draft_review_restricted, service
+def test_accept_public_record_review_to_restricted_community(
+    running_app, public_draft_review_restricted, service, requests_service
 ):
-    """Test creation of review after draft was created."""
-    # Create draft
+    """Test invalid record/community restrictions on accept."""
+    request = service.review.submit(
+        running_app.superuser_identity, public_draft_review_restricted.id
+    )
     with pytest.raises(InvalidAccessRestrictions):
-        service.review.submit(
-            running_app.superuser_identity, public_draft_review_restricted.id
+        requests_service.execute_action(
+            running_app.superuser_identity, request.id, "accept", {}
         )