Go through GroupManager for group deletion

[PVince81]

This will trigger the delete events and make sure that related shares
get automatically deleted.

Fixes #89

Also ref #91

[jvillafanez]

I don't have the interface at hand, but this method should handle error cases somehow. What if the group can't be deleted? It should either throw a specific exception or return true / false to indicate success or failure.

[jvillafanez]

I don't like the approach. The main problem I see is that there is a cyclic dependency with the core's group manager, which is quite scary and we should avoid.

I think this should be handled by core somehow. Maybe the deletion should be done by core and the backend would just need to notify about it somehow. This might need architectural changes 😫

[PVince81]

cyclic dependency with the core's group manager

Not exactly. My feeling is that we should move to only use core's group manager in the end, see #91

So the customgroups UI + DAV layer would only use the Group Manager, and the Group Manager talks to the Group Backend provided by the app. But the Group Backend in customgroups does not depend on the Group Manager.

[PVince81]

@jvillafanez the other solution I dislike even more is to manually trigger the OC\Group::deleteGroup events from inside the DAV code onto the GroupManager instance.

[jvillafanez]

Although I'll have to check what we have now, I'd rather try an observer pattern here.

Basically, the backend would have a couple of methods to register observers. These observers would be notified by the backend via $observer1->notify() when the backend needs to notify an action to the observers. The parameters for the call will need to be decided: the common approach is to send the observed instance ($observer1->notify($this)), but I don't think this will be useful, so maybe a event approach such as ($observer1->notify($eventType, $eventData)) might be better.

The key point is that the core's group manager will be registered as observer when the backend is registered in the core's group manager. This way we achieve several things:

• The backend can notify core when a deletion (or any other planned event) happens so core can take the appropiated messures.
• The "notifications" are completely decoupled. In case there are bugs because we left something, or because more things are needed, if core is the one to blame, then we can fix it in core without need to touch an of the backends. For example, if core stores some metadata for the groups later at some point, this can be handled transparently without the backends knowing it.

[PVince81]

I understand your point. This reminds me that we already have Symfony dispatcher events which do not require a target object. So we could actually just listen to those in the Application class and rewire them to the IGroupManager instance. This would need tweaking of their parameters as some info is missing.

Would you agree with the latter approach ?

[jvillafanez]

I think the symfony's dispatcher is too wide for this.

The backend will have an empty list of observers, and it will add the observers there. When an event needs to be notified, it will be notified to all of the observers in the list. Just traversing the list is enough. There is no need for the backend to check anything about the elements in the list.

[PVince81]

Ok, indeed it is too wide. The symfony dispatcher was originally added by @sharidas to be able to catch the events in the audit app. If we trigger regular \OC\Group events then the audit app will get these anyway.

One thing about symfony dispatcher though: the reason we want to use it more often is to avoid the observer pattern on individual objects. Observer pattern is fine for big objects like IGroupManager but for individual objects like IGroup it is not suitable because one would need to first get a list of all possible groups to be able to register events on each one of them.

[jvillafanez]

I think there are a couple of things to consider in order to choose using either an observer or the symfony's dispatcher (aka, mediator): the intrinsic dependency among the components, which for this particular case is something like a parent-child relationship (being the core's group manager as the parent and each backend as the child), and also the lifespan of the affected components.

For our particular case, there is an obvious dependency between the core's group manager and the backends. Ideally, the backends shouldn't live outside the core's group manager, and every access should be done through core. This also means that the lifespan of the backend objects should be dependent of the lifespan of the core's group manager, so if the core's group manager stop existing somehow, then the backends should do the same.
In addition, the expected number of backends where the core's group manager should register itself is rather low: normally 2-3, maybe 10 at most if things go wild.

Using the symfony's dispatcher as mediator is fine if we don't expect explicit dependencies between the components and the lifespan among them is independent. This is the case of the communication among several apps. Some of the components might be disabled or not present, but the system should keep running without any problem; there shouldn't be any bugs caused by a component being disabled.

On the other hand, if we must make sure groups are handled properly in any situation, I don't think there is any other option than going always through the core's group manager for any operation. This means that direct access to the backend is forbidden because we can't guarantee the core's group manager will be notified about the changes (core's group manager would be expected to self-register as an observer when the backends are registered in it, but it isn't expected that the app do this by itself).
So, if in any case we'll need to fetch the core's group manager BEFORE accessing to the backend, I wonder if we need all of this instead of calling directly to the core's group manager.

[PVince81]

Yeah, I'm also wondering if we shouldn't just stick to always go through the core Group Manager. In this case the custom groups UI really is just a "better" UI than the usual group management stuff, except that it limits itself to custom groups.

Originally I decided to bypass the group manager mostly because we hadn't invented the concept of scopes yet. So this was a way to prevent custom groups to be visible in the users page by having it not implement certain methods. Now that we have the scope concept, there might not be anything preventing us to go through the group manager directly.

[jvillafanez]

The whole user and group management should have an architectural review, and check what we can do to improve it short term and long term. Just this is a lot of work to do. Making "user management 2.0" would be really nice but I don't think we'll able to start implementing something until 2019.
My point is that the "scope" concept was something that could solve the problem at hand and could fit more or less in the current architecture, but I don't think it's the best option.

[PVince81]

I suggest we move this to the next version to give us more time to settle on a solution

[PVince81]

@jvillafanez so what middle ground should we use ?

[jvillafanez]

Honestly, the user management is more chaotic than homer simpson's web page.

Let's see if we can have some clear responsabilities:

• The stored data for the group must be deleted by the custom group backend. This is limited to whatever the data the custom group backend stores, and not any other associated data. This is ok.
• To delete all the data of a group, including shares, tags, or whatever other metadata associated to it, the deletion should be done by the core's group manager because this is the main entry point for group management. This should be a cascade deletion: metadata of a deleted group needs to be deleted. There could be 2 options:
  • The group manager delete the metadata by itself. This could include sending "delete" events to listeners so they can delete whatever metadata is stored by themselves.
  • Any other component must check if the group is still alive (via core's group manager) and delete the data on access if the group isn't available any longer.

Now, what I don't understand is where all the dav code is in the architecture. My guess is that it should be on top the core's group manager and act as a controller. If this is the case, the customgroups shouldn't need to know anything about it.

I think we can sum up all of this in the question "why do we need direct access to the backend?" and then the follow up question: "if we need direct access, why the backend doesn't have the tools to notify changes?"

@jvillafanez so what middle ground should we use ?

I don't have easy solutions other than limit the mess to the minimum, which in this case would be use the core's group manager only where it's needed. Other solutions will need heavy changes:

• Let each component clean up when the group isn't available. This implies changes in several apps, and might require checking the group's existence which might be heavy.
• Add support to notify changes from the backend to the core's manager. This will require changes in core and probably in any other app that handle users and groups.

[PVince81]

The group manager delete the metadata by itself. This could include sending "delete" events to listeners so they can delete whatever metadata is stored by themselves.

It already does that with the hook events when calling IGroupManager->getGroup()->delete() (basically IGroup->delete()).

Any other component must check if the group is still alive (via core's group manager) and delete the data on access if the group isn't available any longer.

The sharing code already has safeties in place in case it finds shares pointing to non-existing users or groups, in which case the share is discarded from the result list, but not deleted on the fly.
There are cron jobs that would delete such shares, but as long as they don't run the group ressurection bug stays possible.

all the dav code is in the architecture

The DAV code of customgroups is only its API layer. The actual logic should be done by either the helper, service classes or the custom groups backend.

would be use the core's group manager only where it's needed

This is what this PR does: it only uses the core group manager for the delete operation, for now.

Let each component clean up when the group isn't available. This implies changes in several apps, and might require checking the group's existence which might be heavy.

Same as above, for shares at least. But it only happens in cron.
Even if we make share deletion happen on-demand whenever a share is queried for a non-existing group, what happens if said query has never be run until group ressurection ?

Add support to notify changes from the backend to the core's manager. This will require changes in core and probably in any other app that handle users and groups.

It would already be possible by using this event: https://github.com/owncloud/core/blob/master/lib/private/Group/Manager.php#L88. Also see https://github.com/owncloud/core/blob/master/lib/private/Group/Group.php#L269. The main problem here is that the listener expects us to pass a IGroup instance which we don't have from anywhere inside the customgroups app. Well, we could retrieve it with IGroupManager->getGroup(), but that's what we do in this PR already, except that we then call $group->delete() instead of $groupManager->trigger('postDelete', $group).
To bypass the requirement of having a IGroup we'd need an additional set of events in GroupManager that a backend can trigger that takes a string instead of a group object (requires core changes). Not sure if there is much benefit going that far ?

So keep this PR as is to match "use the core's group manager only where it's needed" or introduce group backend events that the group manager listens to in core to match "Add support to notify changes from the backend to the core's manager. This will require changes in core and probably in any other app that handle users and groups." ?

[jvillafanez]

The main problem here is that the listener expects us to pass a IGroup instance which we don't have from anywhere inside the customgroups app

This is where the user management architecture fails quite bad. The customgroups app should be able to generate their own IGroup implementation (such as CustomGroup) and core should be able to use it because core depends on IGroup. Having this setup, the customgroups app can create the specific CustomGroup and notify the deletion of the group.

Anyway, taking into account that it will require architectural changes that won't happen in the short term, I guess will need to go with this PR and inject the core's user manager

[PVince81]

The customgroups app should be able to generate their own IGroup implementation

Indeed, this bit bothered me as well.

[PVince81]

Another little concern is that now if we trigger the core hooks, some apps (audit_log) will receive two events: the customgroups special one and also the core delete one. Not that bad though.

[PVince81]

Rebased

[jvillafanez]

Code-wise this is fine 👍

If this is urgent we'll need to go with this code. I don't have better solutions without requiring heavy changes.

[SergioBertolinSG]

Ok it works. Tested with core's stable10 and master. 👍