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

Restrict setting privileges on groups #8511

Merged

Conversation

tylerjmchugh
Copy link
Contributor

@tylerjmchugh tylerjmchugh commented Nov 22, 2024

Related to issue: #8283

Currently, users with access to the privileges interface can assign privileges (e.g., "publishing") to any group without restrictions. There is no built-in way to prevent users from assigning privileges to specific groups based on their profiles for that group. This pull request addresses this issue by introducing a minimumProfileForPrivileges for groups. This field specifies the minimum user profile on a group required to assign privileges for that group.

Changes

  • Group Entity: Added a minimumProfileForPrivileges property to the Group class to define the minimum profile required to set privileges for the group.
    image
  • Privileges Interface: Updated the privileges UI to disable groups where the user does not meet the minimumProfileForPrivileges requirement, preventing unauthorized privilege assignments. (See group 'b' below)
    image
  • Access Control: Modified backend checks to enforce the minimum profile requirement when users attempt to assign privileges to a group, ensuring that only users with sufficient profiles can do so.
  • Documentation: Updated the administrator guide to explain the new setting and provide guidance on how it can be used effectively.

Example Use Case

Consider two groups:

  • Group A: No minimumProfileForPrivileges set.
  • Group B: minimumProfileForPrivileges set to Reviewer.

We want Group B to be used exclusively for access permissions, and we need to control who can assign privileges to it.

Workflow:

  1. Content Creation: Users add records to Group A and submit them for review.
  2. Review Process: A reviewer in Group A approves the records.
  3. Privilege Assignment: A user with at least the Reviewer profile in Group B (could be a service account used by another application) updates the privileges for the approved records, allowing users with view access in Group B to see them.

This setup ensures that only users with at least the Reviewer profile on Group B can assign privileges to Group B, providing better control over who can publish records to that group. It prevents users with lower profiles from inadvertently or intentionally assigning privileges to Group B, which could expose sensitive information or violate access policies.

If the default option of "No restrictions" is set for a group then the functionality remains the same as before these changes were implemented.

Checklist

  • I have read the contribution guidelines
  • Pull request provided for main branch, backports managed with label
  • Good housekeeping of code, cleaning up comments, tests, and documentation
  • Clean commit history broken into understandable chucks, avoiding big commits with hundreds of files, cautious of reformatting and whitespace changes
  • Clean commit messages, longer verbose messages are encouraged
  • API Changes are identified in commit messages
  • Testing provided for features or enhancements using automatic tests
  • User documentation provided for new features or enhancements in manual
  • Build documentation provided for development instructions in README.md files
  • Library management using pom.xml dependency management. Update build documentation with intended library use and library tutorials or documentation

@tylerjmchugh tylerjmchugh force-pushed the main.restrict-privileges-on-groups branch from d0fbcc9 to b05f8d7 Compare November 25, 2024 17:57
@tylerjmchugh tylerjmchugh marked this pull request as ready for review November 25, 2024 18:49
@ianwallen ianwallen requested a review from josegar74 November 26, 2024 17:12
@ianwallen ianwallen added this to the 4.4.7 milestone Nov 26, 2024
@josegar74 josegar74 requested a review from fxprunayre November 28, 2024 07:32
@josegar74
Copy link
Member

josegar74 commented Nov 28, 2024

@tylerjmchugh I started testing the pull request, but I'm getting some results that seem not correct. Let me add my test case:

  • Group A, initially with Minimum user profile allowed to set privileges: No restrictions
  • Group B, initially with Minimum user profile allowed to set privileges: No restrictions
  • EditorA, EditorA1 editors in Group A
  • ReviewerA reviewer in Group A
  • EditorB editor in Group B
  • ReviewerB reviewer in Group B

EditorA creates a metadata and shares full privileges with Group B.

  • EditorA can manage the privileges as is the metadata owner (correct)
  • EditorA1 can not manage the privileges as by default edit privileges are not set to GroupA (correct)
  • ReviewerA can manage the privileges as even if edit privilege is not set to GroupA, his profile allows it (correct, but probably questionable about being consistent with the edit privilege)
  • EditorB and ReviewerB can manage the privileges as it was shared with group B (correct)

EditorA adds edit privileges to Group A.

  • EditorA1 can manage now the privileges (correct)

Group A: Minimum user profile allowed to set privileges: Editor

  • Same results as in previous tests. It's a bit unclear which is the difference with No restrictions and Editor

Group A: Minimum user profile allowed to set privileges: Reviewer

  • EditorA can manage the privileges as is the metadata owner (correct)
  • EditorA1 can still manage the privileges. This seems no correct.

Group B: Minimum user profile allowed to set privileges: Reviewer

  • EditorB can access the privileges, but can not change privileges in Group B (this is correct, but inconsistent with previous case with EditorA1).
  • Editor B can change privileges for GroupA. This doesn't seem fine to me. Both GroupA and Group B have the minimum profile set to Reviewer.

Something I don't have clear in this case, which setting should affect as the metadata is in GroupA, but the user is in GroupB. This should be clearly explained.


About the help text for the field Minimum user profile allowed to set privileges, I think this could be more clear:

Minimum user profile allowed to set privileges to the editable metadata for users in this group (No Restrictions, Editor, Reviewer or Administrator). The default value is 'No Restrictions'.

It needs clarification also about the difference between the values No Restrictions and Editor as apparently seem the same.

Also if the setting affects to users in that group or users that can edit metadata in owned by that group.


Please also execute mvn install so Prettier is executed and commit the formatted JS / html files updated:

  • web-ui/src/main/resources/catalog/components/common/share/ShareService.js
  • web-ui/src/main/resources/catalog/templates/admin/usergroup/groups.html

@tylerjmchugh
Copy link
Contributor Author

tylerjmchugh commented Nov 28, 2024

Group A: Minimum user profile allowed to set privileges: Editor

  • Same results as in previous tests. It's a bit unclear which is the difference with No restrictions and Editor

The difference here should have been that with No restrictions set on GroupA, EditorB can set the privileges for GroupA. If instead Editor is set on GroupA, EditorB sees the GroupA row as readonly in the privileges ui as they are not an Editor on GroupA.

Looks like one of my commits did not get pushed. My latest push fixes this. This is that EditorB sees:

image


Group A: Minimum user profile allowed to set privileges: Reviewer

  • EditorA can manage the privileges as is the metadata owner (correct)
  • EditorA1 can still manage the privileges. This seems no correct.

Same situation here. My latest push should fix the issue where GroupA privileges can be managed if the record owner group is GroupA. EditorA1 now sees the UI like in the above screenshot.

But so does EditorA as they are not a reviewer for GroupA. For consistency of behaviour I have it working this way however I could include an exception for the record owner if you recommend that behaviour is better. (In our desired use case there would be no records owned by our restricted groups anyways)


Group B: Minimum user profile allowed to set privileges: Reviewer

  • EditorB can access the privileges, but can not change privileges in Group B (this is correct, but inconsistent with previous case with EditorA1).
  • Editor B can change privileges for GroupA. This doesn't seem fine to me. Both GroupA and Group B have the minimum profile set to Reviewer.

This should now be consistent as well. With Reviewer set for both GroupA and GroupB, EditorB sees the following as they are not a Reviewer on either group:

image


Something I don't have clear in this case, which setting should affect as the metadata is in GroupA, but the user is in GroupB. This should be clearly explained.

About the help text for the field Minimum user profile allowed to set privileges, I think this could be more clear:

Minimum user profile allowed to set privileges to the editable metadata for users in this group (No Restrictions, Editor, Reviewer or Administrator). The default value is 'No Restrictions'.

It needs clarification also about the difference between the values No Restrictions and Editor as apparently seem the same.

Also if the setting affects to users in that group or users that can edit metadata in owned by that group.

The idea here is that the setting is not meant to prevent setting privileges for certain records but to prevent setting privileges for certain groups on all records. And this setting affects all users unless they have the minimum profile on the group they want to change privileges for.

My missed commit made this confusing as the logic changed depending on whether the group to have privileges changed was also the record's owner group. This should not be the case. It should be consistent for all users and all groups.

I have updated the help text to the following:

Specifies the lowest user profile needed within a group to assign privileges for that group on a record. Users below this profile cannot set privileges for the group, and the group will be disabled in the privilege assignment interface. Default: No Restrictions (any editor for a record can set privileges for this group).

Hopefully this is more clear now with the consistent behaviour and updated help text. Let me know if anything else could be better explained.


Updated results for test cases (See bold for relevant changes):

  • Group A, initially with Minimum user profile allowed to set privileges: No restrictions
  • Group B, initially with Minimum user profile allowed to set privileges: No restrictions
  • EditorA, EditorA1 editors in Group A
  • ReviewerA reviewer in Group A
  • EditorB editor in Group B
  • ReviewerB reviewer in Group B

EditorA creates a metadata and shares full privileges with Group B.

  • EditorA
    • Can manage the privileges (record owner)
    • All groups enabled (no groups have minimum profile)
  • EditorA1
    • Cannot manage the privileges (GroupA does not have edit)
  • ReviewerA
    • Can manage the privileges (reviewer profile allows it)
    • All groups enabled (no groups have minimum profile)
  • EditorB
    • Can manage the privileges (GroupB has edit)
    • All groups enabled (no groups have minimum profile)
  • ReviewerB
    • Can manage the privileges (GroupB has edit)
    • All groups enabled (no groups have minimum profile)

EditorA adds edit privileges to Group A.

  • EditorA1
    • Can now manage the privileges (GroupA has edit)
    • All groups enabled (no groups have minimum profile)

Group A: Minimum user profile allowed to set privileges: Editor

  • EditorA
    • Can manage the privileges (record owner / GroupA has edit)
    • GroupA enabled (editor or more on GroupA), GroupB enabled (no minimum profile)
  • EditorA1
    • Can manage the privileges (GroupA has edit)
    • GroupA enabled (editor or more on GroupA), GroupB enabled (no minimum profile)
  • ReviewerA
    • Can manage the privileges (reviewer profile allows it / GroupA has edit)
    • GroupA enabled (editor or more on GroupA), GroupB enabled (no minimum profile)
  • EditorB
    • Can manage the privileges (GroupB has edit)
    • GroupA disabled (not an editor or more on GroupA), GroupB enabled (no minimum profile)
  • ReviewerB
    • Can manage the privileges (GroupB has edit)
    • GroupA disabled (not an editor or more on GroupA), GroupB enabled (no minimum profile)

Group A: Minimum user profile allowed to set privileges: Reviewer

  • EditorA
    • Can manage the privileges (record owner / GroupA has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB enabled (no minimum profile)
  • EditorA1
    • Can manage the privileges (GroupA has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB enabled (no minimum profile)
  • ReviewerA
    • Can manage the privileges (reviewer profile allows it / GroupA has edit)
    • GroupA enabled (reviewer or more on GroupA), GroupB enabled (no minimum profile)
  • EditorB
    • Can manage the privileges (GroupB has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB enabled (no minimum profile)
  • ReviewerB
    • Can manage the privileges (GroupB has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB enabled (no minimum profile)

Group B: Minimum user profile allowed to set privileges: Reviewer

  • EditorA
    • Can manage the privileges (record owner / GroupA has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB disabled (not reviewer or more on GroupB)
  • EditorA1
    • Can manage the privileges (GroupA has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB disabled (not reviewer or more on GroupB)
  • ReviewerA
    • Can manage the privileges (reviewer profile allows it / GroupA has edit)
    • GroupA enabled (reviewer or more on GroupA), GroupB disabled (not reviewer or more on GroupB)
  • EditorB
    • Can manage the privileges (GroupB has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB disabled (not reviewer or more on GroupB)
  • ReviewerB
    • Can manage the privileges (GroupB has edit)
    • GroupA disabled (not reviewer or more on GroupA), GroupB enabled (reviewer or more on GroupB)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@josegar74
Copy link
Member

@tylerjmchugh I got this error in this case, when changing any privilege with EditorA1 and saving the changes:

error-updating-privileges

EditorA adds edit privileges to Group A.

  • EditorA1
  • Can now manage the privileges (GroupA has edit)
  • All groups enabled (no groups have minimum profile)

It only happens if the metadata is published. But I think is not related to the changes in the pull request as the related code throwing the error hasn't been changed. I'll check in a vanilla version of GeoNetwork to confirm this.

@josegar74
Copy link
Member

@tylerjmchugh I've confirmed the previous error is not due to your changes. I've created the following issue to track it: #8551

Copy link
Member

@josegar74 josegar74 left a comment

Choose a reason for hiding this comment

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

Tested and looks ok. Code changes looks also fine.

Thanks.

@josegar74 josegar74 merged commit 18b00b3 into geonetwork:main Dec 10, 2024
7 of 8 checks passed
@geonetworkbuild
Copy link
Collaborator

The backport to 4.2.x failed:

The process '/usr/bin/git' failed with exit code 1
stderr
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git cherry-pick --skip'

stdout
Auto-merging domain/src/main/java/org/fao/geonet/domain/Group.java
Auto-merging web-ui/src/main/resources/catalog/js/admin/UserGroupController.js
Auto-merging web-ui/src/main/resources/catalog/locales/en-admin.json
Auto-merging web-ui/src/main/resources/catalog/templates/admin/usergroup/groups.html
[backport-8511-to-4.2.x 128a567654] Add new minimumProfileForPrivileges property to groups
 Author: tylerjmchugh <Tyler.McHugh@dfo-mpo.gc.ca>
 Date: Fri Nov 22 12:59:49 2024 -0500
 4 files changed, 48 insertions(+)
Auto-merging services/src/main/java/org/fao/geonet/api/records/MetadataSharingApi.java
[backport-8511-to-4.2.x ab7203eb9c] Disable the group in privileges UI if missing required profile
 Author: tylerjmchugh <Tyler.McHugh@dfo-mpo.gc.ca>
 Date: Fri Nov 22 13:06:25 2024 -0500
 11 files changed, 201 insertions(+), 33 deletions(-)
Auto-merging services/src/main/java/org/fao/geonet/api/records/MetadataSharingApi.java
[backport-8511-to-4.2.x 5d85f4e2f7] Restrict setting permissions for groups with a minimumProfileForPrivileges
 Author: tylerjmchugh <Tyler.McHugh@dfo-mpo.gc.ca>
 Date: Fri Nov 22 13:08:57 2024 -0500
 6 files changed, 66 insertions(+), 4 deletions(-)
[backport-8511-to-4.2.x f803e63818] Documentation
 Author: tylerjmchugh <Tyler.McHugh@dfo-mpo.gc.ca>
 Date: Mon Nov 25 12:57:40 2024 -0500
 1 file changed, 26 insertions(+), 5 deletions(-)
[backport-8511-to-4.2.x 2550788844] Update documentation
 Author: tylerjmchugh <Tyler.McHugh@dfo-mpo.gc.ca>
 Date: Mon Nov 25 13:04:26 2024 -0500
 1 file changed, 5 insertions(+), 5 deletions(-)
On branch backport-8511-to-4.2.x
You are currently cherry-picking commit b1a0c7b282.
  (all conflicts fixed: run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

nothing to commit, working tree clean

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-4.2.x 4.2.x
# Navigate to the new working tree
cd .worktrees/backport-4.2.x
# Create a new branch
git switch --create backport-8511-to-4.2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick e1545f359973437fe80fbd13e25502ddb0160a97,fefe4d9bbc4f0c3ccbbee7bb7626c19351cf8da8,505733fbf200cfe53875d49c25441de133113e6c,b05f8d709d75a63d077f2a24e628ed81b7825628,4a137c2a733ae52ba0c95f57a64675cee4a477bc,b1a0c7b2825b76fe5a48d2d7ca7ad1f9533c28bc,c5f9dbd2d3168f462209c359c353114ebc691959,2851ea2174e3492d4469107713d3cdf37819ebdb,de5a00c1282f819ba4c84549e0162ae86cf512f8
# Push it to GitHub
git push --set-upstream origin backport-8511-to-4.2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-4.2.x

Then, create a pull request where the base branch is 4.2.x and the compare/head branch is backport-8511-to-4.2.x.

ianwallen pushed a commit to ianwallen/core-geonetwork that referenced this pull request Dec 10, 2024
* Add new minimumProfileForPrivileges property to groups

* Disable the group in privileges UI if missing required profile

* Restrict setting permissions for groups with a minimumProfileForPrivileges

* Documentation

* Update documentation

* retrigger checks

* Remove special case for groups that own the record

* Update help text for clarity

* Update formatting
ianwallen pushed a commit to ianwallen/core-geonetwork that referenced this pull request Dec 10, 2024
* Add new minimumProfileForPrivileges property to groups

* Disable the group in privileges UI if missing required profile

* Restrict setting permissions for groups with a minimumProfileForPrivileges

* Documentation

* Update documentation

* retrigger checks

* Remove special case for groups that own the record

* Update help text for clarity

* Update formatting
ianwallen added a commit that referenced this pull request Dec 10, 2024
* Add new minimumProfileForPrivileges property to groups

* Disable the group in privileges UI if missing required profile

* Restrict setting permissions for groups with a minimumProfileForPrivileges

* Documentation

* Update documentation

* retrigger checks

* Remove special case for groups that own the record

* Update help text for clarity

* Update formatting

Co-authored-by: tylerjmchugh <163562062+tylerjmchugh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants