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.

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

Fix/stale authenticator reference for external SSO connection caching #918

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

supermacro
Copy link

@supermacro supermacro commented Sep 26, 2024

Description

#820 Implemented sso token caching. However token caching does not work in version 1.13.1 of this lib (and possibly other versions). Every new connection fails to use the id token that is written to disk.

Issues found:

  • in order for an ID token to be written to disk, clientStoreTemporaryCredential needs to be set to true in the connection config, yet it's not a field defined in index.d.ts.
  • The connectionConfig (in connection.js) that the sf service has access to does not have a idToken set (even though there is one available), which seems to indicate that the sf service has a stale closure / reference to connectionConfig
    • one can prove that one is set by console.log'ing connectionConfig.idToken inside of connection.js:295

relatedly, the docs are outdated as they do not specify that connection caching is supported for the NodeJS driver

Testing the fix

Run the following script twice:

const sf = require('./supermacro-snowflake-connector-nodejs/lib/snowflake')

const conn = sf.createConnection({
  username: '<redacted>',
  account: '<redacted>',
  authenticator: 'externalbrowser',
  database: '<redacted>',
  schema: '<redacted>',
  clientStoreTemporaryCredential: true,
  credentialCacheDir: '.', // relative to current dir where the app is run
})

conn.connectAsync((err, econnected) => {
  if (err) {
    console.error('Unable to connect: ' + err.message);
    return
  }

  console.log('Connection successfully established!')
})

Expected / fixed behaviour:

  • First run opens up a new browser tab to initiate external SSO flow
  • Second run DOES NOT open a new browser tab and instead snowflake-sdk uses id token inside of temporary_credential.json file that was written to disk during initial authentication flow

Checklist

  • Format code according to the existing code style (run npm run lint:check -- CHANGED_FILES and fix problems in changed code)
  • Create tests which fail without the change (if possible)
  • Make all tests (unit and integration) pass (npm run test:unit and npm run test:integration)
  • Extend the README / documentation and ensure is properly displayed (if necessary)
  • Provide JIRA issue id (if possible) or GitHub issue id in commit message

@supermacro supermacro requested a review from a team as a code owner September 26, 2024 20:45
Copy link

github-actions bot commented Sep 26, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@supermacro
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@supermacro
Copy link
Author

recheck

@sfc-gh-dszmolka
Copy link
Collaborator

sfc-gh-dszmolka commented Sep 27, 2024

can you please check https://docs.snowflake.com/en/developer-guide/node-js/nodejs-driver-authenticate#authentication-token-caching ? the token cache should not work without the server side component turned on. tested it after the feature was released and worked for me (on Linux).

if you see an issue, please file an Issue.

@supermacro
Copy link
Author

Related: #919

@supermacro
Copy link
Author

can you please check https://docs.snowflake.com/en/developer-guide/node-js/nodejs-driver-authenticate#authentication-token-caching ? the token cache should not work without the server side component turned on. tested it after the feature was released and worked for me (on Linux).

if you see an issue, please file an Issue.

We eneabled the server side component by running

ALTER ACCOUNT SET ALLOW_ID_TOKEN = TRUE;

#921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants