iTwin / auth-clients

MIT License
6 stars 3 forks source link

Separate context bridge for electron-auth #89

Open aruniverse opened 1 year ago

aruniverse commented 1 year ago

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 commented 1 year ago

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.

johnnyd710 commented 1 year ago

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:

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