[DSpace-CRIS] Administrative Edit of archived items via submission form and security configuration for metadata visibility (Backend)#11964
Conversation
|
@AdamF42 : Which DSpace-CRIS feature is this on the differences list? We need to link this up to one of the tickets, and I'm trying to understand which ticket this relates to. Is this related to #11749? UPDATE: Never mind, as I look at this closer, I realize this appears to be at least part of #11749 |
1ac2d2f to
b0ef5a6
Compare
7814b2b to
4c4ba4f
Compare
|
@AdamF42 I performed a quick code review (see below), however to fully understand how things tie together it would be good to test it with the UI. Is there an ANGULAR PR open for this anywhere ? The code reviewIssue 1Why is there a separate "RelatedItemEnhancerUpdatePoller" dispatcher in the dspace.cfg, the consumer list appears to be the same, maybe we can remove this ? Issue 2I noticed the Issue 3The Issue 4The
Issue 5The |
41cca6e to
3734c3b
Compare
@KevinVdV, thank you for the code review. Here's the status of each issue: Issue 1 - Came from #11718 (does not belong to this pr) Issue 2 - Came from #11945 (inline-group input-type handling) Issue 3 - You were correct, I've removed the unused Issue 4 - You were right about the inconsistency. I've removed the unused Issue 5 - You were correct, the Regarding the Angular frontend PR: I've now linked the corresponding frontend PR in the description. However, I suggest reviewing this PR after the other PRs in the dependency chain (#11718, #11945, #11963) are addressed, as this PR builds on top of those changes. |
|
Hi @AdamF42, |
3734c3b to
fa05e12
Compare
There was a problem hiding this comment.
Made an initial not too deep review to see what the features are about, you can find my feedback below.
Issue 1
Tried to create a collection & got an exception:
Caused by: java.sql.SQLException: bad_dublin_core schema=cris.submission.definition. Metadata field does not exist!
After testing more I also found the following metadata fields are missing (full list):
- cris.submission.definition
- cris.submission.definition-correction
- cris.workspace.shared
Issue 2
You cannot create a collection without entities anymore, the entity type is mandatory (even if you have a clean database without any entities setup)
Issue 3
When I start a new submission any metadata I add gets lost after pressing the save button. The PATCH request is properly fired, but the item that is returned doesn't contain any of the metadata that I added.
So I can't finish a submission at this point.
Questions
Question 1
When creating a collection the "Submission definition" is mandatory, if we want to do things like this we either need to:
- Make it optional
- If we keep it mandatory
- Create a migration script that will set the submission definition on the collections based on the submission-forms.xml
- I would also then remove the configuration from the submisssion-forms.xml (because any new collections will have it set through the UI)
- Support in the struct-builder would also be very useful (we already have it for the shared workspace)
Question 2
The feature set for this PR mentions: Granular Edit/Security Privileges , how can I test this from the UI ? As the ANGULAR PR doesn't mention this. The traces I find in the REST side indicate that the security level is set through the PATCH command.
Issue 1: Schema Migration IssueThe schema Issue 2: Mandatory Entity TypeThis issue has been fixed on the Angular side. Issue 3: Lost Method During RebaseDuring the rebase, a mistake was made resulting in the loss of Question 2: Testing "Granular Edit/Security Privileges"The frontend PR has been updated to include information for testing the "Granular Edit/Security Privileges" feature. |
5c15c67 to
4d24968
Compare
6d8a502 to
6770c02
Compare
|
Hi @KevinVdV, thank you for the thorough review! Here are my responses to your feedback: Issue 5 - CrisConstants classWe decided to keep those constants in a separate Issue 6 - Unused Resource Policy codeI've removed the unused code:
Issue 7 - DSpaceControlledVocabulary modificationsThe The code was using Then created secure helper methods ( Issue 8 - Metadata security usageThe metadata security system is being used, but the integration happens through the Issue 9 - ExtractMetadataStepExceptionRemoved Issue 10 - admin.rest.properties.exposedYou're right - I've removed the usage of Question around BitstreamCrisSecurityService"Authors can download restricted bitstreams" refers to the frontend PR DSpace/dspace-angular#5132 with testing instructions. Regarding your concern about READ policies - could you clarify which specific code changes are concerning? The intent is to allow item authors/owners to download their own bitstreams even when they have embargo/restriction policies, but this should not affect how READ policies work for other users. Question around RelationshipAuthorizerI updated the initial PR description to remove the RelationshipAuthorizer mention since that code has been removed from this PR. |
|
@AdamF42 : Thanks for your work on this PR. Just a friendly note to say that only the first 15 (of about 60) inline comments have been addressed in my prior review. I'm assuming you are working on the others already, but I wanted to point this out in case the other comments were overlooked (because GitHub collapses that many comments so that most are not visible until you click "Load more.."). Thanks for what you've done so far though. It all looks good! |
There was a problem hiding this comment.
@AdamF42 Thanks for looking into my feedback, approving this PR 👍
That said, I consider two issues I opened to be blocking, as they raise concerns around consistent metadata exposure enforcement and restricted bitstream authorization/auditability. These should be addressed before the release of DSpace 10.
85b471b to
755c223
Compare
d1c765f to
509b40c
Compare
There was a problem hiding this comment.
Thanks @AdamF42 for all your hard work on this PR over the last few days!
I've retested today and everything is working well.
I've also verified that almost all of my prior feedback has been addressed. The only feedback that still needs fixing are my comments in these two files:
metadata-security.cfg- I pointed out that this file is missing a detailed description. It should have comments at the top that describe the behavior/purpose of these newmetadata.visibility.*settings and describe what the values 0, 1 and 2 mean.rest.cfg- I pointed out that a lot of the newrest.properties.exposedconfigs seem to reference settings (inmetadata-security.cfg) that don't exist
All my other feedback has been addressed. So, if you can clean up these two very minor things, I think this PR is ready to be merged. Thank you so much!
tdonohue
left a comment
There was a problem hiding this comment.
@AdamF42 : Just after submitting my basic approval, I was testing the REST API directly and I found one more minor bug. We need to add a permissions check to the /server/api/core/edititems endpoint as described below. This should also be very quick to fix though!
509b40c to
322baab
Compare
322baab to
5ed5fb4
Compare
|
Thanks for the updates today @AdamF42 ! The code now looks good to me, and I'm doing final re-testing now (to verify which outstanding issues need to be moved to separate tickets) before I submit a final approval. |
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @AdamF42 ! I've finished re-reviewing and testing this today. It's working well for me now, and I've verified that double upload problem (reported on the frontend) is now fixed.
All my other reported issues I'll move to follow-up tickets, as this is mostly working great!
|
@AdamF42 : One other note. Similar to all other CRIS-related PRs, I'm flagging this as "needs documentation" because this PR needs to have documentation added to our DSpace 10.0 docs: https://wiki.lyrasis.org/display/DSDOC10x Please let me know when you (or one of your colleagues at 4Science) have added the necessary docs, and I'll then remove the label. Thanks again! |
References
Description
This PR ports the Edit Mode feature set from DSpace-CRIS to DSpace core. It enables a submission-like editing experience for archived items with granular security controls, shared workspaces, and enhanced access management. This feature set allows users to edit archived items using familiar submission forms while maintaining proper security and validation.
Note: This PR is part of the DSpace-CRIS merger effort and depends on previous PRs in the series ( #11718, #11945, #11963).
Instructions for Reviewers
List of changes in this PR:
Feature 1: Granular Edit/Security Privileges (
#3in "Feature Differences"))EditItemModeconfiguration system that allows defining multiple edit modes per entity type with different security levels (ITEM_ADMIN, OWNER, CUSTOM, GROUP, ANONYMOUS).CrisSecurityServicefor evaluating complex security policies based on user roles, item ownership, and custom metadata fields.EditItemModeValidatorto ensure edit mode names are unique per entity type and properly configured.Feature 2: Edit Item in Submission Mode (
#9in "Feature Differences")EditItemandEditItemRestmodels to represent archived items in an editable state similar to WorkspaceItems.EditItemServicefor managing the edit lifecycle of archived items including validation and state management.EditItemRestRepositorywith full REST API support for:EditItemConverterto transform archived items into submission-like structures with sections and validation.UnprocessableEditExceptionto handle validation errors gracefully, refusing to save items in invalid states.ValidationServiceto use the correct submission definition from the edit mode configuration rather than the default collection submission.Feature 3: Shared Workspaces (
#14in "Feature Differences")isSharedWorkspacechecks to determine if an item can be accessed by users other than the submitter/owner.Feature 4: Authors can download restricted bitstreams (
#13in "Feature Differences")Feature 5: Submission forms and workflow linked to collection via UI (
#29in "Feature Differences")dspace.submission.definitionto bind specific submission form definitions to collections.SubmissionConfigReaderenhancement to look up the submission definition from collection metadata.Testing/Review Guidance:
Prerequisites
dspace/config/spring/api/edititem-service.xmldspace/config/spring/api/spring-dspace-security-metadata.xmldspace/config/spring/api/relationship-authorizers.xmlTest Scenarios
Test 1: Edit Item as Administrator
/api/core/edititems/{item-uuid}Test 2: Edit Item as Owner
Test 3: Granular Metadata Security
Test 4: Collection-Linked Submission Forms
dspace.submission.definitionmetadata on a collectionTest 5: Relationship Authorization
Test 6: Access Condition Management
Required Configuration
1. Edit Item Service Configuration (
dspace/config/spring/api/edititem-service.xml)Define edit modes for each entity type:
Security Levels:
ITEM_ADMIN: Only item administratorsOWNER: Item owner onlyCUSTOM: Based on metadata fields (e.g., investigators, editors)GROUP: Specific user groupANONYMOUS: All users2. Metadata Security Configuration (
dspace/config/spring/api/spring-dspace-security-metadata.xml)3. Collection Submission Definition Binding
Via REST API:
4. Entity Type Submission Definitions
Add to
item-submission.xml:Add corresponding forms to
submission-forms.xml:API Endpoints
Edit Item Endpoints
Get edit item (submission-like format):
Get available edit modes for an item:
Patch edit item (metadata changes):
Delete edit item (discard changes):
Metadata Value Endpoints (with security)
Metadata values now include security level information:
{ "id": "12345", "value": "Secret information", "securityLevel": "2", "display": "ADMINISTRATOR_AND_OWNER" }Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.