Skip to content

[Identity] Make credentials and DAC list public#9274

Merged
sophiajt merged 3 commits intoAzure:masterfrom
sophiajt:dac_credentials_public
Jun 4, 2020
Merged

[Identity] Make credentials and DAC list public#9274
sophiajt merged 3 commits intoAzure:masterfrom
sophiajt:dac_credentials_public

Conversation

@sophiajt
Copy link
Copy Markdown
Contributor

@sophiajt sophiajt commented Jun 3, 2020

This makes a few changes to adjust to the current DAC recommendation:

  • VSCodeCredential and AzureCliCredential are now publicly exported so devs can get to them directly
  • We also expose a helper method on DefaultAzureCredential which will return the same chain of credentials that DAC will create by default. This will allow users to explore them and learn from them as they see fit.

In addition to these changes, we should also include running the manual tests against a ChainedTokenCredential built from this new DAC helper

@sophiajt sophiajt requested a review from schaabs June 3, 2020 21:31
@sophiajt sophiajt requested review from bterlson and daviwil as code owners June 3, 2020 21:31
// @public
export class AzureCliCredential implements TokenCredential {
constructor();
protected getAzureCliAccessToken(resource: string): Promise<unknown>;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should this be refactored so it's not exposed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The mock will overwrite this. For that to work, it has to at least be protected. We can document that this isn't intended to be used publicly.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there not another way to mock / test this? I'm not a big fan of adding public surface area for testing purposes. If we can't fix it for this preview could we file an issue to fix before GA?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is coming from the mocking that is used for testing, since we can't override that method if it's private.

Can look into a better approach for the next preview.

@@ -35,6 +36,15 @@ export class DefaultAzureCredential extends ChainedTokenCredential {
credentials.push(new AzureCliCredential());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't tokenCredentialOptions also be passed to the AzureCliCredential?

Copy link
Copy Markdown
Contributor Author

@sophiajt sophiajt Jun 4, 2020

Choose a reason for hiding this comment

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

It doesn't currently accept or use the settings. We can take on some work for the next preview to bring AzureCli to the same style that the other credentials are using.

@sophiajt sophiajt merged commit 02348ea into Azure:master Jun 4, 2020
@sophiajt sophiajt deleted the dac_credentials_public branch June 4, 2020 23:17
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.

2 participants