Add support for token based auth to services other than VSTS #6440
Conversation
| let lineEnd = os.EOL; | ||
| let nerfed = util.toNerfDart(url); | ||
| let password64 = (new Buffer(password).toString('base64')); | ||
| console.log("##vso[task.setvariable variable=" + endpointId + "BASE64_PASSWORD;issecret=true;]" + password64); |
There was a problem hiding this comment.
keeping this console.log is important, it basically says "this string is a password, obfuscate it in any logging"
There was a problem hiding this comment.
| @@ -36,6 +40,12 @@ export class NpmRegistry implements INpmRegistry { | |||
|
|
|||
| try { | |||
| url = NormalizeRegistry(tl.getEndpointUrl(endpointId, false)); | |||
| if (endpointAuth.scheme === 'Token' && | |||
| URL.parse(url).hostname.toUpperCase().endsWith('.VISUALSTUDIO.COM')){ | |||
There was a problem hiding this comment.
I really don't like this. I suggest renaming the auth type of the endpoint back to 'external VSTS/TFS', and adding a third 'authToken' option.
There was a problem hiding this comment.
I considered that; however, I don't like the additional complexity/choices that a user would now need to make. The root of the problem, IMO, is that we don't have a JWT generator that has the same capabilities as the PAT generator under security settings. A JWT would work with _authToken a PAT; however, does not.
There was a problem hiding this comment.
We have two options:
- Add a third endpoint type specific to VSTS, so we have Basic, AuthToken (bearer), and VSTS PAT.
- Make the current AuthToken to use bearerToken only and update our guidance for current VSTS customers to use the Basic type and put whatever in the username field.
There was a problem hiding this comment.
2 would break existing users
There was a problem hiding this comment.
Yes, forgot to add that 1 would make AuthToken bearer only, which would break existing users as well. This seems like the best option to keep existing users working without having to bump the major version.
There was a problem hiding this comment.
As we discussed, this is the best approach as it it the least impact on the front-end decision process while taking advantage of the information we have to "do the right thing".
| if (endpointAuth.scheme === 'Token' && | ||
| URL.parse(url).hostname.toUpperCase().endsWith('.VISUALSTUDIO.COM')){ | ||
| // VSTS does not support a PAT in _authToken. Therefore, you must do basic. | ||
| endpointAuth.scheme = 'CrossAccount'; |
There was a problem hiding this comment.
minor, but would be better to treat endpointAuth as immutable and not set a scheme type that it doesn't really support.
|
I would also add some unit tests for the |
keithrob
force-pushed
the
users/kerobert/fixTokenAuth
branch
3 times, most recently
from
8fee658 to
cd66a16
Compare
|
@aldoms, tests added. |
keithrob
force-pushed
the
users/kerobert/fixTokenAuth
branch
from
cd66a16 to
203e9c8
Compare
keithrob
force-pushed
the
users/kerobert/fixTokenAuth
branch
from
203e9c8 to
6e10aa8
Compare
No description provided.