keep the devices' crypto key stored as part of the cookie. linear task depends on D5603
Details
Diff Detail
- Repository
- rCOMM Comm
- Branch
- derek/siwe-client-key
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Please annotate the diff with the correct parent / child relationships with other diffs. I can't see your stack
keyserver/src/creators/account-creator.js | ||
---|---|---|
74 ↗ | (On Diff #18496) | I think the plaintext of the signed message should contain the public key, so shouldn't be a need to transmit it separately... we just need to parse it out |
106 ↗ | (On Diff #18496) | Where does this end up getting used again? Is it currently not used? (Fine if not, just need a reminder) Unrelated – wanted to find the relevant diff, but couldn't because you didn't list your stack, so leaving here – we should make sure a user is only associated with a single ETH address, a single ETH address is only associated with a single user |
native/account/siwe-panel.react.js | ||
57 ↗ | (On Diff #18496) | Why are we sending the same thing up twice? |
native/schema/CommCoreModuleSchema.js | ||
21–24 ↗ | (On Diff #18496) | Thanks for typing this. I think you'll need to rerun codegen now |
keyserver/src/creators/account-creator.js | ||
---|---|---|
74 ↗ | (On Diff #18496) | we removed the public key from users/sessions table, i was under the impression every new account was registering with the public key from their device |
106 ↗ | (On Diff #18496) | public key isn't getting used yet:
added the diff to the stack (i wrote depends on [link] instead of depends on [diff#]) - regarding single eth address, i'm confident that's covered in D5603, but there's definitely more work to be done in that category (making sure normal usernames don't overlap, ens resolution etc.) |
native/account/siwe-panel.react.js | ||
57 ↗ | (On Diff #18496) | it's really a placeholder pending more discussion on what the social proof actually consists of. i removed it and marked it as optional in the flow types. i'll add a comment to the linear task linking back to this diff |
native/schema/CommCoreModuleSchema.js | ||
21–24 ↗ | (On Diff #18496) | reran codegen but no changes came up - i think it was just the lack of typing on js side |
I think there's some confusion about what the socialProof needs to contain. See inline comments
keyserver/src/creators/account-creator.js | ||
---|---|---|
106–107 ↗ | (On Diff #18586) |
Not sure why you're bringing up the user vs. device distinction. Repeating my comment here:
The socialProof is a signed message that contains the publicKey inside it, right? So instead of sending the publicKey twice, can we just parse it out of the socialProof? We can't just take the user's "word for it" that the publicKey matches the socialProof... we need to parse the socialProof to make sure that it includes the publicKey. At that point, we'll have the publicKey... so there's no reason to send it up separately. Let me know over chat if that's unclear! |
keyserver/src/session/cookies.js | ||
645–648 ↗ | (On Diff #18586) | Can we make all of these $ReadOnly by prefixing with +? |
lib/types/account-types.js | ||
161–165 ↗ | (On Diff #18586) | Why'd you remove the +? Let's make it $ReadOnly again |
native/account/siwe-panel.react.js | ||
46 ↗ | (On Diff #18586) | I think we want the signature to contain the publicKey, so this is too late in the process for us to doing this The point of the social proof is to prove that Ethereum account X corresponds to Comm account Y. To do that, we sign a message containing Y's public key using X's private key. Does that make sense? |
need flow fix me help
native/account/siwe-panel.react.js | ||
---|---|---|
50 ↗ | (On Diff #18621) | i'm having trouble with flow here. WebViewProps are typescript declarations & don't seem to play nicely with flow |
spacing, readonly
native/account/siwe-panel.react.js | ||
---|---|---|
50 ↗ | (On Diff #18621) | Cannot instantiate React.Ref because WebViewProps [1] is incompatible with AbstractComponent [2] in type argument ElementType can we leave the FlowFixMe(s) here? |
native/account/siwe-panel.react.js | ||
---|---|---|
50 ↗ | (On Diff #18621) | You have to type WebViewProps in Flow.
You shouldn't be "typecasting" it, you should just make sure you're using the types correctly. Have you tried Googling eg. "how to type a React component in Flow"? The first result for that query (for me, anyways) is Flow's React type reference. You could start by reading React.Ref first (since that's what you're using here), and next try reading React.ComponentType. What's obviously wrong here from a cursory read through those docs is that you're passing a type representing the props of a component (WebViewProps) to a type (React.Ref) that expects its parameter to be a React component (React.ComponentType). |
You're most of the way there, though. Point is basically "keyserver needs to verify that the provided public key matches what is contained within the provided message, and that isn't being done currently. To do that, keyserver needs to extract the public key... but at that point, we don't need to ask the client to pass it in separately.'
Does that make sense? keyserver just needs the ETH-signed message. Then it extracts the Comm public key out of the message, verifies that request.address signed it (not currently being done it seems?), and then goes ahead and associated that cookie with that socialProof
keyserver/src/creators/account-creator.js | ||
---|---|---|
74 | We need to verify that request.address actually signed the socialProof | |
landing/siwe.react.js | ||
46 | I checked the whitepaper draft for what to use. Here's what I found: Comm wants you to sign in with your Ethereum account: 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 I accept the Comm Terms of Service: https://comm.app/tos URI: https://comm.app/login Version: 1 Chain ID: 1 Nonce: 32891756 Issued At: 2022-07-24T16:25:24Z There's a lot of questions here:
We can probably handle all of this stuff in a later diff |
I assume we're abandoning this one as well given @atul's comment. We can always come back to it later if we change our mind