Jasig/uPortal — pull request #481 by apetro (2014-12-09 23:21:48+00:00)
Title: UP-4320 enforce BROWSE permission on related portlets in Marketplace.

Alternative implementation to #478 , accomplishing filtering related Marketplace entries in Marketplace portlet and in Marketplace JSON feed so that only related entries the user has permission to BROWSE are included.

Screenshots
==========

In this example, the staff user has permission to browse both "Faculty Welcome" and its related portlet "Students Cannot Browse This". Fittingly, students do not have permission to browse "Students Do Not Browse This".

As `staff`, the Marketplace entry for "Faculty Welcome" looks like this (shows related portlet):

![marketplace_as_staff](https://cloud.githubusercontent.com/assets/952283/5367920/7fb9cab2-7fc5-11e4-996b-d2e61a8dca62.png)

As `student`, the Marketplace entry for "Faculty Welcome" looks like this (does not show related portlet):

![marketplace_as_student](https://cloud.githubusercontent.com/assets/952283/5367923/8730baee-7fc5-11e4-9e23-c8a7a918f026.png)

Technical Notes
=================

This changeset switches from `MarketplacePortletDefinition` to `MarketplaceEntry` as the primary abstraction used and exposed by the Marketplace portlet. `MarketplaceEntry` instances are user-specific and use their known `IPerson` when querying the underlying `MarketplacePortletDefinition` about

This changeset changes APIs. Under Semantic Versioning, it would only be appropriate for a `MAJOR` release. However, uPortal isn't currently using Semantic Versioning. Under the current versioning policy, this changeset seems consistent with modest pain for appropriate corresponding gain.

Contrasted with #478
================

In general **this approach makes much less change**. It refactors less and introduces less infrastructure.

Unlike #478, this implementation does not make `MarketplacePortletDefinition` instances user-scoped. It instead relies upon `MarketplaceEntry`s being user-scoped (they were already implicitly user-scoped by virtue of their user-permission-reflecting `isCanAdd()`).

Whereas #478 kept `MarketplacePortletDefinition` as the abstraction consumed by the Marketplace portlet and worked to make those definitions user-specific, this changeset moves Marketplace to use `MarketplaceEntry` instances instead of `MarketplacePortletDefinition` instances directly. This yields a slew of trivial changes to the `JSP`s.

This changeset adds (much) less unit testing as compared with #478. On the plus side, that means it contributes less to slow build times. :unamused:

Known Shortcomings
=================

* In general #478 has some commits that could be harvested even if the project goes in this changeset's direction instead.
* In specific, Does not filter by BROWSE permission in Marketplace participation in uPortal search service, but adding that fix would be trivial.
* In specific, Does not include the example permissions data in #478 for demonstrating the feature, but cherry-picking that over into this branch and thereby into `master` would be trivial.
* In specific, moving to more of a layered model with a `Registry` is probably a good idea and would help ensure appropriate cache usage as opposed to `new()`ing up ad-hoc `MarketplacePortletDefinition` and for that matter `MarketplaceEntry` instances.

* In general, there are unrealized opportunities to refactor and improve this code. `#RefactorMercilessly`.
* In specific, it's kind of weird that `MarketplaceEntry` becomes a primary Marketplace domain model object yet is over in the `.rest.` package namespace.
* In specific, the redundancies in `IMarketplaceService` between methods for getting at `MarketplaceEntry` instances and `MarketplacePortletDefinition` instances suggest that there's some confusion in the model about these concepts. It's probably the case that the Service should focus upon the `Entry` abstraction and perhaps the `MarketplacePortletDefinition`s become an implementation detail not exposed by that `Service`

Where to go next
=============

This embracing `MarketplaceEntry` as the user-specific abstraction and keeping `MarketplacePortletDefinition` as user-agnostic is probably the better, more viable direction. This changeset should review to merge as the primary way of accomplishing the UP-4320 requirement of filtering related portlets down to just those the user can `BROWSE`, and can follow up on that with further refactoring and harvesting out the goodness from the #478 implementation.