Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it: This resolves an issue reported in #12742 that appears to have its root in a difference in how Explicit groups map between alias and identifier that is different from other types. Since the indexing being done for Locally FAIR content was not accounting for this difference, members of explicit groups could go directly to Locally FAIR collection/data/file pages they should have access to (if they knew the alias, DOI, identifier, etc.) but would not see that content in search results or collection page listings (since those are powered by solr. The current PR resolves this issue.
In addition, the PR adds support for the builtin groups (All and Auth users - All would be a useless choice for LF, but AuthenticatedUsers could be useful - must login to see content). They use a different prefix (:) and weren't addressed in the initial LF PR.
Which issue(s) this PR closes:
Special notes for your reviewer: FWIW: The convertToIndexableString method was designed to avoid doing a db loookup of the group to get it's alias (for performance), but that means that the code has to convert from identifier to alias and explicit groups need to drop "&explicit/" for this whereas the alias and identifier for shib, ip, persistent global groups just differ by '&'. Not sure there's any reason for that.
Tests are added to assure that the conversion is correct relative to the internal logic in the various group classes.
W.r.t. release notes, the permissions for LF content have to be reindexed. We now have the permission reindex APIs documented and they can be used, but the per-collection perm reindex api only takes a collection id, whereas the general reindex api (content and permissions) allows you to use the collection alias (easier to find). I'm not sure what the best option is to recommend. The release note currently has both options.
Suggestions on how to test this: Install the PR, enable LocallyFAIR content (if not done already) and create a LF collection, subcollection, and dataset. Verify that you can see the subcollection and dataset in the collection listing as a member of an explicit group defined on the collection. Regression test that you can also see the subcollection and dataset pages, that the public can't see this content, etc. (Regression is unlikely as the change only affects indexing.) If Locally FAIR content already exists on the test machine, you can just add an explicit group to it and test that way, reindexing the LF content after installing the PR.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: included.
Additional documentation: