Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

service: prevent creating a request if invalid restrictions #1451

Merged
merged 1 commit into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class CommunityHeaderComponent extends Component {
showCommunitySelectionButton,
disableCommunitySelectionButton,
showCommunityHeader,
record,
} = this.props;
const { modalOpen } = this.state;

Expand Down Expand Up @@ -73,6 +74,7 @@ class CommunityHeaderComponent extends Component {
modalOpen={modalOpen}
chosenCommunity={community}
displaySelected
record={record}
trigger={
<Button
className="community-header-button"
Expand Down Expand Up @@ -119,6 +121,7 @@ CommunityHeaderComponent.propTypes = {
showCommunitySelectionButton: PropTypes.bool.isRequired,
showCommunityHeader: PropTypes.bool.isRequired,
changeSelectedCommunity: PropTypes.func.isRequired,
record: PropTypes.object.isRequired,
};

CommunityHeaderComponent.defaultProps = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 && (
Expand All @@ -55,4 +74,5 @@ export const CommunityListItem = ({ result }) => {

CommunityListItem.propTypes = {
result: PropTypes.object.isRequired,
record: PropTypes.object.isRequired,
};
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export class CommunitySelectionModalComponent extends Component {
modalOpen,
apiConfigs,
handleClose,
record,
} = this.props;

return (
Expand Down Expand Up @@ -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>
Expand All @@ -120,6 +121,7 @@ CommunitySelectionModalComponent.propTypes = {
modalOpen: PropTypes.bool,
apiConfigs: PropTypes.object,
handleClose: PropTypes.func.isRequired,
record: PropTypes.object.isRequired,
};

CommunitySelectionModalComponent.defaultProps = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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}>
Expand Down Expand Up @@ -163,6 +166,7 @@ CommunitySelectionSearch.propTypes = {
searchApi: PropTypes.object.isRequired,
}),
}),
record: PropTypes.object.isRequired,
};

CommunitySelectionSearch.defaultProps = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class SubmitReviewOrPublishComponent extends Component {
showChangeCommunityButton,
showDirectPublishButton,
showSubmitForReviewButton,
record,
...ui
} = this.props;
const { modalOpen } = this.state;
Expand All @@ -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" />
Expand All @@ -77,6 +79,7 @@ SubmitReviewOrPublishComponent.propTypes = {
showChangeCommunityButton: PropTypes.bool.isRequired,
showDirectPublishButton: PropTypes.bool.isRequired,
showSubmitForReviewButton: PropTypes.bool.isRequired,
record: PropTypes.object.isRequired,
};

SubmitReviewOrPublishComponent.defaultProps = {
Expand Down
4 changes: 1 addition & 3 deletions invenio_rdm_records/requests/community_inclusion.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions invenio_rdm_records/requests/community_submission.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


#
Expand Down Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions invenio_rdm_records/services/community_inclusion/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
39 changes: 25 additions & 14 deletions tests/resources/test_resources_communities.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 = {
Expand All @@ -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(
Expand Down
14 changes: 8 additions & 6 deletions tests/services/test_service_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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", {}
)


Expand Down
Loading