diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index c0925ce848..8bbbc7fa7c 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -160,6 +160,26 @@ The following set of [parameters](https://docs.snowflake.com/en/sql-reference/pa - [NETWORK_POLICY](https://docs.snowflake.com/en/sql-reference/parameters#network-policy) - [PREVENT_UNLOAD_TO_INTERNAL_STAGES](https://docs.snowflake.com/en/sql-reference/parameters#prevent-unload-to-internal-stages) +Connected issues: [#2938](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2938) + +### *(breaking change)* Changes in sensitiveness of name and login_name + +According to https://docs.snowflake.com/en/sql-reference/functions/all_user_names#usage-notes, `NAME`s are not considered sensitive data and `LOGIN_NAME`s are. Previous versions of the provider had this the other way around. In this version, `name` attribute was unmarked as sensitive, whereas `login_name` was marked as sensitive. This may break your configuration if you were using `login_name`s before e.g. in a `for_each` loop. + +Connected issues: [#2662](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2662), [#2668](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2668). + +### *(bugfix)* Correctly handle `default_warehouse`, `default_namespace`, and `default_role` + +During the [identifiers rework](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#identifiers-rework), we generalized how we compute the differences correctly for the identifier fields (read more in [this document](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md)). Proper suppressor was applied to `default_warehouse`, `default_namespace`, and `default_role`. Also, all these three attributes were corrected (e.g. handling spaces/hyphens in names). + +Connected issues: [#2836](https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/2836), [#2942](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2942) + +### *(bugfix)* Correctly handle failed update + +Not every attribute can be updated in the state during read (like `password` in the `snowflake_user` resource). In situations where update fails, we may end up with an incorrect state (read more in https://github.com/hashicorp/terraform-plugin-sdk/issues/476). We use a deprecated method from the plugin SDK, and now, for partially failed updates, we preserve the resource's previous state. It fixed this kind of situations for `snowflake_user` resource. + +Connected issues: [#2970](https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/2970) + ## v0.94.0 ➞ v0.94.1 ### changes in snowflake_schema diff --git a/docs/resources/user.md b/docs/resources/user.md index 639f61deed..37317c96cb 100644 --- a/docs/resources/user.md +++ b/docs/resources/user.md @@ -43,7 +43,7 @@ resource "snowflake_user" "user" { ### Required -- `name` (String, Sensitive) Name of the user. Note that if you do not supply login_name this will be used as login_name. [doc](https://docs.snowflake.net/manuals/sql-reference/sql/create-user.html#required-parameters) +- `name` (String) Name of the user. Note that if you do not supply login_name this will be used as login_name. [doc](https://docs.snowflake.net/manuals/sql-reference/sql/create-user.html#required-parameters) ### Optional @@ -83,7 +83,7 @@ resource "snowflake_user" "user" { - `last_name` (String, Sensitive) Last name of the user. - `lock_timeout` (Number) Number of seconds to wait while trying to lock a resource, before timing out and aborting the statement. For more information, check [LOCK_TIMEOUT docs](https://docs.snowflake.com/en/sql-reference/parameters#lock-timeout). - `log_level` (String) Specifies the severity level of messages that should be ingested and made available in the active event table. Messages at the specified level (and at more severe levels) are ingested. For more information about log levels, see [Setting log level](https://docs.snowflake.com/en/developer-guide/logging-tracing/logging-log-level). For more information, check [LOG_LEVEL docs](https://docs.snowflake.com/en/sql-reference/parameters#log-level). -- `login_name` (String) The name users use to log in. If not supplied, snowflake will use name instead. +- `login_name` (String, Sensitive) The name users use to log in. If not supplied, snowflake will use name instead. - `multi_statement_count` (Number) Number of statements to execute when using the multi-statement capability. For more information, check [MULTI_STATEMENT_COUNT docs](https://docs.snowflake.com/en/sql-reference/parameters#multi-statement-count). - `must_change_password` (Boolean) Specifies whether the user is forced to change their password on next login (including their first/initial login) into the system. - `network_policy` (String) Specifies the network policy to enforce for your account. Network policies enable restricting access to your account based on users’ IP address. For more details, see [Controlling network traffic with network policies](https://docs.snowflake.com/en/user-guide/network-policies). Any existing network policy (created using [CREATE NETWORK POLICY](https://docs.snowflake.com/en/sql-reference/sql/create-network-policy)). For more information, check [NETWORK_POLICY docs](https://docs.snowflake.com/en/sql-reference/parameters#network-policy). diff --git a/pkg/acceptance/bettertestspoc/README.md b/pkg/acceptance/bettertestspoc/README.md index ed10d8eda1..f43ce258b0 100644 --- a/pkg/acceptance/bettertestspoc/README.md +++ b/pkg/acceptance/bettertestspoc/README.md @@ -325,3 +325,4 @@ func (w *WarehouseDatasourceShowOutputAssert) IsEmpty() { - handle attribute types in resource assertions (currently strings only; TODO left in `assert/resourceassert/gen/model.go`) - distinguish between different enum types (TODO left in `assert/resourceshowoutputassert/gen/templates.go`) - support the rest of attribute types in config model builders (TODO left in `config/model/gen/model.go`) +- parametrize test client helper used - integration versus acceptance tests - this has to be changed in the generator too (TODO left in `assert/objectassert/user_snowflake_ext.go`) diff --git a/pkg/acceptance/bettertestspoc/assert/objectassert/user_snowflake_ext.go b/pkg/acceptance/bettertestspoc/assert/objectassert/user_snowflake_ext.go index 910672b3be..5e1e916473 100644 --- a/pkg/acceptance/bettertestspoc/assert/objectassert/user_snowflake_ext.go +++ b/pkg/acceptance/bettertestspoc/assert/objectassert/user_snowflake_ext.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" ) @@ -125,3 +127,11 @@ func (w *UserAssert) HasDaysToExpiryNotEmpty() *UserAssert { }) return w } + +// TODO [SNOW-1501905]: the current User func assumes acceptance test client helper, we should paramterize it and change this in the generators +func UserForIntegrationTests(t *testing.T, id sdk.AccountObjectIdentifier, testHelper *helpers.TestClient) *UserAssert { + t.Helper() + return &UserAssert{ + assert.NewSnowflakeObjectAssertWithProvider(sdk.ObjectTypeUser, id, testHelper.User.Show), + } +} diff --git a/pkg/acceptance/bettertestspoc/assert/resourceassert/user_resource_ext.go b/pkg/acceptance/bettertestspoc/assert/resourceassert/user_resource_ext.go new file mode 100644 index 0000000000..fb98079028 --- /dev/null +++ b/pkg/acceptance/bettertestspoc/assert/resourceassert/user_resource_ext.go @@ -0,0 +1,17 @@ +package resourceassert + +import ( + "strconv" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert" +) + +func (u *UserResourceAssert) HasDisabled(expected bool) *UserResourceAssert { + u.AddAssertion(assert.ValueSet("disabled", strconv.FormatBool(expected))) + return u +} + +func (u *UserResourceAssert) HasEmptyPassword() *UserResourceAssert { + u.AddAssertion(assert.ValueSet("password", "")) + return u +} diff --git a/pkg/acceptance/bettertestspoc/config/config.go b/pkg/acceptance/bettertestspoc/config/config.go index c1dea78d7e..b2d0a291fa 100644 --- a/pkg/acceptance/bettertestspoc/config/config.go +++ b/pkg/acceptance/bettertestspoc/config/config.go @@ -20,6 +20,7 @@ type ResourceModel interface { Resource() resources.Resource ResourceName() string SetResourceName(name string) + ResourceReference() string DependsOn() []string SetDependsOn(values []string) } @@ -42,6 +43,10 @@ func (m *ResourceModelMeta) SetResourceName(name string) { m.name = name } +func (m *ResourceModelMeta) ResourceReference() string { + return fmt.Sprintf(`%s.%s`, m.resource, m.name) +} + func (m *ResourceModelMeta) DependsOn() []string { return m.dependsOn } @@ -90,3 +95,15 @@ func FromModel(t *testing.T, model ResourceModel) string { t.Logf("Generated config:\n%s", s) return s } + +type nullVariable struct{} + +// MarshalJSON returns the JSON encoding of nullVariable. +func (v nullVariable) MarshalJSON() ([]byte, error) { + return json.Marshal(nil) +} + +// NullVariable returns nullVariable which implements Variable. +func NullVariable() nullVariable { + return nullVariable{} +} diff --git a/pkg/acceptance/bettertestspoc/config/model/user_model_ext.go b/pkg/acceptance/bettertestspoc/config/model/user_model_ext.go index 8888b90051..11b707d742 100644 --- a/pkg/acceptance/bettertestspoc/config/model/user_model_ext.go +++ b/pkg/acceptance/bettertestspoc/config/model/user_model_ext.go @@ -3,6 +3,7 @@ package model import ( tfconfig "github.com/hashicorp/terraform-plugin-testing/config" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/config" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" ) @@ -60,3 +61,7 @@ func (u *UserModel) WithNetworkPolicyId(networkPolicy sdk.AccountObjectIdentifie u.NetworkPolicy = tfconfig.StringVariable(networkPolicy.Name()) return u } + +func (u *UserModel) WithNullPassword() *UserModel { + return u.WithPasswordValue(config.NullVariable()) +} diff --git a/pkg/acceptance/helpers/random/random_helpers.go b/pkg/acceptance/helpers/random/random_helpers.go index d31458dc86..fcfb5b7208 100644 --- a/pkg/acceptance/helpers/random/random_helpers.go +++ b/pkg/acceptance/helpers/random/random_helpers.go @@ -15,7 +15,7 @@ func Comment() string { } func Password() string { - return StringN(12) + return StringN(30) } // AdminName returns admin name acceptable by Snowflake: diff --git a/pkg/acceptance/helpers/user_client.go b/pkg/acceptance/helpers/user_client.go index 8d39a92700..54d5ce9995 100644 --- a/pkg/acceptance/helpers/user_client.go +++ b/pkg/acceptance/helpers/user_client.go @@ -60,3 +60,11 @@ func (c *UserClient) Show(t *testing.T, id sdk.AccountObjectIdentifier) (*sdk.Us return c.client().ShowByID(ctx, id) } + +func (c *UserClient) Disable(t *testing.T, id sdk.AccountObjectIdentifier) { + t.Helper() + ctx := context.Background() + + err := c.client().Alter(ctx, id, &sdk.AlterUserOptions{Set: &sdk.UserSet{ObjectProperties: &sdk.UserObjectProperties{Disable: sdk.Bool(true)}}}) + require.NoError(t, err) +} diff --git a/pkg/datasources/users_acceptance_test.go b/pkg/datasources/users_acceptance_test.go index ae1d488bad..4f06e8419d 100644 --- a/pkg/datasources/users_acceptance_test.go +++ b/pkg/datasources/users_acceptance_test.go @@ -43,9 +43,9 @@ func users(userName string) string { email = "fake@email.com" disabled = false default_warehouse="foo" - default_role="foo" + default_role="FOO" default_secondary_roles = ["ALL"] - default_namespace="foo" + default_namespace="FOO" } data snowflake_users "u" { diff --git a/pkg/resources/user.go b/pkg/resources/user.go index 2f0c8c0a70..513329a13a 100644 --- a/pkg/resources/user.go +++ b/pkg/resources/user.go @@ -20,14 +20,13 @@ var userSchema = map[string]*schema.Schema{ "name": { Type: schema.TypeString, Required: true, - Sensitive: true, Description: "Name of the user. Note that if you do not supply login_name this will be used as login_name. [doc](https://docs.snowflake.net/manuals/sql-reference/sql/create-user.html#required-parameters)", }, "login_name": { Type: schema.TypeString, Optional: true, Computed: true, - Sensitive: false, + Sensitive: true, Description: "The name users use to log in. If not supplied, snowflake will use name instead.", // login_name is case-insensitive DiffSuppressFunc: ignoreCaseSuppressFunc, @@ -50,21 +49,23 @@ var userSchema = map[string]*schema.Schema{ Computed: true, }, "default_warehouse": { - Type: schema.TypeString, - Optional: true, - Description: "Specifies the virtual warehouse that is active by default for the user’s session upon login.", + Type: schema.TypeString, + Optional: true, + DiffSuppressFunc: suppressIdentifierQuoting, + Description: "Specifies the virtual warehouse that is active by default for the user’s session upon login.", }, + // TODO [SNOW-1348101 - next PR]: check the exact behavior of default_namespace and default_role because it looks like it is handled in a case-insensitive manner on Snowflake side "default_namespace": { Type: schema.TypeString, Optional: true, - DiffSuppressFunc: ignoreCaseSuppressFunc, + DiffSuppressFunc: suppressIdentifierQuoting, Description: "Specifies the namespace (database only or database and schema) that is active by default for the user’s session upon login.", }, "default_role": { Type: schema.TypeString, Optional: true, Computed: true, - DiffSuppressFunc: ignoreCaseSuppressFunc, + DiffSuppressFunc: suppressIdentifierQuoting, Description: "Specifies the role that is active by default for the user’s session upon login.", }, "default_secondary_roles": { @@ -121,6 +122,7 @@ var userSchema = map[string]*schema.Schema{ // MIDDLE_NAME = // SNOWFLAKE_LOCK = TRUE | FALSE // SNOWFLAKE_SUPPORT = TRUE | FALSE + // TODO [SNOW-1348101 - next PR]: handle #1155 by either forceNew or not reading this value from SF (because it changes constantly after setting; check https://docs.snowflake.com/en/sql-reference/sql/create-user#optional-object-properties-objectproperties) // DAYS_TO_EXPIRY = // MINS_TO_UNLOCK = // EXT_AUTHN_DUO = TRUE | FALSE @@ -393,9 +395,24 @@ func UpdateUser(ctx context.Context, d *schema.ResourceData, meta any) diag.Diag alterOptions.Set.ObjectProperties.Comment = sdk.String(n.(string)) } if d.HasChange("password") { - runSet = true - _, n := d.GetChange("password") - alterOptions.Set.ObjectProperties.Password = sdk.String(n.(string)) + if v, ok := d.GetOk("password"); ok { + runSet = true + alterOptions.Set.ObjectProperties.Password = sdk.String(v.(string)) + } else { + // TODO [SNOW-1348101 - next PR]: this is temporary, update logic will be changed with the resource rework + unsetOptions := &sdk.AlterUserOptions{ + Unset: &sdk.UserUnset{ + ObjectProperties: &sdk.UserObjectPropertiesUnset{ + Password: sdk.Bool(true), + }, + }, + } + err := client.Users.Alter(ctx, id, unsetOptions) + if err != nil { + d.Partial(true) + return diag.FromErr(err) + } + } } if d.HasChange("disabled") { @@ -473,6 +490,7 @@ func UpdateUser(ctx context.Context, d *schema.ResourceData, meta any) diag.Diag if runSet { err := client.Users.Alter(ctx, id, alterOptions) if err != nil { + d.Partial(true) return diag.FromErr(err) } } diff --git a/pkg/resources/user_acceptance_test.go b/pkg/resources/user_acceptance_test.go index e63696e9c6..97bc4763c5 100644 --- a/pkg/resources/user_acceptance_test.go +++ b/pkg/resources/user_acceptance_test.go @@ -4,18 +4,23 @@ import ( "errors" "fmt" "log" + "regexp" "strconv" "strings" "testing" acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + tfjson "github.com/hashicorp/terraform-json" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/objectassert" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/objectparametersassert" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/resourceassert" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/resourceparametersassert" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/config" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/config/model" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/planchecks" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/provider/resources" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" @@ -572,3 +577,220 @@ func TestAcc_User_AllParameters(t *testing.T) { }, }) } + +func TestAcc_User_issue2836(t *testing.T) { + userId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + defaultRole := "SOME ROLE WITH SPACE case sensitive" + defaultRoleQuoted := fmt.Sprintf(`"%s"`, defaultRole) + + userModel := model.User("u", userId.Name()). + WithDefaultRole(defaultRoleQuoted) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: acc.CheckDestroy(t, resources.User), + Steps: []resource.TestStep{ + { + Config: config.FromModel(t, userModel), + Check: assert.AssertThat(t, + objectassert.User(t, userId). + HasDefaultRole(defaultRole), + ), + }, + }, + }) +} + +func TestAcc_User_issue2970(t *testing.T) { + userId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + pass := random.Password() + key, _ := random.GenerateRSAPublicKey(t) + resourceName := "u" + + newPass := random.Password() + newKey, _ := random.GenerateRSAPublicKey(t) + incorrectlyFormattedNewKey := fmt.Sprintf("-----BEGIN PUBLIC KEY-----\n%s-----END PUBLIC KEY-----\n", newKey) + + userModel := model.User(resourceName, userId.Name()). + WithPassword(pass). + WithRsaPublicKey(key) + + newUserModelIncorrectNewKey := model.User(resourceName, userId.Name()). + WithPassword(newPass). + WithRsaPublicKey(incorrectlyFormattedNewKey) + + newUserModel := model.User(resourceName, userId.Name()). + WithPassword(newPass). + WithRsaPublicKey(newKey) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: acc.CheckDestroy(t, resources.User), + Steps: []resource.TestStep{ + { + Config: config.FromModel(t, userModel), + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userModel.ResourceReference()). + HasPasswordString(pass). + HasRsaPublicKeyString(key), + ), + }, + { + Config: config.FromModel(t, newUserModelIncorrectNewKey), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + planchecks.ExpectChange(newUserModelIncorrectNewKey.ResourceReference(), "password", tfjson.ActionUpdate, sdk.String(pass), sdk.String(newPass)), + planchecks.ExpectChange(newUserModelIncorrectNewKey.ResourceReference(), "rsa_public_key", tfjson.ActionUpdate, sdk.String(key), sdk.String(incorrectlyFormattedNewKey)), + }, + }, + ExpectError: regexp.MustCompile("New public key rejected by current policy"), + }, + { + Config: config.FromModel(t, newUserModel), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + planchecks.ExpectChange(newUserModel.ResourceReference(), "password", tfjson.ActionUpdate, sdk.String(pass), sdk.String(newPass)), + planchecks.ExpectChange(newUserModel.ResourceReference(), "rsa_public_key", tfjson.ActionUpdate, sdk.String(key), sdk.String(newKey)), + }, + }, + Check: assert.AssertThat(t, + resourceassert.UserResource(t, newUserModel.ResourceReference()). + HasPasswordString(newPass). + HasRsaPublicKeyString(newKey), + ), + }, + }, + }) +} + +// TODO [SNOW-1348101 - next PR]: will be fixed with addition of show_output, its logic, and changing disabled to non-computed attribute +func TestAcc_User_issue1572(t *testing.T) { + t.Skipf("Fix with user rework in SNOW-1348101") + userId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + + userModel := model.UserWithDefaultMeta(userId.Name()) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: acc.CheckDestroy(t, resources.User), + Steps: []resource.TestStep{ + { + Config: config.FromModel(t, userModel), + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userModel.ResourceReference()). + HasDisabled(false), + ), + }, + { + PreConfig: func() { + acc.TestClient().User.Disable(t, userId) + objectassert.User(t, userId).HasDisabled(true) + }, + Config: config.FromModel(t, userModel), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + planchecks.ExpectDrift(userModel.ResourceReference(), "disabled", sdk.String("false"), sdk.String("true")), + planchecks.ExpectChange(userModel.ResourceReference(), "disabled", tfjson.ActionUpdate, sdk.String("false"), sdk.String("true")), + }, + }, + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userModel.ResourceReference()). + HasDisabled(false), + ), + }, + }, + }) +} + +func TestAcc_User_issue1535_withNullPassword(t *testing.T) { + userId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + pass := random.Password() + + userModel := model.UserWithDefaultMeta(userId.Name()). + WithPassword(pass) + + userWithNullPasswordModel := model.UserWithDefaultMeta(userId.Name()). + WithNullPassword() + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: acc.CheckDestroy(t, resources.User), + Steps: []resource.TestStep{ + { + Config: config.FromModel(t, userModel), + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userModel.ResourceReference()). + HasPasswordString(pass), + ), + }, + { + Config: config.FromModel(t, userWithNullPasswordModel), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + planchecks.ExpectChange(userWithNullPasswordModel.ResourceReference(), "password", tfjson.ActionUpdate, sdk.String(pass), nil), + }, + }, + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userWithNullPasswordModel.ResourceReference()). + HasEmptyPassword(), + ), + }, + }, + }) +} + +func TestAcc_User_issue1535_withRemovedPassword(t *testing.T) { + userId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + pass := random.Password() + + userModel := model.UserWithDefaultMeta(userId.Name()). + WithPassword(pass) + + userWithoutPasswordModel := model.UserWithDefaultMeta(userId.Name()) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: acc.CheckDestroy(t, resources.User), + Steps: []resource.TestStep{ + { + Config: config.FromModel(t, userModel), + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userModel.ResourceReference()). + HasPasswordString(pass), + ), + }, + { + Config: config.FromModel(t, userWithoutPasswordModel), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + planchecks.ExpectChange(userWithoutPasswordModel.ResourceReference(), "password", tfjson.ActionUpdate, sdk.String(pass), nil), + }, + }, + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userWithoutPasswordModel.ResourceReference()). + HasEmptyPassword(), + ), + }, + }, + }) +} diff --git a/pkg/resources/user_public_keys_acceptance_test.go b/pkg/resources/user_public_keys_acceptance_test.go index 8ceb564625..0d1d4985cc 100644 --- a/pkg/resources/user_public_keys_acceptance_test.go +++ b/pkg/resources/user_public_keys_acceptance_test.go @@ -73,8 +73,8 @@ resource "snowflake_user" "w" { email = "fake@email.com" disabled = false default_warehouse="foo" - default_role="foo" - default_namespace="foo" + default_role="FOO" + default_namespace="FOO" } resource "snowflake_user_public_keys" "foobar" { diff --git a/pkg/sdk/sql_mapping_helpers.go b/pkg/sdk/sql_mapping_helpers.go new file mode 100644 index 0000000000..1bb5ad4140 --- /dev/null +++ b/pkg/sdk/sql_mapping_helpers.go @@ -0,0 +1,19 @@ +package sdk + +import ( + "database/sql" + "log" + "strconv" +) + +func handleNullableBoolString(nullableBoolString sql.NullString, field *bool) { + if nullableBoolString.Valid && nullableBoolString.String != "" && nullableBoolString.String != "null" { + parsed, err := strconv.ParseBool(nullableBoolString.String) + if err != nil { + // TODO [SNOW-1561641]: address during handling the issue + log.Printf("[DEBUG] Could not parse text boolean value %v", nullableBoolString.String) + } else { + *field = parsed + } + } +} diff --git a/pkg/sdk/testint/users_integration_test.go b/pkg/sdk/testint/users_integration_test.go index 6223b2a706..8ef52ad3a4 100644 --- a/pkg/sdk/testint/users_integration_test.go +++ b/pkg/sdk/testint/users_integration_test.go @@ -978,4 +978,45 @@ func TestInt_Users(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, len(users)) }) + + // This test proves issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2817. + // sql: Scan error on column index 10, name "disabled": sql/driver: couldn't convert "null" into type bool + t.Run("issue #2817: handle show properly without OWNERSHIP and MANAGE GRANTS", func(t *testing.T) { + disabledUser, disabledUserCleanup := testClientHelper().User.CreateUserWithOptions(t, testClientHelper().Ids.RandomAccountObjectIdentifier(), &sdk.CreateUserOptions{ObjectProperties: &sdk.UserObjectProperties{Disable: sdk.Bool(true)}}) + t.Cleanup(disabledUserCleanup) + + assertions.AssertThatObject(t, objectassert.UserForIntegrationTests(t, disabledUser.ID(), testClientHelper()). + HasDisabled(true), + ) + + role, roleCleanup := testClientHelper().Role.CreateRoleGrantedToCurrentUser(t) + t.Cleanup(roleCleanup) + + revertRole := testClientHelper().Role.UseRole(t, role.ID()) + t.Cleanup(revertRole) + + assertions.AssertThatObject(t, objectassert.UserForIntegrationTests(t, disabledUser.ID(), testClientHelper()). + HasDisabled(false), + ) + }) + + t.Run("issue #2817: check the describe behavior", func(t *testing.T) { + disabledUser, disabledUserCleanup := testClientHelper().User.CreateUserWithOptions(t, testClientHelper().Ids.RandomAccountObjectIdentifier(), &sdk.CreateUserOptions{ObjectProperties: &sdk.UserObjectProperties{Disable: sdk.Bool(true)}}) + t.Cleanup(disabledUserCleanup) + + fetchedDisabledUserDetails, err := client.Users.Describe(ctx, disabledUser.ID()) + require.NoError(t, err) + require.NotNil(t, fetchedDisabledUserDetails.Disabled) + require.True(t, fetchedDisabledUserDetails.Disabled.Value) + + role, roleCleanup := testClientHelper().Role.CreateRoleGrantedToCurrentUser(t) + t.Cleanup(roleCleanup) + + revertRole := testClientHelper().Role.UseRole(t, role.ID()) + t.Cleanup(revertRole) + + fetchedDisabledUserDetails, err = client.Users.Describe(ctx, disabledUser.ID()) + require.ErrorContains(t, err, "Insufficient privileges to operate on user") + require.Nil(t, fetchedDisabledUserDetails) + }) } diff --git a/pkg/sdk/users.go b/pkg/sdk/users.go index c177bd5231..344fd2a507 100644 --- a/pkg/sdk/users.go +++ b/pkg/sdk/users.go @@ -72,14 +72,14 @@ type userDBRow struct { MinsToUnlock sql.NullString `db:"mins_to_unlock"` DaysToExpiry sql.NullString `db:"days_to_expiry"` Comment sql.NullString `db:"comment"` - Disabled bool `db:"disabled"` - MustChangePassword bool `db:"must_change_password"` - SnowflakeLock bool `db:"snowflake_lock"` + Disabled sql.NullString `db:"disabled"` + MustChangePassword sql.NullString `db:"must_change_password"` + SnowflakeLock sql.NullString `db:"snowflake_lock"` DefaultWarehouse sql.NullString `db:"default_warehouse"` DefaultNamespace string `db:"default_namespace"` DefaultRole string `db:"default_role"` DefaultSecondaryRoles string `db:"default_secondary_roles"` - ExtAuthnDuo bool `db:"ext_authn_duo"` + ExtAuthnDuo sql.NullString `db:"ext_authn_duo"` ExtAuthnUid string `db:"ext_authn_uid"` MinsToBypassMfa string `db:"mins_to_bypass_mfa"` Owner string `db:"owner"` @@ -95,13 +95,9 @@ func (row userDBRow) convert() *User { Name: row.Name, CreatedOn: row.CreatedOn, LoginName: row.LoginName, - Disabled: row.Disabled, - MustChangePassword: row.MustChangePassword, - SnowflakeLock: row.SnowflakeLock, DefaultNamespace: row.DefaultNamespace, DefaultRole: row.DefaultRole, DefaultSecondaryRoles: row.DefaultSecondaryRoles, - ExtAuthnDuo: row.ExtAuthnDuo, ExtAuthnUid: row.ExtAuthnUid, MinsToBypassMfa: row.MinsToBypassMfa, Owner: row.Owner, @@ -129,6 +125,10 @@ func (row userDBRow) convert() *User { if row.Comment.Valid { user.Comment = row.Comment.String } + handleNullableBoolString(row.Disabled, &user.Disabled) + handleNullableBoolString(row.MustChangePassword, &user.MustChangePassword) + handleNullableBoolString(row.SnowflakeLock, &user.SnowflakeLock) + handleNullableBoolString(row.ExtAuthnDuo, &user.ExtAuthnDuo) if row.DefaultWarehouse.Valid { user.DefaultWarehouse = row.DefaultWarehouse.String }