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

Projects
None yet
3 participants
@nixgadget
Contributor

nixgadget commented Oct 3, 2016

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

@nixgadget

This comment has been minimized.

Contributor

nixgadget commented Nov 9, 2016

What does it take to merge these changes upstream ?

@elhigu

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

Collaborator

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

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Collaborator

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.

README.md Outdated
@@ -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

Collaborator

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

Collaborator

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

@elhigu

elhigu approved these changes Nov 29, 2016

@elhigu

This comment has been minimized.

Collaborator

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 tgriesser:master Nov 30, 2016

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

Adding external authentication capability when using oracledb driver (t…
…griesser#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