[pve-devel] [PATCH access-control/manager] improve realm sync

[pve-devel] [PATCH access-control/manager] improve realm sync

[Dominik Csapak]

by improving wording in the api description and adding a
'remove-vanished' option

pve-access-control:

Dominik Csapak (2):
  fix #3668: realm sync: add 'remove-vanished' parameter to delete
    non-existing users
  realm sync: improve wording in log and description

 src/PVE/API2/Domains.pm | 14 ++++++++++----
 src/PVE/Auth/Plugin.pm  | 14 +++++++++++---
 2 files changed, 21 insertions(+), 7 deletions(-)

pve-manager:

Dominik Csapak (1):
  ui: realm sync: add 'remove-vanished' option

 www/manager6/dc/AuthEditLDAP.js | 14 +++++++++++++-
 www/manager6/dc/SyncWindow.js   | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

-- 
2.30.2

[pve-devel] [PATCH access-control 1/2] fix #3668: realm sync: add 'remove-vanished' parameter to delete non-existing users

[Dominik Csapak]

this way, users can use the 'non-full' mode to provide local
information (like tfa keys), but still remove the users not present in
the ldap anymore.

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

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 9c2b254..55bb7ae 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -279,10 +279,11 @@ my $update_users = sub {
     my $users = $usercfg->{users};
 
     my $oldusers = {};
-    if ($opts->{'full'}) {
-	print "full sync, deleting outdated existing users first\n";
+    if ($opts->{'full'} || $opts->{'remove-vanished'}) {
+	print "deleting outdated existing users first\n";
 	foreach my $userid (sort keys %$users) {
 	    next if $userid !~ m/\@$realm$/;
+	    next if !$opts->{full} && defined($synced_users->{$userid});
 
 	    $oldusers->{$userid} = delete $users->{$userid};
 	    if ($opts->{'purge'} && !$synced_users->{$userid}) {
diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 1413053..27d6294 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -64,6 +64,13 @@ my $realm_sync_options_desc = {
 	type => 'boolean',
 	optional => '1',
     },
+    'remove-vanished' => {
+	description => "Only for non-full mode. If set, removes users that are not"
+	    ." present in the current synced data.",
+	type => 'boolean',
+	optional => '1',
+	default => '0',
+    },
     'enable-new' => {
 	description => "Enable newly synced users immediately.",
 	type => 'boolean',
-- 
2.30.2

[pve-devel] [PATCH access-control 2/2] realm sync: improve wording in log and description

[Dominik Csapak]

by
* differentiating 'updated' users in full sync ('overwrote')
* adding '(full)' in the log for full syncs
* making it more clear what full sync does and what happens otherwise

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

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 55bb7ae..7aff9cb 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -274,7 +274,12 @@ __PACKAGE__->register_method ({
 my $update_users = sub {
     my ($usercfg, $realm, $synced_users, $opts) = @_;
 
-    print "syncing users\n";
+    if ($opts->{'full'}) {
+	print "syncing users (full)\n";
+    } else {
+	print "syncing users\n";
+    }
+
     $usercfg->{users} = {} if !defined($usercfg->{users});
     my $users = $usercfg->{users};
 
@@ -311,7 +316,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";
 	    }
diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 27d6294..bf763ae 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -58,9 +58,10 @@ my $realm_sync_options_desc = {
     },
     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.",
+	    ." 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 manager 1/1] ui: realm sync: add 'remove-vanished' option

[Dominik Csapak]

in default sync options and the sync window. There we disable it if
'full' sync is enabled to make it clear the combination does not make
sense.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/dc/AuthEditLDAP.js | 14 +++++++++++++-
 www/manager6/dc/SyncWindow.js   | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/www/manager6/dc/AuthEditLDAP.js b/www/manager6/dc/AuthEditLDAP.js
index 015a5a6e..2942ae64 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', 'full', 'enable-new', 'purge', 'remove-vanished'],
     default_opts: {},
     sync_attributes: {},
 
@@ -216,6 +216,18 @@ Ext.define('PVE.panel.LDAPSyncInputPanel', {
 	    name: 'full',
 	    fieldLabel: gettext('Full'),
 	},
+	{
+	    xtype: 'proxmoxKVComboBox',
+	    value: '__default__',
+	    deleteEmpty: false,
+	    comboItems: [
+		['__default__', Proxmox.Utils.NoneText],
+		['1', Proxmox.Utils.yesText],
+		['0', Proxmox.Utils.noText],
+	    ],
+	    name: 'remove-vanished',
+	    fieldLabel: gettext('Remove vanished'),
+	},
     ],
 
     column2: [
diff --git a/www/manager6/dc/SyncWindow.js b/www/manager6/dc/SyncWindow.js
index 25a42182..c988a4bf 100644
--- a/www/manager6/dc/SyncWindow.js
+++ b/www/manager6/dc/SyncWindow.js
@@ -12,6 +12,11 @@ Ext.define('PVE.dc.SyncWindow', {
 	xclass: 'Ext.app.ViewController',
 
 	control: {
+	    'field[name=full]': {
+		change: function(field, value) {
+		    this.lookup('remove-vanished').setDisabled(value === '1');
+		},
+	    },
 	    'form': {
 		validitychange: function(field, valid) {
 		    let me = this;
@@ -101,6 +106,20 @@ Ext.define('PVE.dc.SyncWindow', {
 			name: 'full',
 			fieldLabel: gettext('Full'),
 		    },
+		    {
+			xtype: 'proxmoxKVComboBox',
+			value: '',
+			emptyText: gettext('No default available'),
+			deleteEmpty: false,
+			allowBlank: false,
+			comboItems: [
+			    ['1', Proxmox.Utils.yesText],
+			    ['0', Proxmox.Utils.noText],
+			],
+			name: 'remove-vanished',
+			reference: 'remove-vanished',
+			fieldLabel: gettext('Remove vanished'),
+		    },
 		],
 
 		column2: [
-- 
2.30.2

Re: [pve-devel] [PATCH access-control 1/2] fix #3668: realm sync: add 'remove-vanished' parameter to delete non-existing users

[Thomas Lamprecht]

On 27.10.21 14:57, Dominik Csapak wrote:
> this way, users can use the 'non-full' mode to provide local
> information (like tfa keys), but still remove the users not present in
> the ldap anymore.

ps. missing an patch for pve-docs, in any case...
https://d7.work.tlmp.it:8006/pve-docs/chapter-pveum.html#pveum_ldap_sync

The amount of sync related options is a bit confusing/crowded already now,
so did you thought about just expanding the value set of the `full` property
for that (possibly renaming it to "sync mode" or the like)?

Albeit purge and this new one could be chosen independently, that wouldn't make
the list of choises easier..
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/API2/Domains.pm | 5 +++--
>  src/PVE/Auth/Plugin.pm  | 7 +++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
> index 9c2b254..55bb7ae 100644
> --- a/src/PVE/API2/Domains.pm
> +++ b/src/PVE/API2/Domains.pm
> @@ -279,10 +279,11 @@ my $update_users = sub {
>      my $users = $usercfg->{users};
>  
>      my $oldusers = {};
> -    if ($opts->{'full'}) {
> -	print "full sync, deleting outdated existing users first\n";
> +    if ($opts->{'full'} || $opts->{'remove-vanished'}) {
> +	print "deleting outdated existing users first\n";
>  	foreach my $userid (sort keys %$users) {
>  	    next if $userid !~ m/\@$realm$/;
> +	    next if !$opts->{full} && defined($synced_users->{$userid});
>  
>  	    $oldusers->{$userid} = delete $users->{$userid};
>  	    if ($opts->{'purge'} && !$synced_users->{$userid}) {
> diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
> index 1413053..27d6294 100755
> --- a/src/PVE/Auth/Plugin.pm
> +++ b/src/PVE/Auth/Plugin.pm
> @@ -64,6 +64,13 @@ my $realm_sync_options_desc = {
>  	type => 'boolean',
>  	optional => '1',
>      },
> +    'remove-vanished' => {
> +	description => "Only for non-full mode. If set, removes users that are not"
> +	    ." present in the current synced data.",
> +	type => 'boolean',
> +	optional => '1',
> +	default => '0',
> +    },
>      'enable-new' => {
>  	description => "Enable newly synced users immediately.",
>  	type => 'boolean',
>