Implementation of Groupbackend

[JonathanTreffler]

continuation of #536

• [x] Rebase to keep up with master
• [x] migration strategy
• [x] gid collision handling
• [x] automatically delete groups without members
• [x] ensure admins cannot (un)assign members, create or delete saml groups
• [x] tests
• [x] Migrations for groups on existing installations

Since this is a long-running feature implementation with many PRs here's the timeline:

• #263
• #311
• #355
• #536
• this one

[JonathanTreffler]

• Migrations for groups on existing installations

For this step we need input/advice from the maintainers because we are currently not sure which implementation for that would be the best.

[melegiul]

@blizzz Beneath the main parts of this PR, which already worked like a charm, we finished now the last open issues on this PR:

1. gid collision handling
2. tests
3. ensure admins cannot delete SAML groups
4. delete SAML groups without members

Can you please review our changes?

[svenseeberg]

Is there a chance that this will get merged at some point? Can we do anything to help with that? Otherwise we would start searching for a solution outside of the user_saml app.

[svenseeberg]

We're already using the changes introduced with this PR in production and it works quite well. Migrating the groups into a dedicated back end also went smoothly. I think merging this PR adds more flexibility to the group management with this app. However we're able to implement other mechanisms and migrate back to the normal group back end if there is no chance that this PR is going to be merged.

[JonathanTreffler]

Thanks for the review @blizzz :) I will have time to look at it after next sunday (18.09), my colleagues maybe earlier.

[blizzz]

Thanks for the review @blizzz :) I will have time to look at it after next sunday (18.09), my colleagues maybe earlier.

I would have a rebased version with a version bump, dropping NC 22 and 23, and fixing some of the things I found ready. However, I cannot push to your branch. Maybe it's considerable to open the branch for it, or if you like I can provide the patch elsewhere.

[JonathanTreffler]

However, I cannot push to your branch. Maybe it's considerable to open the branch for it, or if you like I can provide the patch elsewhere.

Somehow I don't see the " Allow edits from maintainers." checkbox on this PR, maybe it is a permissions issue. You could create a PR to netzbegruenung:groupbackend with your changes, I would merge it and the changes then also exist in this PR.

[blizzz]

However, I cannot push to your branch. Maybe it's considerable to open the branch for it, or if you like I can provide the patch elsewhere.

Somehow I don't see the " Allow edits from maintainers." checkbox on this PR, maybe it is a permissions issue. You could create a PR to netzbegruenung:groupbackend with your changes, I would merge it and the changes then also exist in this PR.

roger roger, PR opened at https://github.com/netzbegruenung/user_saml/pull/13

[blizzz]

@JonathanTreffler any news? I can also create just another PR in this org, in good old fashion =)

[JonathanTreffler]

@JonathanTreffler any news? I can also create just another PR in this org, in good old fashion =)

No need for another PR, I will merge the one to the netzbegrünung org :)

[blizzz]

@JonathanTreffler I think this commit got lost: https://github.com/netzbegruenung/user_saml/pull/13/commits/f5173a98af53b1f9cceba714b57a8fb3c4704700

[blizzz]

My own smoke-testing otherwise went fine. Also the migration bits. Still would tend to add the IdP-identifiert to the default SAML-Prefix. Also with regard to https://github.com/nextcloud/user_saml/pull/355#issuecomment-540817603 occ commands as migration utilities (removing groups from migration, and reverting migrations) should be added, but I would add this in a seperate PR and not blow up this PR more than further necessary. I consider adding integration tests, does not need to be a blocker either.

[kevinmccurdybrd]

Hi guys! Thanks for spending some time to resolve this. I'm not sure which is the better solution between this and #659, but if we could get one of these reviewed and merged, that would be fantastic.

Selfishly, I'm about to deploy 5 azure instances of NC, and this would completely eliminate LDAP out of my design. This is a game changer, and I deeply appreciate all of the development that has gone into this solution and as well what @Ma27 did in 659. Thanks in advance!

[blizzz]

Closing as work is continued in https://github.com/nextcloud/user_saml/pull/662, where it can also being pushed to. Currently I am looking into integration tests and would like then to conclude the PR.