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.

Sign up for GitHub

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] Make credentials and DAC list public #9274

Merged
merged 3 commits into from
Jun 4, 2020
Merged

[Identity] Make credentials and DAC list public #9274

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc225dd
Make credentials and DAC list public
Jun 3, 2020
502d253
Add API docs
Jun 3, 2020
ce3b6b1
Fix copy/pasted comment
Jun 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions sdk/identity/identity/review/identity.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ export class AuthorizationCodeCredential implements TokenCredential {
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
}

// @public
export class AzureCliCredential implements TokenCredential {
constructor();
protected getAzureCliAccessToken(resource: string): Promise<unknown>;
Copy link
Member

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
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
Member

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
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.

getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
}

// @public
export type BrowserLoginStyle = "redirect" | "popup";

Expand All @@ -61,6 +68,7 @@ export class ClientSecretCredential implements TokenCredential {
// @public
export class DefaultAzureCredential extends ChainedTokenCredential {
constructor(tokenCredentialOptions?: TokenCredentialOptions);
static credentials(tokenCredentialOptions?: TokenCredentialOptions): TokenCredential[];
}

// @public
Expand Down Expand Up @@ -150,6 +158,12 @@ export class UsernamePasswordCredential implements TokenCredential {
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
}

// @public
export class VSCodeCredential implements TokenCredential {
constructor(options?: TokenCredentialOptions);
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
}


// (No @packageDocumentation comment for this package)

Expand Down
14 changes: 12 additions & 2 deletions sdk/identity/identity/src/credentials/defaultAzureCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { EnvironmentCredential } from "./environmentCredential";
import { ManagedIdentityCredential } from "./managedIdentityCredential";
import { AzureCliCredential } from "./azureCliCredential";
import { VSCodeCredential } from "./vscodeCredential";
import { TokenCredential } from "@azure/core-http";

/**
* Provides a default {@link ChainedTokenCredential} configuration for
Expand All @@ -21,11 +22,11 @@ import { VSCodeCredential } from "./vscodeCredential";
*/
export class DefaultAzureCredential extends ChainedTokenCredential {
/**
* Creates an instance of the DefaultAzureCredential class.
* Returns the list of credentials DefaultAzureCredential will use to authenticate.
*
* @param options Options for configuring the client which makes the authentication request.
*/
constructor(tokenCredentialOptions?: TokenCredentialOptions) {
static credentials(tokenCredentialOptions?: TokenCredentialOptions): TokenCredential[] {
let credentials = [];
credentials.push(new EnvironmentCredential(tokenCredentialOptions));
credentials.push(new ManagedIdentityCredential(tokenCredentialOptions));
Expand All @@ -35,6 +36,15 @@ export class DefaultAzureCredential extends ChainedTokenCredential {
credentials.push(new AzureCliCredential());
Copy link
Member

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
Contributor Author

@sophiajt sophiajt Jun 4, 2020 •
edited
Loading

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.

credentials.push(new VSCodeCredential(tokenCredentialOptions));

return credentials;
}
/**
* Creates an instance of the DefaultAzureCredential class.
*
* @param options Options for configuring the client which makes the authentication request.
*/
constructor(tokenCredentialOptions?: TokenCredentialOptions) {
let credentials = DefaultAzureCredential.credentials(tokenCredentialOptions);
super(
...credentials
);
Expand Down
15 changes: 15 additions & 0 deletions sdk/identity/identity/src/credentials/vscodeCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,27 @@ const VSCodeUserName = 'VS Code Azure';
export class VSCodeCredential implements TokenCredential {
private identityClient: IdentityClient;

/**
* Creates an instance of VSCodeCredential to use for automatically authenicating via VSCode.
*
* @param options Options for configuring the client which makes the authentication request.
*/
constructor(
options?: TokenCredentialOptions
) {
this.identityClient = new IdentityClient(options);
}

/**
* Returns the first access token returned by one of the chained
* `TokenCredential` implementations. Throws an {@link AggregateAuthenticationError}
* when one or more credentials throws an {@link AuthenticationError} and
* no credentials have returned an access token.
sophiajt marked this conversation as resolved.
Show resolved Hide resolved
*
* @param scopes The list of scopes for which the token will have access.
* @param options The options used to configure any requests this
* `TokenCredential` implementation might make.
*/
public async getToken(
scopes: string | string[],
options?: GetTokenOptions
Expand Down
3 changes: 3 additions & 0 deletions sdk/identity/identity/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ export { EnvironmentCredential } from "./credentials/environmentCredential";
export { ClientSecretCredential } from "./credentials/clientSecretCredential";
export { ClientCertificateCredential } from "./credentials/clientCertificateCredential";
export { InteractiveBrowserCredential } from "./credentials/interactiveBrowserCredential";
export { VSCodeCredential } from "./credentials/vscodeCredential";
export { AzureCliCredential } from "./credentials/azureCliCredential";

export {
InteractiveBrowserCredentialOptions,
BrowserLoginStyle
Expand Down