Page MenuHomePhabricator

Make client handlers that call authenticated identity RPC's gated on CSAT
ClosedPublic

Authored by marcin on Jul 8 2024, 9:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:10 AM
Unknown Object (File)
Fri, Nov 15, 5:39 AM
Unknown Object (File)
Fri, Nov 15, 5:39 AM
Unknown Object (File)
Fri, Nov 15, 5:37 AM
Unknown Object (File)
Fri, Nov 8, 10:39 PM
Unknown Object (File)
Fri, Nov 8, 7:14 AM
Unknown Object (File)
Fri, Nov 8, 6:15 AM
Unknown Object (File)
Fri, Nov 8, 6:14 AM
Subscribers

Details

Summary

This diff prevents prekey handler, platform details synchronizer and user infos handler from making authorized identity calls without having CSAT.

Test Plan

Set usingCommServicesAccessToken = false but remove if statements checking for usingCAST. Ensure that error logs for prekey handler and platform details synchronizer appear in the console. Now apply this diff and ensure that those error logs disapper.

I couldn't figure a good way to test user infos handler but I think that test plan for other components suggests that it will work too since it uses the same check.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Jul 8 2024, 9:37 AM
lib/components/platform-details-synchronizer.react.js
14–15 ↗(On Diff #42120)
26 ↗(On Diff #42120)

Is this really the right check? I had expected you to add a usingCommServicesAccessToken && commServicesAccessToken check, but it's possible that this is equivalent

lib/components/prekeys-handler.react.js
25–27 ↗(On Diff #42120)

Is this check necessary given the getAuthMetadata check below?

lib/components/platform-details-synchronizer.react.js
26 ↗(On Diff #42120)

Check against usingCommServicesAccessToken is present at line 39.

lib/components/prekeys-handler.react.js
25–27 ↗(On Diff #42120)

Theoretically it prevents us from scheduling recurring task that will never do anything. I think this is some benefit but if you disagree this check can be removed.

This revision is now accepted and ready to land.Jul 9 2024, 10:36 AM

Hey @marcin, any reason this hasn't been landed yet?

Hey @marcin, any reason this hasn't been landed yet?

Totally my oversight. It will be landed in minutes.

This revision was landed with ongoing or failed builds.Jul 11 2024, 6:09 AM
This revision was automatically updated to reflect the committed changes.