[identity] Adding extensions API, identity-vscode, and identity-cache-persistence #15384

witemple-msft · 2021-05-24T17:42:44Z

This PR introduces an extension API for @azure/identity:

import extension from "<name of extension package>";
import { useIdentityExtension } from "@azure/identity";

useIdentityExtension(extension);

// Now identity is imbued with the power of your extension package.

The extension API is based on passing a function that receives a control interface to set certain, sanctioned properties within the MSAL internals. In the first case, the only option exposed is to set a cache plugin. The function can also run any other arbitrary initialization logic it requires.

There are two first-party extension packages:

@azure/identity-vscode, provides VisualStudioCodeCredential and an extension that will add it to the default credentials of DefaultAzureCredential
@azure/identity-cache-persistence registers a CachePlugin to the underlying MSAL flows, allowing some credentials (those based on public client) to utilize persistent token storage based on the host platform's secret management capabilities (DPAPI on windows, keychain on macOS, and libsecret on Linux).

Items:

Adding and restoring tests for the new packages as well as the extensions APIs.
Adding READMEs and documentation for the new extension packages, and adding information to the mainline identity package readme about extensions and the two new extension packages.
Reviewing and adding docstrings to all new functions.
Adding samples (currently, the samples are using my dev appconfig instance scope as an example, for the sake of testing, but we can add live testing capabilities to these packages and test with real scopes.
Needs architectural review.
Ideally, we would not have to expose ICachePlugin in the public API surface (through the extension API), but it may be beneficial to do so to enable future scenarios such as distributed caching.
Currently, the only way that VisualStudioCodeCredential works is by exposing IdentityClient through the identity package API surface and duplicating the logging code, but this isn't desired. A code-sharing approach or common core package is required to remove this leak.

* [identityg Remove machine-code-dependent codepaths * Linter fixes * Disable broken perf test * Added missing import back

…ve-extensions

sdk/identity/identity-persistence/samples-dev/basic.ts

sadasant · 2021-05-24T17:55:12Z

sdk/identity/identity-persistence/tsconfig.json

+  "extends": "../../../tsconfig.package",
+  "compilerOptions": {
+    "target": "es6",
+    "lib": ["DOM"],


Persistance will, for sure, not work on browsers 🤔 I wonder if we should preemptively remove any browser-related logic or configuration.

This ends up being required to build the tests, which import from identity sources. Identity uses lib: DOM, so we need it here for the same reason.

sdk/identity/identity/review/identity.api.md

sadasant · 2021-05-24T18:43:24Z

Ok ok! I like how this is looking. We could get something like this merged and work together on the remaining tasks. For that, I propose the following:

Do not expose anything to end-users that may have any reference to MSAL to be consistent with our efforts in the past and with other languages. Pick some other name that can be meaningful and have no references to MSAL.
Let’s open issues for all of the remaining tasks that won’t be part of this PR to put them in the budget and place them in the schedule. This will help us divide and conquer!

Let me know what you think 🌞

sadasant · 2021-05-24T18:52:13Z

When I say that we can get something like this merged, I don’t mean today. What I mean to say is that: draw a line eventually, and make sure that what couldn’t get in is expressed in the form of issues so that we can follow up together.

…ve-extensions

xirzec

I'm glad we're splitting up identity! A few thoughts along the way, mostly I am confused by global object magic now that we're using an explicit function call to activate extensions.

sdk/identity/identity-persistence/package.json

xirzec · 2021-06-02T16:58:08Z

sdk/identity/identity-persistence/package.json

+  "sideEffects": false,
+  "dependencies": {
+    "@azure/core-auth": "^1.3.0",
+    "@azure/identity": "^2.0.0-beta.4",


It feels a little weird to me that the extension depends on the base lib it is extending -- is this for interfaces?

Yes. It's probably not required for VSCode anymore, but it is required for cache-persistence because it's where we get the options type for the cache from.

xirzec · 2021-06-02T16:59:16Z

sdk/identity/identity-persistence/review/identity-persistence.api.md

+// @public (undocumented)
+const persistence: unique symbol;
+
+export default persistence;


should this be a function, so that the library can receive optional configuration at composition time? I could see this being useful for skipping platform detection in some cases

In the new model without the symbolic stuff, this is already a function that takes a "context". Currently, neither of the extensions are equipped to take any configuration options, so do you think we could revisit this idea in a future beta if there's a clearer picture? There isn't any reason that two extension packages couldn't export different shapes, i.e. one package exports an extension directly and the other exports an extension factory that accepts options.

sdk/identity/identity-persistence/samples-dev/basic.ts

sdk/identity/identity-vscode/package.json

xirzec · 2021-06-02T17:04:46Z

sdk/identity/identity-vscode/tsconfig.json

+  "extends": "../../../tsconfig.package",
+  "compilerOptions": {
+    "target": "es6",
+    "lib": ["DOM"],


similarly, this shouldn't work in the browser, yeah?

For vscode it's fine. For cache-persistence it is required to build the tests, which reach into the identity source code during the build.

sdk/identity/identity/review/identity.api.md

sdk/identity/identity/src/extensionProvider.ts

…red.

…ve-extensions

…al.ts Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

…ve-extensions

… other than Node 14

…-extensions

xirzec

Awesome!! This is a great change and I'm so happy you've landed it.

Now if we can just get identity onto corev2... 😉

xirzec · 2021-06-18T20:30:58Z

sdk/identity/identity-cache-persistence/.eslintrc.json

+  "rules": {
+    "@azure/azure-sdk/ts-package-json-module": "off",
+    "@azure/azure-sdk/ts-modules-only-named": "off",
+    "@azure/azure-sdk/ts-package-json-engine-is-present": "off"


Can you file an issue to fix the linter?

sdk/identity/identity-cache-persistence/README.md

sdk/identity/identity-cache-persistence/samples-dev/extension.ts

sdk/identity/identity-vscode/samples-dev/extension.ts

sdk/identity/identity-vscode/samples-dev/nodeEnv.ts

sdk/identity/identity/src/extensions/consumer.ts

…-extensions

Update readme.md (Azure#15384)

witemple-msft added 10 commits April 26, 2021 16:09

[identity] Remove machine-code-dependent codepaths (Azure#15024)

a60a631

* [identityg Remove machine-code-dependent codepaths * Linter fixes * Disable broken perf test * Added missing import back

[identity] Add extension registration API and default credential stack

6ece97c

[identity-native] Introducing identity-native

459a916

Merge remote-tracking branch 'upstream/master' into identity-dev/nati…

39e070e

…ve-extensions

[identity] Rework extension API.

3ce02a0

Remove identity native

329f5a6

Identity-persistence module

791d303

Adding identity-vscode module

6f1cdb4

Updated rush configuration

5bdf482

Moved identity perf back to linking local identity

88ad371

witemple-msft added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels May 24, 2021

witemple-msft self-assigned this May 24, 2021

sadasant reviewed May 24, 2021

View reviewed changes

sdk/identity/identity-persistence/samples-dev/basic.ts Outdated Show resolved Hide resolved

sadasant reviewed May 24, 2021

View reviewed changes

witemple-msft mentioned this pull request May 26, 2021

Board Review: Introducing an @azure/identity extension API, @azure/identity-persistence and @azure/identity-vscode Azure/azure-sdk#2896

Closed

witemple-msft added 4 commits May 28, 2021 10:41

Merge remote-tracking branch 'upstream/master' into identity-dev/nati…

3d4987d

…ve-extensions

Symbolic refactor of extension system

9e2efc8

Renamed MsalCredentialOptions to CacheableCredentialOptions

5eab663

Use Symbol.for instead of hacking around SymbolConstructor

249b666

xirzec reviewed Jun 2, 2021

View reviewed changes

witemple-msft added 7 commits June 2, 2021 11:11

Improved identity-vscode samples

4f82fc2

Use persistence by default if a persistence provider has been registe…

7e4d8ec

…red.

Improved identity-persistence samples

9b095e4

Merge remote-tracking branch 'upstream/master' into identity-dev/nati…

b12b361

…ve-extensions

Rename to identity-cache-persistence

ad318d1

Feedback from arch board for mainline identity package

15dc574

Rename CredentialPersistenceOptions, format and lint

1b381e7

witemple-msft and others added 20 commits June 16, 2021 10:18

Merge remote-tracking branch 'upstream/master' into identity-dev/nati…

8e03c5d

…ve-extensions

Mock keytar using sinon in live/playback environments

719439a

Disable persistence tests on node !== 12

32240e1

Addressed feedback in identity-cache-persistence

ec98363

Restore perf-test

be56a0f

Removed sanitizeScopeUrl from recorder

1ab9475

Update sdk/identity/identity/src/credentials/visualStudioCodeCredenti…

da784b2

…al.ts Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

Fix gitignores

299d120

Feedback for identity-vscode

bc84872

Changelog entry for recorder changes.

d26f506

core-http -> core-auth

a17f348

setter methods instead of setter props

d394573

Some changes to docs and keywords

982622b

Merge remote-tracking branch 'upstream/master' into identity-dev/nati…

e7a67f7

…ve-extensions

Fixed linter warning

f081aec

Switch persistence tests from 12 to 14.

a969a04

Make the tests even refuse to import msal-node-extensions on anything…

5bb3d56

… other than Node 14

Reverted changes to recorder

9e6dcb1

Merge remote-tracking branch 'upstream/main' into identity-dev/native…

3f9d3ea

…-extensions

master -> main

417eb28

Azure deleted a comment from check-enforcer bot Jun 18, 2021

More master -> main

9e9e9d4

witemple-msft requested review from sadasant and xirzec June 18, 2021 19:40

xirzec approved these changes Jun 18, 2021

View reviewed changes

witemple-msft added 2 commits June 21, 2021 08:49

[identity] Make the samples extract only part of the token data

e062663

Merge remote-tracking branch 'upstream/main' into identity-dev/native…

99fc43b

…-extensions

witemple-msft mentioned this pull request Jun 21, 2021

[eslint-plugin-azure-sdk] Relax package.json module field rule #15873

Closed

witemple-msft merged commit 4471210 into Azure:main Jun 21, 2021

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Aug 2, 2021

CodeGen from PR 15384 in Azure/azure-rest-api-specs

04bf39a

Update readme.md (Azure#15384)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[identity] Adding extensions API, identity-vscode, and identity-cache-persistence #15384

[identity] Adding extensions API, identity-vscode, and identity-cache-persistence #15384

witemple-msft commented May 24, 2021 •

edited

Loading

sadasant May 24, 2021

witemple-msft Jun 10, 2021

sadasant commented May 24, 2021 •

edited

Loading

sadasant commented May 24, 2021

xirzec left a comment

xirzec Jun 2, 2021

witemple-msft Jun 14, 2021

xirzec Jun 2, 2021

witemple-msft Jun 10, 2021

xirzec Jun 2, 2021

witemple-msft Jun 10, 2021

xirzec left a comment

xirzec Jun 18, 2021

[identity] Adding extensions API, identity-vscode, and identity-cache-persistence #15384

[identity] Adding extensions API, identity-vscode, and identity-cache-persistence #15384

Conversation

witemple-msft commented May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant commented May 24, 2021 • edited Loading

sadasant commented May 24, 2021

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witemple-msft commented May 24, 2021 •

edited

Loading

sadasant commented May 24, 2021 •

edited

Loading