graph-ldap: Fix possible races when editing group membership in parallel

As the standard LDAP groups (groupOfNames) require at least one "member"
value to be present in a group, we have workarounds in place that add an
empty member ("") when creating a new group or when removing the last
member from the group. This can cause a race condition when e.g. multiple
request to remove members from a group an running in parallel, as we need
to read the group before we can construct the modification request. If
some other request modified the group (e.g. deleted the 2nd last member)
after we read it, we create non-working modification request.

These changes try to catch those errors and retry the modification
request once.

Fixes: owncloud#6170

services/graph/pkg/identity/ldap.go

@@ -238,12 +238,10 @@ func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) error {
 		for _, group := range groupEntries {
 			logger.Debug().Str("group", group.DN).Str("user", e.DN).Msg("Cleaning up group membership")
 
-			if mr, err := i.removeEntryByDNAndAttributeFromEntry(group, e.DN, i.groupAttributeMap.member); err == nil {
-				if err = i.conn.Modify(mr); err != nil {
-					// Errors when deleting the memberships are only logged as warnings but not returned
-					// to the user as we already successfully deleted the users itself
-					logger.Warn().Str("group", group.DN).Str("user", e.DN).Err(err).Msg("failed to remove member")
-				}
+			if err := i.removeEntryByDNAndAttributeFromEntry(group, e.DN, i.groupAttributeMap.member); err != nil {
+				// Errors when deleting the memberships are only logged as warnings but not returned
+				// to the user as we already successfully deleted the users itself
+				logger.Warn().Str("group", group.DN).Str("user", e.DN).Err(err).Msg("failed to remove member")
 			}
 		}
 	}
@@ -878,40 +876,62 @@ func stringToScope(scope string) (int, error) {
 }
 
 // removeEntryByDNAndAttributeFromEntry creates a request to remove a single member entry by attribute and DN from an ldap entry
-func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string, attribute string) (*ldap.ModifyRequest, error) {
+func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string, attribute string) error {
 	nOldDN, err := ldapdn.ParseNormalize(dn)
 	if err != nil {
-		return nil, err
+		return err
 	}
-	entries := entry.GetEqualFoldAttributeValues(attribute)
+
+	currentValues := entry.GetEqualFoldAttributeValues(attribute)
+	i.logger.Error().Interface("members", currentValues).Msg("current values")
 	found := false
-	for _, entry := range entries {
-		if entry == "" {
+	for _, currentValue := range currentValues {
+		if currentValue == "" {
 			continue
 		}
-		if nEntry, err := ldapdn.ParseNormalize(entry); err != nil {
+		if normalizedCurrentValue, err := ldapdn.ParseNormalize(currentValue); err != nil {
 			// We couldn't parse the entry value as a DN. Let's keep it
 			// as it is but log a warning
-			i.logger.Warn().Str("entryDN", entry).Err(err).Msg("Couldn't parse DN")
+			i.logger.Warn().Str("member", currentValue).Err(err).Msg("Couldn't parse DN")
 			continue
 		} else {
-			if nEntry == nOldDN {
+			if normalizedCurrentValue == nOldDN {
 				found = true
 			}
 		}
 	}
 	if !found {
-		i.logger.Debug().Str("backend", "ldap").Str("entry", entry.DN).Str("target", dn).
-			Msg("The target is not an entry in the attribute list")
-		return nil, ErrNotFound
+		i.logger.Error().Str("backend", "ldap").Str("entry", entry.DN).Str("target", dn).
+			Msg("The target value is not present in the attribute list")
+		return ErrNotFound
 	}
 
-	mr := ldap.ModifyRequest{DN: entry.DN}
-	if len(entries) == 1 {
+	mr := &ldap.ModifyRequest{DN: entry.DN}
+	if len(currentValues) == 1 {
 		mr.Add(attribute, []string{""})
 	}
 	mr.Delete(attribute, []string{dn})
-	return &mr, nil
+
+	err = i.conn.Modify(mr)
+	var lerr *ldap.Error
+	if err != nil && errors.As(err, &lerr) {
+		if lerr.ResultCode == ldap.LDAPResultObjectClassViolation {
+			// objectclass "groupOfName" requires at least one member to be present, some other go-routine
+			// must have removed the 2nd last member from the group after we read the group. We adapt the
+			// modification request to replace the last member with an empty member and re-try.
+			i.logger.Debug().Err(err).
+				Msg("Failed to remove last group member. Retrying once. Replacing last group member with an empty member value.")
+			mr.Add(attribute, []string{""})
+			err = i.conn.Modify(mr)
+		}
+	}
+
+	if err != nil {
+		i.logger.Error().Err(err).Str("entry", entry.DN).Str("attribute", attribute).Str("target value", dn).
+			Msg("Failed to remove dn attribute from entry")
+	}
+
+	return err
 }
 
 // expandLDAPAttributeEntries reads an attribute from an ldap entry and expands to users

services/graph/pkg/identity/ldap_education_class.go

@@ -460,9 +460,5 @@ func (i *LDAP) RemoveTeacherFromEducationClass(ctx context.Context, classID stri
 		return err
 	}
 
-	if mr, err := i.removeEntryByDNAndAttributeFromEntry(class, teacher.DN, i.educationConfig.classAttributeMap.teachers); err == nil {
-		return i.conn.Modify(mr)
-	}
-
-	return nil
+	return i.removeEntryByDNAndAttributeFromEntry(class, teacher.DN, i.educationConfig.classAttributeMap.teachers)
 }

services/graph/pkg/identity/ldap_group.go

@@ -325,18 +325,37 @@ func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs
 	}
 
 	if len(newMemberDN) > 0 {
-		mr.Add(i.groupAttributeMap.member, newMemberDN)
-
-		if err := i.conn.Modify(&mr); err != nil {
-			if lerr, ok := err.(*ldap.Error); ok {
-				if lerr.ResultCode == ldap.LDAPResultAttributeOrValueExists {
-					err = fmt.Errorf("Duplicate member entries in request")
-				} else {
-					logger.Info().Err(err).Msg("Failed to modify group member entries on PATCH group")
-					err = fmt.Errorf("Unknown error when trying to modify group member entries")
+		// Small retry loop. It might be that, when reading the group we found the empty group member ("",
+		// line 289 above). Our modify operation tries to delete that value. However another go-routine
+		// might have done that in parallel. In that case
+		// (LDAPResultNoSuchAttribute) we need to retry the modification
+		// without the delete.
+		for j := 0; j < 2; j++ {
+			mr.Add(i.groupAttributeMap.member, newMemberDN)
+			if err := i.conn.Modify(&mr); err != nil {
+				if lerr, ok := err.(*ldap.Error); ok {
+					switch lerr.ResultCode {
+					case ldap.LDAPResultAttributeOrValueExists:
+						err = fmt.Errorf("Duplicate member entries in request")
+					case ldap.LDAPResultNoSuchAttribute:
+						if len(mr.Changes) == 2 {
+							// We tried the special case for adding the first group member, but some
+							// other request running in parallel did that already. Retry with a "normal"
+							// modification
+							logger.Debug().Err(err).
+								Msg("Failed to add first group member. Retrying once, without deleting the empty member value.")
+							mr.Changes = make([]ldap.Change, 0, 1)
+							continue
+						}
+					default:
+						logger.Info().Err(err).Msg("Failed to modify group member entries on PATCH group")
+						err = fmt.Errorf("Unknown error when trying to modify group member entries")
+					}
 				}
+				return err
 			}
-			return err
+			// succeeded
+			break
 		}
 	}
 	return nil
@@ -365,12 +384,13 @@ func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, member
 		logger.Debug().Str("backend", "ldap").Str("memberID", memberID).Msg("Error looking up group member")
 		return err
 	}
+
 	logger.Debug().Str("backend", "ldap").Str("groupdn", ge.DN).Str("member", me.DN).Msg("remove member")
 
-	if mr, err := i.removeEntryByDNAndAttributeFromEntry(ge, me.DN, i.groupAttributeMap.member); err == nil {
-		return i.conn.Modify(mr)
+	if err = i.removeEntryByDNAndAttributeFromEntry(ge, me.DN, i.groupAttributeMap.member); err != nil {
+		logger.Error().Err(err).Str("backend", "ldap").Str("group", groupID).Str("member", memberID).Msg("Failed to remove member from group.")
 	}
-	return nil
+	return err
 }
 
 func (i *LDAP) groupToAddRequest(group libregraph.Group) (*ldap.AddRequest, error) {