[pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[Dominik Csapak]

this deprecates the 'full' sync option and replaces it with
a 'mode' option, where we add a third one that updates
the current users (while retaining their custom set attributes not
exisiting in the source) and removing users that don't exist anymore
in the source

sorry for the long time between versions, i was distracted by
various different things...

one "weird" thing that happens is when having a cluster and not all
nodes are on the newest version if someone adds this option to the realm
config. then everytime when the config is parsed on the older nodes,
a warning is printed into the journal

though this is the same for all new options in the domains.cfg, so i
don't really see a way around this (besides allowing
additionalProperties, but this would also first work on the next
update)

changes from v1:
* replace the 'remove-vanished' by a new 'mode' selection and adding
  an appropriate mode

pve-access-control:

Dominik Csapak (2):
  realm-sync: replace 'full' option with 'mode'
  fix #3668: realm-sync: add mode 'sync'

 src/PVE/API2/Domains.pm | 59 ++++++++++++++++++++++++++++++++++-------
 src/PVE/Auth/Plugin.pm  | 20 +++++++++++---
 2 files changed, 66 insertions(+), 13 deletions(-)

pve-manager:

Dominik Csapak (1):
  ui: realm sync: replace 'full' with 'mode'

 www/manager6/dc/AuthEditLDAP.js | 11 ++++++-----
 www/manager6/dc/SyncWindow.js   |  9 +++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH access-control v2 1/2] realm-sync: replace 'full' option with 'mode'

[Dominik Csapak]

to be able to add more modes in the future.
full=0 maps to mode=update and full=1 to mode=full

but keep 'full' for backwards-compatibiltiy. On create/update, replace
full=1 with mode=full, on read, return both.

add a deprecation notice to the description of full, and a todo to
remove 'full' with 8.0

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/API2/Domains.pm | 57 ++++++++++++++++++++++++++++++++++-------
 src/PVE/Auth/Plugin.pm  | 19 +++++++++++---
 2 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 56e8394..2f351ad 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -17,6 +17,25 @@ my $domainconfigfile = "domains.cfg";
 
 use base qw(PVE::RESTHandler);
 
+# maps old 'full' parameter to new 'mode'
+# TODO remove when we delete the 'full' parameter
+my $map_sync_mode = sub {
+    my ($cfg, $delete_full) = @_;
+
+    my $opt = $cfg->{'sync-defaults-options'};
+    return if !defined($opt);
+    my $sync_opts_fmt = PVE::JSONSchema::get_format('realm-sync-options');
+
+    my $new_opt = PVE::JSONSchema::parse_property_string($sync_opts_fmt, $opt);
+
+    $new_opt->{mode} = $new_opt->{mode} // ($new_opt->{full} ? 'full' : 'update');
+    delete $new_opt->{full} if $delete_full;
+
+    $cfg->{'sync-defaults-options'} = PVE::JSONSchema::print_property_string($new_opt, $sync_opts_fmt);
+
+    return;
+};
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
@@ -109,6 +128,10 @@ __PACKAGE__->register_method ({
 		die "unable to create builtin type '$type'\n"
 		    if ($type eq 'pam' || $type eq 'pve');
 
+		if ($type eq 'ad' || $type eq 'ldap') {
+		    $map_sync_mode->($param, 1);
+		}
+
 		my $plugin = PVE::Auth::Plugin->lookup($type);
 		my $config = $plugin->check_config($realm, $param, 1, 1);
 
@@ -173,7 +196,12 @@ __PACKAGE__->register_method ({
 		    $delete_pw = 1 if $opt eq 'password';
 		}
 
-		my $plugin = PVE::Auth::Plugin->lookup($ids->{$realm}->{type});
+		my $type = $ids->{$realm}->{type};
+		if ($type eq 'ad' || $type eq 'ldap') {
+		    $map_sync_mode->($param, 1);
+		}
+
+		my $plugin = PVE::Auth::Plugin->lookup($type);
 		my $config = $plugin->check_config($realm, $param, 0, 1);
 
 		if ($config->{default}) {
@@ -225,6 +253,11 @@ __PACKAGE__->register_method ({
 	my $data = $cfg->{ids}->{$realm};
 	die "domain '$realm' does not exist\n" if !$data;
 
+	my $type = $data->{type};
+	if ($type eq 'ad' || $type eq 'ldap') {
+	    $map_sync_mode->($data);
+	}
+
 	$data->{digest} = $cfg->{digest};
 
 	return $data;
@@ -274,13 +307,14 @@ __PACKAGE__->register_method ({
 my $update_users = sub {
     my ($usercfg, $realm, $synced_users, $opts) = @_;
 
-    print "syncing users\n";
+    print "syncing users (mode: $opts->{mode})\n";
+
     $usercfg->{users} = {} if !defined($usercfg->{users});
     my $users = $usercfg->{users};
 
     my $oldusers = {};
-    if ($opts->{'full'}) {
-	print "full sync, deleting outdated existing users first\n";
+    if ($opts->{mode} eq 'full') {
+	print "deleting outdated existing users first\n";
 	foreach my $userid (sort keys %$users) {
 	    next if $userid !~ m/\@$realm$/;
 
@@ -310,7 +344,7 @@ my $update_users = sub {
 		$user->{tokens} = $olduser->{tokens};
 	    }
 	    if (defined($oldusers->{$userid})) {
-		print "updated user '$userid'\n";
+		print "overwrote user '$userid'\n";
 	    } else {
 		print "added user '$userid'\n";
 	    }
@@ -327,13 +361,14 @@ my $update_users = sub {
 my $update_groups = sub {
     my ($usercfg, $realm, $synced_groups, $opts) = @_;
 
-    print "syncing groups\n";
+    print "syncing groups (mode: $opts->{mode})\n";
+
     $usercfg->{groups} = {} if !defined($usercfg->{groups});
     my $groups = $usercfg->{groups};
     my $oldgroups = {};
 
-    if ($opts->{full}) {
-	print "full sync, deleting outdated existing groups first\n";
+    if ($opts->{mode} eq 'full') {
+	print "deleting outdated existing groups first\n";
 	foreach my $groupid (sort keys %$groups) {
 	    next if $groupid !~ m/\-$realm$/;
 
@@ -382,10 +417,14 @@ my $parse_sync_opts = sub {
 
 	$res->{$opt} = $param->{$opt} // $cfg_defaults->{$opt} // $fmt->{default};
 
+	# for now, the default of 'mode' depends on 'full''
 	raise_param_exc({
 	    "$opt" => 'Not passed as parameter and not defined in realm default sync options.'
-	}) if !defined($res->{$opt});
+	}) if !defined($res->{$opt}) && $opt ne 'mode';
     }
+
+    $map_sync_mode->($res, 1);
+
     return $res;
 };
 
diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 1413053..8a60062 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -56,11 +56,22 @@ my $realm_sync_options_desc = {
 	enum => [qw(users groups both)],
 	optional => '1',
     },
+    mode => {
+	description => "Update (Default): Only updates/adds fields/users returned by the server. "
+	    ."Full: Removes any field/user that was not returned and overwrites all "
+	    ."existing users with information from the server. "
+	    ."If set, this parameter supersedes the parameter 'full'.",
+	type => 'string',
+	enum => [qw(update full)],
+	optional => '1',
+    },
+    # TODO check/rewrite in pve7to8, and remove with 8.0
     full => {
-	description => "If set, uses the LDAP Directory as source of truth,"
-	    ." deleting users or groups not returned from the sync. Otherwise"
-	    ." only syncs information which is not already present, and does not"
-	    ." deletes or modifies anything else.",
+	description => "DEPRECATED: use 'mode' instead. If set, uses the LDAP Directory as source of truth,"
+	    ." deleting users or groups not returned from the sync and removing"
+	    ." all locally modified properties of synced users. If not set,"
+	    ." only syncs information which is present in the synced data, and does not"
+	    ." delete or modify anything else.",
 	type => 'boolean',
 	optional => '1',
     },
-- 
2.30.2

[pve-devel] [PATCH access-control v2 2/2] fix #3668: realm-sync: add mode 'sync'

[Dominik Csapak]

this mode behaves like the 'update' mode (so it updates users with
new data from the server, and adds new users), but also deletes
users and groups that do not exist anymore on the sync source.

this way, an admin can add custom data (e.g. keys) to the users in pve while
keeping only the users available at the source without having
to manage those attributes there

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/API2/Domains.pm | 6 ++++--
 src/PVE/Auth/Plugin.pm  | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 2f351ad..e1280c0 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -313,10 +313,12 @@ my $update_users = sub {
     my $users = $usercfg->{users};
 
     my $oldusers = {};
-    if ($opts->{mode} eq 'full') {
+    if ($opts->{mode} eq 'full' || $opts->{mode} eq 'sync') {
 	print "deleting outdated existing users first\n";
 	foreach my $userid (sort keys %$users) {
 	    next if $userid !~ m/\@$realm$/;
+	    # keep users (and their fields) in 'sync' mode
+	    next if $opts->{mode} eq 'sync' && defined($synced_users->{$userid});
 
 	    $oldusers->{$userid} = delete $users->{$userid};
 	    if ($opts->{'purge'} && !$synced_users->{$userid}) {
@@ -367,7 +369,7 @@ my $update_groups = sub {
     my $groups = $usercfg->{groups};
     my $oldgroups = {};
 
-    if ($opts->{mode} eq 'full') {
+    if ($opts->{mode} eq 'full' || $opts->{mode} eq 'sync') {
 	print "deleting outdated existing groups first\n";
 	foreach my $groupid (sort keys %$groups) {
 	    next if $groupid !~ m/\-$realm$/;
diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 8a60062..24c1865 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -58,11 +58,12 @@ my $realm_sync_options_desc = {
     },
     mode => {
 	description => "Update (Default): Only updates/adds fields/users returned by the server. "
+	    ."Sync: Updates/adds fields/users from the server and deletes vanished users. "
 	    ."Full: Removes any field/user that was not returned and overwrites all "
 	    ."existing users with information from the server. "
 	    ."If set, this parameter supersedes the parameter 'full'.",
 	type => 'string',
-	enum => [qw(update full)],
+	enum => [qw(update sync full)],
 	optional => '1',
     },
     # TODO check/rewrite in pve7to8, and remove with 8.0
-- 
2.30.2

[pve-devel] [PATCH manager v2 1/1] ui: realm sync: replace 'full' with 'mode'

[Dominik Csapak]

in default sync options and the sync window. since we get the
mapped mode from the backend on read, a 'full=1' there will map
to 'mode=full' and we can simply use that

using this on a node with an old version will not work though

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/dc/AuthEditLDAP.js | 11 ++++++-----
 www/manager6/dc/SyncWindow.js   |  9 +++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/www/manager6/dc/AuthEditLDAP.js b/www/manager6/dc/AuthEditLDAP.js
index 015a5a6e..bb50e21b 100644
--- a/www/manager6/dc/AuthEditLDAP.js
+++ b/www/manager6/dc/AuthEditLDAP.js
@@ -100,7 +100,7 @@ Ext.define('PVE.panel.LDAPSyncInputPanel', {
     xtype: 'pveAuthLDAPSyncPanel',
 
     editableAttributes: ['email'],
-    editableDefaults: ['scope', 'full', 'enable-new', 'purge'],
+    editableDefaults: ['scope', 'mode', 'enable-new', 'purge'],
     default_opts: {},
     sync_attributes: {},
 
@@ -210,11 +210,12 @@ Ext.define('PVE.panel.LDAPSyncInputPanel', {
 	    deleteEmpty: false,
 	    comboItems: [
 		['__default__', Proxmox.Utils.NoneText],
-		['1', Proxmox.Utils.yesText],
-		['0', Proxmox.Utils.noText],
+		['update', gettext('Update')],
+		['sync', gettext('Sync')],
+		['full', gettext('Full')],
 	    ],
-	    name: 'full',
-	    fieldLabel: gettext('Full'),
+	    name: 'mode',
+	    fieldLabel: gettext('Mode'),
 	},
     ],
 
diff --git a/www/manager6/dc/SyncWindow.js b/www/manager6/dc/SyncWindow.js
index 25a42182..2eeaf572 100644
--- a/www/manager6/dc/SyncWindow.js
+++ b/www/manager6/dc/SyncWindow.js
@@ -95,11 +95,12 @@ Ext.define('PVE.dc.SyncWindow', {
 			deleteEmpty: false,
 			allowBlank: false,
 			comboItems: [
-			    ['1', Proxmox.Utils.yesText],
-			    ['0', Proxmox.Utils.noText],
+			    ['update', gettext('Update')],
+			    ['sync', gettext('Sync')],
+			    ['full', gettext('Full')],
 			],
-			name: 'full',
-			fieldLabel: gettext('Full'),
+			name: 'mode',
+			fieldLabel: gettext('Mode'),
 		    },
 		],
 
-- 
2.30.2

Re: [pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[Thomas Lamprecht]

On 04.02.22 15:24, Dominik Csapak wrote:
> this deprecates the 'full' sync option and replaces it with
> a 'mode' option, where we add a third one that updates
> the current users (while retaining their custom set attributes not
> exisiting in the source) and removing users that don't exist anymore
> in the source
> 

I'm not yet 100% sure about the specific mode names, as sync normally means
100% sync, I'll see if I find some other tool (rsync?) with similar option naming
problems. Independent from the specific names, this really needs a docs patch,
ideally with a table listing the modi as rows and having the various "user added",
"user removed", "properties added/updated", "properties removed" as columns, for a
better understanding of the effects..

> sorry for the long time between versions, i was distracted by
> various different things...
> 
> one "weird" thing that happens is when having a cluster and not all
> nodes are on the newest version if someone adds this option to the realm
> config. then everytime when the config is parsed on the older nodes,
> a warning is printed into the journal

you could work around this by getting the node versions from the pmxcfs node
kv store, currently only the manager version but we can do a bump with versioned
dependency there too, hopefully with a manager that has the ldap sync job (ui)
that I request since years shipped too ;-P

Not that we need to go that mechanism, we already tell everyone that a cluster
needs to be the same level of versions to work 100% correctly anyway.

> though this is the same for all new options in the domains.cfg, so i
> don't really see a way around this (besides allowing
> additionalProperties, but this would also first work on the next
> update)
> 
> changes from v1:
> * replace the 'remove-vanished' by a new 'mode' selection and adding
>   an appropriate mode
> 
> pve-access-control:
> 
> Dominik Csapak (2):
>   realm-sync: replace 'full' option with 'mode'
>   fix #3668: realm-sync: add mode 'sync'
> 
>  src/PVE/API2/Domains.pm | 59 ++++++++++++++++++++++++++++++++++-------
>  src/PVE/Auth/Plugin.pm  | 20 +++++++++++---
>  2 files changed, 66 insertions(+), 13 deletions(-)
> 
> pve-manager:
> 
> Dominik Csapak (1):
>   ui: realm sync: replace 'full' with 'mode'
> 
>  www/manager6/dc/AuthEditLDAP.js | 11 ++++++-----
>  www/manager6/dc/SyncWindow.js   |  9 +++++----
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 

Re: [pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[Thomas Lamprecht]

On 22.03.22 07:11, Thomas Lamprecht wrote:
> On 04.02.22 15:24, Dominik Csapak wrote:
>> this deprecates the 'full' sync option and replaces it with
>> a 'mode' option, where we add a third one that updates
>> the current users (while retaining their custom set attributes not
>> exisiting in the source) and removing users that don't exist anymore
>> in the source
>>
> I'm not yet 100% sure about the specific mode names, as sync normally means
> 100% sync, I'll see if I find some other tool (rsync?) with similar option naming
> problems. Independent from the specific names, this really needs a docs patch,
> ideally with a table listing the modi as rows and having the various "user added",
> "user removed", "properties added/updated", "properties removed" as columns, for a
> better understanding of the effects..
> 
A thought (train): what we decide with this isn't what gets added/updated, that's
always the same, only what gets removed if vanished on the source, so maybe:

remove-vanished: < none | user | user-and-properties >

Or if we can actually also remove either user *or* group then: s/user/entity/ ?

ps. the web interface should probably do a s/Purge/Purge ACLs/ too; or with that
in mind we could actually drop that do and have:

remove-vanished: < none | user | user-and-properties | user-and-properties-and-acl >

And with that, we could go the separate semicolon-endcoded-flag-list like we do for
some CT features (or mount options) IIRC:

remove-vanished: [<user>];[<properties>];[acls]

I.e., those three flags would replace your new mode + purge like:

+--------+--------+---------------------+
|  Mode  | Purge  | -> removed-vanished |
+--------+--------+---------------------+
| update |      0 | "" (none)           |
| sync   |      0 | user                |
| full   |      0 | user;properties     |
| update |      1 | acl                 |
| sync   |      1 | acl;user            |
| full   |      1 | acl;user;properties |
+--------+--------+---------------------+

The selector for them could be either three check boxes on one line (similar to the
privilege level radio buttons from CT restore) or even a full blown combobox with all
the options spelled out.

It's only slightly weird for acl, as there the "remove-vanished" somewhat implies that
we import acl's in the first place, if we really don't want that we could keep
"Purge ACLs" as separate option that is only enabled if "remove-vanished" "user" flag
is set, put IMO not _that_ of a big problem to understand compared to the status quo.

Does (any of) this make sense to you?

Re: [pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[Dominik Csapak]

On 3/22/22 14:44, Thomas Lamprecht wrote:
> On 22.03.22 07:11, Thomas Lamprecht wrote:
>> On 04.02.22 15:24, Dominik Csapak wrote:
>>> this deprecates the 'full' sync option and replaces it with
>>> a 'mode' option, where we add a third one that updates
>>> the current users (while retaining their custom set attributes not
>>> exisiting in the source) and removing users that don't exist anymore
>>> in the source
>>>
>> I'm not yet 100% sure about the specific mode names, as sync normally means
>> 100% sync, I'll see if I find some other tool (rsync?) with similar option naming
>> problems. Independent from the specific names, this really needs a docs patch,
>> ideally with a table listing the modi as rows and having the various "user added",
>> "user removed", "properties added/updated", "properties removed" as columns, for a
>> better understanding of the effects..
>>
> A thought (train): what we decide with this isn't what gets added/updated, that's
> always the same, only what gets removed if vanished on the source, so maybe:
> 
> remove-vanished: < none | user | user-and-properties >
> 
> Or if we can actually also remove either user *or* group then: s/user/entity/ ?
> 
> ps. the web interface should probably do a s/Purge/Purge ACLs/ too; or with that
> in mind we could actually drop that do and have:
> 
> remove-vanished: < none | user | user-and-properties | user-and-properties-and-acl >
> 
> And with that, we could go the separate semicolon-endcoded-flag-list like we do for
> some CT features (or mount options) IIRC:
> 
> remove-vanished: [<user>];[<properties>];[acls]
> 
> I.e., those three flags would replace your new mode + purge like:
> 
> +--------+--------+---------------------+
> |  Mode  | Purge  | -> removed-vanished |
> +--------+--------+---------------------+
> | update |      0 | "" (none)           |
> | sync   |      0 | user                |
> | full   |      0 | user;properties     |
> | update |      1 | acl                 |
> | sync   |      1 | acl;user            |
> | full   |      1 | acl;user;properties |
> +--------+--------+---------------------+
> 
> The selector for them could be either three check boxes on one line (similar to the
> privilege level radio buttons from CT restore) or even a full blown combobox with all
> the options spelled out.
> 
> It's only slightly weird for acl, as there the "remove-vanished" somewhat implies that
> we import acl's in the first place, if we really don't want that we could keep
> "Purge ACLs" as separate option that is only enabled if "remove-vanished" "user" flag
> is set, put IMO not _that_ of a big problem to understand compared to the status quo.
> 
> Does (any of) this make sense to you?

yes this sounds sensible, but i agree about the possibly confusing 'remove-vanished'
implication for acls. Maybe 'remove-on-vanish' ?
this would (semantically) decouple the 'vanished' thing from the 'removed' thing,
at least a little bit.
in either case the docs would have to be updated anyway (as you already said)

aside from that, i think line 4 in your table is not really practical,
since it would remove the acls but leave the users ?

we could enable the 'acl' checkbox only when the 'users' checkbox is active,
similar to what you suggested

Re: [pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[Thomas Lamprecht]

On 22.03.22 16:23, Dominik Csapak wrote:
> On 3/22/22 14:44, Thomas Lamprecht wrote:
>> On 22.03.22 07:11, Thomas Lamprecht wrote:
>>> On 04.02.22 15:24, Dominik Csapak wrote:
>>>> this deprecates the 'full' sync option and replaces it with
>>>> a 'mode' option, where we add a third one that updates
>>>> the current users (while retaining their custom set attributes not
>>>> exisiting in the source) and removing users that don't exist anymore
>>>> in the source
>>>>
>>> I'm not yet 100% sure about the specific mode names, as sync normally means
>>> 100% sync, I'll see if I find some other tool (rsync?) with similar option naming
>>> problems. Independent from the specific names, this really needs a docs patch,
>>> ideally with a table listing the modi as rows and having the various "user added",
>>> "user removed", "properties added/updated", "properties removed" as columns, for a
>>> better understanding of the effects..
>>>
>> A thought (train): what we decide with this isn't what gets added/updated, that's
>> always the same, only what gets removed if vanished on the source, so maybe:
>>
>> remove-vanished: < none | user | user-and-properties >
>>
>> Or if we can actually also remove either user *or* group then: s/user/entity/ ?
>>
>> ps. the web interface should probably do a s/Purge/Purge ACLs/ too; or with that
>> in mind we could actually drop that do and have:
>>
>> remove-vanished: < none | user | user-and-properties | user-and-properties-and-acl >
>>
>> And with that, we could go the separate semicolon-endcoded-flag-list like we do for
>> some CT features (or mount options) IIRC:
>>
>> remove-vanished: [<user>];[<properties>];[acls]
>>
>> I.e., those three flags would replace your new mode + purge like:
>>
>> +--------+--------+---------------------+
>> |  Mode  | Purge  | -> removed-vanished |
>> +--------+--------+---------------------+
>> | update |      0 | "" (none)           |
>> | sync   |      0 | user                |
>> | full   |      0 | user;properties     |
>> | update |      1 | acl                 |
>> | sync   |      1 | acl;user            |
>> | full   |      1 | acl;user;properties |
>> +--------+--------+---------------------+
>>
>> The selector for them could be either three check boxes on one line (similar to the
>> privilege level radio buttons from CT restore) or even a full blown combobox with all
>> the options spelled out.
>>
>> It's only slightly weird for acl, as there the "remove-vanished" somewhat implies that
>> we import acl's in the first place, if we really don't want that we could keep
>> "Purge ACLs" as separate option that is only enabled if "remove-vanished" "user" flag
>> is set, put IMO not _that_ of a big problem to understand compared to the status quo.
>>
>> Does (any of) this make sense to you?
> 
> yes this sounds sensible, but i agree about the possibly confusing 'remove-vanished'
> implication for acls. Maybe 'remove-on-vanish' ?

sounds the same to me semantically, so see no improvement there.

> this would (semantically) decouple the 'vanished' thing from the 'removed' thing,
> at least a little bit.

IMO purely subjective and if a real grammar/semantic connection would be there that
I just miss (always a possibility) it'd be to subtle.

I think that the confusion potential overall would get quite a bit reduced that getting
this slightly confusing one newly is still a net benefit and can be easily defused with
a short docs note.

> in either case the docs would have to be updated anyway (as you already said)
> 
> aside from that, i think line 4 in your table is not really practical,
> since it would remove the acls but leave the users ?

The user cannot do anything anymore (like auto-disable) but you still have a reference
to it and all its configured fields + TFA, either  to re-enable it later or to check
contact info if one would investigate a specific task, so IMO its still practical for
setups that want to auto-disable but not auto-remove.

Re: [pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[Thomas Lamprecht]

On 23.03.22 08:33, Thomas Lamprecht wrote:
>>> remove-vanished: [<user>];[<properties>];[acls]
>>>
>>> I.e., those three flags would replace your new mode + purge like:
>>>
>>> +--------+--------+---------------------+
>>> |  Mode  | Purge  | -> removed-vanished |
>>> +--------+--------+---------------------+
>>> | update |      0 | "" (none)           |
>>> | sync   |      0 | user                |
>>> | full   |      0 | user;properties     |
>>> | update |      1 | acl                 |
>>> | sync   |      1 | acl;user            |
>>> | full   |      1 | acl;user;properties |
>>> +--------+--------+---------------------+
>>>
>>> The selector for them could be either three check boxes on one line (similar to the
>>> privilege level radio buttons from CT restore) or even a full blown combobox with all
>>> the options spelled out.
>>>
>>> It's only slightly weird for acl, as there the "remove-vanished" somewhat implies that
>>> we import acl's in the first place, if we really don't want that we could keep
>>> "Purge ACLs" as separate option that is only enabled if "remove-vanished" "user" flag
>>> is set, put IMO not _that_ of a big problem to understand compared to the status quo.
>>>
>>> Does (any of) this make sense to you?
>> yes this sounds sensible, but i agree about the possibly confusing 'remove-vanished'
>> implication for acls. Maybe 'remove-on-vanish' ?
> sounds the same to me semantically, so see no improvement there.
> 
>> this would (semantically) decouple the 'vanished' thing from the 'removed' thing,
>> at least a little bit.
> IMO purely subjective and if a real grammar/semantic connection would be there that
> I just miss (always a possibility) it'd be to subtle.
> 
> I think that the confusion potential overall would get quite a bit reduced that getting
> this slightly confusing one newly is still a net benefit and can be easily defused with
> a short docs note.
> 


FWIW, for a user interface we can also go for a more detailed, telling approach, e.g.,
with both, field and box labels:

Remove Vanished:
Users      [ ] Remove any realm-user not included in the sync response.
ACLs       [ ] Remove the ACLs of any realm-user not included in the sync response.
Properties [ ] Remove properties not included in the sync response.

(more concise sentences welcome ;-))

For properties we could add a " Note: breaks, among other things, TFA." hint as
tooltip or just in the docs.