Hide custom groups from admin / users page

[PVince81]

No description provided.

[PVince81]

• hide from admin
• hide from Webdav principals

[PVince81]

I tried several approaches, but none is satisfactory.

One of the approaches adds a new method param $manageableOnly to group-related methods on the IGroupManager. The trouble with this is that the group manager caches groups, so it would need to cache them both, one with $manageableOnly = true and one with false...

Now I could first retrieve all groups and then filter them later in the UsersController or Group\Metadata which is used to populate the UI...

[PVince81]

Tentative PR here: owncloud/core#27184

[PVince81]

or maybe we should just allow them to appear there...

The one problem I see is that they are not distinguishable from regular groups. One idea would be to introduce the backend name, so it could appear as:

• localgroup (Local)
• ldapgroup (LDAP)
• My custom group (Custom group)

For this the backend id needs to be exposed to the UI code with a friendly name.

But this brings other issues: the list of groups could become long and it would need to be searchable.

That or simply have a checkbox for the admin "don't show groups from these backends: "... That would be close to what this PR does.

However the groups would still appear in the provisioning API, and there there is also no information about what backend they come from...

[PVince81]

• Approach 1: post-filter group list: Exclude group backend from showing groups in users page (post-filter approach) core#27184
• Approach 2: getUserGroupsForSharing, sharing specific APIs: Only return groups for sharing #25 => bullshit
• Approach 3: actually leave them visible but use their display names: Show group display name instead of id in users page core#27186
• Approach 4: isVisibleFor Exclude group backend from showing groups in users page (group manager filter approach) core#27287

[jvillafanez]

I think the group backend interface should include a isVisibleFor($subsystem, $user). Basically core would ask the backend if that group is visible for the subsystem (sharing, webdav, user listing, etc) and the current user.

The advantages I see with that approach:

• Backends have control over their own visibility in well-known core components. It's their choice to show or hide their groups.
• This is extensible to 3rdparty apps as long as both apps know each other. 3rdparty can check if the group backend is visible for "my_custom_app" and decide.

[PVince81]

Hmm, this might work I guess, I'll give it a try.

I didn't like the idea of having a backend knowing about UI stuff, but there might be no other way.

For now these are the subsystems I'd list:

• "management": meaning users page for management and also provisioning API (no distinction between the two, the users page should really use the provisioning API...)
• "sharing": sharing autocomplete

@jvillafanez then the question is when exactly to call isVisibleFor ? I'd probably do it in a post-filter like owncloud/core#27184 to avoid affecting the internal caches of the GroupManager as we don't want partial caching depending on which subsystem calls its APIs.

[jvillafanez]

From a client's perspective, it should fetch all the groups from the manager and traverse the whole list checking the visibility. This is the least intrusive approach because there isn't any change in the manager.

We could add another method in the manager to fetch the groups matching a filter (this new visiblity filter also included). This is a bit more optimum because it's expected the client will traverse a smaller group set. However, I'm not sure if this will be worthy for now because the optimization will rely on the number of items filtered out.

[PVince81]

Ok, thanks for the feedback.

I think I'll go with the post-filter approach, basically a combination of:

• users page post-filter: Exclude group backend from showing groups in users page (post-filter approach) core#27184 but using isVisibleFor('management') and
• sharing post-filter: Only return groups for sharing #25 but using isVisibleFor('sharing').

That should do for now hopefully.

[jvillafanez]

Passing the current user might be useful. Backends could check if the user is an admin and decide to show all groups or a part of them, for example.

[PVince81]

Maybe.

I had some thoughts and we need to make sure this stays compatible with the future "central group account table" like the "user account table" that is being done as part of owncloud/core#23558

The way I see it:

• add new methodIGroupManager->getVisibleBackendsFor('sharing') which returns a list of backend ids visible for the given context by calling their respective isVisibleFor($scope, $currentUserId) methods.
• then use the list of backends and plug it into a method IGroupManager->search(..., $limitBackends) so that the limiting can be done on DB-level. If we manage to always use search(), there is no caching of groups involved.
• add IGroup->getBackend() to find out what backend a group is in

This means that filtering does happen in the group manager. I'll see if that is feasible.

If not, then the less nice solution is to still do post-filtering: the UsersController of the users page would query which backends must be visible, then iterate over every group and call IGroup->getBackend() to filter out the ones we don't want

[PVince81]

Maybe this can be refined to be called isSearchableFor() and only apply to the search() API, which is usually used anyway when listing groups. However if the group manager is called with a known group, then it is still touchable. That solution might be enough.

[PVince81]

Damn... that's not enough. It is possible to remove these groups from the general result set (left sidebar in users page). But group memberships for custom groups are still visible. That's where filtering out gets more tricky because that information is cached. And if some API expects the membership to be there and the cache tells it doesn't, then 💥.

So maybe for this specific purpose if a scope is given, then it doesn't cache the result.

Needs further research.

[PVince81]

Here is where the user groups are fetched for the users page: https://github.com/owncloud/core/blob/master/settings/Controller/UsersController.php#L191.

This leads to https://github.com/owncloud/core/blob/master/lib/private/Group/Manager.php#L260.
As you can see it caches the user group memberships.

Do you think it would make sense to add a $scope param to this (and other functions leading here) and disable caching if a scope is provided ?

I guess in general that the cache there is only meaningful for the current user's group memberships, and maybe some for sharing users, hmm...

[PVince81]

Preliminary work here for the search method: owncloud/core#27287

[jvillafanez]

I'd rather do the filtering outside of the manager for now. The manager's client will fetch all the groups with the usual search method (no need to change anything there), and then it will filter the results by itself. Something like:

$groupList = $groupManager->search();
foreach ($groupList as $group) {
  if ($group->isVisibleFor('whatever')) {
    // show group.
  }
}

Later we can add a specific method with the filtering included, which can handle the cache better.

[PVince81]

Yeah, I thought of post-filter, but it's not future proof regarding the future group account table. We'll need to rechange every location where we do post-filtering.

In the meantime I just implemented this. It's ugly but it works... Look: https://github.com/owncloud/core/pull/27287/files#diff-93cb10073d420e1ba441767b3a1e6de7R262

I'll add another bit for the search code soon and then let this sleep for a bit... Hoping to find better insights in the next days.

[PVince81]

(we can always discard this PR if this is total bullshit)