From 341f774e11faafddbab06e2c4fe84db3886bbe2d Mon Sep 17 00:00:00 2001 From: Tom Goodsell <56050072+tgoodsell-tempus@users.noreply.github.com> Date: Sat, 16 Dec 2023 00:37:58 -0600 Subject: [PATCH 1/5] feat: re-add changes to omit_secret post git branch fixes --- okta/resource_okta_app_oauth.go | 47 ++++++++++++++++++++------ okta/resource_okta_app_oauth_test.go | 32 ++++++++++++++++++ website/docs/r/app_oauth.html.markdown | 6 ++-- 3 files changed, 72 insertions(+), 13 deletions(-) diff --git a/okta/resource_okta_app_oauth.go b/okta/resource_okta_app_oauth.go index 496c7ed57..16a04af36 100644 --- a/okta/resource_okta_app_oauth.go +++ b/okta/resource_okta_app_oauth.go @@ -136,20 +136,20 @@ func resourceAppOAuth() *schema.Resource { Type: schema.TypeBool, Optional: true, // No ForceNew to avoid recreating when going from false => true - Description: "This tells the provider not to persist the application's secret to state. If this is ever changes from true => false your app will be recreated.", + Description: "This tells the provider not manage the client_secret value in state. When this is false (the default), it will cause the auto-generated client_secret to be persisted in the client_secret attribute in state. This also means that every time an update to this app is run, this value is also set on the API. If this changes from false => true, the `client_secret` is dropped from state and the secret at the time of the apply is what remains. If this is ever changes from true => false your app will be recreated, due to the need to regenerate a secret we can store in state.", Default: false, }, "client_secret": { Type: schema.TypeString, Computed: true, Sensitive: true, - Description: "OAuth client secret key. This will be in plain text in your statefile unless you set omit_secret above.", + Description: "OAuth client secret value, this is output only. This will be in plain text in your statefile unless you set omit_secret above.", }, "client_basic_secret": { Type: schema.TypeString, Optional: true, Sensitive: true, - Description: "OAuth client secret key, this can be set when token_endpoint_auth_method is client_secret_basic.", + Description: "The user provided OAuth client secret key value, this can be set when token_endpoint_auth_method is client_secret_basic. This does nothing when `omit_secret is set to true.", }, "token_endpoint_auth_method": { Type: schema.TypeString, @@ -419,10 +419,14 @@ func resourceAppOAuthCreate(ctx context.Context, d *schema.ResourceData, m inter app := buildAppOAuth(d) activate := d.Get("status").(string) == statusActive params := &query.Params{Activate: &activate} - _, _, err := client.Application.CreateApplication(ctx, app, params) + appResp, _, err := client.Application.CreateApplication(ctx, app, params) if err != nil { return diag.Errorf("failed to create OAuth application: %v", err) } + app, err = verifyOidcAppType(appResp) + if err != nil { + return diag.FromErr(err) + } d.SetId(app.Id) if !d.Get("omit_secret").(bool) { _ = d.Set("client_secret", app.Credentials.OauthClient.ClientSecret) @@ -534,10 +538,6 @@ func resourceAppOAuthRead(ctx context.Context, d *schema.ResourceData, m interfa } else { _ = d.Set("implicit_assignment", false) } - // If this is ever changed omit it. - if d.Get("omit_secret").(bool) { - _ = d.Set("client_secret", "") - } c := m.(*Config) if c.IsOAuth20Auth() { @@ -669,11 +669,30 @@ func resourceAppOAuthUpdate(ctx context.Context, d *schema.ResourceData, m inter return diag.Errorf("failed to create OAuth application: %v", err) } app := buildAppOAuth(d) - _, _, err = client.Application.UpdateApplication(ctx, d.Id(), app) + // When omit_secret is true on update, we make sure that we empty the client secret value + // in the api call. + // This is to ensure that when this is "toggled on", the apply which this occurs also does + // not do a final "reset" of the client secret value to the original stored in state. + if d.Get("omit_secret").(bool) { + app.Credentials.OauthClient.ClientSecret = "" + } + appResp, _, err := client.Application.UpdateApplication(ctx, d.Id(), app) if err != nil { return diag.Errorf("failed to update OAuth application: %v", err) } - if !d.Get("omit_secret").(bool) { + app, err = verifyOidcAppType(appResp) + if err != nil { + return diag.FromErr(err) + } + + // The `client_secret` value is always returned from the API when it set on update + // Regardless if we pass a value or not. + // We need to make sure that we set the value in state based upon the `omit_secret` behavior + // When `true`: We blank out the secret value + // When `false`: We set the secret value to the value returned from the API + if d.Get("omit_secret").(bool) { + _ = d.Set("client_secret", "") + } else { _ = d.Set("client_secret", app.Credentials.OauthClient.ClientSecret) } if d.HasChange("logo") { @@ -931,3 +950,11 @@ func validateAppOAuth(d *schema.ResourceData, m interface{}) error { } return nil } + +func verifyOidcAppType(app sdk.App) (*sdk.OpenIdConnectApplication, error) { + oidcApp, ok := app.(*sdk.OpenIdConnectApplication) + if !ok { + return nil, fmt.Errorf("unexpected application response return from Okta: %v", app) + } + return oidcApp, nil +} diff --git a/okta/resource_okta_app_oauth_test.go b/okta/resource_okta_app_oauth_test.go index c287a99a2..23869da2c 100644 --- a/okta/resource_okta_app_oauth_test.go +++ b/okta/resource_okta_app_oauth_test.go @@ -865,3 +865,35 @@ func TestAccResourceOktaAppOauth_config_combinations(t *testing.T) { }) } } + +func TestAccResourceOktaAppOauth_omitSecretSafeEnable(t *testing.T) { + mgr := newFixtureManager("resources", appOAuth, t.Name()) + omit_secret_off := mgr.GetFixtures("omit_secret_off.tf", t) + omit_secret_on := mgr.GetFixtures("omit_secret_on.tf", t) + resourceName := fmt.Sprintf("%s.test", appOAuth) + + oktaResourceTest(t, resource.TestCase{ + PreCheck: testAccPreCheck(t), + ErrorCheck: testAccErrorChecks(t), + ProviderFactories: testAccProvidersFactories, + CheckDestroy: checkResourceDestroy(appOAuth, createDoesAppExist(sdk.NewOpenIdConnectApplication())), + Steps: []resource.TestStep{ + { + Config: omit_secret_off, + Check: resource.ComposeTestCheckFunc( + ensureResourceExists(resourceName, createDoesAppExist(sdk.NewOpenIdConnectApplication())), + resource.TestCheckResourceAttrSet(resourceName, "client_id"), + resource.TestCheckResourceAttrSet(resourceName, "client_secret"), + ), + }, + { + Config: omit_secret_on, + Check: resource.ComposeTestCheckFunc( + ensureResourceExists(resourceName, createDoesAppExist(sdk.NewOpenIdConnectApplication())), + resource.TestCheckResourceAttrSet(resourceName, "client_id"), + resource.TestCheckResourceAttr(resourceName, "client_secret", ""), + ), + }, + }, + }) +} diff --git a/website/docs/r/app_oauth.html.markdown b/website/docs/r/app_oauth.html.markdown index 6d14d4b14..43af90695 100644 --- a/website/docs/r/app_oauth.html.markdown +++ b/website/docs/r/app_oauth.html.markdown @@ -80,7 +80,7 @@ The following arguments are supported: - `auto_submit_toolbar` - (Optional) Display auto submit toolbar. -- `client_basic_secret` - (Optional) OAuth client secret key, this can be set when `token_endpoint_auth_method` is `"client_secret_basic"`. +- `client_basic_secret` - (Optional) The user provided OAuth client secret key value, this can be set when `token_endpoint_auth_method` is `"client_secret_basic"`. This does nothing when `omit_secret` is set to true. - `client_id` - (Optional) OAuth client ID. If set during creation, app is created with this id. See: https://developer.okta.com/docs/reference/api/apps/#oauth-credential-object @@ -130,7 +130,7 @@ Valid values: `"CUSTOM_URL"`,`"ORG_URL"` or `"DYNAMIC"`. Default is `"ORG_URL"`. - `logo_uri` - (Optional) URI that references a logo for the client. -- `omit_secret` - (Optional) This tells the provider not to persist the application's secret to state. Your app's `client_secret` will be recreated if this ever changes from true => false. +- `omit_secret` - (Optional) This tells the provider not manage the `client_secret` value in state. When this is false (the default), it will cause the auto-generated `client_secret` to be persisted in the `client_secret` attribute in state. This also means that every time an update to this app is run, this value is also set on the API. If this changes from false => true, the `client_secret` is dropped from state and the secret at the time of the apply is what remains. If this is ever changes from true => false your app will be recreated, due to the need to regenerate a secret we can store in state. - `pkce_required` - (Optional) Require Proof Key for Code Exchange (PKCE) for additional verification. If `pkce_required` isn't specified when adding a new @@ -191,7 +191,7 @@ Valid values: `"CUSTOM_URL"`,`"ORG_URL"` or `"DYNAMIC"`. Default is `"ORG_URL"`. - `client_id` - The client ID of the application. -- `client_secret` - The client secret of the application. See: https://developer.okta.com/docs/reference/api/apps/#oauth-credential-object +- `client_secret` - OAuth client secret value, this is output only. This will be in plain text in your statefile unless you set omit_secret above. See: https://developer.okta.com/docs/reference/api/apps/#oauth-credential-object - `id` - ID of the application. From afc7db8603bbe1de33f9ad02153c66b972060e2b Mon Sep 17 00:00:00 2001 From: Tom Goodsell <56050072+tgoodsell-tempus@users.noreply.github.com> Date: Sat, 16 Dec 2023 00:43:33 -0600 Subject: [PATCH 2/5] docs: fix some of the comments --- okta/resource_okta_app_oauth.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/okta/resource_okta_app_oauth.go b/okta/resource_okta_app_oauth.go index 16a04af36..e4b939f42 100644 --- a/okta/resource_okta_app_oauth.go +++ b/okta/resource_okta_app_oauth.go @@ -669,8 +669,8 @@ func resourceAppOAuthUpdate(ctx context.Context, d *schema.ResourceData, m inter return diag.Errorf("failed to create OAuth application: %v", err) } app := buildAppOAuth(d) - // When omit_secret is true on update, we make sure that we empty the client secret value - // in the api call. + // When omit_secret is true on update, we make sure that do not include + // the client secret value in the api call. // This is to ensure that when this is "toggled on", the apply which this occurs also does // not do a final "reset" of the client secret value to the original stored in state. if d.Get("omit_secret").(bool) { @@ -685,8 +685,8 @@ func resourceAppOAuthUpdate(ctx context.Context, d *schema.ResourceData, m inter return diag.FromErr(err) } - // The `client_secret` value is always returned from the API when it set on update - // Regardless if we pass a value or not. + // The `client_secret` value is always returned from the API on update + // regardless if we pass a value or not. // We need to make sure that we set the value in state based upon the `omit_secret` behavior // When `true`: We blank out the secret value // When `false`: We set the secret value to the value returned from the API From edd0c74d7bdfe9ac67c2f00c57f03bd62b29c6ba Mon Sep 17 00:00:00 2001 From: Tom Goodsell <56050072+tgoodsell-tempus@users.noreply.github.com> Date: Sat, 16 Dec 2023 01:20:21 -0600 Subject: [PATCH 3/5] fix: add back omit secret test files --- examples/resources/okta_app_oauth/omit_secret_off.tf | 9 +++++++++ examples/resources/okta_app_oauth/omit_secret_on.tf | 10 ++++++++++ 2 files changed, 19 insertions(+) create mode 100644 examples/resources/okta_app_oauth/omit_secret_off.tf create mode 100644 examples/resources/okta_app_oauth/omit_secret_on.tf diff --git a/examples/resources/okta_app_oauth/omit_secret_off.tf b/examples/resources/okta_app_oauth/omit_secret_off.tf new file mode 100644 index 000000000..5576dc611 --- /dev/null +++ b/examples/resources/okta_app_oauth/omit_secret_off.tf @@ -0,0 +1,9 @@ +resource "okta_app_oauth" "test" { + label = "testAcc_replace_with_uuid" + type = "web" + grant_types = ["authorization_code"] + redirect_uris = ["http://d.com/"] + response_types = ["code"] + token_endpoint_auth_method = "client_secret_basic" + consent_method = "TRUSTED" +} diff --git a/examples/resources/okta_app_oauth/omit_secret_on.tf b/examples/resources/okta_app_oauth/omit_secret_on.tf new file mode 100644 index 000000000..910a59f33 --- /dev/null +++ b/examples/resources/okta_app_oauth/omit_secret_on.tf @@ -0,0 +1,10 @@ +resource "okta_app_oauth" "test" { + label = "testAcc_replace_with_uuid" + type = "web" + grant_types = ["authorization_code"] + redirect_uris = ["http://d.com/"] + response_types = ["code"] + token_endpoint_auth_method = "client_secret_basic" + consent_method = "TRUSTED" + omit_secret = true +} From 72d0c3db501671ea034abeb0bdbcd90203ca9581 Mon Sep 17 00:00:00 2001 From: Tien Nguyen Date: Wed, 31 Jan 2024 13:06:22 -0500 Subject: [PATCH 4/5] bring in tgoodsell-tempus work on #1852 From e779fbe2f98bd971227dab0b3e9a09d90147cc31 Mon Sep 17 00:00:00 2001 From: Tien Nguyen Date: Wed, 31 Jan 2024 13:17:14 -0500 Subject: [PATCH 5/5] update tf test file --- .../okta_app_oauth/federation_broker_off.tf | 18 +++++------------- .../okta_app_oauth/groups_and_users.tf | 18 +++++------------- okta/resource_okta_app_oauth.go | 2 +- 3 files changed, 11 insertions(+), 27 deletions(-) diff --git a/examples/resources/okta_app_oauth/federation_broker_off.tf b/examples/resources/okta_app_oauth/federation_broker_off.tf index 3e330a77c..045b4cc49 100644 --- a/examples/resources/okta_app_oauth/federation_broker_off.tf +++ b/examples/resources/okta_app_oauth/federation_broker_off.tf @@ -3,12 +3,11 @@ resource "okta_group" "group" { } resource "okta_user" "user" { - admin_roles = ["APP_ADMIN", "USER_ADMIN"] - first_name = "TestAcc" - last_name = "blah" - login = "test-acc-replace_with_uuid@example.com" - email = "test-acc-replace_with_uuid@example.com" - status = "ACTIVE" + first_name = "TestAcc" + last_name = "blah" + login = "test-acc-replace_with_uuid@example.com" + email = "test-acc-replace_with_uuid@example.com" + status = "ACTIVE" } resource "okta_app_oauth" "test" { @@ -21,11 +20,4 @@ resource "okta_app_oauth" "test" { response_types = ["code", "token", "id_token"] consent_method = "TRUSTED" implicit_assignment = false - - users { - id = okta_user.user.id - username = okta_user.user.email - } - - groups = [okta_group.group.id] } diff --git a/examples/resources/okta_app_oauth/groups_and_users.tf b/examples/resources/okta_app_oauth/groups_and_users.tf index 3ca52dcea..b4f11a092 100644 --- a/examples/resources/okta_app_oauth/groups_and_users.tf +++ b/examples/resources/okta_app_oauth/groups_and_users.tf @@ -3,12 +3,11 @@ resource "okta_group" "group" { } resource "okta_user" "user" { - admin_roles = ["APP_ADMIN", "USER_ADMIN"] - first_name = "TestAcc" - last_name = "blah" - login = "testAcc-replace_with_uuid@example.com" - email = "testAcc-replace_with_uuid@example.com" - status = "ACTIVE" + first_name = "TestAcc" + last_name = "blah" + login = "testAcc-replace_with_uuid@example.com" + email = "testAcc-replace_with_uuid@example.com" + status = "ACTIVE" } resource "okta_app_oauth" "test" { @@ -19,11 +18,4 @@ resource "okta_app_oauth" "test" { post_logout_redirect_uris = ["http://d.com/post"] login_uri = "http://test.com" response_types = ["code", "token", "id_token"] - - users { - id = okta_user.user.id - username = okta_user.user.email - } - - groups = [okta_group.group.id] } diff --git a/okta/resource_okta_app_oauth.go b/okta/resource_okta_app_oauth.go index cad5398fd..749ebeba3 100644 --- a/okta/resource_okta_app_oauth.go +++ b/okta/resource_okta_app_oauth.go @@ -664,7 +664,7 @@ func resourceAppOAuthUpdate(ctx context.Context, d *schema.ResourceData, m inter return diag.Errorf("failed to create OAuth application: %v", err) } - app := buildAppOAuth(d) + app := buildAppOAuth(d, false) // When omit_secret is true on update, we make sure that do not include // the client secret value in the api call. // This is to ensure that when this is "toggled on", the apply which this occurs also does