Merge pull request #1161 from okta/issue_1149_resource_okta_group_mem…

…berships

Reestablish old behavior of okta_group_memberships resource

okta/resource_okta_group_memberships.go

@@ -35,6 +35,12 @@ func resourceGroupMemberships() *schema.Resource {
 				Description: "The list of Okta user IDs which the group should have membership managed for.",
 				Elem:        &schema.Schema{Type: schema.TypeString},
 			},
+			"track_all_users": {
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Default:     false,
+				Description: "The resource concerns itself with all users added/deleted to the group; even those managed outside of the resource.",
+			},
 		},
 	}
 }
@@ -60,6 +66,7 @@ func resourceGroupMembershipsCreate(ctx context.Context, d *schema.ResourceData,
 	// adding users to a group. Use a backoff to wait for at list one user to be
 	// associated with the group.
 	err = backoff.Retry(func() error {
+		// TODO, should we wait for all users to be added to the group?
 		ok, err := checkIfGroupHasUsers(ctx, client, groupId, users)
 		if err != nil {
 			return backoff.Permanent(err)
@@ -77,36 +84,35 @@ func resourceGroupMembershipsCreate(ctx context.Context, d *schema.ResourceData,
 }
 
 func resourceGroupMembershipsRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
-	groupId := d.Get("group_id").(string)
 	client := getOktaClientFromMetadata(m)
-	expected_users := convertInterfaceToStringSetNullable(d.Get("users"))
+	groupId := d.Get("group_id").(string)
+	oldUsers := convertInterfaceToStringSetNullable(d.Get("users"))
+	trackAllUsers := d.Get("track_all_users").(bool)
 
-	// handle import edge case
-	if len(expected_users) == 0 {
-		logger(m).Info("reading group membership", "id", d.Id())
-		userIDList, err := listGroupUserIDs(ctx, m, d.Id())
+	// New behavior, tracking all users.
+	if trackAllUsers {
+		changed, newUserIDs, err := checkIfUsersHaveChanged(ctx, client, groupId, &oldUsers)
 		if err != nil {
-			return diag.Errorf("unable to return membership for group (%s) from API", d.Id())
+			return diag.Errorf("An error occured checking user ids for group %q, error: %+v", groupId, err)
 		}
-		d.Set("group_id", d.Id())
-		d.Set("users", convertStringSliceToSet(userIDList))
+		if changed {
+			// set the new user ids if users have changed
+			d.Set("users", convertStringSliceToSet(*newUserIDs))
+		}
+
 		return nil
 	}
 
-	ok, err := checkIfGroupHasUsers(ctx, client, groupId, expected_users)
+	// Legacy behavior is just to check if any users have left the group.
+	changed, newUserIDs, err := checkIfUsersHaveBeenRemoved(ctx, client, groupId, &oldUsers)
 	if err != nil {
-		return diag.Errorf("unable to complete group check for user: %v", err)
+		return diag.Errorf("An error occured checking user ids for group %q, error: %+v", groupId, err)
 	}
-	if ok {
-		d.Set("group_id", d.Id())
-		userIDList, _ := listGroupUserIDs(ctx, m, d.Id())
-		d.Set("users", convertStringSliceToSet(userIDList))
-		return nil
-	} else {
-		d.SetId("")
-		logger(m).Info("group (%s) did not have expected memberships or did not exist", groupId)
-		return nil
+	if changed {
+		// The user list has changed, set the new user ids to the users value.
+		d.Set("users", convertStringSliceToSet(*newUserIDs))
 	}
+	return nil
 }
 
 func resourceGroupMembershipsDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
@@ -145,60 +151,142 @@ func resourceGroupMembershipsUpdate(ctx context.Context, d *schema.ResourceData,
 	return nil
 }
 
-// checkIfGroupHasUsers firstly checks if the group has users given an immediate
-// API call to list group users. It will additionally compare the current list
-// of users found in the API call with a slice of known users passed in to the
-// function. This second comparison is a stateful comparison.
-func checkIfGroupHasUsers(ctx context.Context, client *okta.Client, groupId string, users []string) (bool, error) {
-	// TODO: This method should be renamed and/or refactored. Its name implies
-	// it is only checking for users but it can return false to signal that
-	// users have changed.
+// checkIfUsersHaveChanged If the function returns true then users have been
+// changed and the returned user ids should be considered the new set of users.
+// Returns error for API errors. Returns false if no users have changed and the
+// slice of returned strings will be empty.
+func checkIfUsersHaveChanged(ctx context.Context, client *okta.Client, groupId string, users *[]string) (bool, *[]string, error) {
+	noop := []string{}
+	if users == nil || len(*users) == 0 {
+		return false, &noop, nil
+	}
+
+	// We are using the old users map as a ledger to find users that have been
+	// removed from the user list.
+	oldUsers := toStrIndexedMap(users)
+	changed := false
+	// Collect all user ids that are returned from the API
+	usersFromAPI := []string{}
+
 	groupUsers, resp, err := client.Group.ListGroupUsers(ctx, groupId, &query.Params{Limit: defaultPaginationLimit})
 	if err := suppressErrorOn404(resp, err); err != nil {
-		return false, fmt.Errorf("unable to return membership for group (%s) from API", groupId)
+		return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err)
+	}
+
+	for _, user := range groupUsers {
+		// if the new user id is not in the old users map then the list of users has changed
+		if _, found := (*oldUsers)[user.Id]; !found {
+			changed = true
+		}
+		usersFromAPI = append(usersFromAPI, user.Id)
 	}
 
 	for resp.HasNextPage() {
-		var additionalUsers []*okta.User
-		resp, err = resp.Next(context.Background(), &additionalUsers)
+		groupUsers = nil
+		resp, err = resp.Next(context.Background(), &groupUsers)
 		if err != nil {
-			return false, fmt.Errorf("unable to return membership for group (%s) from API", groupId)
+			return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err)
 		}
-		groupUsers = append(groupUsers, additionalUsers...)
+
+		for _, user := range groupUsers {
+			// if the new user id is not in the old users map then the list of users has changed
+			if _, found := (*oldUsers)[user.Id]; !found {
+				changed = true
+			}
+			usersFromAPI = append(usersFromAPI, user.Id)
+		}
+	}
+	if len(*oldUsers) != len(usersFromAPI) {
+		changed = true
 	}
 
-	// We need to return false if there isn't any users present. For eventual
-	// consistency issues create has a backoff/retry when we return false
-	// without an error here. Read occurs in the future and so it doesn't guard
-	// against eventual consistency.
-	if len(groupUsers) == 0 {
-		return false, nil
+	var result *[]string = &noop
+	if changed {
+		result = &usersFromAPI
+	}
+
+	return changed, result, nil
+}
+
+// checkIfUsersHaveBeenRemoved If the function returns true then users have been
+// removed and the subset of returned user ids should be considered the new set
+// of users. Returns error for API errors. Returns false if no users have been
+// removed and the slice of returned strings will be empty.
+func checkIfUsersHaveBeenRemoved(ctx context.Context, client *okta.Client, groupId string, users *[]string) (bool, *[]string, error) {
+	noop := []string{}
+	if users == nil || len(*users) == 0 {
+		return false, &noop, nil
 	}
 
-	// Create a set to compare the  users slice passed into the check to compare
-	// with what the api returns from list group users API call.
-	expectedUserSet := make(map[string]bool)
+	// We are using the old users map as a ledger to find users that have been
+	// removed from the user list. If it ever becomes sized 0 then we've found
+	// all of our user ids and no longer have to make API calls.
+	oldUsers := toStrIndexedMap(users)
 
-	// We train the set with false values for our previously known users.
-	for _, user := range users {
-		expectedUserSet[user] = false
+	groupUsers, resp, err := client.Group.ListGroupUsers(ctx, groupId, &query.Params{Limit: defaultPaginationLimit})
+	if err := suppressErrorOn404(resp, err); err != nil {
+		return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err)
 	}
 
-	// We confirm the latest user ids from list group users API call are still
-	// present in the set.
 	for _, user := range groupUsers {
-		if _, ok := expectedUserSet[user.Id]; ok {
-			expectedUserSet[user.Id] = true
+		// Deleting user from API from the old users map
+		delete(*oldUsers, user.Id)
+		if len(*oldUsers) == 0 {
+			// All old users have been accounted for.
+			return false, &noop, nil
+		}
+	}
+
+	for resp.HasNextPage() {
+		groupUsers = nil
+		resp, err = resp.Next(context.Background(), &groupUsers)
+		if err != nil {
+			return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err)
+		}
+		for _, user := range groupUsers {
+			// Deleting user from API from the old users map
+			delete(*oldUsers, user.Id)
+			if len(*oldUsers) == 0 {
+				// All old users have been accounted for.
+				return false, &noop, nil
+			}
 		}
 	}
 
-	// If one of the known users in the call to the check function is no longer
-	// in the list group users API call we return false.
-	for _, state := range expectedUserSet {
-		if !state {
-			return false, nil
+	// Any old users left are the IDs that have been removed from group.
+
+	// This loop keeps the returned new user list in the same order as the users
+	// list passed in minus the now missing user IDs.
+	newUsers := []string{}
+	for _, userId := range *users {
+		// Any single user id found in old users should not be appended to the
+		// new users result.
+		if _, found := (*oldUsers)[userId]; !found {
+			newUsers = append(newUsers, userId)
 		}
 	}
 
-	return true, nil
+	return true, &newUsers, nil
+}
+
+func checkIfGroupHasUsers(ctx context.Context, client *okta.Client, groupId string, users []string) (bool, error) {
+	groupUsers, resp, err := client.Group.ListGroupUsers(ctx, groupId, &query.Params{Limit: defaultPaginationLimit})
+	if err := suppressErrorOn404(resp, err); err != nil {
+		return false, fmt.Errorf("unable to return membership for group (%s) from API", groupId)
+	}
+	return (len(groupUsers) > 0), nil
+}
+
+func toStrIndexedMap(strs *[]string) *map[string]int {
+	result := map[string]int{}
+	if strs == nil {
+		return &result
+	}
+
+	length := len(*strs)
+	for i := 0; i < length; i++ {
+		result[(*strs)[i]] = i
+	}
+
+	return &result
 }