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

Edit item page permission checks #1105

Merged
merged 6 commits into from
May 4, 2021

Conversation

Atmire-Kristof
Copy link
Contributor

@Atmire-Kristof Atmire-Kristof commented Apr 16, 2021

References

Description

This PR adds more fine-grained permission checks to the edit item page.

Instructions for Reviewers

Changes made:

  • Added new features to FeatureID enum: canManageVersions, canManageBitstreams, canManageRelationships, canManageMappings, canManagePolicies, canMakePrivate, canMove
  • Created guards for every tab on the item edit page and every action on the status tab. Some of these guards are more advanced (e.g. the guard for the status tab requires at least one of the actions found on the page to be authorized)
  • Unauthorized actions on the status page will contain a disabled button with a tooltip when hovering over it mentioning they're not authorized to perform the action

Mocks:
Since we're waiting on the REST PR to add some of the feature endpoints, I've added mocks for these endpoints on certain items. Rest api containing these items: https://api7.dspace.org/server/#/server/api

  • Authorization for canManageBitstreams is mocked for item 96715576-3748-4761-ad45-001646632963
  • Authorization for canManageRelationships is mocked for item 047556d1-3d01-4c53-bc68-0cee7ad7ed4e
  • Authorizations for canManageVersions and canManageMappings are mocked for item e98b0f27-5c19-49a0-960d-eb6ad5287067

How to test:

  • Visit the edit item page for the following items:
    ** 96715576-3748-4761-ad45-001646632963: The bitstreams tab should be available. Relationships, versions and collection mapper should be inaccessible. The collection mapper action on the status page should also be disabled and show a tooltip when hovered over.
    ** 047556d1-3d01-4c53-bc68-0cee7ad7ed4e: The relationships tab should be available. Bitstreams, versions and collection mapper should be inaccessible. The collection mapper action on the status page should also be disabled and show a tooltip when hovered over.
    ** e98b0f27-5c19-49a0-960d-eb6ad5287067: The versions and collection mapper tabs should be available. Bitstreams and relationships should be inaccessible. The collection mapper action on the status page should be active and link to the collection mapper tab.
    ** Visiting any other item's edit page should have all the above actions disabled

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). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs for any bug fixes, improvements or new features. A few reminders about what constitutes good tests:
    • Include tests for different user types (if behavior differs), including: (1) Anonymous user, (2) Logged in user (non-admin), and (3) Administrator.
    • Include tests for error scenarios, e.g. when errors/warnings should appear (or buttons should be disabled).
    • For bug fixes, include a test that reproduces the bug and proves it is fixed. For clarity, it may be useful to provide the test in a separate commit from the bug fix.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@artlowel artlowel added bug e/16 Estimate in hours labels Apr 16, 2021
@artlowel artlowel added this to the 7.0 milestone Apr 16, 2021
@tdonohue
Copy link
Member

@Atmire-Kristof : the REST API PR for this is now available, see DSpace/DSpace#3240 Therefore, you may want to test this against the REST API PR to verify it's still working as you planned. I'll also find some additional reviewers/testers in this week's meeting.

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.

@Atmire-Kristof : Overall this looks good & appears to work well when I test it with the Backend PR (DSpace/DSpace#3240).

However, I found one small bug (see inline comment) where the Withdraw button is now entirely gone.

I've also found that currently I cannot ever access the Bitstreams tab (it's always disabled saying "You're not authorized to access this tab", even when I'm a full Admin). However, it appears (at a glance) like this feature may not be working properly in the Backend PR.

UPDATE: Actually, the Bitstreams tab issue is a small problem in this PR afterall. It looks like you are looking for a feature named canManageBitstreams, but the feature is instead named canManageBitstreamBundles (because the Bitstream tab lets you manage All Bundles of Bitstreams). Once that name is corrected, I'm betting the "Bitstreams" tab will work again.

@artlowel artlowel requested a review from tdonohue April 29, 2021 12:23
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.

👍 Re-tested with the backend PR (DSpace/DSpace#3240) and the previous issues are now fixed. Thanks @Atmire-Kristof !

Copy link
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

thanks @Atmire-Kristof for this PR, I had a look on the code and it seems good to me

@tdonohue tdonohue merged commit abca095 into DSpace:main May 4, 2021
artlowel added a commit to atmire/dspace-angular that referenced this pull request Oct 26, 2021
tdonohue added a commit that referenced this pull request Oct 26, 2021
remove authorization mocks left over from PR #1105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add fine-grained permission checks for the edit item page
4 participants