Page MenuHomePhabricator
Differential D11465

[protos] Add RPC to log in existing device
ClosedPublic
Actions

Authored by bartek on Thu, Mar 28, 10:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 13, 5:19 PM
Unknown Object (File)
Sat, Apr 13, 9:06 AM
Unknown Object (File)
Wed, Apr 3, 1:58 PM
Unknown Object (File)
Mon, Apr 1, 11:26 PM
Unknown Object (File)
Sun, Mar 31, 2:00 PM
Unknown Object (File)
Sun, Mar 31, 2:00 PM
Unknown Object (File)
Sun, Mar 31, 1:57 PM
Subscribers
tomek

Details

Reviewers
ashoat
varun
Commits
rCOMMeee910d34fc8: [protos] Add RPC to log in existing device
Summary

Adds Existing device login RPC from ENG-6828 to the proto.

Depends on D11435

Test Plan

Only proto changes here + stub code. Everything should compile.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek created this revision.Thu, Mar 28, 10:41 PM
bartek held this revision as a draft.
Herald added a subscriber: tomek. · View Herald TranscriptThu, Mar 28, 10:41 PM
bartek added a child revision: D11466: [identity] Implement LogInExistingDevice RPC.Thu, Mar 28, 10:45 PM
Harbormaster completed remote builds in B27807: Diff 38524.Thu, Mar 28, 10:55 PM
bartek published this revision for review.Thu, Mar 28, 11:49 PM
ashoat accepted this revision.Fri, Mar 29, 10:48 AM
ashoat added inline comments.
shared/protos/identity_unauth.proto
226 ↗(On Diff #38524)

I think it would be helpful to rename this so it's clear what the "challenge" is. For instance, how about signed_nonce?

varun accepted this revision.Fri, Mar 29, 12:21 PM
This revision is now accepted and ready to land.Fri, Mar 29, 12:21 PM
bartek added inline comments.Sat, Mar 30, 1:02 AM
shared/protos/identity_unauth.proto
226 ↗(On Diff #38524)

I followed the SecondaryDeviceKeysUploadRequest format where we call it challenge_response.
We can either:

  • Rename both
  • Add a comment to both describing what this field is
  • Follow up discussions about the Identity.social_proof field being split into two, and split this one into nonce_message and nonce_signature (much more work both client- and server-side)
ashoat added inline comments.Sat, Mar 30, 1:02 PM
shared/protos/identity_unauth.proto
226 ↗(On Diff #38524)

nonce_message is basically just nonce, right? I think it would probably be good to split it for consistency... I think we'll save time in the long term by trying to finalize RPCs early instead of adding backlog tasks to change them later

bartek added inline comments.Wed, Apr 3, 2:53 AM
shared/protos/identity_unauth.proto
226 ↗(On Diff #38524)

I'll add a comment now and do the rename in a separate diff to both RPCs
https://linear.app/comm/issue/ENG-7641/consider-renaming-challenge-response

bartek updated this revision to Diff 38703.Wed, Apr 3, 2:53 AM

Rebase, add comments explaining challenge response

Harbormaster completed remote builds in B27931: Diff 38703.Wed, Apr 3, 3:08 AM
Closed by commit rCOMMeee910d34fc8: [protos] Add RPC to log in existing device (authored by bartek). · Explain WhyWed, Apr 3, 3:17 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMMeee910d34fc8: [protos] Add RPC to log in existing device.

Revision Contents
Changeset List

Diff 38710

View Options

services/identity/src/client_service.rs

Loading...
View Options

shared/protos/identity_unauth.proto

Loading...
View Options

web/protobufs/identity-unauth-structs.cjs

Loading...
View Options

web/protobufs/identity-unauth-structs.cjs.flow

Loading...
View Options

web/protobufs/identity-unauth.cjs

Loading...
View Options

web/protobufs/identity-unauth.cjs.flow

Loading...