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

Replace authentication handlers with handlers from typed-rest-client #175

Closed
stephenmichaelf opened this issue Apr 16, 2018 · 14 comments · Fixed by #206
Closed

Replace authentication handlers with handlers from typed-rest-client #175

stephenmichaelf opened this issue Apr 16, 2018 · 14 comments · Fixed by #206
Assignees

Comments

@stephenmichaelf
Copy link
Member

We don't need to have the handlers in this repo, remove them.

In WebApi we can keep the helper methods to create auth handlers and have them instantiate from the typed-rest-client handlers.

We probably want to bump the dependency version for typed-rest-client to make sure we are getting the latest version that has the working implementation of the NTLM handler.

@nadavsinai
Copy link

nadavsinai commented Aug 20, 2018

Hi @stephenmichaelf ,
I've tried to make vso-node-api work with our on-prem TFS (2018 now) for a while now . I wrote about this in issue #114. didn't have the time to look deeply into the matter until now.
this time around I cloned the repo and looked at the source, I found exactly what you describe in this issue and did exactly what you did in our branch in our fork algotec/vso-node-api branch support-ntlm

  • update typed-rest-client to 1.0.7
  • remove duplicated handlers and types and use ones from the rest lib.

good news I manage to authenticate the initial conntectionData request !
I can see in the headers the NTLM process is successful.
BUT

altough the type 3 message is sent and the response has a 200 status but when I get to res.readBody() in line 149 of RestClient.js (trying to read the final response)
the process unexpectedly quits immediately.

I have no further clues.
Have you any progress on this issue? when will your branch get merged?

@stephenmichaelf
Copy link
Member Author

Are you calling readBody multiple times? Calling it right when you get 200 should work. I haven't done a PR for this fix yet.

@nadavsinai
Copy link

no, not calling it more than once, it fails already on the first time.
I am waiting for your official PR, doesn't seem that anyone got on prem TFS autenticated with NTLM yet.

@nadavsinai
Copy link

@stephenmichaelf or anyone else in VSTS team - can we get a sign to whether NTLM support will be available anytime soon? should I open a separate ticket here?
I've done what I can in the @algotec/vsts-node-api fork, i'm stuck with something I can not even debug - node just quits on me, not respecting any "break on exceptions" flag.

@stephenmichaelf
Copy link
Member Author

Except for a minor change the handlers in this repo haven't been changed in a while. Is there any version, pre this change that works? Or did it never work?

@nadavsinai
Copy link

never managed to get it to work....

@stephenmichaelf
Copy link
Member Author

I think we just need to pull in the handlers from the typed rest client, those are working. I will try and get to this next week. Can you use a different type of auth for now?

@stephenmichaelf
Copy link
Member Author

If this has never worked then you are the first person to try and use NTLM with this lib, or there is an issue with your setup :) I am not sure which.

@stephenmichaelf
Copy link
Member Author

Generally the use case for this is with a bearer token. I will keep this on the backlog and look into it. I would recommend using different auth if possible.

@nadavsinai
Copy link

My use case is this:
I have written a CLI which maintains the developers workspace - cloning, commiting and performing other git/tfs operations on behalf on the user.
I can not use a token for the CLI app (as I need the current user's permissions) and asking each user to create a personal access token is tiresome.
currently on first use I ask the users for their passwords (username is known from env) and I use basic authentication.
my IT department (and I) would like to convert it to NTLM to make it more secure....

@stephenmichaelf
Copy link
Member Author

@bryanmacfarlane To comment on ideal solution.

@damccorm
Copy link

I went ahead and added this to my PR upgrading to the latest typed-rest-client, it now uses typed-rest-client's handlers

@damccorm damccorm self-assigned this Sep 28, 2018
@nadavsinai
Copy link

hi @damccorm , @stephenmichaelf

Thank you for your work on the matter, I upgraded my code to use the new "azure-devops-node-api": "^6.6.0" but still could not use ntlm authentication.
The error is still slient and impossible to debug,
the code for my auth init looks like this (wrapped in an async function):

authHandler = vsts.getNtlmHandler(this.username, this.password, process.env.COMPUTERNAME, process.env.USERDOMAIN);
		let webAPI = new vsts.WebApi(collectionURL, authHandler, { ignoreSslError: true });
		console.log('log1',webAPI);
		await webAPI.connect();
		console.log('log2',webAPI);

the first log is displayed and the second isn't at all. the node process just quits
if I use the getPersonalAccessTokenHandler or getBasicHandler functions, everything works as expected.
Have you any experience with NTLM auth use/debug?

@damccorm
Copy link

damccorm commented Oct 2, 2018

@nadavsinai it looks like NTLM is indeed broken even with the move to typed-rest-client handlers, I'm looking into this but will track it through #172 since that's more dedicated to this exact issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants