fix #3668: realm-sync: replace 'full' & 'purge' with 'remove-vanished'

Both, 'full' & 'purge', configure what is removed when an entry
(or property) is not returned anymore from the sync result. For users
it may be thus easier to understand setting a single property without
wondering about the interaction of the two specific ones.

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

The old 'full' gets mapped to 'entry;properties' and an old 'purge'
to the 'acl'

Keep the old properties for backwards-compatibility but transform to
the new list on edit. Add a deprecation notice to the description
with a TODO reminder to remove with 8.0

Note, this commit changes two things slightly:
* 'purge' (now 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 explicitly on sync or the sync-default. Previously 'full' and
  'purge' must have been configured explicitly (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

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
[ T: some commit message typos/wording ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 56e8394e0165787998081a2b76eeb2494a4b5ac9..870e2446a0daa0c1ad11df3c28010bc92d4d8f19 100644 (file)
--- 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);
 
 
 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 => '',
 __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');
 
                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);
 
                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';
                }
 
                    $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}) {
                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 $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;
        $data->{digest} = $cfg->{digest};
 
        return $data;


@@ -275,51 +323,50 @@ my $update_users = sub {
     my ($usercfg, $realm, $synced_users, $opts) = @_;
 
     print "syncing users\n";
     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};
     $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} // {};
        }
     }
 
     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 $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 {
        } else {

-           my $olduser = $users->{$userid};

            foreach my $attr (keys %$synced_user) {
                $olduser->{$attr} = $synced_user->{$attr};
            }
            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";
     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};
     $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};
        }
     }
 
     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 {
            } else {

-               print "added group '$groupid'\n";
+               print "adding group '$groupid'\n";

            }
            }

+           $groups->{$groupid} = $synced_group;

        } else {
        } else {

-           my $group = $groups->{$groupid};

            foreach my $attr (keys %$synced_group) {
            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};
        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;
 };
 
     return $res;
 };
 

diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 141305336c8640dd40879eb6e084b95c51ad0fa7..7e431f3d7095038563f0e4907aed5b93e00ebd1e 100755 (executable)
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -49,6 +49,8 @@ PVE::JSONSchema::register_standard_option('realm', {
     maxLength => 32,
 });
 
     maxLength => 32,
 });
 

+my $remove_options = "(?:acl|properties|entry)";
+

 my $realm_sync_options_desc = {
     scope => {
        description => "Select what to sync.",
 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',
     },
        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 => {
     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',
     },
        type => 'boolean',
        optional => '1',
     },


@@ -71,8 +86,8 @@ my $realm_sync_options_desc = {
        optional => '1',
     },
     purge => {
        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',
     },
        type => 'boolean',
        optional => '1',
     },

diff --git a/src/test/realm_sync_test.pl b/src/test/realm_sync_test.pl
index 2a4a1327dd3aae07200993e96abc6a6a566d35e3..78089fa410f82a4c7ee3b9384de9a8350b8b1848 100755 (executable)
--- a/src/test/realm_sync_test.pl
+++ b/src/test/realm_sync_test.pl
@@ -276,9 +276,7 @@ my $tests = [
            },
            acl => {
                '/' => {
            },
            acl => {
                '/' => {

-                   users => {
-                       'user3@syncedrealm' => {},
-                   },
+                   users => {},

                    groups => {},
                },
            },
                    groups => {},
                },
            },