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

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

[Dominik Csapak]

this deprecates the 'full' and 'purge' sync options and replaces them with
a 'remove-vanished' option, where we have multiple flags to determine
which things we want to remove when they are not in the sync response.

with the new regression tests, we can see that the sync result stays the
same with one exception of deleting the acls even when we did not delete
the user

changes from v3:
* added regression tests (i found some bugs with those ;) )
* fixed the mapping of parameters and not only the 'defaul-sync-options'
* fixed use of 'remove_vanished' instead of 'remove-vanished'

changes from v2:
* instead of having a mode, define what we actually do: configure what
  we remove when it (or the depending entry) vanishes
* let the user remove the ACLs only, even when not removing the users
* have less fields that the user *must* give on sync, since there are
  more defaults that are explained in the gui

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

pve-access-control:

Dominik Csapak (4):
  add regression tests for realm-sync
  fix #3668: realm-sync: replace 'full' and 'purge' options with
    'remove-vanished'
  convert regression tests to new 'remove-vanished' parameter
  add realm-sync regression test for new 'remove-vanished'

 src/PVE/API2/Domains.pm     | 168 ++++++++++------
 src/PVE/Auth/Plugin.pm      |  27 ++-
 src/test/Makefile           |   1 +
 src/test/realm_sync_test.pl | 371 ++++++++++++++++++++++++++++++++++++
 4 files changed, 504 insertions(+), 63 deletions(-)
 create mode 100755 src/test/realm_sync_test.pl

pve-manager:

Dominik Csapak (1):
  ui: realm sync: replace 'full' and 'purge' with 'remove-vanished'

 www/manager6/dc/AuthEditLDAP.js | 63 +++++++++++++++++++------------
 www/manager6/dc/SyncWindow.js   | 66 ++++++++++++++++++++-------------
 2 files changed, 80 insertions(+), 49 deletions(-)

pve-docs:

Dominik Csapak (1):
  update documentation about sync-options

 pveum.adoc | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH access-control v4 1/4] add regression tests for realm-sync

[Dominik Csapak]

to fully test the 'end-to-end' sync api call, we have to mock quite
some methods for cluster/rpcenvironment/ldap

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/test/Makefile           |   1 +
 src/test/realm_sync_test.pl | 338 ++++++++++++++++++++++++++++++++++++
 2 files changed, 339 insertions(+)
 create mode 100755 src/test/realm_sync_test.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index adaacb9..859a84b 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -12,3 +12,4 @@ check:
 	perl -I.. perm-test6.pl
 	perl -I.. perm-test7.pl
 	perl -I.. perm-test8.pl
+	perl -I.. realm_sync_test.pl
diff --git a/src/test/realm_sync_test.pl b/src/test/realm_sync_test.pl
new file mode 100755
index 0000000..2a4a132
--- /dev/null
+++ b/src/test/realm_sync_test.pl
@@ -0,0 +1,338 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::MockModule;
+use Test::More;
+use Storable qw(dclone);
+
+use PVE::AccessControl;
+use PVE::API2::Domains;
+
+my $domainscfg = {
+    ids => {
+	"pam" => { type => 'pam' },
+	"pve" => { type => 'pve' },
+	"syncedrealm" => { type => 'ldap' }
+    },
+};
+
+my $initialusercfg = {
+    users => {
+	'root@pam' => { username => 'root', },
+	'user1@syncedrealm' => {
+	    username => 'user1',
+	    enable => 1,
+	    'keys' => 'some',
+	},
+	'user2@syncedrealm' => {
+	    username => 'user2',
+	    enable => 1,
+	},
+	'user3@syncedrealm' => {
+	    username => 'user3',
+	    enable => 1,
+	},
+    },
+    groups => {
+	'group1-syncedrealm' => { users => {}, },
+	'group2-syncedrealm' => { users => {}, },
+    },
+    acl => {
+	'/' => {
+	    users => {
+		'user3@syncedrealm' => {},
+	    },
+	    groups => {},
+	},
+    },
+};
+
+my $sync_response = {
+    user => [
+	{
+	    attributes => { 'uid' => ['user1'], },
+	    dn => 'uid=user1,dc=syncedrealm',
+	},
+	{
+	    attributes => { 'uid' => ['user2'], },
+	    dn => 'uid=user2,dc=syncedrealm',
+	},
+	{
+	    attributes => { 'uid' => ['user4'], },
+	    dn => 'uid=user4,dc=syncedrealm',
+	},
+    ],
+    groups => [
+	{
+	    dn => 'dc=group1,dc=syncedrealm',
+	    members => [
+		'uid=user1,dc=syncedrealm',
+	    ],
+	},
+	{
+	    dn => 'dc=group3,dc=syncedrealm',
+	    members => [
+		'uid=nonexisting,dc=syncedrealm',
+	    ],
+	}
+    ],
+};
+
+my $returned_user_cfg = {};
+
+# mocking all cluster and ldap operations
+my $pve_cluster_module = Test::MockModule->new('PVE::Cluster');
+$pve_cluster_module->mock(
+    cfs_update => sub {},
+    cfs_read_file => sub {
+	my ($filename) = @_;
+	if ($filename eq 'domains.cfg') { return dclone($domainscfg); }
+	if ($filename eq 'user.cfg') { return dclone($initialusercfg); }
+	die "unexpected cfs_read_file";
+    },
+    cfs_write_file => sub {
+	my ($filename, $data) = @_;
+	if ($filename eq 'user.cfg') {
+	    $returned_user_cfg = $data;
+	    return;
+	}
+	die "unexpected cfs_read_file";
+    },
+    cfs_lock_file => sub {
+	my ($filename, $timeout, $code) = @_;
+	return $code->();
+    },
+);
+
+my $pve_api_domains = Test::MockModule->new('PVE::API2::Domains');
+$pve_api_domains->mock(
+    cfs_read_file => sub { PVE::Cluster::cfs_read_file(@_); },
+    cfs_write_file => sub { PVE::Cluster::cfs_write_file(@_); },
+);
+
+my $pve_accesscontrol = Test::MockModule->new('PVE::AccessControl');
+$pve_accesscontrol->mock(
+    cfs_lock_file => sub { PVE::Cluster::cfs_lock_file(@_); },
+);
+
+my $pve_rpcenvironment = Test::MockModule->new('PVE::RPCEnvironment');
+$pve_rpcenvironment->mock(
+    get => sub { return bless {}, 'PVE::RPCEnvironment'; },
+    get_user => sub { return 'root@pam'; },
+    fork_worker => sub {
+	my ($class, $workertype, $id, $user, $code) = @_;
+
+	return $code->();
+    },
+);
+
+my $pve_ldap_module = Test::MockModule->new('PVE::LDAP');
+$pve_ldap_module->mock(
+    ldap_connect => sub { return {}; },
+    ldap_bind => sub {},
+    query_users => sub {
+	return $sync_response->{user};
+    },
+    query_groups => sub {
+	return $sync_response->{groups};
+    },
+);
+
+my $pve_auth_ldap = Test::MockModule->new('PVE::Auth::LDAP');
+$pve_auth_ldap->mock(
+    connect_and_bind => sub { return {}; },
+);
+
+my $tests = [
+    [
+	"non-full without purge",
+	{
+	    realm => 'syncedrealm',
+	    full => 0,
+	    purge => 0,
+	    scope => 'both',
+	},
+	{
+	    users => {
+		'root@pam' => { username => 'root', },
+		'user1@syncedrealm' => {
+		    username => 'user1',
+		    enable => 1,
+		    'keys' => 'some',
+		},
+		'user2@syncedrealm' => {
+		    username => 'user2',
+		    enable => 1,
+		},
+		'user3@syncedrealm' => {
+		    username => 'user3',
+		    enable => 1,
+		},
+		'user4@syncedrealm' => {
+		    username => 'user4',
+		    enable => 1,
+		},
+	    },
+	    groups => {
+		'group1-syncedrealm' => {
+		    users => {
+			'user1@syncedrealm' => 1,
+		    },
+		},
+		'group2-syncedrealm' => { users => {}, },
+		'group3-syncedrealm' => { users => {}, },
+	    },
+	    acl => {
+		'/' => {
+		    users => {
+			'user3@syncedrealm' => {},
+		    },
+		    groups => {},
+		},
+	    },
+	},
+    ],
+    [
+	"full without purge",
+	{
+	    realm => 'syncedrealm',
+	    full => 1,
+	    purge => 0,
+	    scope => 'both',
+	},
+	{
+	    users => {
+		'root@pam' => { username => 'root', },
+		'user1@syncedrealm' => {
+		    username => 'user1',
+		    enable => 1,
+		},
+		'user2@syncedrealm' => {
+		    username => 'user2',
+		    enable => 1,
+		},
+		'user4@syncedrealm' => {
+		    username => 'user4',
+		    enable => 1,
+		},
+	    },
+	    groups => {
+		'group1-syncedrealm' => {
+		    users => {
+			'user1@syncedrealm' => 1,
+		    },
+		},
+		'group3-syncedrealm' => { users => {}, }
+	    },
+	    acl => {
+		'/' => {
+		    users => {
+			'user3@syncedrealm' => {},
+		    },
+		    groups => {},
+		},
+	    },
+	},
+    ],
+    [
+	"non-full with purge",
+	{
+	    realm => 'syncedrealm',
+	    full => 0,
+	    purge => 1,
+	    scope => 'both',
+	},
+	{
+	    users => {
+		'root@pam' => { username => 'root', },
+		'user1@syncedrealm' => {
+		    username => 'user1',
+		    enable => 1,
+		    'keys' => 'some',
+		},
+		'user2@syncedrealm' => {
+		    username => 'user2',
+		    enable => 1,
+		},
+		'user3@syncedrealm' => {
+		    username => 'user3',
+		    enable => 1,
+		},
+		'user4@syncedrealm' => {
+		    username => 'user4',
+		    enable => 1,
+		},
+	    },
+	    groups => {
+		'group1-syncedrealm' => {
+		    users => {
+			'user1@syncedrealm' => 1,
+		    },
+		},
+		'group2-syncedrealm' => { users => {}, },
+		'group3-syncedrealm' => { users => {}, },
+	    },
+	    acl => {
+		'/' => {
+		    users => {
+			'user3@syncedrealm' => {},
+		    },
+		    groups => {},
+		},
+	    },
+	},
+    ],
+    [
+	"full with purge",
+	{
+	    realm => 'syncedrealm',
+	    full => 1,
+	    purge => 1,
+	    scope => 'both',
+	},
+	{
+	    users => {
+		'root@pam' => { username => 'root', },
+		'user1@syncedrealm' => {
+		    username => 'user1',
+		    enable => 1,
+		},
+		'user2@syncedrealm' => {
+		    username => 'user2',
+		    enable => 1,
+		},
+		'user4@syncedrealm' => {
+		    username => 'user4',
+		    enable => 1,
+		},
+	    },
+	    groups => {
+		'group1-syncedrealm' => {
+		    users => {
+			'user1@syncedrealm' => 1,
+		    },
+		},
+		'group3-syncedrealm' => { users => {}, },
+	    },
+	    acl => {
+		'/' => {
+		    users => {},
+		    groups => {},
+		},
+	    },
+	},
+    ],
+];
+
+for my $test (@$tests) {
+    my $name = $test->[0];
+    my $parameters = $test->[1];
+    my $expected = $test->[2];
+    $returned_user_cfg = {};
+    PVE::API2::Domains->sync($parameters);
+    is_deeply($returned_user_cfg, $expected, $name);
+}
+
+done_testing();
-- 
2.30.2

[pve-devel] [PATCH access-control v4 2/4] fix #3668: realm-sync: replace 'full' and 'purge' options with 'remove-vanished'

[Dominik Csapak]

since both configure what is removed when an entry (or property)
is not returned from the sync result.

'remove-vanished' is a list of things to remove: 'acl', 'entry',
'properties'.

'full' maps to 'entry;properties' and
'purge' to 'acl'

Keep the old properties for backwards-compatibiltiy. On create/update, replace
the old values with the new equivalent in 'remove-vanish'.

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

this changes two things slightly:
* 'purge' (or remove-vanished acl) does now remove ACLs for vanished
  entries even when we keep those entries. Previously those were only
  deleted when we also removed the entries
  (this can be seen by the changed regression test)
* since 'remove-vanished' is empty by default, only the scope must be
  given explicitely on sync or the sync-default. previosly 'full' and
  'purge' must have been configured explicitely
  (no big deal, since the default is to not remove anything)

bug #3668 is fixed when not selecting 'properties' for the sync, so
that existing (custom) properties are not deleted on update

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/API2/Domains.pm     | 168 ++++++++++++++++++++++++------------
 src/PVE/Auth/Plugin.pm      |  27 ++++--
 src/test/realm_sync_test.pl |   4 +-
 3 files changed, 133 insertions(+), 66 deletions(-)

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 56e8394..870e244 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -17,6 +17,40 @@ my $domainconfigfile = "domains.cfg";
 
 use base qw(PVE::RESTHandler);
 
+# maps old 'full'/'purge' parameters to new 'remove-vanished'
+# TODO remove when we delete the 'full'/'purge' parameters
+my $map_remove_vanished = sub {
+    my ($opt, $delete_deprecated) = @_;
+
+    if (!defined($opt->{'remove-vanished'}) && ($opt->{full} || $opt->{purge})) {
+	my $props = [];
+	push @$props, 'entry', 'properties' if $opt->{full};
+	push @$props, 'acl' if $opt->{purge};
+	$opt->{'remove-vanished'} = join(';', @$props);
+    }
+
+    if ($delete_deprecated) {
+	delete $opt->{full};
+	delete $opt->{purge};
+    }
+
+    return $opt;
+};
+
+my $map_sync_default_options = sub {
+    my ($cfg, $delete_deprecated) = @_;
+
+    my $opt = $cfg->{'sync-defaults-options'};
+    return if !defined($opt);
+    my $sync_opts_fmt = PVE::JSONSchema::get_format('realm-sync-options');
+
+    my $old_opt = PVE::JSONSchema::parse_property_string($sync_opts_fmt, $opt);
+
+    my $new_opt = $map_remove_vanished->($old_opt, $delete_deprecated);
+
+    $cfg->{'sync-defaults-options'} = PVE::JSONSchema::print_property_string($new_opt, $sync_opts_fmt);
+};
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
@@ -109,6 +143,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_default_options->($param, 1);
+		}
+
 		my $plugin = PVE::Auth::Plugin->lookup($type);
 		my $config = $plugin->check_config($realm, $param, 1, 1);
 
@@ -173,7 +211,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_default_options->($param, 1);
+		}
+
+		my $plugin = PVE::Auth::Plugin->lookup($type);
 		my $config = $plugin->check_config($realm, $param, 0, 1);
 
 		if ($config->{default}) {
@@ -225,6 +268,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_default_options->($data);
+	}
+
 	$data->{digest} = $cfg->{digest};
 
 	return $data;
@@ -275,51 +323,50 @@ my $update_users = sub {
     my ($usercfg, $realm, $synced_users, $opts) = @_;
 
     print "syncing users\n";
+    print "remove-vanished: $opts->{'remove-vanished'}\n" if defined($opts->{'remove-vanished'});
+
     $usercfg->{users} = {} if !defined($usercfg->{users});
     my $users = $usercfg->{users};
+    my $to_remove = { map { $_ => 1 } split(';', $opts->{'remove-vanished'} // '') };
 
-    my $oldusers = {};
-    if ($opts->{'full'}) {
-	print "full sync, deleting outdated existing users first\n";
-	foreach my $userid (sort keys %$users) {
-	    next if $userid !~ m/\@$realm$/;
-
-	    $oldusers->{$userid} = delete $users->{$userid};
-	    if ($opts->{'purge'} && !$synced_users->{$userid}) {
-		PVE::AccessControl::delete_user_acl($userid, $usercfg);
-		print "purged user '$userid' and all its ACL entries\n";
-	    } elsif (!defined($synced_users->{$userid})) {
-		print "remove user '$userid'\n";
-	    }
+    print "deleting outdated existing users first\n" if $to_remove->{entry};
+    foreach my $userid (sort keys %$users) {
+	next if $userid !~ m/\@$realm$/;
+	next if defined($synced_users->{$userid});
+
+	if ($to_remove->{entry}) {
+	    print "remove user '$userid'\n";
+	    delete $users->{$userid};
+	}
+
+	if ($to_remove->{acl}) {
+	    print "purge users '$userid' ACL entries\n";
+	    PVE::AccessControl::delete_user_acl($userid, $usercfg);
 	}
     }
 
     foreach my $userid (sort keys %$synced_users) {
 	my $synced_user = $synced_users->{$userid} // {};
-	if (!defined($users->{$userid})) {
+	my $olduser = $users->{$userid};
+	if ($to_remove->{properties} || !defined($olduser)) {
+	    # we use the synced user, but want to keep some properties on update
+	    if (defined($olduser)) {
+		print "overwriting user '$userid'\n";
+	    } else {
+		$olduser = {};
+		print "adding user '$userid'\n";
+	    }
 	    my $user = $users->{$userid} = $synced_user;
 
-	    my $olduser = $oldusers->{$userid} // {};
-	    if (defined(my $enabled = $olduser->{enable})) {
-		$user->{enable} = $enabled;
-	    } elsif ($opts->{'enable-new'}) {
-		$user->{enable} = 1;
-	    }
+	    my $enabled = $olduser->{enable} // $opts->{'enable-new'};
+	    $user->{enable} = $enabled if defined($enabled);
+	    $user->{tokens} = $olduser->{tokens} if defined($olduser->{tokens});
 
-	    if (defined($olduser->{tokens})) {
-		$user->{tokens} = $olduser->{tokens};
-	    }
-	    if (defined($oldusers->{$userid})) {
-		print "updated user '$userid'\n";
-	    } else {
-		print "added user '$userid'\n";
-	    }
 	} else {
-	    my $olduser = $users->{$userid};
 	    foreach my $attr (keys %$synced_user) {
 		$olduser->{$attr} = $synced_user->{$attr};
 	    }
-	    print "updated user '$userid'\n";
+	    print "updating user '$userid'\n";
 	}
     }
 };
@@ -328,40 +375,43 @@ my $update_groups = sub {
     my ($usercfg, $realm, $synced_groups, $opts) = @_;
 
     print "syncing groups\n";
+    print "remove-vanished: $opts->{'remove-vanished'}\n" if defined($opts->{'remove-vanished'});
+
     $usercfg->{groups} = {} if !defined($usercfg->{groups});
     my $groups = $usercfg->{groups};
-    my $oldgroups = {};
-
-    if ($opts->{full}) {
-	print "full sync, deleting outdated existing groups first\n";
-	foreach my $groupid (sort keys %$groups) {
-	    next if $groupid !~ m/\-$realm$/;
-
-	    my $oldgroups->{$groupid} = delete $groups->{$groupid};
-	    if ($opts->{purge} && !$synced_groups->{$groupid}) {
-		print "purged group '$groupid' and all its ACL entries\n";
-		PVE::AccessControl::delete_group_acl($groupid, $usercfg)
-	    } elsif (!defined($synced_groups->{$groupid})) {
-		print "removed group '$groupid'\n";
-	    }
+    my $to_remove = { map { $_ => 1 } split(';', $opts->{'remove-vanished'} // '') };
+
+    print "deleting outdated existing groups first\n" if $to_remove->{entry};
+    foreach my $groupid (sort keys %$groups) {
+	next if $groupid !~ m/\-$realm$/;
+	next if defined($synced_groups->{$groupid});
+
+	if ($to_remove->{entry}) {
+	    print "remove group '$groupid'\n";
+	    delete $groups->{$groupid};
+	}
+
+	if ($to_remove->{acl}) {
+	    print "purge groups '$groupid' ACL entries\n";
+	    PVE::AccessControl::delete_group_acl($groupid, $usercfg);
 	}
     }
 
     foreach my $groupid (sort keys %$synced_groups) {
 	my $synced_group = $synced_groups->{$groupid};
-	if (!defined($groups->{$groupid})) {
-	    $groups->{$groupid} = $synced_group;
-	    if (defined($oldgroups->{$groupid})) {
-		print "updated group '$groupid'\n";
+	my $oldgroup = $groups->{$groupid};
+	if ($to_remove->{properties} || !defined($oldgroup)) {
+	    if (defined($oldgroup)) {
+		print "overwriting group '$groupid'\n";
 	    } else {
-		print "added group '$groupid'\n";
+		print "adding group '$groupid'\n";
 	    }
+	    $groups->{$groupid} = $synced_group;
 	} else {
-	    my $group = $groups->{$groupid};
 	    foreach my $attr (keys %$synced_group) {
-		$group->{$attr} = $synced_group->{$attr};
+		$oldgroup->{$attr} = $synced_group->{$attr};
 	    }
-	    print "updated group '$groupid'\n";
+	    print "updating group '$groupid'\n";
 	}
     }
 };
@@ -381,11 +431,15 @@ my $parse_sync_opts = sub {
 	my $fmt = $sync_opts_fmt->{$opt};
 
 	$res->{$opt} = $param->{$opt} // $cfg_defaults->{$opt} // $fmt->{default};
-
-	raise_param_exc({
-	    "$opt" => 'Not passed as parameter and not defined in realm default sync options.'
-	}) if !defined($res->{$opt});
     }
+
+    $map_remove_vanished->($res, 1);
+
+    # only scope has no implicit value
+    raise_param_exc({
+	"scope" => 'Not passed as parameter and not defined in realm default sync options.'
+    }) if !defined($res->{scope});
+
     return $res;
 };
 
diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 1413053..7e431f3 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -49,6 +49,8 @@ PVE::JSONSchema::register_standard_option('realm', {
     maxLength => 32,
 });
 
+my $remove_options = "(?:acl|properties|entry)";
+
 my $realm_sync_options_desc = {
     scope => {
 	description => "Select what to sync.",
@@ -56,11 +58,24 @@ my $realm_sync_options_desc = {
 	enum => [qw(users groups both)],
 	optional => '1',
     },
+    'remove-vanished' => {
+	description => "A semicolon-seperated list of things to remove when they or the user"
+	    ." vanishes during a sync. The following values are possible: 'entry' removes the"
+	    ." user/group when not returned from the sync. 'properties' removes the set"
+	    ." properties on existing user/group that do not appear in the source (even custom ones)."
+	    ." 'acl' removes acls when the user/group is not returned from the sync.",
+	type => 'string',
+	typetext => "[acl];[properties];[entry]",
+	pattern => "(?:$remove_options\;)*$remove_options",
+	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 'remove-vanished' 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',
     },
@@ -71,8 +86,8 @@ my $realm_sync_options_desc = {
 	optional => '1',
     },
     purge => {
-	description => "Remove ACLs for users or groups which were removed from"
-	    ." the config during a sync.",
+	description => "DEPRECATED: use 'remove-vanished' instead. Remove ACLs for users or"
+	    ." groups which were removed from the config during a sync.",
 	type => 'boolean',
 	optional => '1',
     },
diff --git a/src/test/realm_sync_test.pl b/src/test/realm_sync_test.pl
index 2a4a132..78089fa 100755
--- a/src/test/realm_sync_test.pl
+++ b/src/test/realm_sync_test.pl
@@ -276,9 +276,7 @@ my $tests = [
 	    },
 	    acl => {
 		'/' => {
-		    users => {
-			'user3@syncedrealm' => {},
-		    },
+		    users => {},
 		    groups => {},
 		},
 	    },
-- 
2.30.2

[pve-devel] [PATCH access-control v4 3/4] convert regression tests to new 'remove-vanished' parameter

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/test/realm_sync_test.pl | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/test/realm_sync_test.pl b/src/test/realm_sync_test.pl
index 78089fa..304c7ed 100755
--- a/src/test/realm_sync_test.pl
+++ b/src/test/realm_sync_test.pl
@@ -150,8 +150,6 @@ my $tests = [
 	"non-full without purge",
 	{
 	    realm => 'syncedrealm',
-	    full => 0,
-	    purge => 0,
 	    scope => 'both',
 	},
 	{
@@ -198,8 +196,7 @@ my $tests = [
 	"full without purge",
 	{
 	    realm => 'syncedrealm',
-	    full => 1,
-	    purge => 0,
+	    'remove-vanished' => 'entry;properties',
 	    scope => 'both',
 	},
 	{
@@ -240,8 +237,7 @@ my $tests = [
 	"non-full with purge",
 	{
 	    realm => 'syncedrealm',
-	    full => 0,
-	    purge => 1,
+	    'remove-vanished' => 'acl',
 	    scope => 'both',
 	},
 	{
@@ -286,8 +282,7 @@ my $tests = [
 	"full with purge",
 	{
 	    realm => 'syncedrealm',
-	    full => 1,
-	    purge => 1,
+	    'remove-vanished' => 'acl;entry;properties',
 	    scope => 'both',
 	},
 	{
-- 
2.30.2

[pve-devel] [PATCH access-control v4 4/4] add realm-sync regression test for new 'remove-vanished'

[Dominik Csapak]

by having a test case that does not delete properties, but acls and
entries

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/test/realm_sync_test.pl | 40 +++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/src/test/realm_sync_test.pl b/src/test/realm_sync_test.pl
index 304c7ed..ea083f3 100755
--- a/src/test/realm_sync_test.pl
+++ b/src/test/realm_sync_test.pl
@@ -317,6 +317,46 @@ my $tests = [
 	    },
 	},
     ],
+    [
+	"don't delete properties, but users and acls",
+	{
+	    realm => 'syncedrealm',
+	    'remove-vanished' => 'acl;entry',
+	    scope => 'both',
+	},
+	{
+	    users => {
+		'root@pam' => { username => 'root', },
+		'user1@syncedrealm' => {
+		    username => 'user1',
+		    enable => 1,
+		    'keys' => 'some',
+		},
+		'user2@syncedrealm' => {
+		    username => 'user2',
+		    enable => 1,
+		},
+		'user4@syncedrealm' => {
+		    username => 'user4',
+		    enable => 1,
+		},
+	    },
+	    groups => {
+		'group1-syncedrealm' => {
+		    users => {
+			'user1@syncedrealm' => 1,
+		    },
+		},
+		'group3-syncedrealm' => { users => {}, },
+	    },
+	    acl => {
+		'/' => {
+		    users => {},
+		    groups => {},
+		},
+	    },
+	},
+    ],
 ];
 
 for my $test (@$tests) {
-- 
2.30.2

[pve-devel] [PATCH manager v4 1/1] ui: realm sync: replace 'full' and 'purge' with 'remove-vanished'

[Dominik Csapak]

in default sync options and the sync window. We do this by exposing
the individual flags as checkboxes. We get the mapped value from the
backend so we do not have to handle 'old' values here.

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

diff --git a/www/manager6/dc/AuthEditLDAP.js b/www/manager6/dc/AuthEditLDAP.js
index 015a5a6e..50505ae2 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', 'enable-new'],
     default_opts: {},
     sync_attributes: {},
 
@@ -116,6 +116,15 @@ Ext.define('PVE.panel.LDAPSyncInputPanel', {
 		delete me.default_opts[attr];
 	    }
 	});
+	let vanished_opts = [];
+	['acl', 'entry', 'properties'].forEach((prop) => {
+	    if (values[`remove-vanished-${prop}`]) {
+		vanished_opts.push(prop);
+	    }
+	    delete values[`remove-vanished-${prop}`];
+	});
+	me.default_opts['remove-vanished'] = vanished_opts.join(';');
+
 	values['sync-defaults-options'] = PVE.Parser.printPropertyString(me.default_opts);
 	me.editableAttributes.forEach((attr) => {
 	    if (values[attr]) {
@@ -156,6 +165,13 @@ Ext.define('PVE.panel.LDAPSyncInputPanel', {
 		    values[attr] = me.default_opts[attr];
 		}
 	    });
+
+	    if (me.default_opts['remove-vanished']) {
+		let opts = me.default_opts['remove-vanished'].split(';');
+		for (const opt of opts) {
+		    values[`remove-vanished-${opt}`] = 1;
+		}
+	    }
 	}
 	return me.callParent([values]);
     },
@@ -204,18 +220,6 @@ Ext.define('PVE.panel.LDAPSyncInputPanel', {
 		['both', gettext('Users and Groups')],
 	    ],
 	},
-	{
-	    xtype: 'proxmoxKVComboBox',
-	    value: '__default__',
-	    deleteEmpty: false,
-	    comboItems: [
-		['__default__', Proxmox.Utils.NoneText],
-		['1', Proxmox.Utils.yesText],
-		['0', Proxmox.Utils.noText],
-	    ],
-	    name: 'full',
-	    fieldLabel: gettext('Full'),
-	},
     ],
 
     column2: [
@@ -269,17 +273,30 @@ Ext.define('PVE.panel.LDAPSyncInputPanel', {
 	    name: 'enable-new',
 	    fieldLabel: gettext('Enable new users'),
 	},
+    ],
+
+    columnB: [
 	{
-	    xtype: 'proxmoxKVComboBox',
-	    value: '__default__',
-	    deleteEmpty: false,
-	    comboItems: [
-		['__default__', Proxmox.Utils.NoneText],
-		['1', Proxmox.Utils.yesText],
-		['0', Proxmox.Utils.noText],
-	    ],
-	    name: 'purge',
-	    fieldLabel: gettext('Purge'),
+	    xtype: 'displayfield',
+	    fieldLabel: gettext('Remove Vanished'),
+	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('ACL'),
+	    name: 'remove-vanished-acl',
+	    boxLabel: gettext('Remove ACLs of users and groups which are not in the sync response.'),
+	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Entry'),
+	    name: 'remove-vanished-entry',
+	    boxLabel: gettext('Remove users and groups that are not in the sync response.'),
+	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Properties'),
+	    name: 'remove-vanished-properties',
+	    boxLabel: gettext('Remove user-properties that are not in the sync response.'),
 	},
     ],
 });
diff --git a/www/manager6/dc/SyncWindow.js b/www/manager6/dc/SyncWindow.js
index 25a42182..b32926dd 100644
--- a/www/manager6/dc/SyncWindow.js
+++ b/www/manager6/dc/SyncWindow.js
@@ -32,6 +32,18 @@ Ext.define('PVE.dc.SyncWindow', {
 	    let view = me.getView();
 	    let ipanel = me.lookup('ipanel');
 	    let params = ipanel.getValues();
+
+	    let vanished_opts = [];
+	    ['acl', 'entry', 'properties'].forEach((prop) => {
+		if (params[`remove-vanished-${prop}`]) {
+		    vanished_opts.push(prop);
+		}
+		delete params[`remove-vanished-${prop}`];
+	    });
+	    if (vanished_opts.length > 0) {
+		params['remove-vanished'] = vanished_opts.join(';');
+	    }
+
 	    params['dry-run'] = is_preview ? 1 : 0;
 	    Proxmox.Utils.API2Request({
 		url: `/access/domains/${view.realm}/sync`,
@@ -88,19 +100,6 @@ Ext.define('PVE.dc.SyncWindow', {
 			    ['both', gettext('Users and Groups')],
 			],
 		    },
-		    {
-			xtype: 'proxmoxKVComboBox',
-			value: '',
-			emptyText: gettext('No default available'),
-			deleteEmpty: false,
-			allowBlank: false,
-			comboItems: [
-			    ['1', Proxmox.Utils.yesText],
-			    ['0', Proxmox.Utils.noText],
-			],
-			name: 'full',
-			fieldLabel: gettext('Full'),
-		    },
 		],
 
 		column2: [
@@ -116,22 +115,31 @@ Ext.define('PVE.dc.SyncWindow', {
 			name: 'enable-new',
 			fieldLabel: gettext('Enable new'),
 		    },
-		    {
-			xtype: 'proxmoxKVComboBox',
-			value: '',
-			emptyText: gettext('No default available'),
-			deleteEmpty: false,
-			allowBlank: false,
-			comboItems: [
-			    ['1', Proxmox.Utils.yesText],
-			    ['0', Proxmox.Utils.noText],
-			],
-			name: 'purge',
-			fieldLabel: gettext('Purge ACLs'),
-		    },
 		],
 
 		columnB: [
+		    {
+			xtype: 'displayfield',
+			fieldLabel: gettext('Remove Vanished'),
+		    },
+		    {
+			xtype: 'proxmoxcheckbox',
+			fieldLabel: gettext('ACL'),
+			name: 'remove-vanished-acl',
+			boxLabel: gettext('Remove ACLs of users and groups which are not in the sync response.'),
+		    },
+		    {
+			xtype: 'proxmoxcheckbox',
+			fieldLabel: gettext('Entry'),
+			name: 'remove-vanished-entry',
+			boxLabel: gettext('Remove users and groups that are not in the sync response.'),
+		    },
+		    {
+			xtype: 'proxmoxcheckbox',
+			fieldLabel: gettext('Properties'),
+			name: 'remove-vanished-properties',
+			boxLabel: gettext('Remove user-properties that are not in the sync response.'),
+		    },
 		    {
 			xtype: 'displayfield',
 			reference: 'defaulthint',
@@ -183,6 +191,12 @@ Ext.define('PVE.dc.SyncWindow', {
 		let default_options = response.result.data['sync-defaults-options'];
 		if (default_options) {
 		    let options = PVE.Parser.parsePropertyString(default_options);
+		    if (options['remove-vanished']) {
+			let opts = options['remove-vanished'].split(';');
+			for (const opt of opts) {
+			    options[`remove-vanished-${opt}`] = 1;
+			}
+		    }
 		    let ipanel = me.lookup('ipanel');
 		    ipanel.setValues(options);
 		} else {
-- 
2.30.2

[pve-devel] [PATCH docs v4 1/1] update documentation about sync-options

[Dominik Csapak]

describe the new 'remove-vanished' option and what the options are doing

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pveum.adoc | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/pveum.adoc b/pveum.adoc
index a5c8906..99e1a45 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -355,13 +355,21 @@ The main options for syncing are:
 * `Enable new` (`enable-new`): If set, the newly synced users are enabled and
   can log in. The default is `true`.
 
-* `Full` (`full`): If set, the sync uses the LDAP directory as a source of
-  truth, overwriting information set manually in the `user.cfg` and deleting
-  users and groups which are not present in the LDAP directory. If not set, only
-  new data is written to the configuration, and no stale users are deleted.
+* `Remove Vanished` (`remove-vanished`): This is a list of options which, when
+  activated, determine if they are removed when they are not returned from
+  the sync response. The options are:
 
-* `Purge ACLs` (`purge`): If set, sync removes all corresponding ACLs when
-  removing users and groups. This is only useful with the option `full`.
+    - `ACL` (`acl)`: Remove ACLs of users and groups which were not returned
+      returned in the sync response. This most often makes sense together with
+      `Entry`.
+
+    - `Entry` (`entry`): Removes entries (i.e. users and groups) when they are
+      not returned in the sync response.
+
+    - `Properties` (`properties`): Removes properties of entries which were
+      not returned in the sync response. This includes custom properties
+      which were never set by the sync. Exceptions are tokens and the enable
+      flag. Those will be retained even with this option.
 
 * `Preview` (`dry-run`): No data is written to the config. This is useful if you
   want to see which users and groups would get synced to the `user.cfg`.
-- 
2.30.2

[pve-devel] applied-series: [PATCH access-control/manager/docs v4] fix #3668: improving realm sync

[Thomas Lamprecht]

On 28.03.22 14:38, Dominik Csapak wrote:
> this deprecates the 'full' and 'purge' sync options and replaces them with
> a 'remove-vanished' option, where we have multiple flags to determine
> which things we want to remove when they are not in the sync response.
> 
> with the new regression tests, we can see that the sync result stays the
> same with one exception of deleting the acls even when we did not delete
> the user
> 
> changes from v3:
> * added regression tests (i found some bugs with those ;) )
> * fixed the mapping of parameters and not only the 'defaul-sync-options'
> * fixed use of 'remove_vanished' instead of 'remove-vanished'
> 
> changes from v2:
> * instead of having a mode, define what we actually do: configure what
>   we remove when it (or the depending entry) vanishes
> * let the user remove the ACLs only, even when not removing the users
> * have less fields that the user *must* give on sync, since there are
>   more defaults that are explained in the gui
> 
> changes from v1:
> * replace the 'remove-vanished' by a new 'mode' selection and adding
>   an appropriate mode
> 
> pve-access-control:
> 
> Dominik Csapak (4):
>   add regression tests for realm-sync
>   fix #3668: realm-sync: replace 'full' and 'purge' options with
>     'remove-vanished'
>   convert regression tests to new 'remove-vanished' parameter
>   add realm-sync regression test for new 'remove-vanished'
> 
>  src/PVE/API2/Domains.pm     | 168 ++++++++++------
>  src/PVE/Auth/Plugin.pm      |  27 ++-
>  src/test/Makefile           |   1 +
>  src/test/realm_sync_test.pl | 371 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 504 insertions(+), 63 deletions(-)
>  create mode 100755 src/test/realm_sync_test.pl
> 
> pve-manager:
> 
> Dominik Csapak (1):
>   ui: realm sync: replace 'full' and 'purge' with 'remove-vanished'
> 
>  www/manager6/dc/AuthEditLDAP.js | 63 +++++++++++++++++++------------
>  www/manager6/dc/SyncWindow.js   | 66 ++++++++++++++++++++-------------
>  2 files changed, 80 insertions(+), 49 deletions(-)
> 
> pve-docs:
> 
> Dominik Csapak (1):
>   update documentation about sync-options
> 
>  pveum.adoc | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 


applied series with minor GUI ux followups as talked offlist, thanks!