Skip to content

fix: prevent group loss when paginating checkIfUserHasGroups past 2 pages - #2898

Open
rasta-rocket wants to merge 1 commit into
okta:masterfrom
rasta-rocket:fix_checkIfUserHasGroups
Open

fix: prevent group loss when paginating checkIfUserHasGroups past 2 pages#2898
rasta-rocket wants to merge 1 commit into
okta:masterfrom
rasta-rocket:fix_checkIfUserHasGroups

Conversation

@rasta-rocket

Copy link
Copy Markdown
Contributor

What

checkIfUserHasGroups (used by okta_user_group_memberships create/read) declared its per-page slice outside the pagination loop and reused it across pages:

var nextUserGroups []*sdk.Group        // declared once
for resp.HasNextPage() {
    resp, err = resp.Next(ctx, &nextUserGroups)   // decoded into the same slice every time
    ...
    userGroups = append(userGroups, nextUserGroups...)
}

encoding/json does not allocate new structs when the destination slice already holds non-nil pointers - it follows the existing pointers and overwrites the structs in place. Since userGroups holds those same pointers, decoding page 3 silently rewrites the entries copied from page 2.

Why it matters

Any user with more than 400 group memberships (2 pages of the API's 200-per-page limit) triggers this on the third page onward. Groups the user is genuinely a member of vanish from the collected set, checkIfUserHasGroups reports them as missing, and resourceUserGroupMembershipsCreate's backoff.Retry exhausts with:

Error: user (<id>) did not have expected group memberships after multiple checks

...even though addUserToGroups already succeeded and the group assignment is correct in Okta. terraform apply fails deterministically every time for these users - it isn't a flaky eventual-consistency race.

This is the same bug class as #1542 (pagination wasn't looped at all), one layer deeper: the loop is correct, but the accumulation across iterations isn't.

Fix

Move the slice declaration inside the loop so each page decodes into its own freshly allocated slice:

for resp.HasNextPage() {
    var nextUserGroups []*sdk.Group   // fresh per iteration
    resp, err = resp.Next(ctx, &nextUserGroups)
    ...
    userGroups = append(userGroups, nextUserGroups...)
}

Testing

Added TestUserGroupMembershipsReadPaginationDoesNotDropGroups, a unit test (httpmock, no TF_ACC) that mocks a 3-page GET /users/{id}/groups response with the target group on page 2 and asserts it survives to the final result. Verified it fails on main (before this fix) with:

expected: "<user_id>"
actual:   ""

and passes with the fix applied. go vet and gofmt clean.

Reproduced against my company's real Okta org data with a local build - the failing users have 401-600 group memberships (3 pages), and the missing group falls in the corrupted range as predicted.

Cheers ☀️

…ages

checkIfUserHasGroups declared the per-page slice once, outside the
pagination loop, and reused it across resp.Next() calls. Decoding a
JSON array into a slice that already holds non-nil pointers makes
encoding/json reuse those pointers and overwrite the pointed-to
structs in place, so decoding page N clobbered entries already
appended to the accumulator from page N-1.

The corruption starts on the third page (page two decodes into a nil
slice, so it allocates fresh). Users with more than 400 group
memberships (2 pages of 200) hit this: real memberships silently
disappeared from the read-back, checkIfUserHasGroups reported them
missing, and Create's backoff.Retry exhausted with "did not have
expected group memberships after multiple checks" even though the
group assignment had succeeded.

Move the slice declaration inside the loop so each page decodes into
a fresh slice.
@github-actions
github-actions Bot requested a review from pranav-okta July 28, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant