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

9369 Shib groups (and other custom groups), as subgroups of an explicit group #9597

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

landreev
Copy link
Contributor

@landreev landreev commented May 16, 2023

What this PR does / why we need it:

See the issue, it's fairly straightforward.
The actual production group mentioned in the description is "All Known Shibboleth Groups" aka "&explicit/1-shibb_groups", defined at the top-level collection. I have a group with the same name defined on demo.dataverse.org demonstrating the issue.

Which issue(s) this PR closes:

Closes #9369

Special notes for your reviewer:

The reason assigning roles to the parent group aren't taking effect is as follows:
When the list of user's groups is retrieved in GroupServiceBean.groupsFor( DataverseRequest, DvObject) we go through all the available GroupProviders and ask each one for groupprovider.groupsFor(req, dvo), then combine the lists, apparently expecting the result to contain all the ancestor groups, not just the groups via direct assignment. In reality this is only true for the list returned by ExplicitGroupProvider. It relies on ExplicitGroupsServiceBean that uses a monstrous native query (see findClosure(...) there) to get the full hierarchical list. ShibGroupProvider only returns the shib groups (our group providers appear to be meant to only return the groups of their "own kind"). So this is how a parent group containing shib groups falls through the cracks. My solution in this PR is to look up the immediate explicit groups ancestors of any shib groups found in the method above, and if any exist, run the recursive query method in `ExplicitGroupService' (mentioned above) on them to find any ancestral groups that may exist further up.

Idk if this is too hacky. I don't know what a prettier approach would look like. Should this code be moved into the group provider? But then I'm assuming that the above is not really specific to shib groups, but also applies to the ip and domain groups; i.e., all the non-explicit groups. So maybe it should stay in the central place. Idk.

Suggestions on how to test this:

I'm not sure if we have a straightforward/clean way of testing shib-related issues. Please talk to me, provided this pr makes it into qa. I can share the hacky way I used to test it and/or try to help find a non-hacky way.

UPDATE: the original bug/behavior was not unique to Shib groups, the same thing was happening with IP and Domain groups. The fix in the branch addresses all of the above as well. So one, easier way to test the "before" and "after" behavior would be to create an IP group, then create an explicit parent group... etc.

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?:

Additional documentation:

@landreev landreev added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation May 16, 2023
@landreev landreev requested a review from scolapasta May 16, 2023 20:39
@github-actions

This comment has been minimized.

@coveralls
Copy link

coveralls commented May 16, 2023

Coverage Status

Coverage: 20.211% (-0.003%) from 20.214% when pulling f93cf48 on 9369-shib-subgroup into fc23042 on develop.

@github-actions

This comment has been minimized.

@landreev
Copy link
Contributor Author

So, should I replace my

if (group instanceof ShibGroup) {

in GroupServiceBean.java with

if ( ! group instanceof ExplicitGroup) {

... or should it be

if (group instanceof ShibGroup || instanceof IpGroup || MailDomainGroup) {

?

@scolapasta scolapasta moved this from Ready for Review ⏩ to In Review 🔎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) May 17, 2023
@scolapasta scolapasta self-assigned this May 17, 2023
@landreev
Copy link
Contributor Author

landreev commented May 24, 2023

I'll go ahead and extend it to cover the IP and Domain groups as well, per my last comment.

@github-actions
Copy link

📦 Pushed preview application image as

ghcr.io/gdcc/dataverse:9369-shib-subgroup

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@landreev landreev changed the title 9369 Shib groups, as subgroups of an explicit group 9369 Shib groups (and other custom groups), as subgroups of an explicit group May 25, 2023
Copy link
Contributor

@scolapasta scolapasta left a comment

Choose a reason for hiding this comment

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

I think it's ok to leave this centralized here. Looks good.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from In Review 🔎 to Ready for QA ⏩ May 30, 2023
@scolapasta scolapasta removed their assignment May 30, 2023
@kcondon kcondon self-assigned this May 31, 2023
@kcondon kcondon merged commit 2d29201 into develop Jun 5, 2023
19 of 20 checks passed
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Jun 5, 2023
@kcondon kcondon deleted the 9369-shib-subgroup branch June 5, 2023 21:19
@pdurbin pdurbin added this to the 5.14 milestone Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Shib group in another group doesn't work
5 participants