diff --git a/okta/resource_okta_app_group_assignments.go b/okta/resource_okta_app_group_assignments.go index 3801984d2..45aecdcdc 100644 --- a/okta/resource_okta_app_group_assignments.go +++ b/okta/resource_okta_app_group_assignments.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "reflect" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" @@ -93,7 +92,10 @@ func resourceAppGroupAssignmentsCreate(ctx context.Context, d *schema.ResourceDa func resourceAppGroupAssignmentsRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { client := getOktaClientFromMetadata(m) - assignments, resp, err := listApplicationGroupAssignments( + // remember, current group assignments is an API call and are all groups + // assigned to the app, even those initiated outside the provider, for + // instance those assignments from "click ops" + currentGroupAssignments, resp, err := listApplicationGroupAssignments( ctx, client, d.Get("app_id").(string), @@ -101,22 +103,23 @@ func resourceAppGroupAssignmentsRead(ctx context.Context, d *schema.ResourceData if err := suppressErrorOn404(resp, err); err != nil { return diag.Errorf("failed to fetch group assignments: %v", err) } - if assignments == nil { + if currentGroupAssignments == nil { d.SetId("") return nil } g, ok := d.GetOk("group") if ok { - err := setNonPrimitives(d, map[string]interface{}{"group": syncGroups(d, g.([]interface{}), assignments)}) + groupAssignments := syncGroups(d, g.([]interface{}), currentGroupAssignments) + err := setNonPrimitives(d, map[string]interface{}{"group": groupAssignments}) if err != nil { return diag.Errorf("failed to set OAuth application properties: %v", err) } } else { - arr := make([]map[string]interface{}, len(assignments)) - for i := range assignments { - arr[i] = groupAssignmentToTFGroup(assignments[i]) + groupAssignments := make([]map[string]interface{}, len(currentGroupAssignments)) + for i := range currentGroupAssignments { + groupAssignments[i] = groupAssignmentToTFGroup(currentGroupAssignments[i]) } - err := setNonPrimitives(d, map[string]interface{}{"group": arr}) + err := setNonPrimitives(d, map[string]interface{}{"group": groupAssignments}) if err != nil { return diag.Errorf("failed to set OAuth application properties: %v", err) } @@ -127,15 +130,10 @@ func resourceAppGroupAssignmentsRead(ctx context.Context, d *schema.ResourceData func resourceAppGroupAssignmentsUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { client := getOktaClientFromMetadata(m) appID := d.Get("app_id").(string) - assignments, _, err := listApplicationGroupAssignments( - ctx, - client, - d.Get("app_id").(string), - ) + toAssign, toRemove, err := splitAssignmentsTargets(d) if err != nil { - return diag.Errorf("failed to fetch group assignments: %v", err) + return diag.Errorf("failed to discern group assignment splits: %v", err) } - toAssign, toRemove := splitAssignmentsTargets(tfGroupsToGroupAssignments(d), assignments) err = deleteGroupAssignments( client.Application.DeleteApplicationGroupAssignment, ctx, @@ -173,24 +171,64 @@ func resourceAppGroupAssignmentsDelete(ctx context.Context, d *schema.ResourceDa return nil } -func syncGroups(d *schema.ResourceData, groups []interface{}, assignments []*sdk.ApplicationGroupAssignment) []interface{} { - var newGroups []interface{} - for i := range groups { - present := false - for _, assignment := range assignments { - if assignment.Id == d.Get(fmt.Sprintf("group.%d.id", i)).(string) { - present = true +// syncGroups compares tfGroups - the groups set in the config, with all group +// assignments, and all assignments known to the API. If there is new +// information from all group assignments for a group already in the config, +// that information will be updated (id, priority, profile). If the group no +// longer exists as an assignment it is removed from the groups locally. +// Otherwise, the group is is added to results slice as net new data. Change +// detection will occur if anything changes API side compared to local state +// side. +func syncGroups(d *schema.ResourceData, tfGroups []interface{}, groupAssignments []*sdk.ApplicationGroupAssignment) []interface{} { + var result []interface{} + // Two passes are required for the result. + // First pass keeps the order of tfGroup, but only keeps/updates the group + // info if it is present in group assignments. + // Second pass is to add in any new additions from group assignments. + for i := range tfGroups { + // if group is no longer assigned it will not be added back to the results + for _, assignment := range groupAssignments { + groupId := fmt.Sprintf("group.%d.id", i) + if assignment.Id == d.Get(groupId).(string) { + group := map[string]interface{}{ + "id": assignment.Id, + "profile": buildProfile(d, i, assignment), + } if assignment.PriorityPtr != nil && *assignment.PriorityPtr >= 0 { - groups[i].(map[string]interface{})["priority"] = *assignment.PriorityPtr + group["priority"] = *assignment.PriorityPtr } - groups[i].(map[string]interface{})["profile"] = buildProfile(d, i, assignment) + result = append(result, group) + } + } + } + + for _, assignment := range groupAssignments { + found := false + for _, g := range tfGroups { + group := g.(map[string]interface{}) + id := group["id"] + if id == assignment.Id { + found = true + } + } + if found { + continue + } + + newGroup := map[string]interface{}{ + "id": assignment.Id, + } + if assignment.Profile != nil { + if p, ok := assignment.Profile.(string); ok { + newGroup["profile"] = p } } - if present { - newGroups = append(newGroups, groups[i]) + if assignment.PriorityPtr != nil && *assignment.PriorityPtr >= 0 { + newGroup["priority"] = *assignment.PriorityPtr } + result = append(result, newGroup) } - return newGroups + return result } func buildProfile(d *schema.ResourceData, i int, assignment *sdk.ApplicationGroupAssignment) string { @@ -232,39 +270,74 @@ func buildProfile(d *schema.ResourceData, i int, assignment *sdk.ApplicationGrou return string(jsonProfile) } -func splitAssignmentsTargets(expectedAssignments, existingAssignments []*sdk.ApplicationGroupAssignment) (toAssign, toRemove []*sdk.ApplicationGroupAssignment) { - for i := range expectedAssignments { - if !containsEqualAssignment(existingAssignments, expectedAssignments[i]) { - toAssign = append(toAssign, expectedAssignments[i]) +// splitAssignmentsTargets uses schema change to determine what if any +// assignments to keep and which to remove. This is in the context of the local +// terraform state. Get changes returns old state vs new state. Anything in the +// old state but not in the new state will be removed. Otherwise, everything is +// to be assigned. That way, if there are changes to an existing assignment +// (e.g. on priority or profile) they'll still be posted to the API for update. +func splitAssignmentsTargets(d *schema.ResourceData) (toAssign, toRemove []*sdk.ApplicationGroupAssignment, err error) { + // 1. Anything in old, but not in new, needs to be deleted + // 2. Treat everything else as to be added that will also take care of field + // updates on priority and profile + o, n := d.GetChange("group") + oldState, ok := o.([]interface{}) + if !ok { + err = fmt.Errorf("expected old groups to be slice, got %T", o) + return + } + newState, ok := n.([]interface{}) + if !ok { + err = fmt.Errorf("expected new groups to be slice, got %T", n) + return + } + + oldIDs := map[string]interface{}{} + newIDs := map[string]interface{}{} + for _, old := range oldState { + if o, ok := old.(map[string]interface{}); ok { + id := o["id"].(string) + oldIDs[id] = o } } - for i := range existingAssignments { - if !containsAssignment(expectedAssignments, existingAssignments[i]) { - toRemove = append(toRemove, existingAssignments[i]) + for _, new := range newState { + if n, ok := new.(map[string]interface{}); ok { + id := n["id"].(string) + newIDs[id] = n } } - return -} -func containsAssignment(assignments []*sdk.ApplicationGroupAssignment, assignment *sdk.ApplicationGroupAssignment) bool { - for i := range assignments { - if assignments[i].Id == assignment.Id { - return true + // delete + for id := range oldIDs { + if newIDs[id] == nil { + // only id is needed + toRemove = append(toRemove, &sdk.ApplicationGroupAssignment{ + Id: id, + }) } } - return false -} -func containsEqualAssignment(assignments []*sdk.ApplicationGroupAssignment, assignment *sdk.ApplicationGroupAssignment) bool { - for i := range assignments { - if assignments[i].Id == assignment.Id && reflect.DeepEqual(assignments[i].Profile, assignment.Profile) { - if assignments[i].PriorityPtr != nil && assignment.PriorityPtr != nil { - return reflect.DeepEqual(*assignments[i].PriorityPtr, *assignment.PriorityPtr) + // anything in the new state treat as an assign even though it might already + // exist and might be unchanged + for id, group := range newIDs { + a := group.(map[string]interface{}) + assignment := &sdk.ApplicationGroupAssignment{ + Id: id, + } + if profile, ok := a["profile"]; ok { + var p interface{} + if err = json.Unmarshal([]byte(profile.(string)), &p); err == nil { + assignment.Profile = p } - return true + err = nil // need to reset err as it is a named return value } + if priority, ok := a["priority"]; ok { + assignment.PriorityPtr = int64Ptr(priority.(int)) + } + toAssign = append(toAssign, assignment) } - return false + + return } func groupAssignmentToTFGroup(assignment *sdk.ApplicationGroupAssignment) map[string]interface{} { diff --git a/okta/resource_okta_app_group_assignments_test.go b/okta/resource_okta_app_group_assignments_test.go index 3e3bd829e..e1685a0fd 100644 --- a/okta/resource_okta_app_group_assignments_test.go +++ b/okta/resource_okta_app_group_assignments_test.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/okta/terraform-provider-okta/sdk" ) func TestAccResourceOktaAppGroupAssignments_crud(t *testing.T) { @@ -57,6 +58,185 @@ func TestAccResourceOktaAppGroupAssignments_crud(t *testing.T) { }) } +// TestAccResourceOktaAppGroupAssignments_1088_unplanned_changes This test +// demonstrates incorrect behavior in okta_app_group_assignments has been +// corrected. The original author implemented incorrect behavior, in terms of +// idiomatic design principles of TF providers, where it would proactively +// remove group assignments from an app if they were made outside of the +// resource. The correct behavior is to surface drift detection if a group is +// assigned to an app outside of this resource. +func TestAccResourceOktaAppGroupAssignments_1088_unplanned_changes(t *testing.T) { + mgr := newFixtureManager("resources", appGroupAssignments, t.Name()) + assignments1 := fmt.Sprintf("%s.test", appGroupAssignments) + bookmarkApp := fmt.Sprintf("%s.test", appBookmark) + groupA := fmt.Sprintf("%s.a", group) + groupB := fmt.Sprintf("%s.b", group) + groupC := fmt.Sprintf("%s.c", group) + + baseConfig := ` +resource "okta_app_bookmark" "test" { + label = "testAcc_replace_with_uuid" + url = "https://test.com" +} +resource "okta_group" "a" { + name = "testAcc-group-a_replace_with_uuid" + description = "Group A" +} +resource "okta_group" "b" { + name = "testAcc-group-b_replace_with_uuid" + description = "Group B" +} +resource "okta_group" "c" { + name = "testAcc-group-c_replace_with_uuid" + description = "Group C" +}` + + step1Config := ` +resource "okta_app_group_assignments" "test" { + app_id = okta_app_bookmark.test.id + group { + id = okta_group.a.id + priority = 1 + profile = jsonencode({"test": "a"}) + } +}` + + step2Config := ` +resource "okta_app_group_assignments" "test" { + app_id = okta_app_bookmark.test.id + group { + id = okta_group.a.id + priority = 1 + profile = jsonencode({"test": "a"}) + } + group { + id = okta_group.b.id + priority = 2 + profile = jsonencode({"test": "b"}) + } +}` + + step3Config := ` +resource "okta_app_group_assignments" "test" { + app_id = okta_app_bookmark.test.id + group { + id = okta_group.a.id + priority = 1 + profile = jsonencode({"test": "a"}) + } + group { + id = okta_group.b.id + priority = 2 + profile = jsonencode({"test": "b"}) + } + group { + id = okta_group.c.id + priority = 4 + profile = jsonencode({"test": "c"}) + } +}` + + stepLastConfig := ` +resource "okta_app_group_assignments" "test" { + app_id = okta_app_bookmark.test.id + group { + id = okta_group.a.id + priority = 99 + profile = jsonencode({"different": "profile value"}) + } +}` + + oktaResourceTest(t, resource.TestCase{ + PreCheck: testAccPreCheck(t), + ErrorCheck: testAccErrorChecks(t), + ProviderFactories: testAccProvidersFactories, + Steps: []resource.TestStep{ + { + // Vanilla step + Config: mgr.ConfigReplace(fmt.Sprintf("%s\n%s", baseConfig, step1Config)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(assignments1, "group.#", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.priority", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.profile", `{"test":"a"}`), + ensureAppGroupAssignmentsExist(assignments1, groupA), + ), + }, + { + Config: mgr.ConfigReplace(fmt.Sprintf("%s\n%s", baseConfig, step2Config)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(assignments1, "group.#", "2"), + resource.TestCheckResourceAttr(assignments1, "group.0.priority", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.profile", `{"test":"a"}`), + resource.TestCheckResourceAttr(assignments1, "group.1.priority", "2"), + resource.TestCheckResourceAttr(assignments1, "group.1.profile", `{"test":"b"}`), + ensureAppGroupAssignmentsExist(assignments1, groupA, groupB), + + // This mimics assigning Group C to the app outside of + // Terraform. In this case doing so with a direct API call + // via the test harness which is equivalent to "Click Ops" + clickOpsAssignGroupToApp(bookmarkApp, groupC), + clickOpsCheckIfGroupIsAssignedToApp(bookmarkApp, groupA, groupB, groupC), + + // NOTE: after these checks run the terraform test runner + // will do a refresh and catch that group C has been added + // to the app outside of the terraform config and emit a + // non-empty plan + ), + + // side effect of the TF test runner is expecting a non-empty + // plan is treated as an apply accept and adds group c to local + // state + ExpectNonEmptyPlan: true, + }, + { + Config: mgr.ConfigReplace(fmt.Sprintf("%s\n%s", baseConfig, step3Config)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(assignments1, "group.#", "3"), + resource.TestCheckResourceAttr(assignments1, "group.0.priority", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.profile", `{"test":"a"}`), + resource.TestCheckResourceAttr(assignments1, "group.1.priority", "2"), + resource.TestCheckResourceAttr(assignments1, "group.1.profile", `{"test":"b"}`), + resource.TestCheckResourceAttr(assignments1, "group.2.priority", "4"), + resource.TestCheckResourceAttr(assignments1, "group.2.profile", `{"test":"c"}`), + ensureAppGroupAssignmentsExist(assignments1, groupA, groupB, groupC), + ), + }, + { + // check that we can do removing group assignments + Config: mgr.ConfigReplace(fmt.Sprintf("%s\n%s", baseConfig, step2Config)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(assignments1, "group.#", "2"), + resource.TestCheckResourceAttr(assignments1, "group.0.priority", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.profile", `{"test":"a"}`), + resource.TestCheckResourceAttr(assignments1, "group.1.priority", "2"), + resource.TestCheckResourceAttr(assignments1, "group.1.profile", `{"test":"b"}`), + ensureAppGroupAssignmentsExist(assignments1, groupA, groupB), + ), + }, + { + Config: mgr.ConfigReplace(fmt.Sprintf("%s\n%s", baseConfig, step1Config)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(assignments1, "group.#", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.priority", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.profile", `{"test":"a"}`), + ensureAppGroupAssignmentsExist(assignments1, groupA), + ), + }, + { + // Check that priority and profile can be changed on the group + // itself + Config: mgr.ConfigReplace(fmt.Sprintf("%s\n%s", baseConfig, stepLastConfig)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(assignments1, "group.#", "1"), + resource.TestCheckResourceAttr(assignments1, "group.0.priority", "99"), + resource.TestCheckResourceAttr(assignments1, "group.0.profile", `{"different":"profile value"}`), + ensureAppGroupAssignmentsExist(assignments1, groupA), + ), + }, + }, + }) +} + func ensureAppGroupAssignmentsExist(resourceName string, groupsExpected ...string) resource.TestCheckFunc { return func(s *terraform.State) error { missingErr := fmt.Errorf("resource not found: %s", resourceName) @@ -104,3 +284,53 @@ func ensureAppGroupAssignmentsExist(resourceName string, groupsExpected ...strin return nil } } + +func clickOpsAssignGroupToApp(appResourceName, groupResourceName string) resource.TestCheckFunc { + return func(s *terraform.State) error { + resources := []string{appResourceName, groupResourceName} + for _, resourceName := range resources { + missingErr := fmt.Errorf("resource not found: %s", resourceName) + if _, ok := s.RootModule().Resources[resourceName]; !ok { + return missingErr + } + } + + appRS := s.RootModule().Resources[appResourceName] + appID := appRS.Primary.Attributes["id"] + groupRS := s.RootModule().Resources[groupResourceName] + groupID := groupRS.Primary.Attributes["id"] + client := sdkV2ClientForTest() + _, _, err := client.Application.CreateApplicationGroupAssignment(context.Background(), appID, groupID, sdk.ApplicationGroupAssignment{}) + if err != nil { + return fmt.Errorf("API: unable to assign app %q to group %q, err: %+v", appID, groupID, err) + } + + return nil + } +} + +func clickOpsCheckIfGroupIsAssignedToApp(appResourceName string, groups ...string) resource.TestCheckFunc { + return func(s *terraform.State) error { + for _, groupResourceName := range groups { + resources := []string{appResourceName, groupResourceName} + for _, resourceName := range resources { + missingErr := fmt.Errorf("resource not found: %s", resourceName) + if _, ok := s.RootModule().Resources[resourceName]; !ok { + return missingErr + } + } + + appRS := s.RootModule().Resources[appResourceName] + appID := appRS.Primary.Attributes["id"] + groupRS := s.RootModule().Resources[groupResourceName] + groupID := groupRS.Primary.Attributes["id"] + client := sdkV2ClientForTest() + _, _, err := client.Application.GetApplicationGroupAssignment(context.Background(), appID, groupID, nil) + if err != nil { + return fmt.Errorf("API: app %q is not assigned to group %s", appID, groupID) + } + } + + return nil + } +} diff --git a/okta/resource_okta_app_oauth_role_assignment.go b/okta/resource_okta_app_oauth_role_assignment.go index 7dc47ac0f..940d0fa80 100644 --- a/okta/resource_okta_app_oauth_role_assignment.go +++ b/okta/resource_okta_app_oauth_role_assignment.go @@ -14,8 +14,10 @@ import ( "github.com/okta/terraform-provider-okta/sdk" ) -var _ resource.ResourceWithValidateConfig = &appOAuthRoleAssignmentResource{} -var _ resource.ResourceWithImportState = &appOAuthRoleAssignmentResource{} +var ( + _ resource.ResourceWithValidateConfig = &appOAuthRoleAssignmentResource{} + _ resource.ResourceWithImportState = &appOAuthRoleAssignmentResource{} +) func NewAppOAuthRoleAssignmentResource() resource.Resource { return &appOAuthRoleAssignmentResource{} diff --git a/website/docs/r/app_group_assignments.html.markdown b/website/docs/r/app_group_assignments.html.markdown index 33bf1a74d..d5e2bd5f2 100644 --- a/website/docs/r/app_group_assignments.html.markdown +++ b/website/docs/r/app_group_assignments.html.markdown @@ -30,17 +30,6 @@ resource "okta_app_group_assignments" "example" { ``` -!> **NOTE** It would seem that setting/updating base/custom group schema values -was the original purpose for setting a `profile` JSON value during the [Assign -group to -application](https://developer.okta.com/docs/reference/api/apps/#assign-group-to-application) -API call that will take place when the `priority` value is changed. We couldn't -verify this works when writing a new integration test against this old feature -and were receiving an API 400 error. This feature may work for older orgs, or -classic orgs, but we can not guarantee for all orgs. - -~> **IMPORTANT:** When using `okta_app_group_assignments` it is expected to manage ALL group assignments for the target application. - ## Argument Reference The following arguments are supported: