From 51783c1cbb3dfa8ee95f885946b27363fc81be8b Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Tue, 9 Nov 2021 21:16:59 +0100 Subject: [PATCH] sorces/ldap: fix user/group sync overwriting attributes instead of merging them Signed-off-by: Jens Langhammer --- authentik/sources/ldap/sync/base.py | 23 +++++++++++++++++++++++ authentik/sources/ldap/sync/groups.py | 9 +++++---- authentik/sources/ldap/sync/users.py | 7 ++----- authentik/sources/ldap/tests/test_sync.py | 16 ++++++++++++++++ 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/authentik/sources/ldap/sync/base.py b/authentik/sources/ldap/sync/base.py index 9c196595e..e8b9cd36e 100644 --- a/authentik/sources/ldap/sync/base.py +++ b/authentik/sources/ldap/sync/base.py @@ -1,6 +1,8 @@ """Sync LDAP Users and groups into authentik""" from typing import Any +from deepmerge import always_merger +from django.db.models.base import Model from django.db.models.query import QuerySet from structlog.stdlib import BoundLogger, get_logger @@ -105,3 +107,24 @@ class BaseLDAPSynchronizer: ) properties["attributes"][LDAP_DISTINGUISHED_NAME] = object_dn return properties + + def update_or_create_attributes( + self, + obj: type[Model], + query: dict[str, Any], + data: dict[str, Any], + ) -> tuple[Model, bool]: + """Same as django's update_or_create but correctly update attributes by merging dicts""" + instance = obj.objects.filter(**query).first() + if not instance: + return (obj.objects.create(**data), True) + for key, value in data.items(): + if key == "attributes": + continue + setattr(instance, key, value) + final_atttributes = {} + always_merger.merge(final_atttributes, instance.attributes) + always_merger.merge(final_atttributes, data.get("attributes", {})) + instance.attributes = final_atttributes + instance.save() + return (instance, False) diff --git a/authentik/sources/ldap/sync/groups.py b/authentik/sources/ldap/sync/groups.py index 29e54fb31..318038a14 100644 --- a/authentik/sources/ldap/sync/groups.py +++ b/authentik/sources/ldap/sync/groups.py @@ -43,12 +43,13 @@ class GroupLDAPSynchronizer(BaseLDAPSynchronizer): # Special check for `users` field, as this is an M2M relation, and cannot be sync'd if "users" in defaults: del defaults["users"] - ak_group, created = Group.objects.update_or_create( - **{ + ak_group, created = self.update_or_create_attributes( + Group, + { f"attributes__{LDAP_UNIQUENESS}": uniq, "parent": self._source.sync_parent_group, - "defaults": defaults, - } + }, + defaults, ) except (IntegrityError, FieldError) as exc: Event.new( diff --git a/authentik/sources/ldap/sync/users.py b/authentik/sources/ldap/sync/users.py index 8a5ce734f..00a1574b7 100644 --- a/authentik/sources/ldap/sync/users.py +++ b/authentik/sources/ldap/sync/users.py @@ -42,11 +42,8 @@ class UserLDAPSynchronizer(BaseLDAPSynchronizer): self._logger.debug("Creating user with attributes", **defaults) if "username" not in defaults: raise IntegrityError("Username was not set by propertymappings") - ak_user, created = User.objects.update_or_create( - **{ - f"attributes__{LDAP_UNIQUENESS}": uniq, - "defaults": defaults, - } + ak_user, created = self.update_or_create_attributes( + User, {f"attributes__{LDAP_UNIQUENESS}": uniq}, defaults ) except (IntegrityError, FieldError) as exc: Event.new( diff --git a/authentik/sources/ldap/tests/test_sync.py b/authentik/sources/ldap/tests/test_sync.py index 02b500d7f..9c8f6928f 100644 --- a/authentik/sources/ldap/tests/test_sync.py +++ b/authentik/sources/ldap/tests/test_sync.py @@ -69,10 +69,26 @@ class LDAPSyncTests(TestCase): ) self.source.save() connection = PropertyMock(return_value=mock_ad_connection(LDAP_PASSWORD)) + + # Create the user beforehand so we can set attributes and check they aren't removed + user = User.objects.create( + username="user0_sn", + attributes={ + "ldap_uniq": ( + "S-117-6648368-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-" + "0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-" + "0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-" + "0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0-0" + ), + "foo": "bar,", + }, + ) + with patch("authentik.sources.ldap.models.LDAPSource.connection", connection): user_sync = UserLDAPSynchronizer(self.source) user_sync.sync() user = User.objects.filter(username="user0_sn").first() + self.assertEqual(user.attributes["foo"], "bar") self.assertFalse(user.is_active) self.assertFalse(User.objects.filter(username="user1_sn").exists())