Core: Prevent sharing with system groups

[PVince81]

System has internal groups (groups not for public use) that are important in OC

• groups are useful for features like "exclude app from groups", "external storage with groups", whatever
• but group must not be used for sharing, for example because it contains all system users so admin wants to exclude these
• Filtering will be done by the files_sharing app. No changes in core should be required.

• [ ] add new "oc_appconfig" setting for blacklisting groups from a specific backend: an array of "${backendClassName}::${groupName}". Backend class name is mandatory.
• [ ] filter out results when querying groups to share based on blacklist
• [ ] ~UI with autocomplete to set the name of groups~ Settings page will include a textarea to enter the groups manually as specified above. No autocomplete will be provided for now.

Need to find the right technical approach for this as there are many challenges.

Edited

[PVince81]

Some requirements:

• the share autocomplete result must be filtered by the server so that any client receives the pre-filtered list
• sharing directly with blacklisted groups must be forbidden by API (403)

[PVince81]

Possible approaches:

• filtering the list

  • above GroupManager level, OCS Share API level:
    
    
    
    • advantage: closer to share autocomplete call
    
    
  • GroupManager level, when scope is "sharing":
    
    
    
    • advantage: autocomplete call will be pre-filtered
    
    
    • disadvantage: will break pagination because it will remove entries from within the middle of pages
    
    
  • on group backend level (below GroupManager):
    
    
    
    • advantage: blacklist entries could be included in SQL query or LDAP query for exclusion
    
    
    • advantage: pagination still works
    
    
    • disadvantage: too specific, every backend needs adjustment and specific implementation
    
    
  • other approaches ?

• preventing to share with blacklisted groups:

  • to be done on OCS Share API level, likely ShareManager

@jvillafanez any thoughts or suggestions ?

[PVince81]

Also to think about:

• can custom groups be blacklisted ?

[ownclouders]

GitMate.io thinks possibly related issues are https://github.com/owncloud/core/issues/398 (Problems with group sharing), https://github.com/owncloud/core/issues/11677 (Unable to share files with new users/groups), https://github.com/owncloud/core/issues/1218 (Share problem with too much similar groups), https://github.com/owncloud/core/issues/881 (Can't share calendar within the same group), and https://github.com/owncloud/core/issues/18192 (Prevent downgrades).

[PVince81]

the dilemma is: either filter in the GroupManager and break pagination because we remove the blacklisted group in the middle or expect the group backend to filter based on given array of "to exclude" groups which needs touching every group backend but might be more efficient

[jvillafanez]

GroupManager level, when scope is "sharing"

I'm discarding this. Scopes were mainly designed to give user / group backends some context for them to decide which users or groups should be returned. This would means that each backend should be doing the filtering instead of the sharing piece.

The filtering should be done by the sharing piece because this is only related to sharing. Other components shouldn't care about this.

on group backend level (below GroupManager)

Discarding due to the same reason as above.

I think the sharing API is part of the files_sharing app. This opens the possiblity to set this option as part of the sharing app instead of as part of core (shareManager and related classes)

Viable options I see are:

• Set the option as part of the files_sharing app.
  
  
  
  • As long as the API is there, the option can be used by the API, and the app can also filter the groups for the sharing autocomplete.
  
  
  • Other apps might access to the sharing component of core, and these apps can include shares bypassing the limitations set by the files_sharing app
  
  
• Set the option as part of core (around ShareManager).
  
  
  
  • Preventing sharing with groups seems possible, but I don't think we can have control over the sharing autocomplete. This would mean that groups will be shown to the users although some of them will return an error.
  
  
  • All apps would be limited by the configuration
  
  
  • The code is quite overloaded already for the ShareManager. Adding more responsabilities might not be a good idea, so unless the code changes are small we might need to find another solution.
  
  

An hybrid solution might be another option: core would provide the configuration option as well as prevent the sharing with those groups, meanwhile, the files_sharing app would reuse the configuration option to use it in the API and in the sharing autocomplete.
Core would somehow expose the configuration option for the app to consume.

If we opt for the hybrid solution we should consider to refactor the ShareManager to move the sharing configuration to another class. As said, adding more responsabilities to the ShareManager doesn't look good.

[PVince81]

@jvillafanez so if I understand correctly, if we go with the first option "Set the option as part of the files_sharing app." the restriction will be in place in:

• OCS Share API autocomplete endpoint, filter out dynamically after getting the results from the group backend (might break pagination, but we don't use "offset" in there AFAIK so it's ok)
• OCS Share API when creating new shares, prevent the blacklisted groups to be added/used
• pros: more isolated in the app, responsibility clearer
• cons: apps can still share with blacklisted groups by talking to ShareManager directly and would require to know about this option

In any case, if an app is requiring a user to specify a group, they'd better use the existing share autocomplete endpoint instead of providing their own.

[PVince81]

I think the latter solution "set the option as part of files_sharing app" is the one I tend to prefer despite the cons.

If @tomneedham @butonic @DeepDiver1975 have no objections I suggest moving forward with this solution

[butonic]

@tomneedham may have a stronger opinion because he used scopes for the support portal

[tomneedham]

I don't understand the terminology here:

• What is a 'system group' ??
• When you say "Custom group" do you mean specifically the customgroups app? Or generic !database group backends?

[tomneedham]

can custom groups be blacklisted ?

If by custom group you mean any other group backend:

I was this to be true. So that an app can define a grouping of users as a group and you can apply any restrictions on this group.

[PVince81]

@tomneedham with system group mostly is meaning as viewed by the system administrator: it could be an LDAP group containing all users from the system which is usually a group you don't want to expose for sharing. Not sure if there's a better name for that.

With custom groups I mean groups from the "customgroups" app which are exposed with its own specific group backend.

[jvillafanez]

As things are right now, groups from the customgroups app should also be blacklisted. The reason is that sharing shouldn't care about the place where the groups come from, it just gets a list of groups from the group manager and blacklist them according to some rules.

I agree that this is kind of bad because admin shouldn't blacklist groups created by the users. We'll need to figure out how can we deal with this properly.
A simple idea could be to include a isCreatedByUser() function in the Group class so we can distinguish this scenario. Note that the blacklist will be by group name, but I guess we might need to add more restrictions in the future such as depending on the number of members in the group (this shouldn't be a problem in this case).

[PVince81]

@jvillafanez I don't like isCreatedByUser because it sounds very specific to custom groups and I'm not sure there will be other apps out there.

How about have the option of adding a backend class name to blacklisted groups ? So the internal config setting would say something like "Group_LDAP::all_ldap_users_group". So we'd exclude by backend name. I believe you can retrieve the backend class/string from the Group object.

[jvillafanez]

How about have the option of adding a backend class name to blacklisted groups ? So the internal config setting would say something like "Group_LDAP::all_ldap_users_group"

That sounds better. Files_sharing would just blindly check it according to a predefined format

[PVince81]

I've updated the original post with: " add new setting for blacklisting groups from a specific backend: an array of "${backendClassName}::${groupName}" (exact format to be decided/improved)"

[jvillafanez]

Note that we might need to force the usage of the backendClassName. Since the backendClassName shouldn't have "::" in it, we have a clear separator between both components. Group names can have "::" in the name that would cause problems if we don't make the backendClassName mandatory.

How are the admin going to modify the option?

• Getting the option from the config.php is easy. Development effort would be near zero.
• Showing the option as part of the sharing panel in the settings page. This might need some extra time. We'll probably need a textarea so the admin can enter multiple groups, one per line.
• Any other?

I think this is the last piece before starting the development.

[PVince81]

I agree to make the backend class name mandatory for now.
If the need arises we could allow a single star wildcard "*::groupName", but I'm not sure we should do that right now without a clear requirement.

Regarding where to put the config, based on the roles: there are actually two admins, the system administrator who has FS access and can edit config.php and the OC admin who can configure users over the web UI and APIs, but no CLI access.

Since configuring sharing and LDAP is usually an OC admin thing, I think we should provide a field in the sharing section and store into "oc_appconfig" under the "files_sharing" app as this is app specific.

• Minimal version would be to have a simple text field with comma-separated values (or a better LDAP-proof separator?).
• Maximal version would have an autocomplete field (like the one from "exclude groups from sharing") that allows typing in group names and selecting.
  
  
  
  • might be a matter of copy-pasting the logic from "exclude groups from sharing" that uses select2
  
  
  • need to add "backendClass" in OCS Share API autocomplete endpoint results
  
  
  • need to figure out a separator because select2 stores the value as a plain string anyway, and we'll store the value as a plain string in oc_appconfig
  
  

@jvillafanez can you estimate both minimal and maximal version for the settings UI ? We should start with the backend change first and implement the UI last. (being able to set with occ config* command is already nice)

[PVince81]

I've updated the original post to include a note about the UI and the fact the backend class name is mandatory.

[jvillafanez]

• minimal version: 1-2 md just for the UI changes (likely 1 md, but 2 md if anything happens. This also includes some basic dev testing, but not QA)
• maximal version: I can't estimate properly since I'm not sure about what changes are required. I'd say roughly 1-2 weeks for the UI changes, but it could take less time if there are things that can be copy&pasted

[PVince81]

@jvillafanez based on my own knowledge of the code I'd rather estimate as: minimal version: 0.5-1md and maximal version 2-4md as adding autocomplete in there can be copy pasted and then adjusted.

Let's go with the minimal version for now within the main PR to make it easy to test.

[PVince81]

@jvillafanez anything else needed before you can start ? If not please summarize the various decisions into the original post. Thanks!

[jvillafanez]

This is ready. The original post has been updated with the decissions taken.

[jvillafanez]

Looks like we'll need to create new templates for the files_sharing app. Surprisingly, the app doesn't have any option; most of the sharing options seems to be controlled by core.

[jvillafanez]

@pmaier1 I'll need some wording for the text explaining what you should write in the textarea. I'm not sure if just the placeholder is enough. We might also need to change the position, maybe above the federation section.

---

(showing the placeholder)

Note that we'll need to include somehow a list with all available group backends. This requires changes in core. The problem I see right now is that the admin won't know what backends are available, and since we'll need to handle 3rdparty apps, we can't provide a nice mapping, so we'll need to use the class name.

Pinging also @felixheidecke for css magic. Current visual problems:

• We'll need to add a list with the available backends. The list is informative for now (I'd rather keep things simple for now). I'd expect 10 items at most, so maybe a classic bullet point list at the left of t he text area is enough, or just enumerate the items in a paragraph like "this are the available backends: 'backend1', 'backend2', 'backend3'"
• The size of the textarea might need some adjustments. Seems a bit small for desktop (I'd prefer to not wrap the text if possible), but otherwise we'll need to scroll horizontally in a mobile device.

The code in the PR works for whoever wants to play with it (https://github.com/owncloud/core/pull/31672), but as said, it still needs to be polished a lot.

[PVince81]

Note that we'll need to include somehow a list with all available group backends.

Indeed. That's a prerequisite for the autocomplete solution for a later point. (https://github.com/owncloud/core/issues/31578#issuecomment-393886142)

Maybe an occ command would be enough ?
In general, I'd expect the admin who has setup "occ user:sync" to know the backend names as they had to set it up there.

[jvillafanez]

I'd rather keep things as near as possible. If the expectation is that the admin will set the feature through the web interface, it would be annoying if he has to go to the console to get the information. In addition, we've decided that the feature shouldn't require shell access.

We might need to add the command, but later.

[PVince81]

@jvillafanez I agree that the mixing up locations isn't that nice. For now it's more important to have the feature in the first place so we could schedule another block of time to make it more friendly

[jvillafanez]

Adding some screenshots with the latest changes.

---

---

I don't expect more than 5 different backends in a system, usually 3 (local, ldap and custom groups), so I don't think we'll need to handle a big list of backends.

The textarea is 400px width, which should fit most of the groups in one line. If the screen has less width (mobile phone for example), it will take the whole available width; this should avoid having to scroll horizontally.
In addition, the textarea will be a minimum of 80px height which should be enough to hold 4 groups without scrolling (do we need more height?). The height will also adapt to fill the vertical space in case that there are a lot of backends. With 5 backends the height of the textarea is 100-105px, so it won't have a big visual impact.

The bullet points have been added to make sure there is no confusion about the names. However we might want to remove them because it should be impossible to have spaces in the backend name, so the name will stay in one line and not split into several lines as shown in the last screenshot.

[PVince81]

@jvillafanez how about putting the backend names on the right of the text area instead ?

[jvillafanez]

Makes sense. The list will be below the textarea in case of small screens though.

[tomneedham]

Why do we identify groups using displaynames?

[tomneedham]

I think we need to make some text clearer. We already have:

Exclude groups from sharing

And now we will have:

Blacklist the following groups so that no one can share with them.

These sound similar, but as I understand it, the first one still allows receiving of shares?

[tomneedham]

Can we not use exactly the same UI as the exclude groups from sharing box? Why do we need this new style box? Typing in the backend name is not admin friendly.

[phil-davis]

The two things are "opposite". Maybe:
"Exclude groups from creating shares"
and
"Exclude groups from receiving shares"
?

[tomneedham]

And can the UI / feel be exactly the same / next to each other?

[jvillafanez]

Can we guarantee that the group id IS unique across all the backends? As far as I know, the group id should be unique in a specific backend but right now it could be duplicated in other backends. Matching against the id won't be enough to uniquely identify an specific group.

The reason to use the displayname is because it will be easier for the admin to know which group he wants to blacklist. The backend is there to specify that group in that backend, and not any other group with the same name that might be in any other backend.

I've been checking if we could use /settings/users/groups?pattern=d&filterGroups=1 endpoint, but it searches by group id. This is quite bad because the id is intended to be private, and it just matches the displayname by luck.
Since we can't search groups by displayname, I don't think it's a good solution for now. We might need to wait until the group table arrives to implement this solution.

[tomneedham]

Well then it seems we have some concepts we need to firm up:

• Should group ids be unique across backends, or just within backends?
• Can group id's contain commas? (I know they are imploded with comma as glue in at least one place)
• Is the group id private considered private?

My preference is that we implement this the same way as the current 'exclude groups from sharing' and rename the settings as @phil-davis suggested. Then refactor this to use dispalynames when it is possible .

[phil-davis]

Can group id's contain commas?
It works in the Provisioning API: #31719

[PVince81]

From what I see the PR was adjusted accordingly: https://github.com/owncloud/core/pull/31672

I suggest to work on the acceptance tests separately:

• check that autocomplete doesn't return blacklisted groups for sharing
• check that blacklisted group is still visible in other locations (ex: users page)

[PVince81]

One concern to confirm before closing this ticket:

What happens if shares exist that were shared with a group that has now been blacklisted ? I assume these must still appear in the shares list and be unshareable by users. I don't think we need to add a mechanism to also exclude existing shares/mounts that point to blacklisted groups.

@tomneedham

[PVince81]

docs ticket: https://github.com/owncloud/documentation/issues/4296

[PVince81]

@phil-davis do we have all the required scenario steps to be able to test this ?

Would suggest having @jvillafanez write said tests

[PVince81]

@phil-davis API level tests should be enough here I think

[phil-davis]

Assigned myself to look at tests. OOTO for a few days though.

[phil-davis]

I removed my assignment - tests were done in https://github.com/owncloud/QA/issues/594
@PVince81 maybe this issue is all done and can be closed?

[phil-davis]

Any left here? @PVince81 or?