Details

Reviewers

• jim
• karol
tomek

Commits

rCOMM6b2d0082d936: [Identity] Token constructor

Summary

new function to generate a Token. we use the rand crate to generate a random string for the token, and chrono to get the creation time. I used len 512 for the string because that's what OAuth uses for access tokens.

@jimpo, is this the best way to generate a random String in Rust?

Depends on D3957

Test Plan

Called constructor and verified the contents of the Token

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.May 6 2022, 12:03 PM

Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptMay 6 2022, 12:04 PM

Harbormaster completed remote builds in B8880: Diff 12376.May 6 2022, 12:08 PM

varun requested review of this revision.May 6 2022, 12:08 PM

varun added a child revision: D3960: [Identity] DatabaseClient method to put pake registration data.May 6 2022, 12:51 PM

make constructor public

Harbormaster completed remote builds in B8886: Diff 12389.May 7 2022, 7:49 AM

What kind of token is this exactly? Wonder if we should have a more precise name

• karol accepted this revision.May 9 2022, 4:33 AM

• karol added inline comments.

services/identity/src/token.rs
21 ↗	(On Diff #12389)	Cannot this be provided to the function inline?

This revision is now accepted and ready to land.May 9 2022, 4:33 AM

I'm not sure why this is in an "accepted" state. I hope it cannot be landed... After I accept, rust diffs should not be accepted, I know rust far too little to be decisive. Adding Jim as blocking.

This revision now requires review to proceed.May 9 2022, 4:53 AM

• jim requested changes to this revision.May 9 2022, 10:14 AM

• jim added inline comments.

services/identity/src/token.rs
14 ↗	(On Diff #12389)	Why are these fields all optional?
21 ↗	(On Diff #12389)	Agree, the constructor should take a `&mut impl (Rng + CryptoRng)` argument
23 ↗	(On Diff #12389)	Are you sure this will sample readable characters?

This revision now requires changes to proceed.May 9 2022, 10:14 AM

In D3959#110792, @ashoat wrote:

What kind of token is this exactly? Wonder if we should have a more precise name

It's an access token. We should name this struct AccessToken to be more precise.

Requesting review cos I have a couple q's for @jimpo

services/identity/src/token.rs
14 ↗	(On Diff #12389)	To handle incomplete data from DynamoDB. The token field should actually be optional too, fixing that in a separate diff
21 ↗	(On Diff #12389)	I see how adding the rng as an argument could make testing easier, but is there another reason why we should do this?
23 ↗	(On Diff #12389)	It won't. I'd need to use Alphanumeric instead of Standard to sample readable characters. Perhaps a naive q - why is readability important?

Addressing feedback. Still have a couple unanswered q's on the last revision

Harbormaster completed remote builds in B9041: Diff 12584.May 12 2022, 10:13 AM

• jim requested changes to this revision.May 17 2022, 8:04 AM

• jim added inline comments.

services/identity/src/token.rs
14 ↗	(On Diff #12389)	I don't see why that needs to be handled at the struct layer. Like if you try to fetch an invalid record from the DB with incomplete fields, just return an error. Basically, my recommendation is to drop the optionals on any fields where if it's None, the token would be considered invalid.