New core setting : shareapi_only_share_with_group_members_exclude_gro…

[smarinier]

…up_list (issue #37677)

• Resolves: #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.

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.

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.

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

• Code is properly formatted
• Sign-off message is added to all commits
• [ ] Tests (unit, integration, api and/or acceptance) are included
• [ ] Screenshots before/after for front-end changes
• [ ] Documentation (manuals or wiki) has been updated or is not required
• [ ] Backports requested where applicable (ex: critical bugfixes)

[artonge]

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

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

[zak39]

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

I edited the first comment to answer you :slightly_smiling_face:

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 ?

[come-nc]

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?

[zak39]

Hi @come-nc :slightly_smiling_face:

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 : https://github.com/nextcloud/server/issues/37677 .

[zak39]

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

[nimishavijay]

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.

[come-nc]

• -> 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.

[come-nc]

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]

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]

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]

@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]

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]

Hi!

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

[jancborchardt]

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

@AndyScherzinger @schiessle what do you think about the explanation in https://github.com/nextcloud/server/pull/38173#issuecomment-1551094957 ?

[smarinier]

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

[dorianne-arawa]

Hello @artonge

To answer your question, we believe that both settings are definitely bound ("Restrict users to only share with users in their groups" and "Ignore these groups for sharing restriction").

Let's say the setting 1 is the existing checkbox "Restrict users to only share with users in their groups" and the setting 2 is the new option we would like to add (sentence + multiselect for groups).

The setting 1 is a restriction : it ensures that users no longer see each other (except if they belong to the same group) The setting 2 offers an additional restriction to setting 1 to prevent users from a same group to see each other (by ignoring specified groups) As setting 2 is additional to the first one, the 2 are linked.

Arawa

[dorianne-arawa]

Hi! To follow up on @jancborchardt 's question, what do you think of our last updates @AndyScherzinger, @artonge and @schiessle ? Dorianne - Arawa

[zak39]

Hi everyone!

Do you have an issue with CI on this PR?

Baptiste - Arawa

[maximelehericy]

@artonge could you have a look here ? thank you !

[AndyScherzinger]

@zak39 I enabled the CI (is blocked due to the PR coming from a fork - different remote)

@maximelehericy @artonge is back next week.

[zak39]

@zak39 I enabled the CI (is blocked due to the PR coming from a fork - different remote)

@maximelehericy @artonge is back next week.

Thanks @AndyScherzinger :)

I guess we have to resolve all the CI's fails ?

[AndyScherzinger]

I guess we have to resolve all the CI's fails ?

Can't say for sure, since I am not a php-dev, so I would rely on @artonge's feedback here.

[zak39]

Hi everyone :)

I don't understand why the dco is failled ? Can you explain me, please ?

I saw all commits have sign-off

[dorianne-arawa]

Indeed, we have heard @artonge 's comments and are working on the subject to propose a new wording and submit it to you.

[AndyScherzinger]

I don't understand why the dco is failled ? Can you explain me, please ?

I saw all commits have sign-off

@zak39 see at the very end when opening the dco check:

Summary

Commit sha: b1f6ba2, Author: zak39, Committer: zak39; Expected "zak39 [email protected]", but got "Baptiste Fotia [email protected]".

[zak39]

I don't understand why the dco is failled ? Can you explain me, please ? I saw all commits have sign-off

@zak39 see at the very end when opening the dco check:

Summary Commit sha: b1f6ba2, Author: zak39, Committer: zak39; Expected "zak39 [email protected]", but got "Baptiste Fotia [email protected]".

But it's strange ! My sing-off is present :monocle_face:

So, maybe I have to add a blank line or a message ? :thinking:

I will try it.

[botsarenthuman]

Hi, is there any movement on this?

[smarinier]

Hi, is there any movement on this?

The code has been ok for a few months. But there were discussions about the message labels. We are waiting now for PR approvals.

[artonge]

@smarinier I think your rebase went wrong:

[smarinier]

@smarinier I think your rebase went wrong:

We have DCO issues. Because of signing off. The command given by the DCO help comments doesn't work. Could we have git help on this stuff. I wouldn't like to put more garbage in the code than the fact the commit comment does not have exactly the expected name.