Jasig/uPortal — pull request #478 by apetro (2014-12-04 21:53:38+00:00)
Title: UP-4320 enforce BROWSE permission on related portlets, MarketplaceSearchService results

This change set makes **`MarketplacePortletDefinition` instances scoped to a particular viewing user** and **reflect that user's `BROWSE` permission** as regards related portlet definitions. Also filters Marketplace portal search service participation such that only search results for portlets upon which the requesting user enjoys `BROWSE` are returned (rather than including search results upon which the user does not enjoy BROWSE).

Adds an example portlet definition, sub-category, and permission grants and denies so that Students in general have `BROWSE` on the `Instructors` category of portlets (status quo) but lack that permission on a new sub-category of `Instructors` that contains a new portlet publication. This facilitates verifying in the UI that `staff` enjoys `BROWSE` over that portlet as a related portlet of the existing portlet definition in `Instructors` whereas `student` does not.

Screenshots
-----------------

Before: Student sees the related portlet even while lacking `BROWSE` permission on it:

![before_student_browses_related_portlet](https://cloud.githubusercontent.com/assets/952283/5306687/308c2b62-7bcc-11e4-9c1e-3afc5bfe97b5.png)

(Kindly ignore the portlet being related to itself in that screenshot -- that was the Set math bug.)

After: Student does not see the related portlet because lacks `BROWSE` permission on it.

![student_does_not_see_related](https://cloud.githubusercontent.com/assets/952283/5306707/4d61c4e0-7bcc-11e4-94ca-02d42cfd730b.png)

Users enjoying `BROWSE` over the related portlet will still see it as related:

![admin_still_browse_related_portlet](https://cloud.githubusercontent.com/assets/952283/5306726/7276c2ee-7bcc-11e4-8adc-be67319a8ad0.png)

Implementation notes
-----------------------------

Adds a Registry to encapsulate responsibility for instantiating and caching these user-scoped `MarketplacePortletDefinition` instances.

Adjusts the Marketplace controllers to get the `IPerson` being serviced to rely more upon a refactored `IMarketplaceService` to get the right data for those persons.

Caches more and makes more effective use of the asynchronous caches in `MarketplaceService`.

Adds some `equals()` and `hashCode()` implementations to get `Set` math to work.

On API changes
----------------------

**Refactors rather aggressively.** These API changes would not fly for a minor release under Semantic Versioning, but uPortal isn't currently using Semantic Versioning, and they do seem consistent with the "minor upgrade pain" threshold for minor releases under the current versioning approach. Most uPortal adopters are going to take these controllers and services as they come in the product if they use them at all, and so will not be significantly burdened by the API changes in e.g. `IMarketplaceService`.

Known shortcomings
----------------------------

* `MarketplacePortletDefinition`s becoming user-scoped is a tough pill to swallow, since they seem to be kind of expensive to instantiate. Looking at the code, maybe they don't need to be so expensive, really, if the underlying registries depended upon are caching properly. Parsing some `String`s isn't really that expensive of an activity. Anyway, losing `MarketplacePortletDefinition` cache hits across users is a hard blow, perhaps even an unacceptably hard blow. However, went that direction (rather than, say, restricting the user-specific-ness to just `MarketplaceEntry` instances), because `MarketplacePortletDefinition` is used already under the not-previously-warranted expectation that it is reflecting a user-specific view on a Marketplace definition, namely in the portlet's rendering of a focused-upon Marketplace definition. This architectural approach makes that natural usage start working as one would expect and makes it very difficult for code to get confused about whether it is appropriate to show a given `MarketplacePortletDefinition` to a given user. Only implementing the user-specific-ness of the view on the data in `MarketplaceEntry` would keep the trip hazard of grabbing a `MarketplacePortletDefinition` and grabbing its (random, even) related portlets and showing those without remembering to filter out those the requesting user does not have permission to `BROWSE`. Of course, another approach could have been to move those related-portlet features to only live in `MarketplaceEntry`. That may be a better approach.
* While individual `MarketplacePortletDefinition`s are cached and the `Set` of `MarketplacePortletDefinition`s and of `MarketplaceEntry`s `BROWSE`-able by a given user are cached (as asyncronous `Future`s no less; thanks @jhelmer-unicon :smile: ), more careful caching could be applied. I also suspect the caching that is here could be implemented more elegantly either through self-populating caches or through effective use of `@Aspects`.
* While the Marketplace plugin itself to the Search Service starts enforcing `BROWSE` in its own search results, does not enforce `BROWSE` in other search service plugins but that probably should be done so that lacking `BROWSE` permission has the semantic effect of making a portlet not-discoverable (though the user might still enjoy `SUBSCRIBE` and be able to *use* the portlet, which is especially handy for meta-portlets like Attachments etc.) The kind of portlet that you don't want showing up as a related portlet or in the Marketplace portlet table at all is the kind of portlet that you don't want showing up in search results either.
* While unit test coverage is increased, it is not comprehensive. In particular, `MarketplaceRegistryImpl` has no coverage.