Prepare for OAuth: Added `handleUri`, `platform.openWindow`, and `papi.dataProtection` #1425

tjcouch-sil · 2025-01-21T23:45:41Z

Added handleUri extension privilege for handling navigation to platform-bible://<extension_publisher>.<extension_name>
- BREAKING CHANGE: Now we must use the folder platform-bible instead of platform.bible in our temp folder to find logs and such. This means all dock layouts will be reset along with anything else in frontend localstorage.
Added platform.openWindow to open a link in a browser
Added papi.dataProtection to papi backend for encrypting stuff before saving it
Fixed a couple little bugs with how requests were being sent to and from main (params and error)

These should get us properly ready for OAuth logins.

Mostly resolves https://paratextstudio.atlassian.net/browse/PT-1640

Resolves #841
Resolves #1369

This change is

lyonsil

Thanks for slogging through all these changes. I have a few things to consider, but the overall approach looks great!

Reviewed 22 of 22 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tjcouch-sil)

src/shared/models/data-protection.service-model.ts line 18 at r1 (raw file):

   *
   * WARNING: This command passes the string across processes over WebSocket, so please note it is
   * not secure to the same extent that Electron's `safeStorage` API is.

The phrasing of this warning makes it, I think, come across as if devs should use Electron's API (which we don't allow). I wonder it it would be more helpful to say something like:

 WARNING: The purpose of this method is to enable encrypting and decrypting data that will be stored in local files. It is not intended to protect data passed over a network connection. In fact, calling this method passes the unencrypted string between local processes using the PAPI web socket.

The same thought applies to all the places in this PR where we have a similar warning.

src/main/services/rpc-websocket-listener.ts line 121 at r1 (raw file):

          );
        try {
          const result = method(...requestParams);

At first glance this looks like a valid change, but are you sure this doesn't cause other problems? I remember having to go back and forth with using ... in a few places because it was breaking things in some cases. I don't remember all the details right now, but I remember having obscure problems sometimes.

My guess is that we intentionally don't want to use ... for handler.request above because the library needs the parameters to be an array, but here we want to flatten them for the local method call. It's just weird that, if this is correct, we weren't seeing any other problems for the last couple of months.

package.json line 174 at r1 (raw file):

    "detect-port": "^1.6.1",
    "electron": "^32.1.2",
    "electron-builder": "^25.1.8",

Is it safe/OK to bump this up without changing anything else with ERB? What does the new version buy us?

src/main/main.ts line 41 at r1 (raw file):

/** Whether this is the first instance of this application. */
const isFirstInstance = app.requestSingleInstanceLock();

Did you test this with the app automatic restart? I wonder if this might create race conditions when restarting.

src/main/main.ts line 66 at r1 (raw file):

 */
async function openExternal(url: string) {
  if (!url.startsWith('https://')) throw new Error(`URL must start with 'https://': ${url}`);

Good call!

src/main/main.ts line 87 at r1 (raw file):

  await startProjectLookupService();

  await startDataProtectionService();

Might be nice to add a comment why this is being added at this point in the startup process like the other services, and not earlier or later. Also does this need to be awaited at this level, or could it be asynchronous?

src/main/main.ts line 147 at r1 (raw file):

  }
  // Resolve the path to this file if we're running electron, not the packaged app (probably means running in dev)
  if (process.defaultApp && args.length > 2) args[2] = path.resolve(args[2]);

Isn't process.defaultApp the same meaning (inverted) as process.isPackaged? If so, would it make more sense to stick with isPackaged for consistency since we're using that in other places and not using defaultApp anywhere else?

tjcouch-sil

Reviewable status: 13 of 23 files reviewed, 5 unresolved discussions (waiting on @lyonsil)

src/main/main.ts line 41 at r1 (raw file):