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

Pluggable sspi-client #41

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

Conversation

Suraiya-Hameed
Copy link
Collaborator

Refactored sspi-client to go in hand with tediousjs/tedious#624 (native.js was moved from there)

To use windows integrated auth in tedious, the connection would be,

var NativeAuthProvider = require('sspi-client');
const connection = new Connection({
...
authProvider: NativeAuthProvider({securityPackage: 'negotiate'}),
...
})

@arthurschreiber Can you have a look at this?

TODO

  • add tests for native.js
  • set negotiate as default security package, if undefined

Suraiya Hameed added 3 commits October 27, 2017 15:20
@arthurschreiber
Copy link

Heya @v-suhame 👋,

I'm really sorry I did not get to this sooner! 😞

What do you think about actually keeping the authentication provider for tedious and the sspi-client itself as separate modules? 🤔

The way sspi-client is written right now, it seems to be re-usable and not specific to tedious, which I believe is a good thing. Small, focussed modules is seen as a best practice in the Node.js ecosystem.

Whatever you decide here, I'll also leave a separate review for the code as it is proposed in this pull request. 😄

Also, on an unrelated note, I remember you suggested transferring sspi-client to the tediousjs organization -- I think that'd be a great idea! 👍

// what to do if the module is not supported on the platform where it's running.
if (require('semver').gte(process.version, '4.0.0') && os.type() === 'Windows_NT') {
module.exports.ModuleSupported = true;
const NativeAuthProvider = require('./native.js').NativeAuthProvider;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about removing the .js extension from the paths passed to the require calls?

// in applications like Tedious which supports older version of node.js, even
// if the functionality itself won't be available. The application can decide
// what to do if the module is not supported on the platform where it's running.
if (require('semver').gte(process.version, '4.0.0') && os.type() === 'Windows_NT') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about putting the version requirement for Node.js into the package.json file, and removing it from here? That'd allow you to get rid of the semver dependency as well.

@@ -0,0 +1,78 @@

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about removing this whitespace here?

@Suraiya-Hameed
Copy link
Collaborator Author

What do you think about actually keeping the authentication provider for tedious and the sspi-client itself as separate modules?

👍 I'm thinking of doing the same for node-kerberos too, it will be easier to get the changes from its parent package

@tvrprasad can you transfer sspi-client to tediousjs at your convenience 😄 Thanks!

@tvrprasad
Copy link
Owner

@tvrprasad can you transfer sspi-client to tediousjs at your convenience 😄 Thanks!

If you can share the steps on how I may do that, I'll be happy to do that. Is there some permission I have that you don't that makes it necessary that I do it?

If you're able to do it, please go for it. You have my permission :-)

@Suraiya-Hameed
Copy link
Collaborator Author

@tvrprasad
Copy link
Owner

@v-suhame I got an error saying I don't have permission to create a repository under tediousjs. Can you give me permission so I can complete the transfer?

@Suraiya-Hameed
Copy link
Collaborator Author

I don't have that access either 😅
@arthurschreiber would be the right person for that.

@tvrprasad
Copy link
Owner

@arthurschreiber Can you just fork sspi-client and transfer to tediousjs? Or you can give me temporary permission to create a repository under tediousjs and I'll do the transfer.

@gannons gannons mentioned this pull request Jun 16, 2020
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.

3 participants