fix(ldap): prevent DN collisions from assigning another user's privileges

backend/internal/ldapsync/service.go

@@ -779,29 +779,24 @@ func (s *Service) saveProfilePicture(parentCtx context.Context, userId string, p
 	return nil
 }
 
-// normalizeLDAPDN returns a canonical lowercase form of a DN for use as a map key.
-// Different LDAP servers may format the same DN with varying attribute type casing (e.g. "CN=" vs "cn=") or extra whitespace (e.g. "dc=example, dc=com").
-// Without normalization, cache lookups in usernamesByDN would miss when a member attribute value uses a different format than the DN returned in the search entry
-//
-// ldap.ParseDN is used instead of simple lowercasing because it correctly handles multi-valued RDNs (joined with "+") and strips inter-component whitespace.
-// If parsing fails for any reason, we fall back to a simple lowercase+trim.
+// normalizeLDAPDN returns a canonical lowercase form of a DN for use as a map key
+// Escaping preserves the distinction between separators and literal characters inside attribute values
 func normalizeLDAPDN(dn string) string {
+	// Parse components so equivalent formatting shares a cache key
 	parsed, err := ldap.ParseDN(dn)
 	if err != nil {
 		return strings.ToLower(strings.TrimSpace(dn))
 	}
 
-	// Reconstruct the DN in a canonical form: lowercase type=lowercase value, with RDN components separated by "," and multi-value attributes by "+"
-	parts := make([]string, 0, len(parsed.RDNs))
+	// Preserve case-insensitive lookups while letting the LDAP serializer escape values and sort multi-valued RDNs
 	for _, rdn := range parsed.RDNs {
-		attrs := make([]string, 0, len(rdn.Attributes))
 		for _, attr := range rdn.Attributes {
-			attrs = append(attrs, strings.ToLower(attr.Type)+"="+strings.ToLower(attr.Value))
+			attr.Type = strings.ToLower(attr.Type)
+			attr.Value = strings.ToLower(attr.Value)
 		}
-		parts = append(parts, strings.Join(attrs, "+"))
 	}
 
-	return strings.Join(parts, ",")
+	return parsed.String()
 }
 
 // getDNProperty returns the value of a property from a LDAP identifier

backend/internal/ldapsync/service_test.go

@@ -196,6 +196,84 @@ func TestLdapServiceSyncAllMapsPosixGroupMemberUid(t *testing.T) {
 	assert.ElementsMatch(t, []string{"alice", "bob"}, usernames(group.Users))
 }
 
+func TestLdapServiceSyncAllKeepsDistinctDNMemberships(t *testing.T) {
+	tests := []struct {
+		name       string
+		memberDN   string
+		attackerDN string
+	}{
+		{
+			name:       "escaped comma",
+			memberDN:   "cn=admin,ou=staff,dc=example,dc=com",
+			attackerDN: `cn=admin\,ou=staff,dc=example,dc=com`,
+		},
+		{
+			name:       "escaped plus",
+			memberDN:   "cn=admin+ou=staff,dc=example,dc=com",
+			attackerDN: `cn=admin\+ou=staff,dc=example,dc=com`,
+		},
+	}
+
+	for _, tt := range tests {
+		for _, attackerFirst := range []bool{false, true} {
+			order := "/attacker last"
+			if attackerFirst {
+				order = "/attacker first"
+			}
+			t.Run(tt.name+order, func(t *testing.T) {
+				// Keep the usernames and immutable IDs distinct so only DN resolution can confuse the users
+				entries := []*ldap.Entry{
+					ldapEntry(tt.memberDN, map[string][]string{
+						"entryUUID": {"u-admin"},
+						"uid":       {"admin"},
+						"givenName": {"Admin"},
+						"sn":        {"User"},
+					}),
+					ldapEntry(tt.attackerDN, map[string][]string{
+						"entryUUID": {"u-attacker"},
+						"uid":       {"attacker"},
+						"givenName": {"Attacker"},
+						"sn":        {"User"},
+					}),
+				}
+				if attackerFirst {
+					entries[0], entries[1] = entries[1], entries[0]
+				}
+
+				// Exercise the persisted admin flag and ordinary group memberships through the same sync
+				service, db := newTestLdapService(t, newFakeLDAPClient(
+					ldapSearchResult(entries...),
+					ldapSearchResult(
+						ldapEntry("cn=admins,ou=groups,dc=example,dc=com", map[string][]string{
+							"entryUUID": {"g-admins"},
+							"cn":        {"admins"},
+							"member":    {tt.memberDN},
+						}),
+						ldapEntry("cn=restricted,ou=groups,dc=example,dc=com", map[string][]string{
+							"entryUUID": {"g-restricted"},
+							"cn":        {"restricted"},
+							"member":    {tt.memberDN},
+						}),
+					),
+				))
+				require.NoError(t, service.SyncAll(t.Context(), defaultTestLDAPAppConfig()))
+
+				var admin, attacker model.User
+				require.NoError(t, db.First(&admin, "ldap_id = ?", "u-admin").Error)
+				require.NoError(t, db.First(&attacker, "ldap_id = ?", "u-attacker").Error)
+				assert.True(t, admin.IsAdmin)
+				assert.False(t, attacker.IsAdmin)
+
+				for _, groupID := range []string{"g-admins", "g-restricted"} {
+					var group model.UserGroup
+					require.NoError(t, db.Preload("Users").First(&group, "ldap_id = ?", groupID).Error)
+					assert.Equal(t, []string{"admin"}, usernames(group.Users))
+				}
+			})
+		}
+	}
+}
+
 func TestLdapServiceSyncAllHandlesDuplicateLDAPIDsInSingleRun(t *testing.T) {
 	service, db := newTestLdapService(t, newFakeLDAPClient(
 		ldapSearchResult(
@@ -547,6 +625,26 @@ func TestNormalizeLDAPDN(t *testing.T) {
 			input:    "cn=alice+uid=a123,dc=example,dc=com",
 			expected: "cn=alice+uid=a123,dc=example,dc=com",
 		},
+		{
+			name:     "reordered multi-valued RDN",
+			input:    "UID=A123+CN=Alice,dc=example,dc=com",
+			expected: "cn=alice+uid=a123,dc=example,dc=com",
+		},
+		{
+			name:     "escaped comma remains part of the value",
+			input:    `CN=Admin\,OU=Staff,DC=example,DC=com`,
+			expected: `cn=admin\,ou=staff,dc=example,dc=com`,
+		},
+		{
+			name:     "hex-escaped comma shares the same key",
+			input:    `cn=admin\2cou=staff,dc=example,dc=com`,
+			expected: `cn=admin\,ou=staff,dc=example,dc=com`,
+		},
+		{
+			name:     "escaped plus remains part of the value",
+			input:    `cn=admin\+ou=staff,dc=example,dc=com`,
+			expected: `cn=admin\+ou=staff,dc=example,dc=com`,
+		},
 		{
 			name:     "invalid DN falls back to lowercase+trim",
 			input:    "  NOT A VALID DN  ",