microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
945 stars 303 forks source link

[BUG] Electron support broken when ContextIsolation is needed. #2618

Closed NikiLentz closed 9 months ago

NikiLentz commented 1 year ago

Describe the bug Hi, I am currently developing an app using the quasar framework and electronjs. I have tried to work through the tutorial you provide https://learn.microsoft.com/en-us/graph/toolkit/get-started/build-an-electron-app. I quickly realized that i would need to activate NodeIntegration, which simply isn't possible if I want to use the ContextBridge, since ContextBridge needs ContextIsolation set to true.

Steps to Reproduce 1) Create an Electron app according to the tutorial 2) Try using the Context Bridge for any communication between renderer and backend process 3) Be happy about these errors: image

If you reactivate ContextIsolation, the graph libraries are obviously missing a lot of node functions to work.

What would i need to restructure and expose in my ContextBridge to make this work? Simply putting the stuff from the renderer in there didn't work either unfortunately and resulted in this errror:


MY_PROJECT_FOLDER/node_modules/@microsoft/mgt-element/dist/es6/index.js:7
export * from './IBatch';
^^^^^^

SyntaxError: Unexpected token 'export'
    at new Script (node:vm:100:7)
    at wrapSafe (node:internal/modules/cjs/loader:1135:20)
    at Module._compile (node:internal/modules/cjs/loader:1197:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1296:10)
    at Module.load (node:internal/modules/cjs/loader:1096:32)
    at Module._load (node:internal/modules/cjs/loader:937:12)
    at f._load (node:electron/js2c/asar_bundle:2:13330)
    at o._load (node:electron/js2c/renderer_init:2:3109)
    at Module.require (node:internal/modules/cjs/loader:1120:19)
    at require (node:internal/modules/cjs/helpers:103:18)
(
gavinbarron commented 1 year ago

@NikiLentz thanks for raising this.

At present while we investigate and understand this issue better the work around is to disable contextIsolation.

We're looking into how we can support having contextIsolation enabled. Related: #2599

NikiLentz commented 1 year ago

Ok thanks, I will try Microsoft Authentication Library (MSAL) for now since the problem doesn't seem to exist there and tbh I am not quite sure where the differences in scope are compared to this project.

I would definitely add a disclaimer somewhere because communication between renderer and backend isn't possible in the current electronjs without contextisolation unfortunatel.

gavinbarron commented 1 year ago

This provider is primarily intended to allow the use of the toolkit components inside an electron context. If all you need is auth then MSAL is probably a better dependency for your use case.

I'm really not an expert at electron at all and am just learning in the context of maintaining this library so can't speak to the implication of contextIsolation or not

trulysinclair commented 10 months ago

So for my use case, I'm trying to use the web components for a client application, they want to be able to authenticate using MSAL as well as having a Teams widget which requires the electron provider. For security, nodeIntegration is disabled and heavily reliant on the contextBridge. There should be an alternative implementation that supports the contextBridge.

trulysinclair commented 10 months ago

I was able to get it working with the contextBridge by setting up a preload script that replicates the internal IPC calls. My ElectronProvider calls the contextBridge commands and works perfectly. Without forcing your users to use either option, I would recommend an ElectronNodeProvider and an ElectronBridgeProvider. It is up to you, but you can just have devs manually set the context bridge per your specs.

import { AuthenticationProviderOptions } from '@microsoft/microsoft-graph-client';
import { IpcRendererEvent, contextBridge, ipcRenderer } from "electron";

contextBridge.exposeInMainWorld("main", {
  electronProvider: {
    onMgtAuthState: (callback: (event: IpcRendererEvent, authState: string) => void) => ipcRenderer.on('mgtAuthState', callback),
    invokeToken: (options?: AuthenticationProviderOptions) => ipcRenderer.invoke('token', options),
    invokeLogin: () => ipcRenderer.invoke('login'),
    invokeLogout: () => ipcRenderer.invoke('logout')
  }
});
/**
 * -------------------------------------------------------------------------------------------
 * Copyright (c) Microsoft Corporation.  All Rights Reserved.  Licensed under the MIT License.
 * See License in the project root for license information.
 * -------------------------------------------------------------------------------------------
 */

import {
  GraphEndpoint,
  IProvider,
  MICROSOFT_GRAPH_DEFAULT_ENDPOINT,
  ProviderState,
  Providers,
  createFromProvider
} from '@microsoft/mgt-element';
import { AuthenticationProviderOptions } from '@microsoft/microsoft-graph-client';

/**
 * ElectronProvider class to be instantiated in the renderer process.
 * Responsible for communicating with ElectronAuthenticator in the main process to acquire tokens
 *
 * @export
 * @class ElectronProvider
 * @extends {IProvider}
 */
export class ElectronProvider extends IProvider {
  /**
   * Name used for analytics
   *
   * @readonly
   * @memberof IProvider
   */
  public get name() {
    return 'MgtElectronProvider';
  }

  constructor(baseUrl: GraphEndpoint = MICROSOFT_GRAPH_DEFAULT_ENDPOINT) {
    super();
    this.baseURL = baseUrl;
    this.graph = createFromProvider(this);
    this.setupProvider();
  }

  /**
   * Sets up messaging between main and renderer to receive SignedIn/SignedOut state information
   *
   * @memberof ElectronProvider
   */
  setupProvider() {
    // ipcRenderer.on('mgtAuthState', (event, authState) => {
    //   if (authState === 'logged_in') {
    //     Providers.globalProvider.setState(ProviderState.SignedIn);
    //   } else if (authState === 'logged_out') {
    //     Providers.globalProvider.setState(ProviderState.SignedOut);
    //   }
    // });
    window.main.electronProvider.onMgtAuthState((event, authState) => {
      if (authState === 'logged_in') {
        Providers.globalProvider.setState(ProviderState.SignedIn);
      } else if (authState === 'logged_out') {
        Providers.globalProvider.setState(ProviderState.SignedOut);
      }
    });
  }

  /**
   * Gets access token (called by MGT components)
   *
   * @param {AuthenticationProviderOptions} [options]
   * @return {*}  {Promise<string>}
   * @memberof ElectronProvider
   */
  async getAccessToken(options?: AuthenticationProviderOptions): Promise<string> {
    // const token = (await ipcRenderer.invoke('token', options)) as string;
    const token = (await window.main.electronProvider.invokeToken(options)) as string;
    return token;
  }

  /**
   * Log in to set account information (called by mgt-login)
   *
   * @return {*}  {Promise<void>}
   * @memberof ElectronProvider
   */
  async login(): Promise<void> {
    Providers.globalProvider.setState(ProviderState.Loading);
    // await ipcRenderer.invoke('login');
    await window.main.electronProvider.invokeLogin();
  }

  /**
   * Log out (called by mgt-login)
   *
   * @return {*}  {Promise<void>}
   * @memberof ElectronProvider
   */
  async logout(): Promise<void> {
    // await ipcRenderer.invoke('logout');
    await window.main.electronProvider.invokeLogout();
  }
}
sebastienlevert commented 10 months ago

Thanks for all this context. We understand our electron story is lacking today but we are limited from a resourcing standpoint to dive deep in this topic. @trulysinclair Would you be willing to contribute this change to our codebase? This would benefit all electron developers and we could spend time to dive deeper in the review.

Thanks!

trulysinclair commented 10 months ago

Absolutely, I'll have a PR soon! Thank you

DMiradakis commented 8 months ago

The docs need to be updated SO badly for Electron. I have been fighting MSAL and the Microsoft Graph Toolkit for almost 2 straight weeks, this has been MISERABLE. Absolutely miserable. It took me until tonight to realize I needed to load this stuff in my preload script since the Microsoft docs assume you aren't using contextIsolation, and I'm still far from having the whole integration working.

Please consider updating the documentation with a note about using preload scripts.