diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 259b32b587..980cb21ca2 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/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 diff --git a/services/graph/pkg/identity/ldap_education_class.go b/services/graph/pkg/identity/ldap_education_class.go index c7f657355d..283506e39a 100644 --- a/services/graph/pkg/identity/ldap_education_class.go +++ b/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) } diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go index 67a4abe268..d19b86e12c 100644 --- a/services/graph/pkg/identity/ldap_group.go +++ b/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) {