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] Update identity plugin file structure so that versions aren't strictly tied to identity package #20340

Closed
813 changes: 615 additions & 198 deletions common/config/rush/pnpm-lock.yaml

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion sdk/identity/identity-cache-persistence/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
},
"files": [
"dist/",
"dist-esm/identity/src",
"dist-esm/identity-common/src",
"dist-esm/identity-cache-persistence/src",
"types/identity-cache-persistence.d.ts",
"README.md",
Expand Down Expand Up @@ -61,6 +61,7 @@
"dependencies": {
"@azure/core-auth": "^1.3.0",
"@azure/identity": "^2.0.1",
"@azure/msal-common": "^4.5.1",
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change required? Are we importing from it anywhere? Does this have to do with msalTestUtils?

"@azure/msal-node": "^1.4.0",
"@azure/msal-node-extensions": "1.0.0-alpha.13",
"keytar": "^7.6.0",
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity-cache-persistence/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { AzurePluginContext } from "../../identity/src/plugins/provider";
import { AzurePluginContext } from "../../identity-common/src/plugins/provider";
import { IdentityPlugin } from "@azure/identity";
import { createPersistenceCachePlugin } from "./provider";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,8 @@
/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */

import * as path from "path";
import {
ClientCertificateCredential,
TokenCachePersistenceOptions,
} from "../../../../identity/src";
import { MsalTestCleanup, msalNodeTestSetup } from "../../../../identity/test/msalTestUtils";
import { ClientCertificateCredential, TokenCachePersistenceOptions } from "@azure/identity";
import { msalNodeTestSetup, MsalTestCleanup } from "../../../../identity-common/test/msalTestUtils";
import { env, isPlaybackMode } from "@azure-tools/test-recorder";
import { ConfidentialClientApplication } from "@azure/msal-node";
import { MsalNode } from "../../../../identity/src/msal/nodeFlows/msalNodeCommon";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */

import { ClientSecretCredential, TokenCachePersistenceOptions } from "../../../../identity/src";
import { MsalTestCleanup, msalNodeTestSetup } from "../../../../identity/test/msalTestUtils";
import { ClientSecretCredential, TokenCachePersistenceOptions } from "@azure/identity";
import { msalNodeTestSetup, MsalTestCleanup } from "../../../../identity-common/test/msalTestUtils";
import { ConfidentialClientApplication } from "@azure/msal-node";
import { MsalNode } from "../../../../identity/src/msal/nodeFlows/msalNodeCommon";
import Sinon from "sinon";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */

import { DeviceCodeCredential, TokenCachePersistenceOptions } from "../../../../identity/src";
import { MsalTestCleanup, msalNodeTestSetup } from "../../../../identity/test/msalTestUtils";
import { DeviceCodeCredential, TokenCachePersistenceOptions } from "@azure/identity";
import { MsalTestCleanup, msalNodeTestSetup } from "../../../../identity-common/test/msalTestUtils";
import { MsalNode } from "../../../../identity/src/msal/nodeFlows/msalNodeCommon";
import { PublicClientApplication } from "@azure/msal-node";
import Sinon from "sinon";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

// We need to set up the plugin for the tests!

import { useIdentityPlugin } from "../../../../identity/src";
import { useIdentityPlugin } from "@azure/identity";

// The persistence tests have to run on the same version of Node that's used to
// install dependencies, currently 14.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */

import { MsalTestCleanup, msalNodeTestSetup } from "../../../../identity/test/msalTestUtils";
import { TokenCachePersistenceOptions, UsernamePasswordCredential } from "../../../../identity/src";
import { TokenCachePersistenceOptions, UsernamePasswordCredential } from "@azure/identity";
import { MsalNode } from "../../../../identity/src/msal/nodeFlows/msalNodeCommon";
import { PublicClientApplication } from "@azure/msal-node";
import Sinon from "sinon";
Expand Down
39 changes: 39 additions & 0 deletions sdk/identity/identity-common/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Azure Identity Common client library for JavaScript
Copy link
Member

Choose a reason for hiding this comment

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

NIT: This isn't a "client" library and we should freely break the README template here. Just "Azure Identity Common Plugin Library" or something like that.

I do think it's nice to have the README, even if we ditch the package.json or other associated files.


An internal support library for the various Azure Identity libraries.

This package contains common code that needs to be shared among the other Azure Identity libraries. It is not published to NPM and is not meant for usage by any other consumers.

## Identity client libraries

The following client libraries use this package:

- [@azure/identity](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/README.md)
- [@azure/identity-vscode](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity-vscode/README.md)
- [@azure/identity-cache-persistence](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity-cache-persistence/README.md)

## Getting started

For information on getting started, please see our [Identity client libraries](#identity-client-libraries).

## Key concepts

For information on key concepts, please see our [Identity client libraries](#identity-client-libraries).

## Examples

For examples, please see our [Identity client libraries](#identity-client-libraries).

## Next steps

For information on next steps, please see our [Identity client libraries](#identity-client-libraries).

## Troubleshooting

If you run into issues while using this library, directly or indirectly, please feel free to [file an issue](https://github.com/Azure/azure-sdk-for-js/issues/new).

## Contributing

If you'd like to contribute to this library, please read the [contributing guide](https://github.com/Azure/azure-sdk-for-js/blob/main/CONTRIBUTING.md) to learn more about how to build and test the code.

![Impressions](https://azure-sdk-impressions.azurewebsites.net/api/impressions/azure-sdk-for-js%2Fsdk%2Fidentity%2Fidentity-common%2FREADME.png)
75 changes: 75 additions & 0 deletions sdk/identity/identity-common/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
{
"name": "@azure/identity-common",
Copy link
Member

Choose a reason for hiding this comment

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

If we add this package.json here, we need to also add it to the rush configuration and set up a full build setup for it. There are pros and cons to that approach. At the end of the day I don't think that's what we want, and we need to make sure these files in identity-common are isolated modules without any external dependencies. That's problematic for the shared MSAL test configuration.

I think the easiest solution for the test configuration is probably just to duplicate it, much as it may pain me.

"sideEffects": false,
"sdk-type": "client",
"private": true,
"author": "Microsoft Corporation",
"version": "1.0.0",
"license": "MIT",
"description": "Common internal functionality for all of the Azure Identity clients and plugins in the Azure SDK for JavaScript",
"repository": "github:Azure/azure-sdk-for-js",
"keywords": [
"azure",
"cloud",
"active directory",
"authentication",
"credential",
"certificate",
"managed identity",
"client secret",
"access token"
],
"bugs": {
"url": "https://github.com/Azure/azure-sdk-for-js/issues"
},
"main": "./src/index.ts",
"module": "dist-esm/index.js",
"types": "./types/index.d.ts",
"engines": {
"node": ">=12.0.0"
},
"scripts": {
"audit": "echo skipped",
"build:samples": "echo skipped",
"build:es6": "tsc -p tsconfig.json",
"build:nodebrowser": "echo skipped",
"build:test": "echo skipped",
"build": "npm run clean && npm run extract-api && npm run build:es6 && npm run build:nodebrowser",
"check-format": "prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
"clean": "rimraf dist-esm dist-test types *.tgz *.log samples/typescript/dist",
"execute:js-samples": "echo skipped",
"execute:ts-samples": "echo skipped",
"execute:samples": "npm run build:samples && npm run execute:js-samples && npm run execute:ts-samples",
"extract-api": "echo skipped",
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"*.{js,json}\"",
"integration-test:browser": "echo skipped",
"integration-test:node": "echo skipped",
"integration-test:node:no-timeout": "echo skipped",
"integration-test": "npm run integration-test:node && npm run integration-test:browser",
"lint:fix": "eslint package.json src --ext .ts --fix --fix-type [problem,suggestion]",
"lint": "eslint package.json src --ext .ts",
"pack": "npm pack 2>&1",
"test:browser": "npm run clean && npm run build:test && npm run unit-test:browser",
"test:node": "npm run clean && npm run build:test && npm run unit-test:node",
"test": "npm run clean && npm run build:test && npm run unit-test",
"unit-test:browser": "echo skipped",
"unit-test:node": "echo skipped",
"unit-test:node:no-timeout": "echo skipped",
"unit-test": "npm run unit-test:node && npm run unit-test:browser",
"docs": "echo Skipped."
},
"dependencies": {
"@azure/core-client": "^1.4.0",
"tslib": "^2.2.0"
},
"devDependencies": {
"@azure/test-utils": "^1.0.0",
"@azure/eslint-plugin-azure-sdk": "^3.0.0",
"@azure-tools/test-recorder": "^1.0.0",
"eslint": "^7.15.0",
"prettier": "^2.5.1",
"rimraf": "^3.0.0",
"typescript": "~4.2.0",
"sinon": "^9.0.2"
}
}
4 changes: 4 additions & 0 deletions sdk/identity/identity-common/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.

export * from "../src";
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this. This is src/index.ts, and it does export * from "../src", so doesn't this module refer to this same file?

Do we need this file at all?

31 changes: 31 additions & 0 deletions sdk/identity/identity-common/src/plugins/provider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { CachePluginControl } from "../../../identity/src/plugins/provider";
import { VSCodeCredentialFinder } from "../visualStudioCodeCredentialPlugin";

/**
* The type of an Azure Identity plugin, a function accepting a plugin
* context.
*/
export type IdentityPlugin = (context: unknown) => void;

/**
* Plugin context entries for controlling VisualStudioCodeCredential.
*/
export interface VisualStudioCodeCredentialControl {
setVsCodeCredentialFinder(finder: VSCodeCredentialFinder): void;
}

/**
* Context options passed to a plugin during initialization.
*
* Plugin authors are responsible for casting their plugin context values
* to this type.
*
* @internal
*/
export interface AzurePluginContext {
cachePluginControl: CachePluginControl;
vsCodeCredentialControl: VisualStudioCodeCredentialControl;
}
100 changes: 100 additions & 0 deletions sdk/identity/identity-common/test/msalTestUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// Copyright (c) Microsoft Corporation.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should move tests to identity-common, just sources.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the utility file for the tests that the plugin packages depend on. I had to do that to silence the build errors that I was receiving because of the utils for tests and sources from plugins

// Licensed under the MIT license.

import {
record,
Recorder,
RecorderEnvironmentSetup,
TestContextInterface,
pluginForIdentitySDK,
} from "@azure-tools/test-recorder";
import Sinon, { createSandbox } from "sinon";
import { MsalBaseUtilities } from "../../identity/src/msal/utils";

export type MsalTestCleanup = () => Promise<void>;

export interface MsalTestSetupResponse {
cleanup: MsalTestCleanup;
recorder: Recorder;
sandbox: Sinon.SinonSandbox;
}

export function msalNodeTestSetup(
testContext: TestContextInterface | Mocha.Context
): MsalTestSetupResponse {
const playbackValues = {
correlationId: "client-request-id",
};
const recorderEnvSetup: RecorderEnvironmentSetup = {
replaceableVariables: {
AZURE_TENANT_ID: PlaybackTenantId,
AZURE_CLIENT_ID: "azure_client_id",
AZURE_CLIENT_SECRET: "azure_client_secret",
AZURE_USERNAME: "azure_username",
AZURE_PASSWORD: "azure_password",
},
customizationsOnRecordings: [
(recording: string): string =>
recording.replace(/"access_token":"[^"]*"/g, `"access_token":"access_token"`),
(recording: string): string =>
recording.replace(/"refresh_token":"[^"]*"/g, `"refresh_token":"refresh_token"`),
(recording: string): string =>
recording.replace(/refresh_token=[^&]*/g, `refresh_token=refresh_token`),
(recording: string): string =>
recording.replace(
/client-request-id=[a-z0-9-]*/g,
`client-request-id=${playbackValues.correlationId}`
),
(recording: string): string =>
recording.replace(/client_assertion=[a-zA-Z0-9-._]*/g, `client_assertion=client_assertion`),
(recording: string): string => recording.replace(/esctx=[a-zA-Z0-9-_]*/g, `esctx=esctx`),
(recording: string): string => recording.replace(/'fpc=[^;]*/g, `'fpc=fpc;`),
// Device code specific
(recording: string): string =>
recording.replace(/user_code":"[^"]*/g, `user_code":"USER_CODE`),
(recording: string): string =>
recording.replace(
/enter the code [A-Z0-9]* to authenticate/g,
`enter the code USER_CODE to authenticate`
),
(recording: string): string =>
recording.replace(/device_code":"[^"]*/g, `device_code":"DEVICE_CODE`),
(recording: string): string =>
recording.replace(/device_code=[^&]*/g, `device_code=DEVICE_CODE`),
(recording: string): string => recording.replace(/"interval": *[0-9]*/g, `"interval": 0`),
// This last part is a JWT token that comes from the service, that has three parts joined by a dot.
// Our fake id_token has the following parts encoded in base64 and joined by a dot:
// - {"typ":"JWT","alg":"RS256","kid":"kid"}
// - {"aud":"aud","iss":"https://login.microsoftonline.com/12345678-1234-1234-1234-123456789012/v2.0","iat":1615337163,"nbf":1615337163,"exp":1615341063,"aio":"aio","idp":"https://sts.windows.net/idp/","name":"Daniel Rodríguez","oid":"oid","preferred_username":"danrodri@microsoft.com","rh":"rh.","sub":"sub","tid":"12345678-1234-1234-1234-123456789012","uti":"uti","ver":"2.0"}
// - no_idea_whats_this
(recording: string): string =>
recording.replace(
/id_token":"[^"]*/g,
`id_token":"eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImtpZCJ9.eyJhdWQiOiJhdWQiLCJpc3MiOiJodHRwczovL2xvZ2luLm1pY3Jvc29mdG9ubGluZS5jb20vMTIzNDU2NzgtMTIzNC0xMjM0LTEyMzQtMTIzNDU2Nzg5MDEyL3YyLjAiLCJpYXQiOjE2MTUzMzcxNjMsIm5iZiI6MTYxNTMzNzE2MywiZXhwIjoxNjE1MzQxMDYzLCJhaW8iOiJhaW8iLCJpZHAiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9pZHAvIiwibmFtZSI6IkRhbmllbCBSb2Ryw61ndWV6Iiwib2lkIjoib2lkIiwicHJlZmVycmVkX3VzZXJuYW1lIjoiZGFucm9kcmlAbWljcm9zb2Z0LmNvbSIsInJoIjoicmguIiwic3ViIjoic3ViIiwidGlkIjoiMTIzNDU2NzgtMTIzNC0xMjM0LTEyMzQtMTIzNDU2Nzg5MDEyIiwidXRpIjoidXRpIiwidmVyIjoiMi4wIn0=.bm9faWRlYV93aGF0c190aGlz`
),
// client_info is base64-encoded JSON that contains information about the user and tenant IDs
// The following replaces it with some dummy JSON that uses a UID/UTID of 12345678-1234-1234-1234-123456789012
(recording) =>
recording.replace(
/client_info":"[^"]*/g,
'client_info":"eyJ1aWQiOiIxMjM0NTY3OC0xMjM0LTEyMzQtMTIzNC0xMjM0NTY3ODkwMTIiLCJ1dGlkIjoiMTIzNDU2NzgtMTIzNC0xMjM0LTEyMzQtMTIzNDU2Nzg5MDEyIn0K'
),
],
queryParametersToSkip: [],
onLoadCallbackForPlayback: pluginForIdentitySDK,
};
const recorder = record(testContext, recorderEnvSetup);
const sandbox = createSandbox();

const stub = sandbox.stub(MsalBaseUtilities.prototype, "generateUuid");
stub.returns(playbackValues.correlationId);

return {
sandbox,
recorder,
async cleanup() {
await recorder.stop();
sandbox.restore();
},
};
}
3 changes: 2 additions & 1 deletion sdk/identity/identity-vscode/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
},
"files": [
"dist/",
"dist-esm/identity/src",
"dist-esm/identity-common/src",
"dist-esm/identity-vscode/src",
"types/identity-vscode.d.ts",
"README.md",
Expand Down Expand Up @@ -60,6 +60,7 @@
"sideEffects": false,
"dependencies": {
"@azure/identity": "^2.0.1",
"@azure/msal-common": "^4.5.1",
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about the same line in identity-cache-persistence

"keytar": "^7.6.0",
"tslib": "^2.2.0"
},
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity-vscode/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { AzurePluginContext } from "../../identity/src/plugins/provider";
import { AzurePluginContext } from "../../identity-common/src/plugins/provider";
import { IdentityPlugin } from "@azure/identity";
import keytar from "keytar";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */
/* eslint-disable @typescript-eslint/no-require-imports */

import { MsalTestCleanup, msalNodeTestSetup } from "../../../../identity/test/msalTestUtils";
import { msalNodeTestSetup, MsalTestCleanup } from "../../../../identity-common/test/msalTestUtils";
import { VisualStudioCodeCredential } from "@azure/identity";
import assert from "assert";
import { isRecordMode } from "@azure-tools/test-recorder";
Expand Down
5 changes: 3 additions & 2 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ useIdentityPlugin(cachePersistencePlugin);
async function main() {
const credential = new DeviceCodeCredential({
tokenCachePersistenceOptions: {
enabled: true
}
enabled: true,
},
});
}
```
Expand Down Expand Up @@ -149,6 +149,7 @@ Azure Service Fabric support hasn't been added on the initial version 2 of Ident
- We have also renamed the error `CredentialUnavailable` to `CredentialUnavailableError`, to align with the naming convention used for error classes in the Azure SDKs in JavaScript.
- In v1 of Identity some `getToken` calls could resolve with `null` in the case the authentication request succeeded with a malformed output. In v2, issues with the `getToken` method will always throw errors.
- Breaking changes to InteractiveBrowserCredential

- The `InteractiveBrowserCredential` will use the [Auth Code Flow](https://docs.microsoft.com/azure/active-directory/develop/v2-oauth2-auth-code-flow) with [PKCE](https://tools.ietf.org/html/rfc7636) rather than [Implicit Grant Flow](https://docs.microsoft.com/azure/active-directory/develop/v2-oauth2-implicit-grant-flow) to better support browsers with enhanced security restrictions. Learn how to migrate in the [migration guide](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/migration-v1-v2.md). Read more about the latest `InteractiveBrowserCredential` [here](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/interactive-browser-credential.md).
- The default client ID used for `InteractiveBrowserCredential` was viable only in Node.js and not for the browser. Therefore, on v2 client ID is a required parameter when using this credential in browser apps.
- Identity v2 also removes the `postLogoutRedirectUri` from the options to the constructor for `InteractiveBrowserCredential`. This option wasn't being used. Instead of using this option, use MSAL directly. For more information, see [Authenticating with the @azure/msal-browser Public Client](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/samples/AzureIdentityExamples.md#authenticating-with-the-azuremsal-browser-public-client).
Expand Down
Loading