Page MenuHomePhabricator
Differential D4285

[services][identity] wallet login
ClosedPublic
Actions

Authored by varun on Jun 16 2022, 12:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 7:58 PM
Unknown Object (File)
Wed, Nov 27, 7:44 PM
Unknown Object (File)
Wed, Nov 27, 7:36 PM
Unknown Object (File)
Wed, Nov 27, 7:34 PM
Unknown Object (File)
Sun, Nov 24, 4:36 PM
Unknown Object (File)
Mon, Nov 18, 1:52 PM
Unknown Object (File)
Sun, Nov 17, 10:13 PM
Unknown Object (File)
Wed, Nov 13, 2:09 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul

Details

Reviewers
• karol
tomek
Commits
rCOMMe7885e5a97fa: [services][identity] wallet login
Summary

Full implementation of wallet login. Steps:

  1. Parse SIWE message and signature from request
  2. Verify message using siwe-rs crate
  3. If verification is successful, generate a token, store it in the token table in DynamoDB, and then return it to the caller

Note that PAKE login is not covered here. It will be in a separate diff.

Test Plan

Successfully signed in with a metamask wallet

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Jun 16 2022, 12:46 PM
Herald added subscribers: • abosh, atul, • adrian, ashoat. · View Herald TranscriptJun 16 2022, 12:46 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 16 2022, 12:49 PM
Harbormaster failed remote builds in B9784: Diff 13543!
varun updated this revision to Diff 13545.Jun 16 2022, 1:07 PM

rebase

Harbormaster completed remote builds in B9786: Diff 13545.Jun 16 2022, 1:15 PM
varun requested review of this revision.Jun 16 2022, 1:15 PM
varun updated this revision to Diff 13549.Jun 17 2022, 8:26 AM

changed the type of token in the .proto file to string, so this is the corresponding code change

Harbormaster completed remote builds in B9790: Diff 13549.Jun 17 2022, 8:31 AM
ashoat added a comment.Jun 17 2022, 9:23 AM

New external dependencies look good

• jim requested changes to this revision.Jun 17 2022, 11:53 AM
• jim added inline comments.
services/identity/src/service.rs
71 ↗(On Diff #13549)

Error case isn't handled

72 ↗(On Diff #13549)

When would this be None?

73 ↗(On Diff #13549)

This is really hard to read, please refactor. The error handling in the match arms is heavily duplicated. For starters, make a subroutine that returns a Result, and in here do the if let Err(err) = result { log and break } thing once.

This revision now requires changes to proceed.Jun 17 2022, 11:53 AM
varun updated this revision to Diff 13730.Jun 23 2022, 12:52 PM

create a subroutine to improve error handling

varun planned changes to this revision.Jun 23 2022, 12:52 PM
Harbormaster completed remote builds in B9930: Diff 13730.Jun 23 2022, 12:57 PM
varun updated this revision to Diff 13732.Jun 23 2022, 1:26 PM
varun marked 3 inline comments as done.

handle unhandled error case and unhandled option case

services/identity/src/service.rs
72 ↗(On Diff #13549)

oneof fields are completely optional, which is why this can be None. we are reconsidering the use of oneof across all our .proto files, but i've handled the None case in the latest revision.

Harbormaster completed remote builds in B9932: Diff 13732.Jun 23 2022, 1:32 PM
varun added inline comments.Jun 23 2022, 1:34 PM
services/identity/src/service.rs
87–105 ↗(On Diff #13732)

Don't love how I'm repeating myself here but wasn't sure how to avoid it

tomek accepted this revision.Jun 24 2022, 4:37 AM
tomek added inline comments.
services/identity/src/service.rs
62 ↗(On Diff #13732)

What is the purpose of it?

87–105 ↗(On Diff #13732)

I think you can create a function that contains the repeated logic

varun added inline comments.Jun 24 2022, 5:28 AM
services/identity/src/service.rs
62 ↗(On Diff #13732)

MyIdentityService doesn't implement Debug, so we have to skip self when we do tracing

varun added a child revision: D4350: [services][identity] helper function to start PAKE login.Jun 24 2022, 6:32 AM
varun updated this revision to Diff 13811.Jun 27 2022, 12:22 AM

rebase

Harbormaster completed remote builds in B9994: Diff 13811.Jun 27 2022, 12:39 AM
varun added a parent revision: D4360: [services][identity] PAKE workflow enum.Jun 27 2022, 12:55 AM
varun updated this revision to Diff 13814.Jun 27 2022, 12:57 AM

rebase

varun edited parent revisions, added: D4361: [services][identity] update put_token_helper to return a String; removed: D4360: [services][identity] PAKE workflow enum.Jun 27 2022, 12:58 AM
varun added a child revision: D4362: [services][identity] update Config struct to hold keypair instead of just private key.Jun 27 2022, 1:00 AM
varun removed a child revision: D4350: [services][identity] helper function to start PAKE login.
Harbormaster completed remote builds in B9997: Diff 13814.Jun 27 2022, 1:11 AM
varun removed a reviewer: • jim.Jun 27 2022, 2:45 PM
This revision is now accepted and ready to land.Jun 27 2022, 2:45 PM
Closed by commit rCOMMe7885e5a97fa: [services][identity] wallet login (authored by varun). · Explain WhyJun 27 2022, 3:04 PM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMMe7885e5a97fa: [services][identity] wallet login.
ashoat added inline comments.Jun 28 2022, 1:10 PM
services/identity/src/service.rs
75–123 ↗(On Diff #13860)

Readability is not great here imo....

varun added inline comments.Jun 28 2022, 1:19 PM
services/identity/src/service.rs
75–123 ↗(On Diff #13860)

Yeah, agreed. Made it a little better in a subsequent diff that handled PAKE login, and have a diff out to improve it further

Revision Contents
Changeset List

PathSize
services/
identity/
2 lines
src/
1 line
133 lines

Diff 13811

View Options

services/identity/Cargo.lock

Loading...
View Options

services/identity/Cargo.toml

Loading...
View Options

services/identity/src/database.rs

Loading...
View Options

services/identity/src/service.rs

Loading...