-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Adding external authentication capability when using oracledb driver #1716
Conversation
What does it take to merge these changes upstream ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to just passing different auth setting, also an unit test should be written, which creates knex instance with externalAuth param and test should check that configuration doesn't have user / password set in that case. Logging in with username / password is already tested in integration tests. Also tests should be fixed... problem with failures did seem to be just linting errors.
Lastly some kind of documentation/example of the new feature would be good to have.
} | ||
|
||
// In the case of external authentication connection string will be given | ||
if (client.connectionSettings.connectString){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the original style
oracleDbConfig.connectString = client.connectionSettings.connectString ||
(client.connectionSettings.host + '/' + client.connectionSettings.database);
is easier to read and understand.
const oracleDbConfig = {}; | ||
|
||
// If external authentication dont have to worry about username/password | ||
if (client.connectionSettings.externalAuth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the same but more readable:
const oracleDbConfig = client.connectionSettings.externalAuth ?
{ externalAuth : client.connectionSettings.externalAuth } :
{
user : client.connectionSettings.user,
password: client.connectionSettings.password
};
Hi, I did the changes you mentioned in the previous comment. |
Thanks. Yeah, all builds has been annoyingly semi-random-pretty-much-always failing for some time. I'll look to the random fails and afterwards I'll review this one again. |
@@ -8,7 +8,7 @@ | |||
|
|||
> **A SQL query builder that is _flexible_, _portable_, and _fun_ to use!** | |||
|
|||
A batteries-included, multi-dialect (MSSQL, MySQL, PostgreSQL, SQLite3, WebSQL, Oracle) query builder for | |||
A batteries-included, multi-dialect (MSSQL, MySQL, PostgreSQL, SQLite3, Oracle(include Oracle Wallet Authentication), WebSQL) query builder for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... including Oracle Wallet...
// If external authentication dont have to worry about username/password and | ||
// if not need to set the username and password | ||
const oracleDbConfig = client.connectionSettings.externalAuth ? | ||
{ externalAuth : client.connectionSettings.externalAuth } : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 spaces too wide indentation... or below 2 spaces too narrow
Looks good now, thanks! I added separate issue to documentation branch about documenting all the supported oracledb driver parameters. |
…nex#1716) * Adding external authentication capability as mentioned in https://github.com/oracle/node-oracledb/blob/master/doc/api.md#extauth * Add unit test for externalAuth * Cover the connection string part * Update documentation * fix spaces * Hide the fake Oracle server error * minor grammar changes and spaces changes
Adding external authentication capability as mentioned in https://github.com/oracle/node-oracledb/blob/master/doc/api.md#extauth