Add external browser-based authentication support#160
Add external browser-based authentication support#160wpfleger96 wants to merge 1 commit intorinsed-org:masterfrom
Conversation
wpfleger96
force-pushed
the
wpfleger/browser-auth-support
branch
from
857533c to
5e013b5
Compare
reidnimz
left a comment
reidnimz
left a comment
There was a problem hiding this comment.
I think this makes sense. My big concern is just making sure that the v1 of the API behaves the same as the v2 with regards to results and that it all works and is verified. There's a lot going on here, so I will need to take some more time and go through it again.
|
|
||
| if data[index].is_a? Concurrent::Future | ||
| data[index] = data[index].value # wait for it to finish | ||
| data[index] = data[index].value! # wait for it to finish, raises on exception |
There was a problem hiding this comment.
Is this related? I am not sure we want to change the contract here
| ENV.fetch("SNOWFLAKE_URI"), | ||
| private_key, | ||
| ENV.fetch("SNOWFLAKE_ORGANIZATION"), | ||
| ENV.fetch("SNOWFLAKE_ORGANIZATION", nil), |
There was a problem hiding this comment.
This should probably be dependent on whether or not they're using keypair_jwt or not. It's unlikely someone is actually relying on that behavior (raising if org isn't set), but still for backwards compatibility it's probably best still error if they're using keypair and it's not set
| raise ArgumentError, "Bindings are not supported with externalbrowser authentication. Use string interpolation or switch to keypair auth." | ||
| end | ||
| with_instrumentation({ database:, schema:, warehouse:, query_name: }) do | ||
| query_v1(query, warehouse: warehouse, database: database, schema: schema, role: role, streaming: streaming, query_timeout: query_timeout) |
There was a problem hiding this comment.
Since this is using the older v1 form of the API I think a couple of things:
- we should call out that using this auth flow uses a different version of the API (in the readme)
- we should have tests that exercise all of the same existing tests, but through this version of the API
- probably add something to the testing setup instructions, and then I'll figure out how to set this up so we can run those specs against our snowflake and the project CI
- maybe we should have a query_v1 and query_v2 method in this class, I think it might make it easier to follow
2 participants
This PR adds external browser (SSO/SAML) authentication support. Previously, the gem only supported key pair JWT authentication, which doesn't work for organizations that require browser-based SSO login through identity providers like Okta or Azure AD.
When a user sets
SNOWFLAKE_AUTHENTICATOR=externalbrowser, the client opens their default browser to complete SSO authentication, receives a session token, and caches it for the 59-minute session window. The goal is to mirror theexternalbrowserauth method supported in the Python client for SnowflakeImplementation:
SNOWFLAKE_ORGANIZATIONoptional to support varied account configurationsArgumentError)I added unit tests for this functionality, and also manually tested the browser sign in flow locally (we use Okta for SSO at Block) on my Mac. I still need to properly test the browser flow end-to-end on Linux and Windows (WSL) systems
Resolves #78