Adopt the MSAL broker to talk to the OS for Microsoft auth #233739

TylerLeonhardt · 2024-11-13T01:37:17Z

This adopts the NativeBrokerPlugin provided by @azure/msal-node-extensions to provide the ability to use auth state from the OS, and show native auth dialogs instead of going to the browser.

This has several pieces:

The adoption of the broker in the microsoft-authentication extension:
- Adding NativeBrokerPlugin to our PCAs
- Using the proposed handle API to pass the native window handle down to MSAL calls (btw, this API will change in a follow up PR)
- Adopting an AccountAccess layer to handle:
  - giving the user control of which accounts VS Code uses
  - an eventing layer so that auth state can be updated across multiple windows
Getting the extension to build properly and only build what it really needs. This required several package.json/webpack hacks:
- Use a fake keytar since we don't use the feature in @azure/msal-node-extensions that uses keytar
- Use a fake dpapi layer since we don't use the feature in @azure/msal-node-extensions that uses it
- Ensure the msal runtime .node and .dll files are included in the bundle
Get the VS Code build to allow a native node module in an extension: by having a list of native extensions that will be built in the "ci" part of the build - in other words when VS Code is building on the target platform

There are a couple of followups:

Refactor the handle API to handle (heh) Auxiliary Windows Proposed API for window handle #233106
Separate the call to acquireTokenSilent and acquireTokenInteractive and all the usage of this native node module into a separate process or maybe in Core... we'll see. Something to experiment with after we have something working. NEEDS FOLLOW UP ISSUE

Fixes #229431

Copilot reviewed 5 out of 16 changed files in this pull request and generated no suggestions.

Files not reviewed (11)

extensions/microsoft-authentication/.vscodeignore: Language not supported
extensions/microsoft-authentication/package-lock.json: Language not supported
extensions/microsoft-authentication/package.json: Language not supported
extensions/microsoft-authentication/packageMocks/keytar/package.json: Language not supported
extensions/microsoft-authentication/tsconfig.json: Language not supported
build/lib/extensions.js: Evaluated as low risk
extensions/microsoft-authentication/src/node/publicClientCache.ts: Evaluated as low risk
build/gulpfile.extensions.js: Evaluated as low risk
build/gulpfile.vscode.js: Evaluated as low risk
extensions/microsoft-authentication/src/node/authProvider.ts: Evaluated as low risk
build/lib/extensions.ts: Evaluated as low risk

Comments skipped due to low confidence (7)

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts:7

Ensure that the NativeBrokerPlugin is correctly imported and used.

import { NativeBrokerPlugin } from '@azure/msal-node-extensions';

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts:29

Verify that ScopedAccountAccess is correctly instantiated and used.

private readonly _accountAccess = new ScopedAccountAccess(this._secretStorage, this._cloudName, this._clientId, this._authority);

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts:89

Ensure that initialize method is correctly called and handled.

await this._accountAccess.initialize();

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts:126

Ensure that setAllowedAccess method is correctly called and handled.

await this._accountAccess.setAllowedAccess(result.account!, true);

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts:134

Ensure that setAllowedAccess method is correctly called and handled.

return this._accountAccess.setAllowedAccess(account, false);

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts:141

Ensure that onDidAccountAccessChange method is correctly called and handled.

return this._accountAccess.onDidAccountAccessChange(() => this._update());

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts:180

Ensure that isAllowedAccess method is correctly called and handled.

after = after.filter(a => this._accountAccess.isAllowedAccess(a));

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

TylerLeonhardt · 2024-11-13T01:57:56Z

extensions/microsoft-authentication/extension.webpack.config.js

+		// We don't use the feature that uses Dpapi, so we can just replace it with a mock.
+		// This is a bit of a hack, but it's the easiest way to do it. Really, msal should
+		// handle when this native node module is not available.
+		new NormalModuleReplacementPlugin(


This can go away when this is released: AzureAD/microsoft-authentication-library-for-js#7412

deepak1556

Reviewed the build part. LGTM overall, minor nits

build/lib/extensions.ts

build/gulpfile.vscode.js

deepak1556 · 2024-11-13T03:51:18Z

Running a full build for sanity https://monacotools.visualstudio.com/Monaco/_build/results?buildId=304750&view=results

TylerLeonhardt · 2024-11-13T19:59:24Z

New sanity build after changes: https://monacotools.visualstudio.com/Monaco/_build/results?buildId=304949&view=results

deepak1556 · 2024-11-14T01:09:42Z

build/gulpfile.reh.js

@@ -476,7 +477,8 @@ function tweakProductForServerWeb(product) {

 			const serverTask = task.define(`vscode-${type}${dashed(platform)}${dashed(arch)}${dashed(minified)}`, task.series(
 				compileBuildTask,
-				compileExtensionsBuildTask,
+				cleanExtensionsBuildTask,
+				compileNonNativeExtensionsBuildTask,
 				compileExtensionMediaBuildTask,


Don't we want compileExtensionsBuildTask here ? I believe this task is only used for local build, so useful to build all targets ?

It does build all targets. We build the non-native ones here, and then the native ones in the serverTaskCI which is included in this local build task at the bottom

deepak1556 · 2024-11-14T01:20:07Z

build/gulpfile.vscode.js

@@ -500,7 +501,8 @@ BUILD_TARGETS.forEach(buildTarget => {

 		const vscodeTask = task.define(`vscode${dashed(platform)}${dashed(arch)}${dashed(minified)}`, task.series(
 			compileBuildTask,
-			compileExtensionsBuildTask,
+			cleanExtensionsBuildTask,
+			compileNonNativeExtensionsBuildTask,


Suggested change

compileNonNativeExtensionsBuildTask,

compileExtensionsBuildTask,

?

With this suggestion, we will be building the native ones twice. I separated them out so that we didn't double build based on your original feedback.

The non-native ones are built here, and then the native ones are part of vscodeTaskCI which is added to the end of this local build task.

Am I misunderstanding something?

deepak1556 · 2024-11-14T01:21:50Z

Not sure why the universal build fails on the parcel watcher, I will take a look at it today.

TylerLeonhardt · 2024-11-14T18:51:37Z

build/darwin/create-universal-app.ts

+		// TODO: Should we consider expanding this to other files in this area?
+		'**/node_modules/@parcel/node-addon-api/nothing.target.mk'


@deepak1556 this fixed the Create Universal App step of the build for me. Let me know if you'd like to expand the scope but until then I've scoped it as small as possible.

TylerLeonhardt · 2024-11-14T21:06:45Z

Sanity build here: https://monacotools.visualstudio.com/Monaco/_build/results?buildId=305103&view=results ✅

smoke tested on macOS and Windows and everything seems operational.

TylerLeonhardt marked this pull request as ready for review November 13, 2024 01:55

TylerLeonhardt requested review from deepak1556 and Copilot November 13, 2024 01:55

vs-code-engineering bot assigned TylerLeonhardt Nov 13, 2024

vs-code-engineering bot added this to the November 2024 milestone Nov 13, 2024

Copilot AI reviewed Nov 13, 2024

View reviewed changes

TylerLeonhardt commented Nov 13, 2024

View reviewed changes

deepak1556 reviewed Nov 13, 2024

View reviewed changes

build/lib/extensions.ts Outdated Show resolved Hide resolved

build/gulpfile.vscode.js Outdated Show resolved Hide resolved

build/gulpfile.vscode.js Outdated Show resolved Hide resolved

TylerLeonhardt added 12 commits November 13, 2024 15:08

Try adopting the broker

9ac92eb

no error on missing

8b80da7

mock keytar for the build

ed70558

uncomment keytar

afb68b3

packaging requirements

89af916

exclude packageMocks

6674d50

debugging

bcb892a

idk

002b628

try an idea of having another task

d0edf91

Add compile back

329e5a6

Separate Native and Non-Native building

faedfaf

include marketplace extensions in the "non-native" part

087a4e0

TylerLeonhardt force-pushed the tyler/neighbouring-sheep branch from beaa10f to 087a4e0 Compare November 13, 2024 23:08

deepak1556 reviewed Nov 14, 2024

View reviewed changes

TylerLeonhardt added 2 commits November 14, 2024 09:33

exclude file complaining in CI that we don't need to check

12e1e57

remove comment

fa88ced

TylerLeonhardt commented Nov 14, 2024

View reviewed changes

TylerLeonhardt added 2 commits November 14, 2024 10:58

Add some space and make exports be next to their respective tasks

abcb768

Rename the "all" functions to include "all" & add more docs

0cdb01b

TylerLeonhardt enabled auto-merge (squash) November 14, 2024 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt the MSAL broker to talk to the OS for Microsoft auth #233739

Adopt the MSAL broker to talk to the OS for Microsoft auth #233739

TylerLeonhardt commented Nov 13, 2024 •

edited

Loading

TylerLeonhardt Nov 13, 2024

deepak1556 left a comment

deepak1556 commented Nov 13, 2024

TylerLeonhardt commented Nov 13, 2024 •

edited

Loading

deepak1556 Nov 14, 2024

TylerLeonhardt Nov 14, 2024

deepak1556 Nov 14, 2024

TylerLeonhardt Nov 14, 2024

deepak1556 commented Nov 14, 2024

TylerLeonhardt Nov 14, 2024 •

edited

Loading

TylerLeonhardt commented Nov 14, 2024 •

edited

Loading

	compileNonNativeExtensionsBuildTask,
	compileExtensionsBuildTask,

		// TODO: Should we consider expanding this to other files in this area?
		'**/node_modules/@parcel/node-addon-api/nothing.target.mk'

Adopt the MSAL broker to talk to the OS for Microsoft auth #233739

Are you sure you want to change the base?

Adopt the MSAL broker to talk to the OS for Microsoft auth #233739

Conversation

TylerLeonhardt commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

TylerLeonhardt Nov 13, 2024

Choose a reason for hiding this comment

deepak1556 left a comment

Choose a reason for hiding this comment

deepak1556 commented Nov 13, 2024

TylerLeonhardt commented Nov 13, 2024 • edited Loading

deepak1556 Nov 14, 2024

Choose a reason for hiding this comment

TylerLeonhardt Nov 14, 2024

Choose a reason for hiding this comment

deepak1556 Nov 14, 2024

Choose a reason for hiding this comment

TylerLeonhardt Nov 14, 2024

Choose a reason for hiding this comment

deepak1556 commented Nov 14, 2024

TylerLeonhardt Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

TylerLeonhardt commented Nov 14, 2024 • edited Loading

TylerLeonhardt commented Nov 13, 2024 •

edited

Loading

TylerLeonhardt commented Nov 13, 2024 •

edited

Loading

TylerLeonhardt Nov 14, 2024 •

edited

Loading

TylerLeonhardt commented Nov 14, 2024 •

edited

Loading