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

Features support part 2 #822

Merged
merged 11 commits into from Sep 23, 2020
Merged

Conversation

Atmire-Kristof
Copy link
Contributor

@Atmire-Kristof Atmire-Kristof commented Jul 30, 2020

References

This PR expands on #717
and fixes #738

Description

This PR hides / protects certain components and routes behind feature authorization checks. Support for this was added in #717, this PR aims to cover more features using these changes.

Instructions for Reviewers

Changes made:

  • Implemented abstract DsoPageFeatureGuard, accepting a resolver (resolving the route's ID to a remote-data object). getObjectUrl returns the self-link of the object resolved from the URL, which will be used for checking the Feature's authorization
  • Implementations of this abstract guard:
    -- CollectionPageAdministratorGuard: checks admin authorization for collection pages. Configured for /collection/${uuid}/edit routes.
    -- CommunityPageAdministratorGuard: same, but for communities
    -- ItemPageAdministratorGuard: same, but for items
    -- ItemPageReinstateGuard: checks "reinstateitem" feature authorization for item pages. Configured for /items/${uuid}/edit/reinstate routes.
    -- ItemPageWithdrawGuard: checks "withdrawitem" feature authorization for item pages. Configured for /items/${uuid}/edit/withdraw routes.
  • Created SiteRegisterGuard checking "epersonRegistration" feature authorization for the Site. Configured for /register route.
  • Added checks for the register link (in the login dropdown) to be hidden when not authorized.
  • Added checks for the withdraw or reinstate button (on the edit item page) to be hidden when not authorized.

How to test:

  • Visiting the mentioned pages containing links/buttons while logged in as an unauthorized user, should not display these links/buttons.
  • Trying to visit the mentioned routes with an unauthorized should result in the user ending up on the /unauthorized page.

Warnings

  • #2912: This issue might cause the administratorOf feature to not work as expected. Take this into account when testing this PR.
  • Cache should be authorization aware #635: Authorization requests will still be cached for the previous user when logging in (usually anonymous), which causes some stuff like the sidebar to not update accordingly. Also take this into account when testing this PR.

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 this to Needs Reviewers Assigned in DSpace 7 Beta 4 Jul 30, 2020
@artlowel artlowel linked an issue Jul 30, 2020 that may be closed by this pull request
@lgtm-com
Copy link

lgtm-com bot commented Jul 30, 2020

This pull request introduces 2 alerts when merging 23354b4 into 8b639bc - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@tdonohue tdonohue self-requested a review July 30, 2020 14:42
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Jul 30, 2020
@tdonohue tdonohue requested a review from abollini July 30, 2020 14:45
@tdonohue tdonohue moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 4 Jul 30, 2020
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 : I gave this a quick test today (haven't done a full code review yet), and found that it almost seems to work too well.

Essentially, with this PR installed, I'm no longer able to access many Admin Tools. More specifically, when logged in as an Admin, these Admin menus are no longer available/accessible:

  • Access Control
  • Admin Search
  • Registries
  • Administer Workflow
  • Import (menu exists but all submenus are grayed out)
  • Export (menu exists but all submenus are grayed out)

If I attempt to access one of those Admin tools by typing in the URL in my browser (e.g. http://localhost:4000/admin/search) I am sent to a 401 Unauthorized page.

So, I think this PR has tightened access rights significantly and it no longer allows full Site Administrators to have access to Admin tools. I'm not exactly sure why that is, but obviously we'd need that resolved before we can look to merge this.

Conflicts:
	src/app/core/core.module.ts
@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2020

This pull request introduces 2 alerts when merging 4297893 into eb98098 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@artlowel
Copy link
Member

artlowel commented Aug 6, 2020

@tdonohue I don't think this PR is the direct cause of any of those issues.

It looks like the rest demo branch was hasn't been updated in quite some time, and both DSpace/DSpace#2867 and DSpace/DSpace#2910 are necessary for this PR to function. That seems to be at least part of the reason why so many things are not authorized for you.

However even with those PRs in place this PR is still affected by DSpace/DSpace#2912 and #635 as mentioned in the description. So e.g. the edit item page can still be unaccessible.

We probably won't be able to merge this PR until at least DSpace/DSpace#2912 has a solution.

Conflicts:
	src/app/+collection-page/collection-page-routing.module.ts
	src/app/+community-page/community-page-routing.module.ts
	src/app/+item-page/edit-item-page/edit-item-page.routing.module.ts
	src/app/+item-page/edit-item-page/item-status/item-status.component.ts
	src/app/+item-page/item-page-routing.module.ts
	src/app/app-routing.module.ts
	src/app/core/core.module.ts
	src/app/shared/log-in/log-in.component.html
	src/app/shared/log-in/log-in.component.spec.ts
	src/app/shared/log-in/log-in.component.ts
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.

👍 Finally got around to re-testing this PR. It all works well now. Code looks good as well. Thanks @Atmire-Kristof !

DSpace 7 Beta 4 automation moved this from Under Review to Reviewer Approved Sep 23, 2020
@tdonohue
Copy link
Member

Merging immediately as this was previously flagged for one approval

@tdonohue tdonohue added this to the 7.0beta4 milestone Oct 5, 2020
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
Projects
No open projects
3 participants