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

Proposed API for window handle #233106

Open
TylerLeonhardt opened this issue Nov 5, 2024 · 6 comments · Fixed by #233267
Open

Proposed API for window handle #233106

TylerLeonhardt opened this issue Nov 5, 2024 · 6 comments · Fixed by #233267
Assignees
Labels
api api-proposal feature-request Request for new features or functionality microsoft-authentication Issues with the Microsoft Authentication extension
Milestone

Comments

@TylerLeonhardt
Copy link
Member

For Broker support in Microsoft Authentication (#229431) we need the window handle for this API:
https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-node/docs/brokering.md#window-parenting

So, I need to be able to access the window handle from Main alllll the way in an extension.

Here's the proposal:

	export namespace env {
		/**
		 * Retrieves the native window handle of the current window.
		 *
		 * @returns A promise that resolves to a Buffer containing the native window handle.
		 */
		export function getNativeWindowHandle(): Thenable<Buffer | undefined>;
	}
@TylerLeonhardt TylerLeonhardt added api api-proposal feature-request Request for new features or functionality microsoft-authentication Issues with the Microsoft Authentication extension labels Nov 5, 2024
@TylerLeonhardt TylerLeonhardt added this to the November 2024 milestone Nov 5, 2024
@TylerLeonhardt TylerLeonhardt self-assigned this Nov 5, 2024
@jrieken
Copy link
Member

jrieken commented Nov 5, 2024

  • use Uint8Array over Buffer
  • use property over async-function
  • make it init data

@TylerLeonhardt
Copy link
Member Author

TylerLeonhardt commented Nov 7, 2024

UInt8Array was cumbersome to pass across processes... so I landed on a base64 string. From there, an extension (Microsoft auth) can simply turn that into a Buffer Buffer.from(env.handle, 'base64') and pass it through.

New proposal:

	export namespace env {
 		/**
 		 * Retrieves a base64 representation of a native window
 		 * handle of the current window.
 		 */
 		export const handle: string | undefined;
 	}

@vs-code-engineering vs-code-engineering bot added the unreleased Patch has not yet been released in VS Code Insiders label Nov 7, 2024
@jrieken jrieken reopened this Nov 7, 2024
@vs-code-engineering vs-code-engineering bot removed the unreleased Patch has not yet been released in VS Code Insiders label Nov 7, 2024
@jrieken
Copy link
Member

jrieken commented Nov 7, 2024

Let's keep this open to track lifecyle of this proposal

@TylerLeonhardt
Copy link
Member Author

handle on env feels weird. Couple ideas:

  • window.handle
  • env.windowHandle

@jrieken
Copy link
Member

jrieken commented Nov 8, 2024

I believe window.handle is the most correct but maybe a more telling name, like nativeHandle or so?

@jrieken
Copy link
Member

jrieken commented Nov 12, 2024

what about AUX windows?

TylerLeonhardt added a commit that referenced this issue Nov 15, 2024
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 #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api api-proposal feature-request Request for new features or functionality microsoft-authentication Issues with the Microsoft Authentication extension
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants