OAuth Authentication Support #146
Conversation
| client_pid: params.client_pid | ||
| client_pid: params.client_pid, | ||
| binary_data_protocol: '0', // Defaults to text format '0' | ||
| protocol_compat: 'VER', |
There was a problem hiding this comment.
For nodejs client, do we need an undocumented parameter to control whether send oauth_access_token in startup message for old servers?
There was a problem hiding this comment.
Since OAuth in vertica-nodejs was not supported when those old servers were released we can just say this is a new feature that works with newer server versions starting at the version where we removed access token from the startup message.
packages/vertica-nodejs/mochatest/integration/client/oauth-tests.js
Outdated
Show resolved
Hide resolved
| @@ -30,7 +30,7 @@ describe('vertica client label connection parameter', function () { | |||
| client_default.query('SELECT GET_CLIENT_LABEL()', (err, res) => { | |||
| if (err){ | |||
| console.log(err) | |||
| assert(false) | |||
| done(err) | |||
packages/vertica-nodejs/mochatest/integration/client/vertica-connection-params-tests.js
Outdated
Show resolved
Hide resolved
| assert.equal(res.rows[0].client_pid, process.pid) | ||
| assert.equal(res.rows[0].client_type, "Node.js Driver") | ||
| assert.equal(res.rows[0].client_version, vertica.version) | ||
| assert.equal(res.rows[0].client_os, os.platform()) | ||
| assert.equal(res.rows[0].client_os_user_name, os.userInfo().username) | ||
| assert.equal(res.rows[0].client_os_hostname, os.hostname()) | ||
| client.end() | ||
| done() |
There was a problem hiding this comment.
This will call the done() callback function twice now, right? Since we provided the callback to it() and also we are explicitly calling it here.
There was a problem hiding this comment.
I'm not really sure how to write mocha test, I made this change because I notice if I provide invalid connection info, this test ignores the connection failure error and passes.
There was a problem hiding this comment.
I would expect that the explicit call to done(err) would fix that, but the extra call to done() on line 121 is probably unnecessary since the callback is provided on line 109 to the it function. . I can test it out when I play around with oauth.
| @@ -7,13 +7,15 @@ var suite = new helper.Suite() | |||
| // clear process.env | |||
| var realEnv = {} | |||
| for (var key in process.env) { | |||
| realEnv[key] = process.env[key] | |||
| if (!key.indexOf('PG')) delete process.env[key] | |||
| if (key.startsWith('V_')) { | |||
| @@ -1,5 +1,11 @@ | |||
| 'use strict' | |||
| require('./test-helper') | |||
| /* | |||
There was a problem hiding this comment.
I'll take a look at this and log an issue if it can be removed or concatenated. This uses the homemade test infrastructure test-helper which I don't love either, so I'm all for reducing this down to one place.
|
Took a first pass. It looks like a great implementation plus a lot of good clean up. I left a few comments. I'll do another more thorough review when I have enough time to test it out locally |
| // user is required for non-OAuth connections | ||
| this.user = val('user', config) | ||
| if (!this.user && !this.oauth_access_token) { | ||
| this.user = process.platform === 'win32' ? process.env.USERNAME : process.env.USER |
There was a problem hiding this comment.
Can you add a comment here so that we will log a notice once we have that option. Something like
// TODO: log a notice to the user that the user property was taken from the environment once we fully support logging
|
I added one new request for a comment. It is not a blocking concern so I will approve now. This looks great, both the new feature implementation and all of the cleanup is very nice to have. |
vertica://, and fix test regressions