fix: Fix known user resource issues

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
 

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).

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`)

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),
+	}
+}

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
+}

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{}
+}

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())
+}

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:

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)
+}

pkg/datasources/users_acceptance_test.go

@@ -43,9 +43,9 @@ func users(userName string) string {
 		email = "[email protected]"
 		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" {

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 = <string>
 	//    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 = <integer>
 	//    MINS_TO_UNLOCK = <integer>
 	//    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)
 		}
 	}