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

Adding external authentication capability when using oracledb driver #1716

Merged
merged 8 commits into from Nov 30, 2016

Conversation

@kasunt-nixdev
Copy link
Contributor

@kasunt-nixdev kasunt-nixdev commented Oct 3, 2016

Adding external authentication capability as mentioned in https://github.com/oracle/node-oracledb/blob/master/doc/api.md#extauth

@kasunt-nixdev
Copy link
Contributor Author

@kasunt-nixdev kasunt-nixdev commented Nov 9, 2016

What does it take to merge these changes upstream ?

Copy link
Member

@elhigu elhigu left a comment

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){

This comment has been minimized.

@elhigu

elhigu Nov 9, 2016
Member

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) {

This comment has been minimized.

@elhigu

elhigu Nov 9, 2016
Member

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
  };
@xudingding978
Copy link

@xudingding978 xudingding978 commented Nov 29, 2016

Hi, I did the changes you mentioned in the previous comment.
And the tests are failing at the "npm run babel" step, and mysql part.

@elhigu
Copy link
Member

@elhigu elhigu commented Nov 29, 2016

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

This comment has been minimized.

@elhigu

elhigu Nov 29, 2016
Member

... 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 } :

This comment has been minimized.

@elhigu

elhigu Nov 29, 2016
Member

2 spaces too wide indentation... or below 2 spaces too narrow

@elhigu
elhigu approved these changes Nov 29, 2016
@elhigu
Copy link
Member

@elhigu elhigu commented Nov 30, 2016

Looks good now, thanks! I added separate issue to documentation branch about documenting all the supported oracledb driver parameters.

@elhigu elhigu merged commit da5ed96 into knex:master Nov 30, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
elhigu added a commit to elhigu/knex that referenced this pull request Feb 15, 2017
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants