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

Jump to bottom

Identity project noUnusedParameters, noImplicitReturns and noUnusedLocals tsconfig fixes #11453

Merged
merged 17 commits into from
Oct 2, 2020
Merged

Identity project noUnusedParameters, noImplicitReturns and noUnusedLocals tsconfig fixes #11453

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3887399
Extend tsconfig in cosmos, identity from base config
ramya-rao-a Sep 23, 2020
432ba01
identity noImplicitReturns tslint fixes
mohsin-mehmood Sep 24, 2020
6dd2c86
identity noUnusedParameters tslint fixes
mohsin-mehmood Sep 24, 2020
f8fb6a8
identity noUnusedLocals tslint fixes
mohsin-mehmood Sep 24, 2020
5fb75dc
Merge remote-tracking branch 'upstream/master' into identity-tsconfig…
mohsin-mehmood Sep 24, 2020
8dc31bb
Merge remote-tracking branch 'upstream/master' into identity-tsconfig…
mohsin-mehmood Sep 24, 2020
17eacfb
Restore options param to getToken method
mohsin-mehmood Sep 24, 2020
e7f3426
Merge remote-tracking branch 'upstream/master' into identity-tsconfig…
mohsin-mehmood Sep 24, 2020
5a08123
Use _options param in getToken method
mohsin-mehmood Sep 24, 2020
525feed
Merge remote-tracking branch 'upstream/master' into identity-tsconfig…
mohsin-mehmood Sep 25, 2020
84d7f75
Restore options param for getToken method
mohsin-mehmood Sep 25, 2020
fbc0688
Removed redundant compilerOptions
mohsin-mehmood Sep 25, 2020
1c57627
update getAzureCliAccessToken return type
mohsin-mehmood Sep 25, 2020
18f18b6
Remove eslint-disable no-unused-vars comment
mohsin-mehmood Sep 25, 2020
afa3697
Merge remote-tracking branch 'upstream/master' into identity-tsconfig…
Sep 26, 2020
585a436
Merge remote-tracking branch 'upstream/master' into identity-tsconfig…
Sep 29, 2020
68efa0a
Merge remote-tracking branch 'upstream/master' into identity-tsconfig…
Oct 1, 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
4 changes: 2 additions & 2 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 @@ -134,7 +134,7 @@ export { GetTokenOptions }
// @public
export class InteractiveBrowserCredential implements TokenCredential {
constructor(options?: InteractiveBrowserCredentialOptions);
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
getToken(scopes: string | string[], _options?: GetTokenOptions): Promise<AccessToken | null>;
mohsin-mehmood marked this conversation as resolved.
Show resolved Hide resolved
}

// @public
Expand Down Expand Up @@ -172,7 +172,7 @@ export class UsernamePasswordCredential implements TokenCredential {
// @public
export class VisualStudioCodeCredential implements TokenCredential {
constructor(options?: VisualStudioCodeCredentialOptions);
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
getToken(scopes: string | string[]): Promise<AccessToken | null>;
mohsin-mehmood marked this conversation as resolved.
Show resolved Hide resolved
}

// @public
Expand Down
16 changes: 3 additions & 13 deletions sdk/identity/identity/src/credentials/authorizationCodeCredential.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

/* eslint-disable @typescript-eslint/no-unused-vars */
mohsin-mehmood marked this conversation as resolved.
Show resolved Hide resolved

import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { TokenCredential, AccessToken } from "@azure/core-http";
import { TokenCredentialOptions } from "../client/identityClient";
import { credentialLogger, formatError } from "../util/logging";

Expand All @@ -28,22 +28,12 @@ export class AuthorizationCodeCredential implements TokenCredential {
redirectUri: string,
options?: TokenCredentialOptions
);
constructor(
tenantId: string | "common",
clientId: string,
clientSecretOrAuthorizationCode: string,
authorizationCodeOrRedirectUri: string,
redirectUriOrOptions: string | TokenCredentialOptions | undefined,
options?: TokenCredentialOptions
) {
constructor() {
logger.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}

public getToken(
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
public getToken(): Promise<AccessToken | null> {
logger.getToken.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}
Expand Down
7 changes: 3 additions & 4 deletions sdk/identity/identity/src/credentials/azureCliCredential.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,19 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { AccessToken, TokenCredential, GetTokenOptions } from "@azure/core-http";
import { TokenCredentialOptions } from "../client/identityClient";
import { AccessToken, TokenCredential } from "@azure/core-http";
import { credentialLogger, formatError } from "../util/logging";

const BrowserNotSupportedError = new Error("AzureCliCredential is not supported in the browser.");
const logger = credentialLogger("AzureCliCredential");

export class AzureCliCredential implements TokenCredential {
constructor(options?: TokenCredentialOptions) {
constructor() {
logger.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}

getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null> {
getToken(): Promise<AccessToken | null> {
logger.getToken.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/identity/identity/src/credentials/azureCliCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ export class AzureCliCredential implements TokenCredential {
* Gets the access token from Azure CLI
* @param resource The resource to use when getting the token
*/
protected async getAzureCliAccessToken(resource: string) {
protected async getAzureCliAccessToken(resource: string): Promise<unknown> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to choose unknown here instead of say inlining the type of the object containing the stdout, stderr and error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently in identity.api.md the return type of this method is Promise<unknown> and I was not sure if we should change the public API. Initially, I thought of setting it to Promise<any>

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe changing from unknown to a strongly typed value especially in the output of a method should not be a breaking change. Doing so from any would be breaking though.

cc @xirzec to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please review this commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Marking this conversation as unresolved to ensure we have input from @xirzec here

Copy link
Member

Choose a reason for hiding this comment

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

In-lining the type is better than unknown and any, though a small type alias or interface would be even better. I'm good with the linked commit though.

In this particular case, I wonder how much we have to worry about breaking changes for protected members. It seems like the only reason this method is protected is out of a misguided strategy for testing. We should be using something like sinon to mock out getAzureCliAccessToken instead of inheritance.

If @jonathandturner is willing, perhaps we could make this private unless we really do anticipate developers to replace this implementation in their live apps?

return new Promise((resolve, reject) => {
try {
child_process.exec(
`az account get-access-token --output json --resource ${resource}`,
{ cwd: getSafeWorkingDir() },
(error, stdout, stderr) => {
resolve({ stdout: stdout, stderr: stderr });
resolve({ stdout: stdout, stderr: stderr, error });
}
);
} catch (err) {
Expand Down
15 changes: 3 additions & 12 deletions sdk/identity/identity/src/credentials/clientCertificateCredential.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { TokenCredentialOptions } from "../client/identityClient";
import { TokenCredential, AccessToken } from "@azure/core-http";
import { credentialLogger, formatError } from "../util/logging";

const BrowserNotSupportedError = new Error(
Expand All @@ -13,20 +12,12 @@ const BrowserNotSupportedError = new Error(
const logger = credentialLogger("ClientCertificateCredential");

export class ClientCertificateCredential implements TokenCredential {
constructor(
tenantId: string,
clientId: string,
certificatePath: string,
options?: TokenCredentialOptions
) {
constructor() {
logger.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}

public getToken(
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
public getToken(): Promise<AccessToken | null> {
logger.getToken.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}
Expand Down
16 changes: 3 additions & 13 deletions sdk/identity/identity/src/credentials/deviceCodeCredential.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,19 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { DeviceCodePromptCallback } from "./deviceCodeCredential";
import { TokenCredentialOptions } from "../client/identityClient";
import { TokenCredential, AccessToken } from "@azure/core-http";
import { credentialLogger, formatError } from "../util/logging";

const BrowserNotSupportedError = new Error("DeviceCodeCredential is not supported in the browser.");
const logger = credentialLogger("DeviceCodeCredential");

export class DeviceCodeCredential implements TokenCredential {
constructor(
tenantId: string | "organizations",
clientId: string,
userPromptCallback: DeviceCodePromptCallback,
options?: TokenCredentialOptions
) {
constructor() {
logger.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}

public getToken(
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
public getToken(): Promise<AccessToken | null> {
logger.getToken.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}
Expand Down
12 changes: 5 additions & 7 deletions sdk/identity/identity/src/credentials/deviceCodeCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
import { AccessToken, TokenCredential, GetTokenOptions, delay } from "@azure/core-http";
import { TokenCredentialOptions, IdentityClient } from "../client/identityClient";
import { AccessToken, TokenCredential, GetTokenOptions } from "@azure/core-http";
import { TokenCredentialOptions } from "../client/identityClient";
import { createSpan } from "../util/tracing";
import { credentialLogger, formatSuccess } from "../util/logging";
import { AuthenticationError, AuthenticationErrorName } from "../client/errors";
import { credentialLogger } from "../util/logging";
import { AuthenticationErrorName } from "../client/errors";
import { CanonicalCode } from "@opentelemetry/api";

import { PublicClientApplication, DeviceCodeRequest } from "@azure/msal-node";
Expand Down Expand Up @@ -55,7 +55,6 @@ export function defaultDeviceCodePromptCallback(deviceCodeInfo: DeviceCodeInfo):
* that the user can enter into https://microsoft.com/devicelogin.
*/
export class DeviceCodeCredential implements TokenCredential {
private identityClient: IdentityClient;
private pca: PublicClientApplication;
private tenantId: string;
private clientId: string;
Expand All @@ -79,7 +78,6 @@ export class DeviceCodeCredential implements TokenCredential {
userPromptCallback: DeviceCodePromptCallback = defaultDeviceCodePromptCallback,
options?: TokenCredentialOptions
) {
this.identityClient = new IdentityClient(options);
this.tenantId = tenantId;
this.clientId = clientId;
this.userPromptCallback = userPromptCallback;
Expand Down Expand Up @@ -117,7 +115,7 @@ export class DeviceCodeCredential implements TokenCredential {
* TokenCredential implementation might make.
*/
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null> {
const { span, options: newOptions } = createSpan("DeviceCodeCredential-getToken", options);
const { span } = createSpan("DeviceCodeCredential-getToken", options);

const scopeArray = typeof scopes === "object" ? scopes : [scopes];

Expand Down
7 changes: 3 additions & 4 deletions sdk/identity/identity/src/credentials/environmentCredential.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { AccessToken, TokenCredential, GetTokenOptions } from "@azure/core-http";
import { TokenCredentialOptions } from "../client/identityClient";
import { AccessToken, TokenCredential } from "@azure/core-http";
import { credentialLogger, formatError } from "../util/logging";

const BrowserNotSupportedError = new Error(
Expand All @@ -13,12 +12,12 @@ const BrowserNotSupportedError = new Error(
const logger = credentialLogger("EnvironmentCredential");

export class EnvironmentCredential implements TokenCredential {
constructor(options?: TokenCredentialOptions) {
constructor() {
logger.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}

getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null> {
getToken(): Promise<AccessToken | null> {
logger.getToken.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}
Expand Down
26 changes: 5 additions & 21 deletions sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { TokenCredential, AccessToken, GetTokenOptions } from "@azure/core-http";
import { InteractiveBrowserCredentialOptions } from "./interactiveBrowserCredentialOptions";
import { credentialLogger, formatError } from "../util/logging";
import { TokenCredentialOptions, IdentityClient } from "../client/identityClient";
import { credentialLogger } from "../util/logging";
import { DefaultTenantId, DeveloperSignOnClientId } from "../constants";
import { Socket } from "net";

Expand All @@ -15,46 +14,30 @@ const SERVER_PORT = process.env.PORT || 80;
import express from "express";
import { PublicClientApplication, TokenCache, AuthorizationCodeRequest } from "@azure/msal-node";
import open from "open";
import path from "path";
import http from "http";

const BrowserNotSupportedError = new Error(
"InteractiveBrowserCredential is not supported in Node.js."
);
const logger = credentialLogger("InteractiveBrowserCredential");

interface AuthenticationRecord {
authority?: string;
homeAccountId: string;
environment: string;
tenantId: string;
username: string;
}

/**
* Enables authentication to Azure Active Directory inside of the web browser
* using the interactive login flow, either via browser redirects or a popup
* window. This credential is not currently supported in Node.js.
*/
export class InteractiveBrowserCredential implements TokenCredential {
private identityClient: IdentityClient;
private pca: PublicClientApplication;
private msalCacheManager: TokenCache;
private tenantId: string;
private clientId: string;
private persistenceEnabled: boolean;
private redirectUri: string;
private authorityHost: string;
private authenticationRecord: AuthenticationRecord | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonathandturner This was added in #10994
Was curious as to how you envisioned this being used as it is currently unused

Copy link
Contributor

Choose a reason for hiding this comment

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

@mohsin-mehmood Looks like the latest changes in the master branch do make use of the authenticationRecord. Can you pull in the changes from master and update the PR?


constructor(options?: InteractiveBrowserCredentialOptions) {
this.identityClient = new IdentityClient(options);
this.tenantId = (options && options.tenantId) || DefaultTenantId;
this.clientId = (options && options.clientId) || DeveloperSignOnClientId;

// Future update: this is for persistent caching
this.persistenceEnabled = false;
this.authenticationRecord = undefined;

if (options && options.redirectUri) {
if (typeof options.redirectUri === "string") {
Expand Down Expand Up @@ -100,7 +83,7 @@ export class InteractiveBrowserCredential implements TokenCredential {
*/
public getToken(
scopes: string | string[],
options?: GetTokenOptions
_options?: GetTokenOptions
): Promise<AccessToken | null> {
const scopeArray = typeof scopes === "object" ? scopes : [scopes];

Expand All @@ -124,13 +107,14 @@ export class InteractiveBrowserCredential implements TokenCredential {
let listen: http.Server | undefined;
let socketToDestroy: Socket | undefined;

function cleanup() {
function cleanup(): void {
if (listen) {
listen.close();
}
if (socketToDestroy) {
socketToDestroy.destroy();
}
return;
}

// Create Express App and Routes
Expand Down
12 changes: 3 additions & 9 deletions sdk/identity/identity/src/credentials/managedIdentityCredential.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { AccessToken, GetTokenOptions, TokenCredential } from "@azure/core-http";
import { AccessToken, TokenCredential } from "@azure/core-http";
import { TokenCredentialOptions } from "../client/identityClient";
import { credentialLogger, formatError } from "../util/logging";

Expand All @@ -15,18 +15,12 @@ const logger = credentialLogger("ManagedIdentityCredential");
export class ManagedIdentityCredential implements TokenCredential {
constructor(clientId: string, options?: TokenCredentialOptions);
constructor(options?: TokenCredentialOptions);
constructor(
clientIdOrOptions: string | TokenCredentialOptions | undefined,
options?: TokenCredentialOptions
) {
constructor() {
logger.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}

public async getToken(
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
public async getToken(): Promise<AccessToken | null> {
logger.getToken.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}
Expand Down
10 changes: 3 additions & 7 deletions sdk/identity/identity/src/credentials/visualStudioCodeCredential.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { TokenCredentialOptions } from "../client/identityClient";
import { TokenCredential, AccessToken } from "@azure/core-http";
import { credentialLogger, formatError } from "../util/logging";

const BrowserNotSupportedError = new Error(
Expand All @@ -13,15 +12,12 @@ const BrowserNotSupportedError = new Error(
const logger = credentialLogger("VisualStudioCodeCredential");

export class VisualStudioCodeCredential implements TokenCredential {
constructor(options?: TokenCredentialOptions) {
constructor() {
logger.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}

public getToken(
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
public getToken(): Promise<AccessToken | null> {
logger.getToken.info(formatError(BrowserNotSupportedError));
throw BrowserNotSupportedError;
}
Expand Down
7 changes: 2 additions & 5 deletions sdk/identity/identity/src/credentials/visualStudioCodeCredential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

/* eslint-disable @typescript-eslint/no-unused-vars */

import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { TokenCredential, AccessToken } from "@azure/core-http";
import { TokenCredentialOptions, IdentityClient } from "../client/identityClient";
import fs from "fs";
import os from "os";
Expand Down Expand Up @@ -140,10 +140,7 @@ export class VisualStudioCodeCredential implements TokenCredential {
* @param options The options used to configure any requests this
* `TokenCredential` implementation might make.
*/
public async getToken(
scopes: string | string[],
options?: GetTokenOptions
): Promise<AccessToken | null> {
public async getToken(scopes: string | string[]): Promise<AccessToken | null> {
await this.prepareOnce();
if (!keytar) {
throw new CredentialUnavailable(
Expand Down
2 changes: 2 additions & 0 deletions sdk/identity/identity/test/internal/identityClient.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ describe("IdentityClient", function() {
assert(new IdentityClient({ authorityHost: "https://correct.url" }));

delete process.env.AZURE_AUTHORITY_HOST;

return;
});

it("returns a usable error when the authentication response doesn't contain a body", async () => {
Expand Down
Loading