Details

Reviewers

• jim
tomek
• karol

Commits

rCOMM8b4f79c57425: [Identity] Implement database client method to get pake registration

Summary

To log in a user with PAKE, the Identity service will need to fetch the user's PAKE registration data from DynamoDB. This diff introduces a method to do that. The user ID is the table's primary key, so we use that to call get_item. Then we deserialize the bytes stored in DDB to get a ServerRegistration object. If the client is unable to get a ServerRegistration object, it logs the error and returns None.

Depends on D3920

Test Plan

Tested all failure cases and the success case by having the client get an item from a sample DDB table

Diff Detail

Repository

rCOMM Comm

Branch

varun/identity_service (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.May 4 2022, 1:40 PM

Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptMay 4 2022, 1:40 PM

Harbormaster completed remote builds in B8796: Diff 12251.May 4 2022, 1:44 PM

varun requested review of this revision.May 4 2022, 1:44 PM

Back to you to get answers.

services/identity/src/database.rs
45–48 ↗	(On Diff #12251)	Sorry if I asked about this before in D3853 but you updated that diff in a way that I lost context somehow. Anyway, I understand this code like this: We check if we can unpack `item.get("pakeRegistrationData")` into a value from which we also want to unpack a field `b`. Only if both unpack are successful, do we proceed into the conditional block. Am I correct? Also, should we use better names than `b` and `s`? Where do they come from? Last thing, what `..` mean in this context? I know it's a range operator, so I assume that we just skip the rest of the items of `item.get("pakeRegistrationData")` for this block, right? Do we always have to use `..`? Cannot it be skipped?
67 ↗	(On Diff #12251)	What exactly are we handling here?

This revision now requires changes to proceed.May 5 2022, 4:18 AM

varun planned changes to this revision.May 5 2022, 7:08 AM

varun added inline comments.

services/identity/src/database.rs
45–48 ↗	(On Diff #12251)	We check if we can unpack item.get("pakeRegistrationData") into a value from which we also want to unpack a field b. Only if both unpack are successful, do we proceed into the conditional block. Am I correct? Correct Also, should we use better names than b and s? Where do they come from? Those can't be changed, they come from here: https://docs.rs/rusoto_dynamodb/latest/rusoto_dynamodb/struct.AttributeValue.html Last thing, what .. mean in this context? I know it's a range operator, so I assume that we just skip the rest of the items of item.get("pakeRegistrationData") for this block, right? Do we always have to use ..? Cannot it be skipped? `..` actually isn't a range operator in this context, it just means we want to ignore the remaining parts of the struct in our pattern matching. We use it here so we don't have to write out the full struct with underscore values. See more here: https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html#ignoring-remaining-parts-of-a-value-with-
67 ↗	(On Diff #12251)	We have to handle the case `Ok(GetItemOutput { item: None, .. })`. `_` is just shorthand when we don't need to use the contents. Should probably log an error message before we return `None`, though. Will fix that.

Add error log if no item exists for user in DynamoDB

Harbormaster completed remote builds in B8817: Diff 12284.May 5 2022, 7:39 AM

Create a real error type

services/identity/src/database.rs
54 ↗	(On Diff #12284)	Propagate the error
63 ↗	(On Diff #12284)	Return an actual error, don't just log
68 ↗	(On Diff #12284)	This doesn't seem like an error. If your return type is `Result<Option<...>, Error>` that lets the caller handle every case the way they need to. Your DB client shouldn't be deciding that this is not an OK outcome.
74 ↗	(On Diff #12284)	Propagate this error, don't just log and drop it.