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

Fix: Collection's admin cannot edit its template item. #8851

Merged
merged 2 commits into from
May 23, 2023

Conversation

buithaihai
Copy link
Contributor

@buithaihai buithaihai commented May 18, 2023

References

Fix #8832

Description

This fixes the error at step 6 of #8832.

  • Please follow the instructed steps to reproduce and verify the fix

In addition, this PR also applies to Template Item removal action.

Instructions for Reviewers

List of changes in this PR:

  • First, I create a new RestObjectPermissionEvaluatorPlugin for TemplateItem to perform extra evaluations when editing or deleting a template item.
    • Making use of an existing RestObjectPermissionEvaluatorPlugin would be ideal, but none suffices. The best candidate I found is WorkspaceItemRestPermissionEvaluatorPlugin but it is too specific to WorkspaceItem to rewrite easily.
  • Second, I changed the parameter from 'ITEM' to 'ITEMTEMPLATE' in @PreAuthorize("hasPermission(...)") of methods patch() and deleteTemplateItem() to take advantage of existing TemplateItemRest class, in order to distinguish it further from the generic Item term.

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
  • My PR passes Checkstyle validation based on the Code Style Guide.
  • My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
  • My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR modifies REST API endpoints, I've opened a separate REST Contract PR related to this change.
  • If my PR includes new configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@buithaihai buithaihai marked this pull request as ready for review May 18, 2023 08:38
@tdonohue tdonohue added bug 1 APPROVAL pull request only requires a single approval to merge. labels May 18, 2023
@tdonohue tdonohue self-requested a review May 18, 2023 14:23
@tdonohue tdonohue added this to the 7.6 milestone May 18, 2023
@paulo-graca paulo-graca self-requested a review May 18, 2023 14:24
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.

@buithaihai : Thanks for this PR! First off, I can verify that this code fixes the bug entirely. Everything works.

However, this PR is missing automated tests. Could you add some new tests to ItemTemplateRestControllerIT which prove that a Collection Admin can create, edit & delete a template item?

It shouldn't be too complex to add the new tests... you just need to create a Collection Admin user in createStructure() similar to this:

collectionAdmin = EPersonBuilder.createEPerson(context)
            .withNameInMetadata("Jane", "Brown")
            .withEmail("collectionAdmin@my.edu")
            .withPassword(password)
            .build();
childCollection = CollectionBuilder.createCollection(context, parentCommunity)
                                           .withName("Collection 1")
                                           .withAdminGroup(collectionAdmin)
                                           .build();

Then you should be able to used that person in tests as the authenticated person & verify they have proper permissions.

Beyond that, great work! If you need any help on the tests let us know.

@buithaihai buithaihai force-pushed the DS-8832 branch 2 times, most recently from 56280c6 to aa197fd Compare May 23, 2023 07:31
@buithaihai
Copy link
Contributor Author

@tdonohue I have made the requested changes.

@tdonohue tdonohue self-requested a review May 23, 2023 16:47
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.

👍 Thank you @buithaihai ! The new tests look good, and as I mentioned previously I was able to verify that this fixes the bug.

@tdonohue tdonohue merged commit 9e5165e into DSpace:main May 23, 2023
9 checks passed
@buithaihai buithaihai deleted the DS-8832 branch May 25, 2023 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge. bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Itemtemplate Forbiden error for a collection's administrator
2 participants