From f33cdd9228f9b9c8ebdbad4a542d5d09257cebe8 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Wed, 10 Apr 2024 09:53:54 -0700 Subject: [PATCH 1/2] Explicitly set state value for 'password' attribute This ensure its value is stored and re-used during Update. Fix incorrect map merging which leads to 'password' schema attribute not being overridden correctly. --- .../user/datasource_artifactory_user_test.go | 2 +- .../resource_artifactory_access_token_test.go | 20 ++++++------ ...urce_artifactory_permission_target_test.go | 6 ++-- .../user/resource_artifactory_managed_user.go | 4 +-- .../resource_artifactory_managed_user_test.go | 15 +++++---- .../resource_artifactory_unmanaged_user.go | 1 + ...esource_artifactory_unmanaged_user_test.go | 21 +++++++++--- .../user/resource_artifactory_user.go | 10 ++++-- .../user/resource_artifactory_user_test.go | 32 +++++++++++++++---- pkg/artifactory/resource/user/user.go | 7 ++++ pkg/artifactory/resource/user/user_fw.go | 12 +++---- sample.tf | 2 +- 12 files changed, 88 insertions(+), 44 deletions(-) diff --git a/pkg/artifactory/datasource/user/datasource_artifactory_user_test.go b/pkg/artifactory/datasource/user/datasource_artifactory_user_test.go index b0c94cc79..ce77d2553 100644 --- a/pkg/artifactory/datasource/user/datasource_artifactory_user_test.go +++ b/pkg/artifactory/datasource/user/datasource_artifactory_user_test.go @@ -18,7 +18,7 @@ func TestAccDataSourceUser_basic(t *testing.T) { temp := ` resource "artifactory_managed_user" "{{ .name }}" { name = "{{ .name }}" - password = "Password1!" + password = "Passw0rd!123" email = "{{ .email }}" groups = ["readers"] admin = false diff --git a/pkg/artifactory/resource/security/resource_artifactory_access_token_test.go b/pkg/artifactory/resource/security/resource_artifactory_access_token_test.go index 8d6324fe7..45231e737 100644 --- a/pkg/artifactory/resource/security/resource_artifactory_access_token_test.go +++ b/pkg/artifactory/resource/security/resource_artifactory_access_token_test.go @@ -21,7 +21,7 @@ func TestAccAccessTokenAudienceBad(t *testing.T) { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -52,7 +52,7 @@ func TestAccAccessTokenAudienceGood(t *testing.T) { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -95,7 +95,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -142,7 +142,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -185,7 +185,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -216,7 +216,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -249,7 +249,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -354,7 +354,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -386,7 +386,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { @@ -433,7 +433,7 @@ resource "artifactory_managed_user" "existinguser" { email = "existinguser@a.com" admin = false groups = ["readers"] - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_access_token" "foobar" { diff --git a/pkg/artifactory/resource/security/resource_artifactory_permission_target_test.go b/pkg/artifactory/resource/security/resource_artifactory_permission_target_test.go index 1d8b4fcc9..aa17f963a 100644 --- a/pkg/artifactory/resource/security/resource_artifactory_permission_target_test.go +++ b/pkg/artifactory/resource/security/resource_artifactory_permission_target_test.go @@ -82,7 +82,7 @@ const permissionFull = ` resource "artifactory_managed_user" "test-user" { name = "terraform" email = "test-user@artifactory-terraform.com" - password = "Passsw0rd!" + password = "Passsw0rd!12" } resource "artifactory_permission_target" "{{ .permission_name }}" { @@ -250,7 +250,7 @@ func TestAccPermissionTarget_GitHubIssue126(t *testing.T) { admin = false disable_ui_access = true internal_password_disabled = false - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_permission_target" "{{ .perm_name }}" { @@ -500,7 +500,7 @@ func TestAccPermissionTarget_MissingRepositories(t *testing.T) { admin = false disable_ui_access = true internal_password_disabled = true - password = "Passw0rd!" + password = "Passw0rd!123" } resource "artifactory_permission_target" "{{ .perm_name }}" { diff --git a/pkg/artifactory/resource/user/resource_artifactory_managed_user.go b/pkg/artifactory/resource/user/resource_artifactory_managed_user.go index 16619a051..19b1d8f87 100644 --- a/pkg/artifactory/resource/user/resource_artifactory_managed_user.go +++ b/pkg/artifactory/resource/user/resource_artifactory_managed_user.go @@ -7,7 +7,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/schema/validator" - "golang.org/x/exp/maps" + "github.com/samber/lo" ) func NewManagedUserResource() resource.Resource { @@ -32,7 +32,7 @@ func (r *ArtifactoryManagedUserResource) Schema(ctx context.Context, req resourc }, } - maps.Copy(managedUserSchemaFramework, baseUserSchemaFramework) + managedUserSchemaFramework = lo.Assign(baseUserSchemaFramework, managedUserSchemaFramework) resp.Schema = schema.Schema{ MarkdownDescription: "Provides an Artifactory managed user resource. This can be used to create and manage Artifactory users. For example, service account where password is known and managed externally.", diff --git a/pkg/artifactory/resource/user/resource_artifactory_managed_user_test.go b/pkg/artifactory/resource/user/resource_artifactory_managed_user_test.go index 1e74c2329..a59888f04 100644 --- a/pkg/artifactory/resource/user/resource_artifactory_managed_user_test.go +++ b/pkg/artifactory/resource/user/resource_artifactory_managed_user_test.go @@ -25,7 +25,7 @@ func TestAccManagedUser_UpgradeFromSDKv2(t *testing.T) { resource "artifactory_managed_user" "{{ .name }}" { name = "{{ .name }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" } `, params) @@ -64,7 +64,7 @@ func TestAccManagedUser_no_groups(t *testing.T) { resource "artifactory_managed_user" "%s" { name = "%s" email = "dummy%d@a.com" - password = "Passsw0rd!" + password = "Passsw0rd!12" } ` id, fqrn, name := testutil.MkNames("foobar-", "artifactory_managed_user") @@ -97,7 +97,7 @@ func TestAccManagedUser_empty_groups(t *testing.T) { resource "artifactory_managed_user" "%s" { name = "%s" email = "dummy%d@a.com" - password = "Passsw0rd!" + password = "Passsw0rd!12" groups = [] } ` @@ -138,16 +138,17 @@ func TestAccManagedUser_invalidName(t *testing.T) { } for _, tc := range testCase { - t.Run(tc.name, testAccManagedUserInvalidName(t, tc.username, tc.errorRegex)) + t.Run(tc.name, testAccManagedUserInvalidName(tc.username, tc.errorRegex)) } } -func testAccManagedUserInvalidName(t *testing.T, username, errorRegex string) func(t *testing.T) { +func testAccManagedUserInvalidName(username, errorRegex string) func(t *testing.T) { return func(t *testing.T) { const userNoGroups = ` resource "artifactory_managed_user" "%s" { name = "%s" email = "dummy%d@a.com" + password = "Passsw0rd!12" } ` id, fqrn, name := testutil.MkNames("test-", "artifactory_managed_user") @@ -187,7 +188,7 @@ func TestAccManagedUser_basic(t *testing.T) { resource "artifactory_managed_user" "{{ .name }}" { name = "{{ .username }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" admin = true profile_updatable = true disable_ui_access = false @@ -205,7 +206,7 @@ func TestAccManagedUser_basic(t *testing.T) { resource "artifactory_managed_user" "{{ .name }}" { name = "{{ .username }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" admin = false profile_updatable = false groups = [ diff --git a/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user.go b/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user.go index 9c49ee813..ca08f547a 100644 --- a/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user.go +++ b/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user.go @@ -15,6 +15,7 @@ func ResourceArtifactoryUser() *schema.Resource { Type: schema.TypeString, Sensitive: true, Optional: true, + Computed: true, Description: "(Optional, Sensitive) Password for the user. When omitted, a random password is generated using the following password policy: " + "12 characters with 1 digit, 1 symbol, with upper and lower case letters", }, diff --git a/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user_test.go b/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user_test.go index aed4584e4..a5ffc6fab 100644 --- a/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user_test.go +++ b/pkg/artifactory/resource/user/resource_artifactory_unmanaged_user_test.go @@ -118,10 +118,17 @@ func TestAccUnmanagedUser_basic(t *testing.T) { } func TestAccUnmanagedUserShouldCreateWithoutPassword(t *testing.T) { - const userBasic = ` + const config = ` + resource "artifactory_unmanaged_user" "%s" { + name = "%s" + email = "dummy_user%d@a.com" + } + ` + const updatedConfig = ` resource "artifactory_unmanaged_user" "%s" { name = "%s" email = "dummy_user%d@a.com" + profile_updatable = false } ` id := testutil.RandomInt() @@ -134,13 +141,19 @@ func TestAccUnmanagedUserShouldCreateWithoutPassword(t *testing.T) { CheckDestroy: testAccCheckUserDestroy(fqrn), Steps: []resource.TestStep{ { - Config: fmt.Sprintf(userBasic, name, username, id), + Config: fmt.Sprintf(config, name, username, id), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(fqrn, "name", username), resource.TestCheckResourceAttr(fqrn, "email", fmt.Sprintf("dummy_user%d@a.com", id)), resource.TestCheckNoResourceAttr(fqrn, "groups"), ), }, + { + Config: fmt.Sprintf(updatedConfig, name, username, id), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(fqrn, "profile_updatable", "false"), + ), + }, { ResourceName: fqrn, ImportState: true, @@ -283,11 +296,11 @@ func TestAccUnmanagedUser_invalidName(t *testing.T) { } for _, tc := range testCase { - t.Run(tc.name, testAccUnmanagedUserInvalidName(t, tc.username, tc.errorRegex)) + t.Run(tc.name, testAccUnmanagedUserInvalidName(tc.username, tc.errorRegex)) } } -func testAccUnmanagedUserInvalidName(t *testing.T, username, errorRegex string) func(t *testing.T) { +func testAccUnmanagedUserInvalidName(username, errorRegex string) func(t *testing.T) { return func(t *testing.T) { const userNoGroups = ` resource "artifactory_unmanaged_user" "%s" { diff --git a/pkg/artifactory/resource/user/resource_artifactory_user.go b/pkg/artifactory/resource/user/resource_artifactory_user.go index f120df240..8946803ba 100644 --- a/pkg/artifactory/resource/user/resource_artifactory_user.go +++ b/pkg/artifactory/resource/user/resource_artifactory_user.go @@ -5,7 +5,9 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" - "golang.org/x/exp/maps" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" + "github.com/samber/lo" ) func NewUserResource() resource.Resource { @@ -27,10 +29,14 @@ func (r *ArtifactoryUserResource) Schema(ctx context.Context, req resource.Schem "12 characters with 1 digit, 1 symbol, with upper and lower case letters", Optional: true, Sensitive: true, + Computed: true, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.UseStateForUnknown(), + }, }, } - maps.Copy(userSchemaFramework, baseUserSchemaFramework) + userSchemaFramework = lo.Assign(baseUserSchemaFramework, userSchemaFramework) resp.Schema = schema.Schema{ MarkdownDescription: "Provides an Artifactory user resource. This can be used to create and manage Artifactory users. The password is a required field by the [Artifactory API](https://www.jfrog.com/confluence/display/JFROG/Artifactory+REST+API#ArtifactoryRESTAPI-CreateorReplaceUser), but we made it optional in this resource to accommodate the scenario where the password is not needed and will be reset by the actual user later. When the optional attribute `password` is omitted, a random password is generated according to current Artifactory password policy.", diff --git a/pkg/artifactory/resource/user/resource_artifactory_user_test.go b/pkg/artifactory/resource/user/resource_artifactory_user_test.go index cc7529673..582605f91 100644 --- a/pkg/artifactory/resource/user/resource_artifactory_user_test.go +++ b/pkg/artifactory/resource/user/resource_artifactory_user_test.go @@ -28,7 +28,7 @@ func TestAccUser_UpgradeFromSDKv2(t *testing.T) { resource "artifactory_user" "{{ .name }}" { name = "{{ .name }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" } `, params) @@ -81,7 +81,7 @@ func TestAccUser_full_groups(t *testing.T) { resource "artifactory_user" "{{ .name }}" { name = "{{ .name }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" admin = false groups = [ artifactory_group.{{ .groupName1 }}.name, @@ -101,7 +101,7 @@ func TestAccUser_full_groups(t *testing.T) { resource "artifactory_user" "{{ .name }}" { name = "{{ .name }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" admin = false groups = [ artifactory_group.{{ .groupName1 }}.name, @@ -161,11 +161,21 @@ func TestAccUser_no_password(t *testing.T) { "username": username, "email": email, } - userNoGroups := util.ExecuteTemplate("TestAccUserBasic", ` + config := util.ExecuteTemplate("TestAccUserBasic", ` + resource "artifactory_user" "{{ .name }}" { + name = "{{ .name }}" + email = "{{ .email }}" + admin = false + groups = [ "readers" ] + } + `, params) + + updatedConfig := util.ExecuteTemplate("TestAccUserBasic", ` resource "artifactory_user" "{{ .name }}" { name = "{{ .name }}" email = "{{ .email }}" admin = false + profile_updatable = false groups = [ "readers" ] } `, params) @@ -176,9 +186,17 @@ func TestAccUser_no_password(t *testing.T) { CheckDestroy: testAccCheckManagedUserDestroy(fqrn), Steps: []resource.TestStep{ { - Config: userNoGroups, + Config: config, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(fqrn, "name", fmt.Sprintf("foobar-%d", id)), + resource.TestCheckResourceAttr(fqrn, "groups.#", "1"), + ), + }, + { + Config: updatedConfig, Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(fqrn, "name", fmt.Sprintf("foobar-%d", id)), + resource.TestCheckResourceAttr(fqrn, "profile_updatable", "false"), resource.TestCheckResourceAttr(fqrn, "groups.#", "1"), ), }, @@ -207,7 +225,7 @@ func TestAccUser_no_groups(t *testing.T) { resource "artifactory_user" "{{ .name }}" { name = "{{ .name }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" admin = false } `, params) @@ -249,7 +267,7 @@ func TestAccUser_empty_groups(t *testing.T) { resource "artifactory_user" "{{ .name }}" { name = "{{ .name }}" email = "{{ .email }}" - password = "Passsw0rd!" + password = "Passsw0rd!12" admin = false groups = [] } diff --git a/pkg/artifactory/resource/user/user.go b/pkg/artifactory/resource/user/user.go index a1af2974d..e5ecaae9b 100644 --- a/pkg/artifactory/resource/user/user.go +++ b/pkg/artifactory/resource/user/user.go @@ -386,6 +386,13 @@ func resourceBaseUserCreate(ctx context.Context, d *schema.ResourceData, m inter if passwordGenerator != nil && !user.InternalPasswordDisabled { diags = passwordGenerator(&user) + if diags.HasError() { + return diags + } + + // explicity set this attribute with password so it gets stored in the state + // and allows update to work + d.Set("password", user.Password) } var result User diff --git a/pkg/artifactory/resource/user/user_fw.go b/pkg/artifactory/resource/user/user_fw.go index 7586d0233..cf3d9d3f4 100644 --- a/pkg/artifactory/resource/user/user_fw.go +++ b/pkg/artifactory/resource/user/user_fw.go @@ -101,12 +101,6 @@ var baseUserSchemaFramework = map[string]schema.Attribute{ MarkdownDescription: "Email for user.", Required: true, }, - "password": schema.StringAttribute{ - MarkdownDescription: "(Optional, Sensitive) Password for the user. When omitted, a random password is generated using the following password policy: " + - "12 characters with 1 digit, 1 symbol, with upper and lower case letters", - Optional: true, - Sensitive: true, - }, "admin": schema.BoolAttribute{ MarkdownDescription: "(Optional, Default: false) When enabled, this user is an administrator with all the ensuing privileges.", Optional: true, @@ -204,7 +198,7 @@ func (r *ArtifactoryBaseUserResource) syncReadersGroup(ctx context.Context, clie return nil } -func (r *ArtifactoryBaseUserResource) createUser(ctx context.Context, req *resty.Request, artifactoryVersion string, user ArtifactoryUserResourceAPIModel, result *ArtifactoryUserResourceAPIModel, artifactoryError *artifactory.ArtifactoryErrorsResponse) (*resty.Response, error) { +func (r *ArtifactoryBaseUserResource) createUser(_ context.Context, req *resty.Request, artifactoryVersion string, user ArtifactoryUserResourceAPIModel, result *ArtifactoryUserResourceAPIModel, artifactoryError *artifactory.ArtifactoryErrorsResponse) (*resty.Response, error) { // 7.49.3 or later, use Access API if ok, err := util.CheckVersion(artifactoryVersion, AccessAPIArtifactoryVersion); err == nil && ok { return req. @@ -393,6 +387,10 @@ func (r *ArtifactoryBaseUserResource) Create(ctx context.Context, req resource.C } user.Password = randomPassword + + // explicity set this attribute with password so it gets stored in the state + // and allows update to work + plan.Password = types.StringValue(randomPassword) } var result ArtifactoryUserResourceAPIModel diff --git a/sample.tf b/sample.tf index cc424de56..fe7619690 100644 --- a/sample.tf +++ b/sample.tf @@ -2,7 +2,7 @@ terraform { required_providers { artifactory = { source = "jfrog/artifactory" - version = "10.1.4" + version = "10.5.1" } } } From 0ed6f37fc43e5ee9b5e50f1a42920f6ee867e295 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Wed, 10 Apr 2024 11:00:42 -0700 Subject: [PATCH 2/2] Update CHANGELOG --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53b920694..dcb9debfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 10.5.1 (Apr 10, 2024) + +BUG FIXES: + +* resource/artifactory_user, resource/artifactory_managed_user: Fix error when updating resource without `password` attribute defined (i.e. relying on auto generated password). Issue: [#931](https://github.com/jfrog/terraform-provider-artifactory/issues/931) PR: [#932](https://github.com/jfrog/terraform-provider-artifactory/pull/932) + ## 10.5.0 (Apr 9, 2024) FEATURES: