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

Separate context bridge for electron-auth #89

Open
aruniverse opened this issue Oct 27, 2022 · 2 comments
Open

Separate context bridge for electron-auth #89

aruniverse opened this issue Oct 27, 2022 · 2 comments
Assignees
Labels
electron Related to the desktop/electron authorization

Comments

@aruniverse
Copy link
Member

We have an electron preload script, that exposes some ipc message handling for itwinjs channels on the window, namely core-electron and electron-auth.

Luckily for us, its the same api rn in both and theres no collision, but if there ever arises a time when there is a difference in the apis, it might cause an issue for us. It also really bothers me that we have pretty much the exact same file duplicated across both pkgs. Wondering if we need to consolidate, have an electron-common pkg or some shim. Also wondering if maybe core-electron takes a dep on electron-auth or vice versa. Just food for thought, something Id like to discuss further with our electron experts

@calebmshafer
Copy link
Member

We should consider adding a different context bridge for electron-auth so that it doesn't accidentally share the same one as core-electron. However, we thought it would be safe since you'd have to merge the two if you wanted to have both.

tl;dr

We went back and forth on this a lot when the split happened during the 3.x release. The overriding goal was to avoid any explicit dependency in the core-electron (or any core package) on a specific type of authorization workflow and keep it as agnostic as possible. This was the main reason for not having a dependency on an OIDC/OAuth2 specific package for our default electron configuration.

The reason for not having it the other way is to allow other Electron apps that we may have to use the package without needing to pull along all of our backend code which is required from core-electron.

Given those two goals and the fact you can't import additional javascript files, we had two preload scripts to ensure the electron-auth package could stand on its own with the core-electron package and not rely on the user of it to have to add code specifically to their preload script if they didn't need to do so. As mentioned above, we could have a separate bridge that makes it clear it's for auth and not just the general auth.

@aruniverse aruniverse changed the title Electron preload consolidation? Separate context bridge for electron-auth Oct 28, 2022
@aruniverse aruniverse transferred this issue from iTwin/itwinjs-core Oct 28, 2022
@johnnyd710
Copy link
Contributor

johnnyd710 commented Oct 28, 2022

My two cents (not really what you guys were talking about, but related)...

Our app has a problem with this package because @itwin/electron-authorization assumes authorization takes place on Electron's main process.
But it might soon have a separate process for the iTwin backend, so authorization can not take place on Electron's ipcMain.

What I suggest, is either:

  • Without needing to pull along all of our backend code which is required from core-electron: ElectronMainAuthorization (and the frontend client as well) should allow the user to specify the IPC to use in the config (i.e., the IpcSocketBackend/IpcSocketFrontend instance), else it can default to ipcMain
  • Pulling along all of our backend code which is required from core-electron: use IpcHost instead of ipcMain, use IpcApp instead of window.itwinjs

I can submit a PR with the config change if you agree

@ben-polinsky ben-polinsky self-assigned this Jan 20, 2023
@calebmshafer calebmshafer added the electron Related to the desktop/electron authorization label Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron Related to the desktop/electron authorization
Projects
None yet
Development

No branches or pull requests

4 participants