Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
3 tasks done
MasterOdin opened this issue Aug 8, 2023 · 1 comment · Fixed by #1078
Closed
3 tasks done

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

MasterOdin opened this issue Aug 8, 2023 · 1 comment · Fixed by #1078
Labels
enhancement New feature or request good_first_issue Good for newcomers help_wanted Extra attention is needed

Comments

@MasterOdin
Copy link

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • 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 #474 (comment), where they don't include the username.
@MasterOdin MasterOdin added enhancement New feature or request triage labels Aug 8, 2023
@github-actions github-actions bot changed the title [Feature] Allow user to be omitted if using oauth authenticator [ADAP-783] [Feature] Allow user to be omitted if using oauth authenticator Aug 8, 2023
@dataders
Copy link
Contributor

@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

dbt-snowflake/dbt/adapters/snowflake/connections.py

Lines 62 to 63 in 26c1c8b

user: str
warehouse: Optional[str] = None

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.

dbt-snowflake/dbt/adapters/snowflake/connections.py

Lines 336 to 339 in 26c1c8b

handle = snowflake.connector.connect(
account=creds.account,
user=creds.user,
database=creds.database,

dbt-snowflake/dbt/adapters/snowflake/connections.py

Lines 347 to 349 in 26c1c8b

session_parameters=session_parameters,
**creds.auth_args(),
)

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 dataders added good_first_issue Good for newcomers help_wanted Extra attention is needed and removed triage labels Aug 17, 2023
@MasterOdin MasterOdin mentioned this issue Dec 14, 2023
Closed
4 tasks
@llam15 llam15 mentioned this issue Jun 10, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good_first_issue Good for newcomers help_wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
2 participants
@MasterOdin @dataders