Skip to content

Block sharing with selected groups. #49897

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

redblom
Copy link

@redblom redblom commented Dec 17, 2024

Summary

Continuation of PR #49217

Checklist

@redblom
Copy link
Author

redblom commented Dec 17, 2024

Status: Fully functional
To exclude groups from sharing, as an admin, open Administration settings and navigate to Administration -> Sharing
Below the 'Allow sharing with groups' checkbox you can add/remove groups to exclude from sharing.
As a consequence these groups will not appear in the share dialog when searching for groups to share with.

@redblom redblom force-pushed the sharing-exclude-groups branch 2 times, most recently from d665dfd to 3824a88 Compare December 20, 2024 11:38
@redblom redblom changed the title [WIP] Block sharing with selected groups. Block sharing with selected groups. Dec 20, 2024
Copy link
Contributor

github-actions bot commented Jan 1, 2025

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@redblom redblom force-pushed the sharing-exclude-groups branch 2 times, most recently from 6f8db90 to 6d54dc3 Compare February 19, 2025 15:05
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Looks mostly fine to me, but you need to make cs-fixer happy.

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Looks fine, did not test.

Would be better to do the check in the hideFromCollaboration method on groups, as the method is part of OCP and could be used by other apps.

@redblom
Copy link
Author

redblom commented Feb 20, 2025

Looks fine, did not test.

Would be better to do the check in the hideFromCollaboration method on groups, as the method is part of OCP and could be used by other apps.

Please correct me if I'm wrong but as far as I can tell the IHideFromCollaborationBackend isn't implemented anywhere and $group->hideFromCollaboration() currently always returns false.
So perhaps leave this for now (with a comment) until that implementation has been done, or at least implement that in another PR. What do you think?

@susnux
Copy link
Contributor

susnux commented Feb 20, 2025

Please correct me if I'm wrong but as far as I can tell the IHideFromCollaborationBackend isn't implemented anywhere and $group->hideFromCollaboration() currently always returns false.

You can add your code (the check of the config option) inside of hideFromCollaboration:

public function hideFromCollaboration(): bool {

Here you would add it like:

  public function hideFromCollaboration(): bool {
+ 	$groupsBlockList = $this->appConfig->getValueArray('core', 'shareapi_groups_block_list', []);
- 	return array_reduce($this->backends, function (bool $hide, GroupInterface $backend) {
+ 	return in_array($this->gid, $groupsBlockList)
+ 		|| array_reduce($this->backends, function (bool $hide, GroupInterface $backend) {

@come-nc
Copy link
Contributor

come-nc commented Feb 20, 2025

Looks fine, did not test.
Would be better to do the check in the hideFromCollaboration method on groups, as the method is part of OCP and could be used by other apps.

Please correct me if I'm wrong but as far as I can tell the IHideFromCollaborationBackend isn't implemented anywhere and $group->hideFromCollaboration() currently always returns false. So perhaps leave this for now (with a comment) until that implementation has been done, or at least implement that in another PR. What do you think?

I do not know about the backends, but in this case you do not care about the backend you can implement that directly in nextcloud/server/lib/private/Group/Group.php
It’s always this class that represents a group, whatever its backend.

@redblom redblom force-pushed the sharing-exclude-groups branch from 0a2c06b to 26e342c Compare February 24, 2025 13:36
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I only have a few small remarks, the approach and logic looks good to me!

@redblom redblom force-pushed the sharing-exclude-groups branch 2 times, most recently from c42ff59 to 1cb5453 Compare February 25, 2025 09:15
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks really nice now!

@redblom redblom force-pushed the sharing-exclude-groups branch from d0c793c to 3b03177 Compare February 25, 2025 16:12
@redblom
Copy link
Author

redblom commented Feb 25, 2025

@provokateurin

Still some meaningless changes here.

Right I see, that's coming from the cs-fix. But I reverted it.

@provokateurin
Copy link
Member

provokateurin commented Feb 25, 2025

Can you squash the commits? Your commit message will also have to follow https://www.conventionalcommits.org/en/v1.0.0/ to pass CI.

@redblom redblom force-pushed the sharing-exclude-groups branch from 3b03177 to 90fc3bd Compare February 26, 2025 09:27
@susnux susnux added this to the Nextcloud 32 milestone Mar 2, 2025
@susnux susnux force-pushed the sharing-exclude-groups branch from e848cb9 to 4c1afe0 Compare April 22, 2025 23:29
@susnux susnux requested review from a team as code owners April 22, 2025 23:29
redblom and others added 3 commits April 23, 2025 21:21
Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the sharing-exclude-groups branch from 33c0e4b to b7d2961 Compare April 23, 2025 19:21
@susnux
Copy link
Contributor

susnux commented Apr 23, 2025

Tests need to be adjusted:

  1. Test\Group\GroupTest::testGetUsersSingleBackend
    ArgumentCountError: Too few arguments to function OC\Group\Group::__construct(), 4 passed in /home/runner/actions-runner/_work/server/server/tests/lib/Group/GroupTest.php on line 68 and at least 5 expected

/home/runner/actions-runner/_work/server/server/lib/private/Group/Group.php:55
/home/runner/actions-runner/_work/server/server/tests/lib/Group/GroupTest.php:68

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.

Option to exclude native groups to share with for security perspective
5 participants