Page MenuHomePhabricator

[Identity] Implement database client method to get pake registration
ClosedPublic

Authored by varun on May 4 2022, 1:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 14, 5:54 AM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:16 PM

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.May 4 2022, 1:44 PM
karol added 1 blocking reviewer(s): jim.

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

jim requested changes to this revision.May 5 2022, 8:47 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.

This revision now requires changes to proceed.May 5 2022, 8:47 AM
varun marked 4 inline comments as done.

Address feedback

karol added inline comments.
services/identity/src/database.rs
45–48 ↗(On Diff #12251)

Thanks!

Use helper function to construct primary key

This revision is now accepted and ready to land.May 17 2022, 8:23 AM