Page MenuHomePhabricator

Transfer CSAT to IndexedDB from service worker and implement call to identity to query for inbound keys
ClosedPublic

Authored by marcin on Jul 9 2024, 9:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Sep 9, 9:06 AM
Unknown Object (File)
Sun, Sep 8, 3:03 PM
Unknown Object (File)
Sun, Sep 8, 3:02 PM
Unknown Object (File)
Sun, Sep 8, 3:02 PM
Unknown Object (File)
Sat, Sep 7, 1:45 PM
Unknown Object (File)
Sat, Sep 7, 1:44 PM
Unknown Object (File)
Thu, Sep 5, 5:15 PM
Unknown Object (File)
Thu, Sep 5, 5:15 PM
Subscribers

Details

Summary

This differential does two things

  1. Adds CSAT to initial message that the app sends to service worker with olm wasm path. The service worker encrypts and persists it in IndexedDB.
  2. Implements HTTP call to identity service to get inbound keys for particular device.
Test Plan
  1. Add logg statement to message handler in service worker that logs CSAT. Ensure that the same value is logged with each refresh and that after logging out and in new value starts being logged.
  2. Pick a random device id from staging identity database and pass it to getNotifsInboundKeysForDevice method. Log the result.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin requested review of this revision.Jul 9 2024, 9:45 AM
lib/utils/identity-service.js
41–43 ↗(On Diff #42144)

Should we use base64 and encodeURIComponent directly here? I worry that this approach is brittle

web/push-notif/services-client.js
94 ↗(On Diff #42144)

Can't we call .json() directly and skip the JSON.parse below?

lib/utils/identity-service.js
41–43 ↗(On Diff #42144)

I don't think I follow. What causes brittleness here? Should we base64 and encodeURIComponent before passing to this function and only validate inside this function?

lib/utils/identity-service.js
41–43 ↗(On Diff #42144)

What I'm worried (and describe as "brittle") is that I suspect this would no longer accurately reproduce base64/urlencode if we change the format of deviceID

lib/utils/identity-service.js
41–43 ↗(On Diff #42144)

I spoke to @marcin and he clarified that base64URLEncodedDeviceID is not supposed to be performing base64 encoding. The naming here is confusing for me

In addition, it's not performing URL encoding here

Rather, deviceID is a base-64 encoded string, which is not safe to use in a URL without calling encodeURIComponent because it contains the characters + and /

Rather than using encodeURIComponent here, we choose to rewrite the deviceID in our own custom way, such that calling encodeURIComponent on the result will return the same string, unmodified

Three follow-ups here:

  1. @marcin should understand why he's writing the code he's writing. From talking to him, it appears that he's unclear as to why we're doing this, and why we're doing it this way. It sounds like @bartek told him to do it, and @marcin didn't ask why it's necessary – just simply went ahead and implemented it
  2. If we're keeping this logic, we definitely need to rename base64URLEncodedDeviceID. This is a very inaccurate name – the result is NOT base64 encoded, and NOT url-encoded either. Rather, we are taking an already base64-encoded string, and implementing our own custom encoding such that URL encoding is not necessary
  3. It's unclear to me why we implemented this custom approach rather than just using encodeURIComponent. I suppose our custom approach looks cleaner and avoids the extra "%2" characters that come with encodeURIComponent. Is there any other advantage?
lib/utils/identity-service.js
41–43 ↗(On Diff #42144)

The base64-url is a standard, definitely not "our own custom encoding". We already use this format e.g. to encode blob hash before sending a request to Blob service, for the same reason. I decided to use it here for the sake of simplicity.

Rather, deviceID is a base-64 encoded string, which is not safe to use in a URL without calling encodeURIComponent because it contains the characters + and /

Yes, device ID is always a base64-encoded string, not "any" string, so we don't need to use encodeURIcomponent. Simple b64->b64url conversion is enough. We could, but it's a bit more complexity server-side, and URLs become uglier because of these %2s.

That said, I don't think this name is that inaccurate. But I'm okay with e.g urlSafeDeviceID.

lib/utils/identity-service.js
41–43 ↗(On Diff #42144)

Ah, I think I get it... thanks for explaining

base64URLEncodedDeviceID is meant to be read as "base64-url-encoded device ID", but I'm reading it as "base64 urlEncoded device ID"

Still think a rename would be good for clarity

This revision is now accepted and ready to land.Jul 25 2024, 7:19 AM
lib/facts/identity-service.js
25–33 ↗(On Diff #43140)

Why are these being hardcoded? This was bad enough when you did it for iOS / Android, but at least there was an excuse that it's harder in Obj-C / Java to access the same config. The same concern should not be present for JS

We spent several days making sure that Rust accessed the same config as JS. It's frustrating that you've broken that here by relying on a completely separate (hardcoded) config

I'll leave the same comments in ENG-8894