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

UP-4966 Implement lifecycle state BROWSE permissions #1068

Merged
merged 3 commits into from
Dec 8, 2017

Conversation

mairi
Copy link
Contributor

@mairi mairi commented Dec 7, 2017

Replicate the portlet SUBSCRIBE permission model for portlet BROWSE permissions so that expired portlets cannot be browsed by non-admin users in the marketplace or searched for using the portal search.

Checklist
Description of change

The BROWSE permission which governs access to portlets in search, Customize, and in the Marketplace has been updated to take account of portlet lifecycle state in the same manner as the SUBSCRIBE permission, thereby preventing non-admin users from seeing unpublished portlets. This reflects the normal expectation that setting a portlet to expired means it is no longer visible, overriding other permissions.

Changes are as follows:

  • new permissions BROWSE_CREATED, BROWSE_APPROVED and BROWSE_EXPIRED have been added to the UP_PORTLET_SUBSCRIBE permission owner;
  • org.apereo.portal.security.provider.AuthorizationImpl.canPrincipalBrowse (IAuthorizationPrincipal, IPortletDefinition) and org.apereo.portal.portlet.marketplace.MarketplaceService.mayBrowsePortlet() have been modified to take account of lifecycle state when checking BROWSE access for portlets;
  • sample data for the new permissions has been added.

Note that the original UP-4966 issue only mentions restricting browse for expired portlets, but it seemed to make sense to avoid confusion and simply mirror the behaviour for SUBSCRIBE permissions.

After applying these changes locally:

  • expired portlets do not appear in the Customize drawer for a non-admin user;
  • the portal search box does not show expired portlets in results for a non-admin user;
  • the Marketplace portlet does not list expired portlets for a non-admin user;
  • the Marketplace portlet search does not return expired portlets in results for a non-admin user.
Tests & Documentation

I have left the Checklist items for tests and documentation in place and unchecked above as I'm not sure whether action is required for these. I couldn't find any tests that ought to be updated - all tests are passing when I run './gradlew test' - and I wasn't sure what new tests might be required or where to add them. For documentation, again I'm not confident about what will need to be changed. Any guidance on these two items would be most welcome!

Impact on uPortal-start

If the change in this pull request is accepted, a corresponding change in uPortal-start will be required. The file which defines the BROWSE & SUBSCRIBE permissions that control rendering of and subscription to uPortal content, UP_PORTLET_SUBSCRIBE.permission-owner.xml, is no longer part of uPortal. It now exists in uPortal-start in data/base/permission_owner. This file requires activity element definitions for the Browse Approved (BROWSE_APPROVED), Browse Created (BROWSE_CREATED) and Browse Expired (BROWSE_EXPIRED) permissions, for example:

`

<activity>
    <name>Browse Approved</name>
    <fname>BROWSE_APPROVED</fname>
    <desc>Browse uPortal content that is approved, but not yet published</desc>
    <targetProvider>channelsAndCategoriesTargetProvider</targetProvider>
</activity>
<activity>
    <name>Browse Created</name>
    <fname>BROWSE_CREATED</fname>
    <desc>Browse to uPortal content that has not yet been approved or published</desc>
    <targetProvider>channelsAndCategoriesTargetProvider</targetProvider>
</activity>
<activity>
    <name>Browse Expired</name>
    <fname>BROWSE_EXPIRED</fname>
    <desc>View expired uPortal content in the Marketplace</desc>
    <targetProvider>channelsAndCategoriesTargetProvider</targetProvider>
</activity>

`

Replicate the portlet SUBSCRIBE permission model for portlet BROWSE
permissions so that expired portlets cannot be browsed by non-admin
users in the marketplace or searched for using the portal search.
@mairi
Copy link
Contributor Author

mairi commented Dec 7, 2017

I can see that this pull request is failing checks, specifically formatting according to the Google Java Style. Unfortunately I cannot tell why this check is failing from the task output as it just refers to 'formatting style violations' so I haven't managed to fix this yet. If anyone can help with this, it would be fantastic. I'm using IntelliJ IDEA 2017.3 but the google-java-format plugin isn't working for me and having looked through the Google Java Style guidelines manually I can't see what I'm doing wrong.

@ChristianMurphy
Copy link
Member

Style can be automatically handled by running the Google Style formatter from Gradle.

./gradlew goJF

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @mairi! 🙇

Code LGTM ✅
I'll defer to others on API/Configuration design around the portlet lifecycle.

@ChristianMurphy ChristianMurphy added this to the 5.1.0 milestone Dec 7, 2017
@mairi
Copy link
Contributor Author

mairi commented Dec 7, 2017

Thanks for that tip @ChristianMurphy! I eventually figured out what I was doing wrong with IntelliJ & got the check task passing locally. Now just waiting to see if it passes here!

Merge required to resolve test failure by pulling in UP-4978 changes.
@drewwills
Copy link
Contributor

This PR looks very good and very thorough. 👍 I'm going to pull it down locally and kick the tires.

@drewwills
Copy link
Contributor

Tested locally -- everything seems to work swimmingly.

@drewwills drewwills merged commit 5509670 into uPortal-Project:master Dec 8, 2017
@ChristianMurphy ChristianMurphy modified the milestones: 5.1.0, 5.0.3 Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants