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

[Identity] Add VSCodeCredential #7994

Merged
merged 5 commits into from
Mar 25, 2020
Merged

[Identity] Add VSCodeCredential #7994

merged 5 commits into from
Mar 25, 2020

Conversation

sophiajt
Copy link
Contributor

This adds the VSCodeCredential and adds it to the DefaultAzureCredential.

Drafting for feedback before I finish.

This PR also adds the user-defined managed identity to the DefaultAzureCredential.

if (scopeString.indexOf("offline_access") < 0) {
scopeString += " offline_access";
}
let refreshToken = await keytar.findPassword("VS Code Azure");
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should try to avoid reading this every time getToken is called. Do you know what the performance of it is?

Copy link
Member

Choose a reason for hiding this comment

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

VS Code Azure should be a const with a descriptive name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Performance-wise it's similar to Azure SDK credentials, perhaps a bit faster for the initial connection.

@sophiajt sophiajt changed the title DRAFT: Add VSCodeCredential [Identity] Add VSCodeCredential Mar 25, 2020
@sophiajt sophiajt marked this pull request as ready for review March 25, 2020 19:18
@sophiajt sophiajt requested a review from daviwil as a code owner March 25, 2020 19:18
Copy link
Member

@schaabs schaabs left a comment

Choose a reason for hiding this comment

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

LGTM. We should add some tests for this though. If not in this PR we should file an issue to track adding the coverage.

@sophiajt
Copy link
Contributor Author

@schaabs - sounds good, I'll file an issue for that.

@sophiajt sophiajt merged commit ffcc5db into Azure:master Mar 25, 2020
@sophiajt sophiajt deleted the keytar branch March 25, 2020 21:12
xirzec added a commit to xirzec/azure-sdk-for-js that referenced this pull request Mar 27, 2020
PR Azure#7994 introduced a build break to rollup by referencing `process`
inside `DefaultAzureCredential`.

This fix adds a browser version of `DefaultAzureCredential` without the reference
to `process`.
xirzec added a commit that referenced this pull request Mar 27, 2020
PR #7994 introduced a build break to rollup by referencing `process`
inside `DefaultAzureCredential`.

This fix adds a browser version of `DefaultAzureCredential` without the reference
to `process`.
xirzec added a commit to xirzec/azure-sdk-for-js that referenced this pull request Mar 28, 2020
PR Azure#7994 also introduced a problem where a new dependency `keytar` is not bundle safe and needed to be excluded in test bundles that referenced it indirectly via @azure/identity.
xirzec added a commit that referenced this pull request Mar 28, 2020
PR #7994 also introduced a problem where a new dependency `keytar` is not bundle safe and needed to be excluded in test bundles that referenced it indirectly via @azure/identity.
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