NM-86: Cleanup User Configs (#3603)

* feat(go): cleanup user extclients;

1. On disabling a user, remove all their extclients.
2. Add comments and rename variables to clarify the user group extclient cleanup function.

* feat(go): add checks for disable and enable user api;

* feat(go): refactor extclient cleanup on group network roles changes;

* feat(go): delete extclient on user group membership changes;

controllers/user.go

@@ -777,6 +777,52 @@ func enableUserAccount(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	var caller *models.User
+	var isMaster bool
+	if r.Header.Get("user") == logic.MasterUser {
+		isMaster = true
+	} else {
+		caller, err = logic.GetUser(r.Header.Get("user"))
+		if err != nil {
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "internal"))
+			return
+		}
+	}
+
+	if !isMaster && caller.UserName == user.UserName {
+		// This implies that a user is trying to enable themselves.
+		// This can never happen, since a disabled user cannot be
+		// authenticated.
+		err := fmt.Errorf("cannot enable self")
+		logger.Log(0, err.Error())
+		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+		return
+	}
+
+	switch user.PlatformRoleID {
+	case models.SuperAdminRole:
+		// This can never happen, since a superadmin user cannot
+		// be disabled.
+	case models.AdminRole:
+		if !isMaster && caller.PlatformRoleID != models.SuperAdminRole {
+			err = fmt.Errorf("%s cannot enable an admin", caller.PlatformRoleID)
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+			return
+		}
+	case models.PlatformUser:
+		if !isMaster && caller.PlatformRoleID != models.SuperAdminRole && caller.PlatformRoleID != models.AdminRole {
+			err = fmt.Errorf("%s cannot enable a platform-user", caller.PlatformRoleID)
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+			return
+		}
+	case models.ServiceUser:
+		if !isMaster && caller.PlatformRoleID != models.SuperAdminRole && caller.PlatformRoleID != models.AdminRole {
+			err = fmt.Errorf("%s cannot enable a service-user", caller.PlatformRoleID)
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+			return
+		}
+	}
+
 	user.AccountDisabled = false
 	err = logic.UpsertUser(*user)
 	if err != nil {
@@ -803,11 +849,49 @@ func disableUserAccount(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if user.PlatformRoleID == models.SuperAdminRole {
-		err = errors.New("cannot disable super-admin user account")
-		logger.Log(0, err.Error())
-		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "badrequest"))
+	var caller *models.User
+	var isMaster bool
+	if r.Header.Get("user") == logic.MasterUser {
+		isMaster = true
+	} else {
+		caller, err = logic.GetUser(r.Header.Get("user"))
+		if err != nil {
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "internal"))
+			return
+		}
+	}
+
+	if !isMaster && caller.UserName == user.UserName {
+		// This implies that a user is trying to disable themselves.
+		// This should not be allowed.
+		err = fmt.Errorf("cannot disable self")
+		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+		return
+	}
+
+	switch user.PlatformRoleID {
+	case models.SuperAdminRole:
+		err = errors.New("cannot disable a super-admin")
+		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
 		return
+	case models.AdminRole:
+		if !isMaster && caller.PlatformRoleID != models.SuperAdminRole {
+			err = fmt.Errorf("%s cannot disable an admin", caller.PlatformRoleID)
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+			return
+		}
+	case models.PlatformUser:
+		if !isMaster && caller.PlatformRoleID != models.SuperAdminRole && caller.PlatformRoleID != models.AdminRole {
+			err = fmt.Errorf("%s cannot disable a platform-user", caller.PlatformRoleID)
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+			return
+		}
+	case models.ServiceUser:
+		if !isMaster && caller.PlatformRoleID != models.SuperAdminRole && caller.PlatformRoleID != models.AdminRole {
+			err = fmt.Errorf("%s cannot disable a service-user", caller.PlatformRoleID)
+			logic.ReturnErrorResponse(w, r, logic.FormatError(err, "forbidden"))
+			return
+		}
 	}
 
 	user.AccountDisabled = true
@@ -817,6 +901,28 @@ func disableUserAccount(w http.ResponseWriter, r *http.Request) {
 		logic.ReturnErrorResponse(w, r, logic.FormatError(err, "internal"))
 	}
 
+	go func() {
+		extclients, err := logic.GetAllExtClients()
+		if err != nil {
+			logger.Log(0, "failed to get user extclients:", err.Error())
+			return
+		}
+
+		for _, extclient := range extclients {
+			if extclient.OwnerID == user.UserName {
+				err = logic.DeleteExtClientAndCleanup(extclient)
+				if err != nil {
+					logger.Log(0, "failed to delete user extclient:", err.Error())
+				} else {
+					err := mq.PublishDeletedClientPeerUpdate(&extclient)
+					if err != nil {
+						logger.Log(0, "failed to publish deleted client peer update:", err.Error())
+					}
+				}
+			}
+		}
+	}()
+
 	logic.ReturnSuccessResponse(w, r, "user account disabled")
 }
 
@@ -1313,6 +1419,67 @@ func updateUser(w http.ResponseWriter, r *http.Request) {
 	}
 	logic.LogEvent(&e)
 	go mq.PublishPeerUpdate(false)
+	go func() {
+		// Populating all the networks the user has access to by
+		// being a member of groups.
+		userMembershipNetworkAccess := make(map[models.NetworkID]struct{})
+		for groupID := range user.UserGroups {
+			userGroup, _ := logic.GetUserGroup(groupID)
+			for netID := range userGroup.NetworkRoles {
+				userMembershipNetworkAccess[netID] = struct{}{}
+			}
+		}
+
+		extclients, err := logic.GetAllExtClients()
+		if err != nil {
+			slog.Error("failed to fetch extclients", "error", err)
+			return
+		}
+
+		for _, extclient := range extclients {
+			if extclient.OwnerID != user.UserName {
+				continue
+			}
+
+			var shouldDelete bool
+			if user.PlatformRoleID == models.SuperAdminRole || user.PlatformRoleID == models.AdminRole {
+				// Super-admin and Admin's access is not determined by group membership
+				// or network roles. Even if a user is removed from the group, they
+				// continue to have access to the network.
+				// So, no need to delete the extclient.
+				shouldDelete = false
+			} else {
+				_, hasAccess := user.NetworkRoles[models.NetworkID(extclient.Network)]
+				if hasAccess {
+					// The user has access to the network by themselves and not by
+					// virtue of being a member of the group.
+					// So, no need to delete the extclient.
+					shouldDelete = false
+				} else {
+					_, hasAccessThroughGroups := userMembershipNetworkAccess[models.NetworkID(extclient.Network)]
+					if !hasAccessThroughGroups {
+						// The user does not have access to the network by either
+						// being a Super-admin or Admin, by network roles or by virtue
+						// of being a member a group that has access to the network.
+						// So, delete the extclient.
+						shouldDelete = true
+					}
+				}
+			}
+
+			if shouldDelete {
+				err = logic.DeleteExtClientAndCleanup(extclient)
+				if err != nil {
+					slog.Error("failed to delete extclient",
+						"id", extclient.ClientID, "owner", user.UserName, "error", err)
+				} else {
+					if err := mq.PublishDeletedClientPeerUpdate(&extclient); err != nil {
+						slog.Error("error setting ext peers: " + err.Error())
+					}
+				}
+			}
+		}
+	}()
 	logger.Log(1, username, "was updated")
 	json.NewEncoder(w).Encode(logic.ToReturnUser(*user))
 }

pro/controllers/users.go

@@ -692,7 +692,7 @@ func updateUserGroup(w http.ResponseWriter, r *http.Request) {
 	}()
 
 	// reset configs for service user
-	go proLogic.UpdatesUserGwAccessOnGrpUpdates(currUserG.NetworkRoles, userGroup.NetworkRoles)
+	go proLogic.UpdatesUserGwAccessOnGrpUpdates(userGroup.ID, currUserG.NetworkRoles, userGroup.NetworkRoles)
 	logic.ReturnSuccessResponseWithJson(w, r, userGroup, "updated user group")
 }
 
@@ -897,7 +897,7 @@ func deleteUserGroup(w http.ResponseWriter, r *http.Request) {
 		}
 	}()
 
-	go proLogic.UpdatesUserGwAccessOnGrpUpdates(userG.NetworkRoles, make(map[models.NetworkID]map[models.UserRoleID]struct{}))
+	go proLogic.UpdatesUserGwAccessOnGrpUpdates(userG.ID, userG.NetworkRoles, make(map[models.NetworkID]map[models.UserRoleID]struct{}))
 	logic.ReturnSuccessResponseWithJson(w, r, nil, "deleted user group")
 }
 

pro/logic/user_mgmt.go

@@ -1086,27 +1086,14 @@ func UpdatesUserGwAccessOnRoleUpdates(currNetworkAccess,
 	}
 }
 
-func UpdatesUserGwAccessOnGrpUpdates(currNetworkRoles, changeNetworkRoles map[models.NetworkID]map[models.UserRoleID]struct{}) {
-	networkChangeMap := make(map[models.NetworkID]map[models.UserRoleID]struct{})
-	for netID, networkUserRoles := range currNetworkRoles {
-		if _, ok := changeNetworkRoles[netID]; !ok {
-			for netRoleID := range networkUserRoles {
-				if _, ok := networkChangeMap[netID]; !ok {
-					networkChangeMap[netID] = make(map[models.UserRoleID]struct{})
-				}
-				networkChangeMap[netID][netRoleID] = struct{}{}
-			}
-		} else {
-			for netRoleID := range networkUserRoles {
-				if _, ok := changeNetworkRoles[netID][netRoleID]; !ok {
-					if _, ok := networkChangeMap[netID]; !ok {
-						networkChangeMap[netID] = make(map[models.UserRoleID]struct{})
-					}
-					networkChangeMap[netID][netRoleID] = struct{}{}
-				}
-			}
+func UpdatesUserGwAccessOnGrpUpdates(groupID models.UserGroupID, oldNetworkRoles, newNetworkRoles map[models.NetworkID]map[models.UserRoleID]struct{}) {
+	networkRemovedMap := make(map[models.NetworkID]struct{})
+	for netID := range oldNetworkRoles {
+		if _, ok := newNetworkRoles[netID]; !ok {
+			networkRemovedMap[netID] = struct{}{}
 		}
 	}
+
 	extclients, err := logic.GetAllExtClients()
 	if err != nil {
 		slog.Error("failed to fetch extclients", "error", err)
@@ -1116,27 +1103,56 @@ func UpdatesUserGwAccessOnGrpUpdates(currNetworkRoles, changeNetworkRoles map[mo
 	if err != nil {
 		return
 	}
-	for _, extclient := range extclients {
 
-		if _, ok := networkChangeMap[models.NetworkID(extclient.Network)]; ok {
-			if user, ok := userMap[extclient.OwnerID]; ok {
-				if user.PlatformRoleID != models.ServiceUser {
-					continue
-				}
-				err = logic.DeleteExtClientAndCleanup(extclient)
-				if err != nil {
-					slog.Error("failed to delete extclient",
-						"id", extclient.ClientID, "owner", user.UserName, "error", err)
+	for _, extclient := range extclients {
+		var shouldDelete bool
+		user, ok := userMap[extclient.OwnerID]
+		if !ok {
+			// user does not exist, delete extclient.
+			shouldDelete = true
+		} else {
+			if user.PlatformRoleID == models.SuperAdminRole || user.PlatformRoleID == models.AdminRole {
+				// Super-admin and Admin's access is not determined by group membership
+				// or network roles. Even if a network is removed from the group, they
+				// continue to have access to the network.
+				// So, no need to delete the extclient.
+				shouldDelete = false
+			} else {
+				_, hasAccess := user.NetworkRoles[models.NetworkID(extclient.Network)]
+				if hasAccess {
+					// The user has access to the network by themselves and not by
+					// virtue of being a member of the group.
+					// So, no need to delete the extclient.
+					shouldDelete = false
 				} else {
-					if err := mq.PublishDeletedClientPeerUpdate(&extclient); err != nil {
-						slog.Error("error setting ext peers: " + err.Error())
+					_, userInGroup := user.UserGroups[groupID]
+					_, networkRemoved := networkRemovedMap[models.NetworkID(extclient.Network)]
+					if userInGroup && networkRemoved {
+						// This group no longer provides it's members access to the
+						// network.
+						// This user is a member of the group and has no direct
+						// access to the network (either by its platform role or by
+						// network roles).
+						// So, delete the extclient.
+						shouldDelete = true
 					}
 				}
 			}
-
 		}
 
+		if shouldDelete {
+			err = logic.DeleteExtClientAndCleanup(extclient)
+			if err != nil {
+				slog.Error("failed to delete extclient",
+					"id", extclient.ClientID, "owner", user.UserName, "error", err)
+			} else {
+				if err := mq.PublishDeletedClientPeerUpdate(&extclient); err != nil {
+					slog.Error("error setting ext peers: " + err.Error())
+				}
+			}
+		}
 	}
+
 	if servercfg.IsDNSMode() {
 		logic.SetDNS()
 	}