Implementation of Groupbackend II #662

blizzz · 2022-10-10T19:42:33Z

Work PR of #545, in good old tradition… For it is not possible to push into #545 it's easier to have the feature branch here.

Default group mapping prefix is not shown in settings UI.
new table(s) are missing primary keys
consider membership changes in local groups during transition Implementation of Groupbackend II #662 (comment)

Group display names

In one of the predecessor PRs group display names were a topic, with the conclusion

makeing use of group display names so present it more user friendly, perhaps with (SAML) in brackets.

The (SAML) suffix is in the code currently. While stated by the younger me, it does not make me very happy:

typical for LTR languages only
technical term exposed to potentially unwitting end users (why should they know about this detail
We have a FIXME that is rather wondering about adding another config flag here…

I am going to revert this, to use the value as provided from SAML again. Besides avoiding the downsides mentioned above, the end user does not have to face a change and will not suffer irritation. Admins may need a closer look to verify the backend, if needed, which is totally fine imo.

Group IDs

We have a group prefix SAMLSettings::DEFAULT_GROUP_PREFIX, that was brought back in the course of this work. At the momend it will only be used, to avoid name collisions. In hindsight this irritated me, I was expecting it to be considered independent of it (now only realizing that there is no way for an empty prefix).

I am not sure what to do about it. For, leaving it as it is results in an unpredictable behaviour of group ids, and groups may end up in an inconsistent naming manner: some with, some without prefix.

Keeping old group ids however could actually be important to keep shares in place, or also permissions. On existing setups the "old" group does not go away, but it theoretically might be that a group wlll be split into ORIGINAL and SAML_ORIGINAL, where only the latter might be populated with new SAML users. In those cases, a doubled group display name would cause more confusion with the admin.

Having the prefix always present makes it more predictable, and while transition may need action of the admin, re-deployments would be reproducible.

blizzz · 2022-10-11T10:27:09Z

@Deltachaos it's been a while, but maybe you still remember: GroupDuplicateChecker is not wired anywhere, where was it intended to be used? And the only purpose was really to only issue a warning in the log?

blizzz · 2022-11-04T19:18:12Z

Added a section on Group Display Names to the opener ^.

blizzz · 2022-11-14T14:12:47Z

Group IDs

We have a group prefix SAMLSettings::DEFAULT_GROUP_PREFIX, that was brought back in the course of this work. At the momend it will only be used, to avoid name collisions. In hindsight this irritated me, I was expecting it to be considered independent of it (now only realizing that there is no way for an empty prefix).

I am not sure what to do about it. For, leaving it as it is results in an unpredictable behaviour of group ids, and groups may end up in an inconsistent naming manner: some with, some without prefix.

Keeping old group ids however could actually be important to keep shares in place, or also permissions. On existing setups the "old" group does not go away, but it theoretically might be that a group wlll be split into ORIGINAL and SAML_ORIGINAL, where only the latter might be populated with new SAML users. In those cases, a doubled group display name would cause more confusion with the admin.

Having the prefix always present makes it more predictable, and while transition may need action of the admin, re-deployments would be reproducible.

blizzz · 2022-12-04T21:22:45Z

on dealing with membership changes in local groups, I'd go for:

flowchart TD
    A[Should remove local group membership?] --> B{Is it about the admin group?};
    B -- Yes --> C[Keep membership!];
    B -- No --> D{Are we still in migration phase?};
    D -- No --> C;
    D -- Yes --> E{Is the group still in allow list?};
    E -- No --> C;
    E -- Yes --> F{Do other members stem<br/>from mixed user backends?};
    F -- Yes --> C;
    F -- No --> G[Remove membership!];

Currently, these are not being considered, while it might be important to revoke membership.

...to separate SAML groups from system/other groups Includes - use of DB migrations and repair steps - specific exceptions - owned database group table - firing of group related events - migration logic of already existing groups on local backend where applicable - group id collission detection and handling - map original gid by saml and avoid collissions - append SAML prefix to gid - create new SAML group on collision - cleanup logic: delete SAML groups without members - Ensure admin cannot unassign SAML groups/members - Append SAML_ prefix to groups - Settings - show default group prefix - order group related settings next to each other - allow local group modifications in some cases - must not be about admin group - must be about local group backend - must be in allow migration list - must not have users from mixed backends Signed-off-by: Arthur Schiwon <[email protected]> Signed-off-by: Giuliano Mele <[email protected]> Signed-off-by: Jonathan Treffler <[email protected]> Signed-off-by: Julius Härtl <[email protected]> Signed-off-by: Maximilian Ruta <[email protected]> Co-authored-by: Carl Schwan <[email protected]> Co-authored-by: Giuliano Mele <[email protected]> Co-authored-by: Jonathan Treffler <[email protected]>

blizzz · 2023-12-06T19:56:00Z

fwiw, pushed some tooling to speed up running and debugging integration tests locally https://github.com/blizzz/user_saml_integration

juliusknorr · 2023-12-07T12:12:23Z

fwiw, pushed some tooling to speed up running and debugging integration tests locally blizzz/user_saml_integration

Oh that is awesome 😍

Signed-off-by: Arthur Schiwon <[email protected]>

it is failing much to often with container init Signed-off-by: Arthur Schiwon <[email protected]>

previous to this change groups would be creating in our own backend without the prefix. This is not a very predictable behaviour, and to be more consistent and with less surprises, we include the prefix now unconditionally. Signed-off-by: Arthur Schiwon <[email protected]>

Signed-off-by: Arthur Schiwon <[email protected]>

blizzz · 2023-12-11T10:31:29Z

bumped the version to 6.1, though since this is a bigger change with deeper impact, bumping the major would feel better. However, as we might backport it, and could not bump previous major ones, i went to bump the minor version bit only.

juliusknorr

Minor nitpicks, but otherwise 👍

juliusknorr · 2023-12-18T08:34:28Z

appinfo/app.php

+
+$samlSettings = \OC::$server->get(SAMLSettings::class);
+
+\OC::$server->registerService(GroupManager::class, function (ContainerInterface $c) use ($groupBackend, $samlSettings) {


Do we actually need this service registration? Isn't it all autowired these days (especially with all dependencies just coming from the DI containers?

I believe to remember something about the groupBackend, but actually it seems to be safe to drop this. I'll check this.

juliusknorr · 2023-12-18T08:36:25Z

lib/Migration/Version6000Date20220912152700.php

+				'length' => 64,
+				'default' => '',
+			]);
+			$table->setPrimaryKey(['gid', 'uid'], 'idx_group_members');


Suggested change

$table->setPrimaryKey(['gid', 'uid'], 'idx_group_members');

$table->setPrimaryKey(['gid', 'uid'], 'idx_saml_group_members');

To avoid too easily conflicting names on postgres

juliusknorr · 2023-12-18T08:36:54Z

lib/Migration/Version6000Date20220912152700.php

+				'default' => '',
+			]);
+			$table->setPrimaryKey(['gid', 'uid'], 'idx_group_members');
+			$table->addIndex(['gid']);


Shall we also name those indices?

blizzz · 2023-12-20T12:01:55Z

@juliushaertl resolved all your smaller findings. Additionally I realized that IGetDisplayname was not implemented, so while the group displayname was set, it was actually not used. Added this as well.

Signed-off-by: Arthur Schiwon <[email protected]>

Deltachaos · 2024-01-24T14:42:00Z

I am glad to see that this topic is now solved and some of my initial work was usefull for it

blizzz added enhancement 2. developing labels Oct 10, 2022

blizzz force-pushed the groupbackend-upup branch 8 times, most recently from c347300 to 640372d Compare October 11, 2022 10:10

blizzz force-pushed the groupbackend-upup branch from 4e5c9aa to db380b4 Compare October 11, 2022 10:57

blizzz force-pushed the groupbackend-upup branch 4 times, most recently from 3d3df70 to 9556523 Compare November 4, 2022 18:48

blizzz force-pushed the groupbackend-upup branch 3 times, most recently from 026f99a to 1055402 Compare November 7, 2022 10:03

This was referenced Nov 7, 2022

feat(user_saml_shibboleth): Provide group information nextcloud/docker-ci#464

Merged

Implementation of Groupbackend #545

Closed

blizzz force-pushed the groupbackend-upup branch from 3e2d177 to edc1c47 Compare November 23, 2022 11:24

blizzz force-pushed the groupbackend-upup branch from 0d72089 to 33271fb Compare December 7, 2022 21:54

blizzz force-pushed the groupbackend-upup branch 2 times, most recently from dee0936 to 16a67a1 Compare January 18, 2023 12:23

kevinmccurdybrd mentioned this pull request Mar 31, 2023

Grant Admin Rights to custom SAML group #561

Open

AndyScherzinger assigned juliusknorr Jul 12, 2023

blizzz force-pushed the groupbackend-upup branch from 5468ab0 to a633310 Compare November 20, 2023 16:27

blizzz force-pushed the groupbackend-upup branch 6 times, most recently from 8c5fa9b to 2c95cdb Compare December 6, 2023 13:37

blizzz force-pushed the groupbackend-upup branch from 8e2e97d to 2adda46 Compare December 6, 2023 19:19

blizzz added 5 commits December 8, 2023 18:08

ci(actions): set debug log level for user_saml

d8d67e6

Signed-off-by: Arthur Schiwon <[email protected]>

ci(oci): run on GH runners

5e192d1

it is failing much to often with container init Signed-off-by: Arthur Schiwon <[email protected]>

ci(deps): use stable27 of nc/ocp

f72cb45

Signed-off-by: Arthur Schiwon <[email protected]>

test(integration): ensure group migration works

8be15ac

Signed-off-by: Arthur Schiwon <[email protected]>

blizzz force-pushed the groupbackend-upup branch from 56d21ff to 8be15ac Compare December 8, 2023 17:08

blizzz added 3. to review and removed 2. developing labels Dec 8, 2023

meta(appinfo): bump version and add hint to changelog

48d9506

Signed-off-by: Arthur Schiwon <[email protected]>

juliusknorr approved these changes Dec 18, 2023

View reviewed changes

blizzz added 3 commits December 20, 2023 13:03

refactor(app): remove unnecessary instantiations and registrations

ab39d2e

Signed-off-by: Arthur Schiwon <[email protected]>

feat(groups): enable group support display names

a5bbde3

Signed-off-by: Arthur Schiwon <[email protected]>

refactor(db): set and improve key/index name

84ec0aa

Signed-off-by: Arthur Schiwon <[email protected]>

blizzz force-pushed the groupbackend-upup branch from 6e05d4c to 84ec0aa Compare December 20, 2023 12:04

blizzz merged commit 4dbbd8e into master Dec 20, 2023
25 checks passed

blizzz deleted the groupbackend-upup branch December 20, 2023 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Groupbackend II #662

Implementation of Groupbackend II #662

blizzz commented Oct 10, 2022 •

edited

Loading

blizzz commented Oct 11, 2022

blizzz commented Nov 4, 2022

blizzz commented Nov 14, 2022

Group IDs

blizzz commented Dec 4, 2022

blizzz commented Dec 6, 2023

juliusknorr commented Dec 7, 2023

blizzz commented Dec 11, 2023

juliusknorr left a comment

juliusknorr Dec 18, 2023

blizzz Dec 20, 2023

juliusknorr Dec 18, 2023

juliusknorr Dec 18, 2023

blizzz commented Dec 20, 2023

Deltachaos commented Jan 24, 2024


		$samlSettings = \OC::$server->get(SAMLSettings::class);

		\OC::$server->registerService(GroupManager::class, function (ContainerInterface $c) use ($groupBackend, $samlSettings) {

	$table->setPrimaryKey(['gid', 'uid'], 'idx_group_members');
	$table->setPrimaryKey(['gid', 'uid'], 'idx_saml_group_members');

Implementation of Groupbackend II #662

Implementation of Groupbackend II #662

Conversation

blizzz commented Oct 10, 2022 • edited Loading

Group display names

Group IDs

blizzz commented Oct 11, 2022

blizzz commented Nov 4, 2022

blizzz commented Nov 14, 2022

Group IDs

blizzz commented Dec 4, 2022

blizzz commented Dec 6, 2023

juliusknorr commented Dec 7, 2023

blizzz commented Dec 11, 2023

juliusknorr left a comment

Choose a reason for hiding this comment

juliusknorr Dec 18, 2023

Choose a reason for hiding this comment

blizzz Dec 20, 2023

Choose a reason for hiding this comment

juliusknorr Dec 18, 2023

Choose a reason for hiding this comment

juliusknorr Dec 18, 2023

Choose a reason for hiding this comment

blizzz commented Dec 20, 2023

Deltachaos commented Jan 24, 2024

blizzz commented Oct 10, 2022 •

edited

Loading