From 433f6bdf5747824f25c882ea765f9490633c71f4 Mon Sep 17 00:00:00 2001
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Date: Thu, 8 Feb 2024 12:10:31 +0100
Subject: [PATCH] LDAP sync: build valid-target-attribute list on the fly to
 avoid coupling

Build the set of valid target attributes on the fly by using the
existing ldap => ours mapping. This avoids that one needs to adapt
both lists when changing this, which even though it should be caught
on testing, is needlessly adding friction.

The is-known-target-attr check could never trigger as this was already
checked in the parent before even calling the verify method, so just
remove it.

Rename the `verify_sync_attribute` to `verify_sync_attribute_value` to
clarify that it really only checks the value of an attribute, not the
attribute (key) itself.

As a side-benefit, this also makes the code shorter and avoids a
permanent global variable using up (a tiny amount of) space.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
(cherry picked from commit 7abb20a1ead8b84dcf0620c11c72462d315f92ac)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/Auth/LDAP.pm | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index f31c655..809a523 100755
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -172,22 +172,9 @@ sub options {
     };
 }
 
-my $valid_sync_attributes = {
-    username => 1,
-    enable => 1,
-    expire => 1,
-    firstname => 1,
-    lastname => 1,
-    email => 1,
-    comment => 1,
-    keys => 1,
-};
-
-my sub verify_sync_attribute {
+my sub verify_sync_attribute_value {
     my ($attr, $value) = @_;
 
-    die "cannot map to invalid user sync attribute '$attr'\n" if !$valid_sync_attributes->{$attr};
-
     # The attribute does not include the realm, so can't use PVE::Auth::Plugin::verify_username
     if ($attr eq 'username') {
 	die "value '$value' does not look like a valid user name\n"
@@ -303,11 +290,13 @@ sub get_users {
 	comment => 'comment',
 	keys => 'keys',
     };
+    # build on the fly as this is small and only called once per realm in a ldap-sync anyway
+    my $valid_sync_attributes = map { $_ => 1 } values $ldap_attribute_map->%*;
 
     foreach my $attr (PVE::Tools::split_list($config->{sync_attributes})) {
 	my ($ours, $ldap) = ($attr =~ m/^\s*(\w+)=(.*)\s*$/);
 	if (!$valid_sync_attributes->{$ours}) {
-	    warn "bad 'sync_attributes': cannot map to invalid attribute '$ours'\n";
+	    warn "skipping bad 'sync_attributes' entry – '$ours' is not a valid target attribute\n";
 	    next;
 	}
 	$ldap_attribute_map->{$ldap} = $ours;
@@ -341,7 +330,7 @@ sub get_users {
 	foreach my $attr (keys %$user_attributes) {
 	    if (my $ours = $ldap_attribute_map->{$attr}) {
 		my $value = $user_attributes->{$attr}->[0];
-		eval { verify_sync_attribute($ours, $value) };
+		eval { verify_sync_attribute_value($ours, $value) };
 		if (my $err = $@) {
 		    warn "skipping attribute mapping '$attr'->'$ours' for user '$username' - $err";
 		    next;
-- 
2.39.2