Remove Username assertion for web browser auth #2142
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
I'm sorry, this doesn't work for me. I think |
|
@sfc-gh-mkeller Ive tested the forked repo in my Snowflake Code, the user context is able to be derived from the webbrowser, and scripts run successfully. Is there any supporting evidence needed to take this further? Were migrating from the JS SDK to the python SDK, in JS SDK the username is not a parameter for webbrowser auth |
| @@ -1238,7 +1238,7 @@ def __config(self, **kwargs): | |||
| self._token = f.read() | |||
|
|
|||
| if not (self._master_token and self._session_token): | |||
| if not self.user and self._authenticator != OAUTH_AUTHENTICATOR: | |||
| if not self.user and self._authenticator != OAUTH_AUTHENTICATOR and self._authenticator != EXTERNAL_BROWSER_AUTHENTICATOR: | |||
There was a problem hiding this comment.
It's true that for authentication itself user is not required, but it is required for storing a temporary credential (token) in the cache storage, to identify the owner.
There was a problem hiding this comment.
You could make one SQL query to fetch the username of the current token and use that to store it.
There was a problem hiding this comment.
Hi @nicornk
You're right provided it would have taken place backend-side; however, the source of concern here is the client-side temporary credentials cache. As it's effectively a key-value storage, a unique key is required to built on the client-side as a prerequisite to authentication.
Technically it could've been some arbitrary identifier provided by a user, but probably it won't look like a much better user experience than currently required username, not sure. What's your opinion?
There was a problem hiding this comment.
How long is the temporary cached item valid for? Is it just for the duration of the login attempt, in that case wouldn’t there be another way to come up with a key for the KV store?
if it’s anyhow arbitrary, why not use a random uuid?
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1892848: Web Browser Auth doesnt need user to be passed but asserts it #2143
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Ignoring missing username for external browser auth
(Optional) PR for stored-proc connector: