Refactor memberOf management logic

3 files changed, 83 insertions, 95 deletions

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