Details

Reviewers

• jim
• karol
tomek
ashoat

Commits

rCOMM71d308d9abbc: [Identity] Add new types to handle tokens

Summary

Our DatabaseClient will fetch user tokens from DynamoDB. We need a struct to store the data pertaining to a token.

We use the chrono crate to handle date/time

Test Plan

cargo build

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 6 2022, 7:15 AM

Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptMay 6 2022, 7:15 AM

Harbormaster returned this revision to the author for changes because remote builds failed.May 6 2022, 7:15 AM

Harbormaster failed remote builds in B8856: Diff 12352!

rebase

Harbormaster completed remote builds in B8857: Diff 12353.May 6 2022, 7:23 AM

varun requested review of this revision.May 6 2022, 7:23 AM

varun added a parent revision: D3921: [Identity] Implement database client method to get pake registration.May 6 2022, 10:34 AM

varun added a child revision: D3957: [Identity] DatabaseClient method to get user token.

Split new types out into new token module

varun added 1 blocking reviewer(s): • jim.May 6 2022, 10:44 AM

Harbormaster completed remote builds in B8878: Diff 12374.May 6 2022, 10:47 AM

Why you don't put [services] prefix in your revisions' names for the identity service? I thought we had a convention that we put a prefix depending on the "module" we work on. So, for instance, [web], [server], [native]. [services] Is at the same level, [identity] is deeper one level. Is there a particular reason you do not do this here? Has this rule changed?

services/identity/src/token.rs
9–12 ↗	(On Diff #12374)	What is the reason to use `Option` here? Why `token` is not optional and the rest of the fields are?

This revision now requires changes to proceed.May 9 2022, 2:51 AM

In D3945#110907, @karol-bisztyga wrote:

Why you don't put [services] prefix in your revisions' names for the identity service? I thought we had a convention that we put a prefix depending on the "module" we work on. So, for instance, [web], [server], [native]. [services] Is at the same level, [identity] is deeper one level. Is there a particular reason you do not do this here? Has this rule changed?

I have this pattern but I've actually never pushed it as a rule. The other day @jonringer-comm asked me if he should prefix his commit titles with [nix], so this is coming up in multiple places. Open to creating a rule for this but curious for other perspectives

In D3945#111222, @ashoat wrote:

In D3945#110907, @karol-bisztyga wrote:

Why you don't put [services] prefix in your revisions' names for the identity service? I thought we had a convention that we put a prefix depending on the "module" we work on. So, for instance, [web], [server], [native]. [services] Is at the same level, [identity] is deeper one level. Is there a particular reason you do not do this here? Has this rule changed?

I have this pattern but I've actually never pushed it as a rule. The other day @jonringer-comm asked me if he should prefix his commit titles with [nix], so this is coming up in multiple places. Open to creating a rule for this but curious for other perspectives

I see. For me, personally, it would be handier because I set my herald rule to detect any revisions with [services] in their names. If you decide to user other naming then fine but I think it will be better for everybody to use one, consistent pattern.

As a side note, maybe we could use tags?

That is a really good point about Herald rules. @benschac was asking on ENG-1055 about whether we should come up with a new Herald rule, since we're no longer recommending subscribing to all diffs... this idea makes sense to me.

i'll use [services][identity] going forward. are herald rules case sensitive?

services/identity/src/token.rs
9–12 ↗	(On Diff #12374)	We use this struct to hold data returned from DynamoDB, so we need to handle incomplete data. I think token should actually be wrapped in an `Option` too... will fix

Rename Token to AccessToken to be more precise, make token field optional (to handle incomplete data in DynamoDB)

Harbormaster completed remote builds in B9037: Diff 12580.May 12 2022, 8:35 AM

Accepting to preempt somebody adding me due to dependency change

back to you

services/identity/src/token.rs
9–12 ↗	(On Diff #12374)	Sorry, but I don't really understand. This seems to be inconsistent with https://phabricator.ashoat.com/D3573?vs=on&id=11131: where's `userID`? why do you use just `auth_type`? I don't see `pakePasswordCiphertext`/`walletAddress`. What's the primary key here?

This revision now requires changes to proceed.May 16 2022, 1:00 AM

varun planned changes to this revision.May 19 2022, 11:21 AM

varun added inline comments.

services/identity/src/token.rs
9–12 ↗	(On Diff #12374)	I was passing userID and deviceID to functions as separate args in addition to the Token struct, but perhaps that's confusing. I can include all of it in the Token struct. https://phabricator.ashoat.com/D3573?vs=on&id=11902#toc in a later revision than the one you linked I realized we didn't actually need to store that stuff in the token table primary key is the userID (partition key) + deviceID (sort key)