dbt-snowflake icon indicating copy to clipboard operation
dbt-snowflake copied to clipboard

[ADAP-783] [Feature] Allow user to be omitted if using oauth authenticator

Open MasterOdin opened this issue 2 years ago • 1 comments

Is this your first time submitting a feature request?

  • [X] I have read the expectations for open source contributors
  • [X] I have searched the existing issues, and I could not find an existing issue for this feature
  • [X] I am requesting a straightforward extension of existing dbt-snowflake functionality, rather than a Big Idea better suited to a discussion

Describe the feature

It would be nice if the user field was not mandatory when using authenticator: oauth. Without specifying it, I get following error:

Runtime Error
  Credentials in profile "default", target "dev" invalid: 'user' is a required property

When specifying it, it means I have to match the username that'd be fetched for oauth, which is a bit redundant, and makes the authenticator type a bit more annoying to use where when constructing a profile for local usage, we're authenticating with snowflake oauth to get the refresh token, and then using that token to run a single query to get the current username which we then use to fully construct the profiles.yml file.

Describe alternatives you've considered

No real alternative other than having to specify username as far as I'm aware. Specifying a "dummy" value gives me the following error:

The database returned the following error:

  >Database Error
  250001 (08001): Failed to connect to DB: <account>.snowflakecomputing.com:443. The user you were trying to authenticate as differs from the user tied to the access token

An alternative to making it totally optional would be to allow some way to specify a dummy value that's not passed to snowflake.connector.connect.

Who will this benefit?

People trying to use the oauth authenticator type

Are you interested in contributing this feature?

yes

Anything else?

We had initially tried configuring the profile based on the comment at https://github.com/dbt-labs/dbt-snowflake/issues/474#issuecomment-1435368808, where they don't include the username.

MasterOdin avatar Aug 08 '23 17:08 MasterOdin

@MasterOdin I think you've laid out a compelling use case. One question I have:

Is authenticator: oauth the only scenario in which user need not be specified and supplied to snowflake.connector.connect()?

Current usage

There are two places where the user field is used

as an attribute of the dataclass SnowflakeCredential

This is the source of the error message you are saying. It should instead be defined as warehouse is below it, with Optional[str] = None. This change is not sufficient because user is always provided to snowflake.connector.connect

https://github.com/dbt-labs/dbt-snowflake/blob/26c1c8bfeddc8c575a0638423e6e3fc5b5417bf4/dbt/adapters/snowflake/connections.py#L62-L63

invocation of snowflake.connector.connect inside of SnowflakeConnectionManager.open()

the unpacked dictionary returned by creds.auth_args() will include a token key if authenticator: oauth is provided. In this scenario, my understanding is that user is not needed in such a scenario.

https://github.com/dbt-labs/dbt-snowflake/blob/26c1c8bfeddc8c575a0638423e6e3fc5b5417bf4/dbt/adapters/snowflake/connections.py#L336-L339

https://github.com/dbt-labs/dbt-snowflake/blob/26c1c8bfeddc8c575a0638423e6e3fc5b5417bf4/dbt/adapters/snowflake/connections.py#L347-L349

Remediation

my hunch is that in addition to making user an optional attribute, user should be provided to snowflake.connector.connect() via auth_args(). the auth_args method should be modified by adding a new conditional to the end of the current list. pseudocode below

if self.authenticator != "oauth":
  if self.user is not None:
    result["user"] = self.user
  else: 
    raise DbtInternalError("user must be provided")

We can't commit to resolving this issue in the near future, however, if there's a PR that does most of the work, we can provide feedback and it would be much lower effort to get merged and shipped.

dataders avatar Aug 17 '23 17:08 dataders