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

Contract for findItemsWithEdit search method #210

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ybnd
Copy link
Member

@ybnd ybnd commented Dec 23, 2022

This search method was added in DSpace/DSpace#8616 to support the edit Item modal.

Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks it almost aligns with other search methods in the collection endpoint, just a small comment to restrict it to authenticated user

items.md Show resolved Hide resolved
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ybnd and @KoenP : This looks good overall, but the new endpoint needs to be only available to logged in Users. See @abollini 's comment & mine below.

items.md Show resolved Hide resolved
@ybnd
Copy link
Member Author

ybnd commented Jan 24, 2023

@tdonohue @abollini added the HTTP 401 response, REST PR will be updated soon.

Note that the very similar Collection findSubmitAuthorized* methods aren't limited to authenticated users either. Maybe they should be too?

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks @ybnd ! This looks good now.

(Regarding the other similar methods, I'd anticipate they likely also should require authentication. So, that would sound like a bug to me, unless there's a documented reason why they are public endpoints. It's likely not a real security issue, since they should always return no permissions for anonymous users, but it does sound like a bug.)

@tdonohue tdonohue added this to the 7.5 milestone Jan 24, 2023
@tdonohue tdonohue removed this from the 7.5 milestone Feb 21, 2023
@tdonohue tdonohue added the work in progress PR is still being worked on & is not currently ready for review label Apr 5, 2023
Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @ybnd my feedback as been addressed.
@tdonohue the discussion about the collection endpoint is of course out-of-scope of this PR but I like to anticipate that imho it is not a bug. We have had a use case some year ago where an institution allow anonymous deposit, it may be not supported out-of-box right now but it could be valid. Of course in a such scenario the Institution should have a validation workflow inplace

@github-actions github-actions bot added the merge conflict PR has a merge conflict that needs resolution label Mar 4, 2024
Copy link

github-actions bot commented Mar 4, 2024

Hi @ybnd,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@github-actions github-actions bot removed the merge conflict PR has a merge conflict that needs resolution label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress PR is still being worked on & is not currently ready for review
Projects
Status: ❓ Stalled/On Hold
Development

Successfully merging this pull request may close these issues.

3 participants