fix: SHOW USERS output with insufficient privileges

[candiduslynx]

Fixes #2817

Test Plan

• credentials that don't have both OWNERSHIP & MANAGE GRANTS privilege (as required for full output in docs

References

We have an internal issue when scanning Users.Show output:

2024/05/15 15:27:31 [DEBUG] err: sql: Scan error on column index 10, name "disabled": sql/driver: couldn't convert "null" into type bool

[candiduslynx]

cc @sfc-gh-asawicki @sfc-gh-jmichalak for review

[sfc-gh-asawicki]

Hey @candiduslynx. Thanks for the contribution!

First, please consult https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#making-a-contribution. There are a few pointers on how to make it easier for us to review and accept the change.

What I lack the most here is:

• issue created with the precise description of the problem, with steps to reproduce
• acceptance test showing that the issue is gone after introduced changes (should be done for the user without the privileges as described)
• integration test of the SDK (because we are altering its behavior)
  On the course of action, it may be necessary to alter the resource somehow (without the tests, it's hard to say).

We will be shortly redoing user resource as part of https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1, we may just address this issue then.

Let me know if you want to proceed or if you prefer to wait for our changes (in such a case, please create an issue with the exact problem description following https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CREATING_ISSUES.md).

[candiduslynx]

Hi @sfc-gh-asawicki!

Thanks for pointing to the missing points for this PR.

We will be shortly redoing user resource as part of https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1, we may just address this issue then.

Could you share ETA for the fix in SHOW USERS scanning? The decision whether to wait for the maintainers team to fix or push onward with this PR depends on the timeline you have for this issue.

Let me know if you want to proceed or if you prefer to wait for our changes (in such a case, please create an issue with the exact problem description following https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CREATING_ISSUES.md).

For now I'll create the issue & try to make the PR more approachable.
I might need help with the tests, specifically, acceptance ones. Do you have any guidelines for those as well?

[sfc-gh-asawicki]

Could you share ETA for the fix in SHOW USERS scanning? The decision whether to wait for the maintainers team to fix or push onward with this PR depends on the timeline you have for this issue.

If nothing unexpected happens, I'd say we will redo user resource within a month.

For now I'll create the issue & try to make the PR more approachable.
I might need help with the tests, specifically, acceptance ones. Do you have any guidelines for those as well?

We do not have guidelines on how to write them, but you can check other tests to get the idea. We have instructions on how to set them up locally, though. You can check the instructions here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#running-the-tests-locally.

Also, because the guidelines have been recently refreshed, they may lack some points, so we would greatly appreciate feedback on what else should be included.

[candiduslynx]

@sfc-gh-asawicki I've opened #2817

I'll be looking for the tests in the meantime.

[candiduslynx]

@sfc-gh-asawicki I've retested the code manually & found that the original issue isn't solvable by sql.NullBool values, as the data is returned as a literal "null" string from Snowflake itself.
While it may be an upstream bug, I've taken a liberty to add a simple snowflakesql package to address this (the snowflakesql.NullBool accepts "null" string and maps it to a nil).
I can add the tests to the package as well, but IDK if the tests for the bad upstream behavior are required.

[sfc-gh-asawicki]

Hey @candiduslynx.

I am not sure if this is a bad upstream behavior. We are handling it this way in multiple places in the project (e.g. common_types.go - func (row *propertyRow) toStringProperty() *StringProperty, external_function.go - if row.Value != "" && row.Value != "null" , and some more).

[candiduslynx]

Thanks @sfc-gh-asawicki for pointing me to the functions you already have here.

I am not sure if this is a bad upstream behavior. We are handling it this way in multiple places in the project (e.g. common_types.go - func (row *propertyRow) toStringProperty() *StringProperty, external_function.go - if row.Value != "" && row.Value != "null" , and some more).

I skimmed through them & the propertyRow is explicitly for parsing the corresponding table results. I think trying the sql.NullString to be used together with "null" check isn't a good option, so I've added the more robust parsing and tests to the snowflakesql.Bool type.

I hope that'll be enough to be merged & released. If not, please advise what should be added to the PR.

[sfc-gh-asawicki]

Hey @candiduslynx. The PR is still missing the acceptance/integration tests pointed out in #2815 (comment). We won't merge/release it without them.

[candiduslynx]

@sfc-gh-asawicki

Hey @candiduslynx. The PR is still missing the acceptance/integration tests pointed out in #2815 (comment). We won't merge/release it without them.

I've added the integration test in 526e9f6.
For the acceptance tests I wonder how that could be implemented. For now, the tests create resources & by default can access all of the resource fields. Do you have the example for the resource that is tested (acceptance) against insufficient privileges? If not, could you exempt this PR from acceptance test requirement (it should pass the original acceptance tests just fine, but the behavioral logic should be done in a separate PR that will add such capability to all of the acceptance tests).

[sfc-gh-asawicki]

@candiduslynx

The rules for writing the acceptance tests that we follow for any other bugfix are:

• setup the acceptance test showing the current problem (so the test is failing in the same way as described in the attached issue)
• introduce a potential fix
• check that the test is passing after the change

In this case, we should:

• set up a role without sufficient privileges (in the PreConfig block, using the helpers from pkg/acceptance/helpers)
• set up terraform config, not only with user resource, but also with a new provider (example in TestAcc_GrantOwnership_RoleBasedAccessControlUseCase or TestAcc_Provider_configHierarchy) and check the fields (assertions should fail now, because of the issue you are trying to fix)
• introduce your suggested change and see the test pass

Sometimes, we tend to show that the setup fails for the given provider version and succeeds for the newest one explicitly (like in TestAcc_Table_issue2535_newConstraint), but for now, we do not favor one over the other.

As you see TestAcc_GrantOwnership_RoleBasedAccessControlUseCase is currently skipped. This is because of the problems with our current CI setup, but the test should run if you unskip it.

The integration test you have added does not show that other fields are not filled for the role without sufficient privileges but are filled when using the privileged one; let's be explicit about this. Also, it makes hidden assumptions about the secondary client setup - it should be clear from the test, why we expect the given outcome (for now comment in code explaining why and linking the issue would be enough).

I still only see two paths that are acceptable from our side:

1. You continue with the PR and introduce the requested changes.
2. We take over and introduce changes while redoing the user resource (as explained in fix: SHOW USERS output with insufficient privileges #2815 (comment)).

[candiduslynx]

OK, @sfc-gh-asawicki I don't think I have too much time adding the acceptance tests for this, so I'll be closing this PR & waiting for the #2817 to be addressed by your team then,

[sfc-gh-asawicki]

Okay @candiduslynx. Thanks for understanding. We will let you know in #2817 when we have it ready.