-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Adopt the MSAL broker to talk to the OS for Microsoft auth #233739
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// 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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can go away when this is released: AzureAD/microsoft-authentication-library-for-js#7412
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the build part. LGTM overall, minor nits
Running a full build for sanity https://monacotools.visualstudio.com/Monaco/_build/results?buildId=304750&view=results |
New sanity build after changes: https://monacotools.visualstudio.com/Monaco/_build/results?buildId=304949&view=results |
beaa10f
to
087a4e0
Compare
Not sure why the universal build fails on the parcel watcher, I will take a look at it today. |
Sanity build here: https://monacotools.visualstudio.com/Monaco/_build/results?buildId=305103&view=results ✅ smoke tested on macOS and Windows and everything seems operational. |
@@ -202,7 +202,7 @@ function packageTask(sourceFolderName, destinationFolderName) { | |||
|
|||
const compileWebExtensionsBuildTask = task.define('compile-web-extensions-build', task.series( | |||
task.define('clean-web-extensions-build', util.rimraf('.build/web/extensions')), | |||
task.define('bundle-web-extensions-build', () => extensions.packageLocalExtensionsStream(true, false).pipe(gulp.dest('.build/web'))), | |||
task.define('bundle-web-extensions-build', () => extensions.packageAllLocalExtensionsStream(true, false).pipe(gulp.dest('.build/web'))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to block the PR on this, but is building the native extensions have an effect in the web pipeline. Maybe we should skip the native ones ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Microsoft Auth's case, it has web code as well that doesn't use any native stuff... so it actually does need to be built for web... but for efficiency we could build everything in compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning up the extension tasks and adding the support to include native modules with them. 🚀
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:
NativeBrokerPlugin
to our PCAs@azure/msal-node-extensions
that uses keytar@azure/msal-node-extensions
that uses it.node
and.dll
files are included in the bundleThere are a couple of followups:
handle
API to handle (heh) Auxiliary Windows Proposed API for window handle #233106acquireTokenSilent
andacquireTokenInteractive
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 ISSUEFixes #229431