Page MenuHomePhabricator
Differential D4140

[services][identity] helper function to handle pake registration
AbandonedPublic
Actions

Authored by varun on May 26 2022, 12:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 10:54 PM
Unknown Object (File)
Fri, Dec 20, 10:53 PM
Unknown Object (File)
Fri, Dec 20, 10:53 PM
Unknown Object (File)
Fri, Dec 20, 10:53 PM
Unknown Object (File)
Fri, Dec 20, 10:53 PM
Unknown Object (File)
Fri, Dec 20, 10:53 PM
Unknown Object (File)
Fri, Dec 20, 10:51 PM
Unknown Object (File)
Fri, Dec 20, 8:06 PM
View All Files
Subscribers
• abosh
• adrian
ashoat

Details

Reviewers
• jim
• karol
atul
tomek
Summary

This helper function takes the PAKE registration bytes from the gRPC request and deserializes them. Then it generates a message to return to the client and sends the message to the bidi stream via the mpsc Sender.

I've also added an Error enum to this module. Thought it made sense to include both in a single diff for context.

Depends on D3962

Test Plan

confirmed that the Receiver of the mpsc channel received a RegistrationResponse

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.May 26 2022, 12:15 PM
Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptMay 26 2022, 12:15 PM
Harbormaster completed remote builds in B9498: Diff 13152.May 26 2022, 12:22 PM
varun requested review of this revision.May 26 2022, 12:22 PM
varun updated this revision to Diff 13172.May 27 2022, 10:10 AM

Remove unused import

varun added a child revision: D4146: [services][identity] helper function to finish pake registration.May 27 2022, 10:14 AM
Harbormaster completed remote builds in B9510: Diff 13172.May 27 2022, 10:17 AM
varun retitled this revision from [service][identity] helper function to handle pake registration to [services][identity] helper function to handle pake registration.May 27 2022, 10:42 AM
varun updated this revision to Diff 13180.May 27 2022, 12:24 PM

Make helper function a method

Harbormaster completed remote builds in B9516: Diff 13180.May 27 2022, 12:32 PM
varun updated this revision to Diff 13186.May 27 2022, 12:56 PM

remove unnecessary import

Harbormaster completed remote builds in B9517: Diff 13186.May 27 2022, 1:01 PM
varun updated this revision to Diff 13187.May 27 2022, 1:02 PM

rebase

Harbormaster completed remote builds in B9518: Diff 13187.May 27 2022, 1:07 PM
• karol requested changes to this revision.May 30 2022, 1:49 AM
• karol added inline comments.
services/identity/src/config.rs
4 ↗(On Diff #13187)

What is an exact reason for giving up on Default here?

services/identity/src/service.rs
70 ↗(On Diff #13187)

Have you considered using Bytes? Maybe this crate also uses Vec<u8> so there's no difference? And if it does it differently, what are the pros and cons, and why do it manually? From Bytes docs:

It is intended for use primarily in networking code

71 ↗(On Diff #13187)

What is this sorcery? Ok, this is a mutable reference to an object of a type that implements Rng and CryptoRng?

79 ↗(On Diff #13187)

I don't get it. Why do we even care about Status (which is essentially an error, right?) if we use this function here to bump the error to the upper scope anyway? Is this Result<RegistrationResponse, Status> forced by some 3rd party lib or something?

86 ↗(On Diff #13187)

So this function is just used to lift an error to the upper scope if something goes wrong, right?

This revision now requires changes to proceed.May 30 2022, 1:49 AM
• karol added a comment.May 30 2022, 1:50 AM
This comment was removed by • karol.
• karol added 1 blocking reviewer(s): • jim.May 30 2022, 2:11 AM
varun requested review of this revision.Jun 2 2022, 4:09 PM
varun added inline comments.
services/identity/src/config.rs
4 ↗(On Diff #13187)

There is no implementation of the Default for Key. There was one for Option (None), which is why it worked before I removed the Option wrapper in the Config struct.

services/identity/src/service.rs
70 ↗(On Diff #13187)

Since we're just serializing and deserializing this data, there's no advantage to using Bytes. If we were doing something more involved like slicing and dicing the bytes, I would probably use Bytes.

71 ↗(On Diff #13187)

correct! traits make dependency injection pretty easy in rust

79 ↗(On Diff #13187)

Is this Result<RegistrationResponse, Status> forced by some 3rd party lib or something?

This is correct. Tonic requires that the register_user method returns Result<Response<Self::RegisterUserStream>, Status>

86 ↗(On Diff #13187)

correct

• jim requested changes to this revision.Jun 3 2022, 8:23 AM
• jim added inline comments.
services/identity/src/service.rs
70 ↗(On Diff #13187)

@karol-bisztyga Is almost right, this should be a &[u8] byte slice, not a Vec. The hint is that deserialize takes a &[u8] and this param is passed straight in.

86 ↗(On Diff #13187)

The map_err is not necessary since Error impls From<SendError<Result<...>>. Try with just the ?.

This revision now requires changes to proceed.Jun 3 2022, 8:23 AM
varun abandoned this revision.Jun 24 2022, 6:19 AM

Revision Contents
Changeset List

PathSize
services/
identity/
src/
6 lines
50 lines

Diff 13180

View Options

services/identity/src/config.rs

Loading...
View Options

services/identity/src/service.rs

Loading...