-
Notifications
You must be signed in to change notification settings - Fork 33
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
Update support for MSI authentication #13
Conversation
…rvice-msi-support
@@ -0,0 +1,110 @@ | |||
import { MSITokenCredentials } from "./msiTokenCredentials"; | |||
import { MSIAppServiceOptions, TokenResponse, Callback } from "../login"; | |||
import * as request from "request"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the request library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* {object} [tokenResponse] The tokenResponse (tokenType and accessToken are the two important properties). | ||
*/ | ||
getToken(callback: Callback<TokenResponse>): void { | ||
const postUrl = `http://localhost:${this.port}/oauth2/token`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this only works on an Azure VM?
When this is ready please bump the minor version and promote the auto-published preview to |
}); | ||
|
||
it("should throw on request with empty resource", function (done) { | ||
it("should throw on request with empty resource", async (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only matters if you actually use it--but note that individual test configuration (conditional skip, per-test timeout, etc) is done by using the dynamic this
inside the test function, which requires the old function syntax.
It actually looks like there is no auto-publish for this 📦 |
/** | ||
* @property {string} access_token - The access token. | ||
* TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a TODO?
* @param {object} [options] - Optional parameters | ||
* @param {string} [options.resource] - The resource uri or token audience for which the token is needed. | ||
* For e.g. it can be: | ||
* - resource management endpoint "https://management.azure.com"(default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation does not end with forward slash, whereas the actual value does. One more thing, it is better to define a constant in lib/util/authConstants.ts and then reference it everywhere.
Corresponds to:
Azure/azure-sdk-for-node@8574c83#diff-312a751ba50d2de2472f78dd7d9a5d8f
Azure/azure-sdk-for-node@fec0b11#diff-312a751ba50d2de2472f78dd7d9a5d8f
Azure/azure-sdk-for-node@2161322#diff-312a751ba50d2de2472f78dd7d9a5d8f
Azure/azure-sdk-for-node@fb804ac#diff-312a751ba50d2de2472f78dd7d9a5d8f
Azure/azure-sdk-for-node@90f5190#diff-312a751ba50d2de2472f78dd7d9a5d8f