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

New core setting : shareapi_only_share_with_group_members_exclude_gro… #38173

Conversation

smarinier
Copy link

@smarinier smarinier commented May 10, 2023

…up_list (issue #37677)

Summary

This implements new settings 'shareapi_only_share_with_group_members_exclude_group_list' wich complements the actual boolean setting 'shareapi_only_share_with_group_members'. Indeed the admin may need to isolate some specific groups (eg admin groups) from this consideration of "we can share each others".

  • Implement the new array of groups settings, enabled/hidden in admin view by shareapi_only_share_with_group_members enabled/disabled.
  • Implement the complement group filtering in different part of code

Screenshots

Here is the list of users with their groups.

users-list-with-groups

In the demonstration, I log in with the alice's account and she is in the "DSI" and "Groupe A" groups.

Before

From the Settings > Sharing page.
I checked "Restrict users to only share with users in their groups" in the checkbox.

settings-sharing-before

As "alice", I would like to share my "Project-A" folder.

alice-search-users-before.webm

After

But, I would like, as "alice", to list the users who are in the same group as me to share my folder... Except for some groups where I wouldn't want to list those users.

So, from the Settings > Sharing page.
I checked "Restrict users to only share with users in their groups" in the checkbox... and a search bar appears.
I enter the groups I don't want to see in the shares.

In the example, I enter the "DSI" and "RH" groups.

settings-sharing-after

As "alice", I would like to share my "Project-A" folder.

alice-search-users-after.webm

And now, I cannot see the user "john" because he is in the DSI group. And yet, we are in the same group.

TODO

  • Should be handled in "dav" application, to be really picky
    • apps/dav/lib/Command/MoveCalendar.php : aff
    • apps/dav/lib/Connector/Sabre/Principal.php :
      *apps/dav/lib/DAV/GroupPrincipalBackend.php

Checklist

@artonge
Copy link
Contributor

artonge commented May 10, 2023

Hey, thanks for the pull request. After a quick glance, it seems to look good, but before diving into it further:

  • DCO does not pass
  • Can you add some screenshots?
  • Can you explain what the change does?
  • You can remove the changes in l10n, this will be handled separately

@smarinier smarinier force-pushed the feature/37677/exclude-some-groups-from-sharing-with-users branch from 645b896 to 4a8a75b Compare May 10, 2023 13:56
@szaimen szaimen added this to the Nextcloud 28 milestone May 10, 2023
@szaimen szaimen added the 2. developing Work in progress label May 10, 2023
@smarinier smarinier force-pushed the feature/37677/exclude-some-groups-from-sharing-with-users branch from 2749cd1 to ae29849 Compare May 10, 2023 15:50
@zak39
Copy link
Contributor

zak39 commented May 10, 2023

Hey, thanks for the pull request. After a quick glance, it seems to look good, but before diving into it further:

* [ ]  DCO does not pass

* [ ]  Can you add some screenshots?

* [ ]  Can you explain what the change does?

* [ ]  You can remove the changes in l10n, this will be handled separately

Hi @artonge 🙂

I edited the first comment to answer you 🙂

I think we have achieved our goals ?

If this Pull Request it's okay for you. Do you think it's possible to backport it to the stable25 ?

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Some nitpicks, but looks fine otherwise

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the change, same with apps/settings/l10n/fr.json

lib/public/Share/IManager.php Outdated Show resolved Hide resolved
lib/public/Share/IManager.php Show resolved Hide resolved
lib/private/Contacts/ContactsMenu/ContactsStore.php Outdated Show resolved Hide resolved
apps/settings/lib/Settings/Admin/Sharing.php Outdated Show resolved Hide resolved
lib/private/Collaboration/Collaborators/MailPlugin.php Outdated Show resolved Hide resolved
lib/private/Collaboration/Collaborators/MailPlugin.php Outdated Show resolved Hide resolved
lib/private/Collaboration/Collaborators/UserPlugin.php Outdated Show resolved Hide resolved
@come-nc
Copy link
Contributor

come-nc commented May 11, 2023

You will have to be really careful with the wording, because it was not clear to me at all before seeing the screencasts.

I thought this was about allowing some users to share with everyone (not be restricted).

But this is the other way around, it’s about restricting even more.
Can you explain the usecase here, why is it so important to hide some of these groups from sharing option?

<p id="selectShareWithGroupMembersExcludeGroups" class="indent <?php if (!$_['onlyShareWithGroupMembers'] || $_['shareAPIEnabled'] === 'no') {
p('hidden');
} ?>">
<em><?php p($l->t('Exclude some groups from sharing with users in their group')); ?></em>
Copy link
Contributor

Choose a reason for hiding this comment

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

People in these groups are still allowed to share stuff, so the wording is unclear I think.

lib/private/Collaboration/Collaborators/GroupPlugin.php Outdated Show resolved Hide resolved
lib/private/Collaboration/Collaborators/MailPlugin.php Outdated Show resolved Hide resolved
lib/private/Collaboration/Collaborators/UserPlugin.php Outdated Show resolved Hide resolved
lib/private/Collaboration/Collaborators/UserPlugin.php Outdated Show resolved Hide resolved
Comment on lines 1962 to 1955
$excludeGroups = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members_exclude_group_list', '');
$decodedExcludeGroups = json_decode($excludeGroups, true);
return $decodedExcludeGroups ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic appears at least 5 times in this PR, may be the time for adding a getAppValueArray method in IConfig.

Copy link
Author

Choose a reason for hiding this comment

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

There is no getAppValueBool, getAppValueInt, aso... I think it would be disturbing to have only this very specific method in an interface. But i think you're right, if you consider all the getAppValue(...) == 'yes', there is a need on getting settings. But maybe it should be handled in a dedicated PR ?

@zak39
Copy link
Contributor

zak39 commented May 11, 2023

Hi @come-nc 🙂

In fact, we wish to add an additional option to the Sharing settings available for the instance admins (Settings > Administration > Sharing).

One of the current available option is "Restrict users to only share with users from their groups"

If the box for this setting is checked, we would like to add a selection field + text below this setting:

  • the text explains what the selection field does:
  • we have suggested a text that can be changed later on, so that it can be as clear as possible
  • the purpose of this field is to make an exception to the rule "Restrict users to only share with users in their groups" if it is checked:
    -> if a group (or groups) is inserted in this field, e.g. the group "Guests", then users in the Guest group will not be able to share with each other
  • the selection field will display the user groups of the instance (local and LDAP) and will allow admins to insert one or more groups in this field

We believe this concept would be beneficial to the administration of Nextcloud. In this example, the organization could check the setting "Restrict users to only share with users in their groups" and use a “Guest” group in their LDAP to group all their external users. This LDAP Guest group would then be created in Nextcloud, meaning all the guests would be able to see each other, even though they are not from the same companies. This represents a loss of privacy.

And this is only one example among many others.

One more, the issue linked to this PR has been reviewed @nimishavijay and it was she who proposed the wording in this issue : #37677 .

@zak39
Copy link
Contributor

zak39 commented May 11, 2023

I updated the screenshot where we can see I added the DSI and RH groups in the first comment

@nimishavijay
Copy link
Member

You will have to be really careful with the wording, because it was not clear to me at all before seeing the screencasts.

The wording is quite tricky with this one, agreed there. Alternative wording:

  • Limit users to share with only users their group
    Restrict some groups from sharing with users in their group
    [ Select groups ]

@jancborchardt or @szaimen for help?

The use case is quite specific but it seems to make sense to me. Another idea for managing permissions for groups is from the user management page, by having a settings for each group. This proposal seems like a quick fix.

@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label May 11, 2023
@come-nc
Copy link
Contributor

come-nc commented May 11, 2023

  • -> if a group (or groups) is inserted in this field, e.g. the group "Guests", then users in the Guest group will not be able to share with each other

Then again this is not true, they will be able to share with each other but only if they are both members of another group.
This is what is worrying me, it is so hard to describe this feature with words, and that means that whatever we do this is something that users will misunderstand, misuse, and see as a magic box that does stuff.

We believe this concept would be beneficial to the administration of Nextcloud. In this example, the organization could check the setting "Restrict users to only share with users in their groups" and use a “Guest” group in their LDAP to group all their external users. This LDAP Guest group would then be created in Nextcloud, meaning all the guests would be able to see each other, even though they are not from the same companies. This represents a loss of privacy.

And this is only one example among many others.

I understand this example and can see cases where you even have some kind of users group with everyone.
But I feel like restricting sharing to groups is already a corner case and not the most common.

What about using "ignore" rather than "exclude" in the wording? "Ignore these groups for the sharing restriction" or something.

And by the way, wording can be fixed later but the variable name also needs to be clear so that we understand the code when we read it.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Further wording adjustment, based on @nimishavijay’s suggestion. Note we want to switch over from the word "users" to either "people" or "accounts", whichever fits better. (In this case it’s "accounts" as it’s technical.)

Limit accounts to share only within their groups
Members of selected groups will be restricted to share only with people in the same group
[ Select groups ]

Does that explain it well?

@come-nc
Copy link
Contributor

come-nc commented May 11, 2023

Further wording adjustment, based on @nimishavijay’s suggestion. Note we want to switch over from the word "users" to either "people" or "accounts", whichever fits better. (In this case it’s "accounts" as it’s technical.)

Limit accounts to share only within their groups
Members of selected groups will be restricted to share only with people in the same group
[ Select groups ]

Does that explain it well?

No, this is not what the new option does.
What it does is ignore some of the groups when checking if sharing is allowed.

So if you are in groups A, B and C.
The checkbox is checked and B is listed in the new options.
You will be able to share only with users from A and/or C (which may also be members of B but that is not relevant).

@jancborchardt
Copy link
Member

Hm ok, in that case I am questioning the use-case of this feature.

If the main case as described in the issue is to prevent guests from sharing among each other, it should not be so generic, and just be called:

Disallow sharing among guests
[Select guest group name]

(Possibly with a good default set)

Also cc @AndyScherzinger @schiessle regarding Files planning as this seems yet another complex feature addition for sharing.

@AndyScherzinger
Copy link
Member

For us to act on this within our plannings we would need to fully understand the use case and agree on having it added.
Else there is not much to do from my side at least.

@maximelehericy
Copy link

@jancborchardt

I think the guest group is one example.
But in my opinion, there are other examples:
For instance, you use functional groups to allow people to collaborate together (project or cust A, project or cust B), but also technical groups to manage your instance (e.g : people with the right to use Talk, people with the right to use Office, and so on).
When you tick the box Restrict users to share only with members of their groups, you don't want user from cust A to share with user from cust B through the "Talk people" group.
Therefore, you'd like to ignore these "technical" groups.

@dorianne-arawa
Copy link

dorianne-arawa commented May 17, 2023

Hello,
As a colleague of @zak39 , I take the liberty of answering on this subject.

Thank you for all the feedback! Here are our answers on the different questions raised:

Regarding the wording

Regarding the wording, our proposal lacked clarity. Wee think the proposal of @come-nc is more relevant:

  • Restrict users to only share with users in their groups
  • Ignore these groups for the sharing restriction
    The correlation between the two sentences is clearer thanks to the words ignore and restriction.

About the use case

About the use case: many companies using Nextcloud can be notable providers for various customers (like consulting firms ). If a company uses its Nextcloud with different customers, they should not be able to see each other for privacy reasons (especially in the field of shares).

However, for management or access rights reasons, it can be convenient for administrators of a Nextcloud platform to define "functional" or "management" groups. These groups gather users and allow to manipulate several of them at once. However, these "functional" or "management" groups are not intended for users to see each other. This can sometimes be prohibitive to the use of the Nextcloud service.

Concrete examples:
* to make a groupfolder containing all "clients" ("Clients" group)
* for mass communication purposes via sharing (sharing documentation, instructions, administrative documents, etc.) This prevents making lots of shares with the same rights, on the same folder (one share per client, if we have to share with 30 clients or more, that's a lot of shares to manage manually)
* in the Workspace application, the default group "WorkspacesManagers" can gather users of several customers
* to have a users group(s) allowed to give access to "administrator privileges" of one or more Nextcloud settings sections
* to restrict the application access to a given group of users (application access limitation option in the application management interface)

  • see also the examples from @maximelehericy (thank you for these concrete examples)*

Even if these example cases might not seem common at first , there remains an existing use case which needs to be addressed to allow a more flexible use of Nextcloud, adapted to different contexts, and as secure and confidential as possible.

Regarding Nimisha's proposal

Regarding Nimisha's proposal: Another idea for managing permissions for groups is from the user management page, by having a settings for each group. This proposal seems like a quick fix.
We think we should refocus on the notion of shares which is addressed here, and which is not managed (at least currently) in the Nextcloud user interface. As a reminder, this feature is meant to keep confidentiality of personal user’s data (names, email address and company), which is visible in the "shares" field.

I hope that my answer and my different arguments will be clear enough, I will answer your questions if there are any misunderstandings.

@dorianne-arawa
Copy link

Hi!

Do you need more information regarding my last message to understand this PR?

@jancborchardt
Copy link
Member

Thank you for the clarification @dorianne-arawa! :) That now makes more sense.

@AndyScherzinger @schiessle what do you think about the explanation in #38173 (comment) ?

@smarinier
Copy link
Author

Hi @jancborchardt , @come-nc and @artonge,

I think we've taken in account all the discussion concerning the wording, and other remarks. Do you think you could have a look on this ?

Thanks a lot

@zak39
Copy link
Contributor

zak39 commented Dec 13, 2023

/compile /

Nextcloud bot could not build as far as I can see.

Do you want to build assets manually ?

@artonge
Copy link
Contributor

artonge commented Dec 13, 2023

Do you want to build assets manually ?

Please do, and also squash your commits

@zak39
Copy link
Contributor

zak39 commented Dec 14, 2023

Do you want to build assets manually ?

Please do, and also squash your commits

I will build assets this morning.
For the squash, if I understand, we have to squash (merge commits) and get a few commits ?

What is the good practice for this command ? Squash all "Merge branch" commits ? Squash all feature commits ?

@artonge
Copy link
Contributor

artonge commented Dec 14, 2023

What is the good practice for this command ? Squash all "Merge branch" commits ? Squash all feature commits ?

This is only to have a cleaner git history. You can try:

git reset --soft HEAD~3
git commit

Where 3 must be replaced by the number of commits you want to squash. In your case, probably 18.

@zak39
Copy link
Contributor

zak39 commented Dec 14, 2023

What is the good practice for this command ? Squash all "Merge branch" commits ? Squash all feature commits ?

This is only to have a cleaner git history. You can try:

git reset --soft HEAD~3
git commit

Where 3 must be replaced by the number of commits you want to squash. In your case, probably 18.

I don't think it's a better solution to use the git reset command. Because, I see files we didn't touch.
Application files in the apps/ folder. I'm trying with squash from the git rebase command.

However, I have to get to 5af683d2c4 commit to squash our first commit.
So up to 2168 commits backawards 😕

It's very strange, because, when I use the git log --oneline -n 20 I get these commits :

9a6b846955 Merge branch 'master' into feature/37677/exclude-some-groups-from-sharing-with-users
d6ac1b9531 Merge pull request #42095 from nextcloud/fix/files/exact-nav
27cbb352c6 Merge pull request #42183 from nextcloud/fix/unified-search-prop
79c25f4cff Merge pull request #42198 from nextcloud/artonge/fix/metadata_error
a6360d4acd chore: Compile assets
a4a1dc85a3 fix(UnifiedSearch): Remove title prop from modal
11e27acf2c Merge pull request #42174 from nextcloud/chore/comments-cleanup-dead-code
8eb58d03a9 Wrap metadata generation in try/catch
6077f26d9c Merge pull request #42064 from nextcloud/feat/settings/add-delegation-commands
63babd2324 Merge pull request #42167 from nextcloud/unified-search-improvements
208e6bc492 feat(settings): add occ commands to handle admin delegation
895eb19449 Fix(l10n): Update translations from Transifex
842953a267 chore(assets): Recompile assets
c0374c79d7 Add authenicated user to person list for search filtering
527c51a18e Unified search: update people select with API calls
bf234f6acc Rephrase unified search helper text
c734a18ee8 Merge pull request #42094 from nextcloud/refactor-global-search-to-unified-search
2f668976de chore: Compile assets
0b171f6095 Rename "global search" to "unified search"

I can't get our first commit 40da042152 New core setting : shareapi_only_share_with_group_members_exclude_group_list (issue 37677), or even the other commits.

I get commits that don't belong to us, for example :

d6ac1b9531 Merge pull request #42095 from nextcloud/fix/files/exact-nav
27cbb352c6 Merge pull request #42183 from nextcloud/fix/unified-search-prop
79c25f4cff Merge pull request #42198 from nextcloud/artonge/fix/metadata_error

@zak39 zak39 force-pushed the feature/37677/exclude-some-groups-from-sharing-with-users branch from eabe8d2 to 936cd96 Compare December 27, 2023 12:00
@zak39
Copy link
Contributor

zak39 commented Dec 27, 2023

@artonge it's okay now 👍

@zak39
Copy link
Contributor

zak39 commented Dec 27, 2023

Thank you a lot @skjnldsv for your help 🙏

@artonge
Copy link
Contributor

artonge commented Jan 2, 2024

@zak39 Node CI is still failing.

@zak39 zak39 force-pushed the feature/37677/exclude-some-groups-from-sharing-with-users branch 3 times, most recently from d9e3713 to 46ba22e Compare January 9, 2024 10:23
@zak39
Copy link
Contributor

zak39 commented Jan 9, 2024

@zak39 Node CI is still failing.

Hi @artonge :)

I rebuilt and rebased this PR and all checks are greens 👌 ✔️

@zak39 zak39 force-pushed the feature/37677/exclude-some-groups-from-sharing-with-users branch 3 times, most recently from 1ea9062 to 4cbe596 Compare January 30, 2024 15:20
@@ -63,6 +63,7 @@
$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', '');
$linksExcludedGroups = $this->config->getAppValue('core', 'shareapi_allow_links_exclude_groups', '');
$excludedPasswordGroups = $this->config->getAppValue('core', 'shareapi_enforce_links_password_excluded_groups', '');
$onlyShareWithGroupMembersExcludeGroupList = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members_exclude_group_list', '');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::getAppValue has been marked as deprecated
@artonge
Copy link
Contributor

artonge commented Jan 31, 2024

@zak39 Have you properly rebased? Looks like there is some merge conflicts left.

@zak39
Copy link
Contributor

zak39 commented Jan 31, 2024

@zak39 Have you properly rebased? Looks like there is some merge conflicts left.

It's strange ! I rebased correctly yesterday. I try rebase again now to see.

zak39 added 4 commits January 31, 2024 10:44
I added our context in the unit tests following the advice of Louis.

Link : nextcloud#43186 (comment)

Signed-off-by: Baptiste Fotia <[email protected]>
Signed-off-by: Baptiste Fotia <[email protected]>
Signed-off-by: Baptiste Fotia <[email protected]>
@zak39 zak39 force-pushed the feature/37677/exclude-some-groups-from-sharing-with-users branch from 4cbe596 to 2f64411 Compare January 31, 2024 09:54
@artonge
Copy link
Contributor

artonge commented Jan 31, 2024

Drone CI is waiting as this is a PR from a fork, but it is green in this PR: #43186
So merging.

@artonge artonge merged commit 7dc5a91 into nextcloud:master Jan 31, 2024
133 checks passed
Copy link

welcome bot commented Jan 31, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@zak39
Copy link
Contributor

zak39 commented Jan 31, 2024

Oooh good, thanks @artonge 🙏

Let's keep our an appointment or it's not necessary ?

@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement feature: settings pending documentation This pull request needs an associated documentation update
Projects
None yet