From 49be2069f63d8f2909840ad504063c773d0ba7ed Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Thu, 13 Feb 2020 14:14:27 +0100 Subject: [PATCH] Refactor memberOf management logic --- Makefile | 2 +- memberof.go | 60 +++++++++++++++++++++++++++ write.go | 116 ++++++++++------------------------------------------ 3 files changed, 83 insertions(+), 95 deletions(-) create mode 100644 memberof.go diff --git a/Makefile b/Makefile index 2956c7f..5489d4c 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,5 @@ BIN=bottin -SRC=main.go ssha.go util.go acl.go read.go write.go +SRC=main.go ssha.go util.go acl.go read.go write.go memberof.go DOCKER=lxpz/bottin_amd64 all: $(BIN) diff --git a/memberof.go b/memberof.go new file mode 100644 index 0000000..c4074e4 --- /dev/null +++ b/memberof.go @@ -0,0 +1,60 @@ +package main + +func (server *Server) memberOfAdd(member string, group string) { + // Retreive previous memberOf value + memberGroups, err := server.getAttribute(member, ATTR_MEMBEROF) + if err != nil { + server.logger.Warnf("Could not add %s to memberOf of %s: %s", group, member, err) + return + } + + // Return early if group is already in memberOf + for _, mb := range memberGroups { + if mb == group { + server.logger.Warnf("Inconsistency detected, %s was memberOf %s at a time when it didn't exist! (not an issue anymore)", + member, group) + return + } + } + + // Add group to memberOf + memberGroups = append(memberGroups, group) + err = server.addElements(member, Entry{ + ATTR_MEMBEROF: memberGroups, + }) + if err != nil { + server.logger.Warnf("Could not add %s to memberOf of %s: %s", group, member, err) + } +} + +func (server *Server) memberOfRemove(member string, group string) { + // Retreive previous memberOf value + memberOf, err := server.getAttribute(member, ATTR_MEMBEROF) + if err != nil || memberOf == nil { + server.logger.Warnf("Could not remove %s from memberOf of %s: %s", group, member, err) + return + } + + // Filter out group + newMemberOf := []string{} + for _, mb := range memberOf { + if mb != group { + newMemberOf = append(newMemberOf, group) + } + } + + // Return early if group already not in memberOf + if len(newMemberOf) == len(memberOf) { + server.logger.Warnf("Inconsistency detected, %s was not memberOf %s at a time when it should have been! (not an issue anymore)", + member, group) + return + } + + // Update value of memberOf + err = server.addElements(member, Entry{ + ATTR_MEMBEROF: newMemberOf, + }) + if err != nil { + server.logger.Warnf("Could not remove %s from memberOf of %s: %s", group, member, err) + } +} diff --git a/write.go b/write.go index 57c642c..24fdc1a 100644 --- a/write.go +++ b/write.go @@ -58,8 +58,10 @@ func (server *Server) handleAddInternal(state *State, r *message.AddRequest) (in return ldap.LDAPResultEntryAlreadyExists, nil } + // TODO: check that parent object exists + // If adding a group, track of who the members will be so that their memberOf field can be updated later - var members []string = nil + members := []string{} // Check attributes entry := Entry{} @@ -77,8 +79,7 @@ func (server *Server) handleAddInternal(state *State, r *message.AddRequest) (in } // If they are writing a member key, we have to check they are adding valid members if strings.EqualFold(key, ATTR_MEMBER) { - members = vals_str - for _, member := range members { + for _, member := range vals_str { member_canonical, err := server.checkDN(member, false) if err != nil { return ldap.LDAPResultInvalidDNSyntax, err @@ -93,8 +94,13 @@ func (server *Server) handleAddInternal(state *State, r *message.AddRequest) (in member_canonical) } } + members = append(members, vals_str...) + } + if prev, ok := entry[key]; ok { + entry[key] = append(prev, vals_str...) + } else { + entry[key] = vals_str } - entry[key] = vals_str } entry[ATTR_CREATORSNAME] = []string{state.login.user} @@ -111,34 +117,8 @@ func (server *Server) handleAddInternal(state *State, r *message.AddRequest) (in // ~~ future errors cause inconsistencies in the DB and are logged ~~ // If our item has a member list, add it to all of its member's memberOf attribute - if members != nil { - for _, member := range members { - memberGroups, err := server.getAttribute(member, ATTR_MEMBEROF) - if err != nil { - server.logger.Warnf("Could not add %s to memberOf of %s: %s", dn, member, err) - continue - } - - alreadyMember := false - for _, mb := range memberGroups { - if mb == dn { - alreadyMember = true - server.logger.Warnf("Warning: inconsistency detected, %s was memberOf %s at a time when it didn't exist!", - member, dn) - break - } - } - - if !alreadyMember { - memberGroups = append(memberGroups, dn) - err = server.addElements(member, Entry{ - ATTR_MEMBEROF: memberGroups, - }) - if err != nil { - server.logger.Warnf("Could not add %s to memberOf of %s: %s", dn, member, err) - } - } - } + for _, member := range members { + server.memberOfAdd(member, dn) } return ldap.LDAPResultSuccess, nil @@ -196,7 +176,7 @@ func (server *Server) handleDeleteInternal(state *State, r *message.DelRequest) } if itemDN != dn { return ldap.LDAPResultNotAllowedOnNonLeaf, fmt.Errorf( - "Cannot delete %d as it has children", dn) + "Cannot delete %s as it has children", dn) } } @@ -244,28 +224,8 @@ func (server *Server) handleDeleteInternal(state *State, r *message.DelRequest) } // Delete it from all of its member's memberOf info - if memberList != nil { - for _, member := range memberList { - memberOf, err := server.getAttribute(member, ATTR_MEMBEROF) - if err != nil || memberOf == nil { - server.logger.Warnf("Could not remove %s from memberOf of %s: %s", dn, member, err) - continue - } - - newMemberOf := []string{} - for _, group := range memberOf { - if group != dn { - newMemberOf = append(newMemberOf, group) - } - } - - err = server.addElements(member, Entry{ - ATTR_MEMBEROF: newMemberOf, - }) - if err != nil { - server.logger.Warnf("Could not remove %s from memberOf of %s: %s", dn, member, err) - } - } + for _, member := range memberList { + server.memberOfRemove(member, dn) } return ldap.LDAPResultSuccess, nil @@ -455,57 +415,25 @@ func (server *Server) handleModifyInternal(state *State, r *message.ModifyReques addMembers[i] = addMem } + // Now, the modification has been processed and accepted and we want to commit it newEntry[ATTR_MODIFIERSNAME] = []string{state.login.user} newEntry[ATTR_MODIFYTIMESTAMP] = []string{genTimestamp()} // Save the edited values - server.addElements(dn, newEntry) + err = server.addElements(dn, newEntry) + if err != nil { + return ldap.LDAPResultOperationsError, err + } // ~~ from this point on, our operation succeeded ~~ // ~~ future errors cause inconsistencies in the DB and are logged ~~ // Update memberOf for added members and deleted members for _, addMem := range addMembers { - memberOf, err := server.getAttribute(addMem, ATTR_MEMBEROF) - if err != nil { - server.logger.Warnf("Could not add %s to memberOf of %s: %s", dn, addMem, err) - continue - } - - alreadyMember := false - for _, mb := range memberOf { - if mb == dn { - alreadyMember = true - break - } - } - if !alreadyMember { - memberOf = append(memberOf, dn) - err = server.addElements(addMem, Entry{ - ATTR_MEMBEROF: memberOf, - }) - if err != nil { - server.logger.Warnf("Could not add %s to memberOf of %s: %s", dn, addMem, err) - } - } + server.memberOfAdd(addMem, dn) } for _, delMem := range delMembers { - memberOf, err := server.getAttribute(delMem, ATTR_MEMBEROF) - if err != nil { - server.logger.Warnf("Could not remove %s from memberOf of %s: %s", dn, delMem, err) - continue - } - newMemberOf := []string{} - for _, g := range memberOf { - if g != dn { - newMemberOf = append(newMemberOf, g) - } - } - - err = server.addElements(delMem, Entry{ATTR_MEMBEROF: newMemberOf}) - if err != nil { - server.logger.Warnf("Could not remove %s from memberOf of %s: %s", dn, delMem, err) - } + server.memberOfRemove(delMem, dn) } return ldap.LDAPResultSuccess, nil