Page MenuHomePhabricator

[services][identity] update put_token_helper to return a String
ClosedPublic

Authored by varun on Jun 27 2022, 12:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 28, 2:54 PM
Unknown Object (File)
Fri, Oct 18, 8:01 PM
Unknown Object (File)
Fri, Oct 18, 6:09 PM
Unknown Object (File)
Tue, Oct 15, 10:30 AM
Unknown Object (File)
Thu, Oct 10, 1:44 AM
Unknown Object (File)
Thu, Oct 10, 1:44 AM
Unknown Object (File)
Thu, Oct 10, 1:42 AM
Unknown Object (File)
Mon, Oct 7, 4:42 AM

Details

Summary

This change lets us reuse this helper function for the PAKE registration workflow. Previously, this function returned a struct specific to the Login workflow.

Depends on D4360

Test Plan

cargo build

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Jun 27 2022, 1:06 AM
tomek added inline comments.
services/identity/src/service.rs
113 ↗(On Diff #13813)

I think it might be more useful if the value was enclosed in e.g. quotation marks so that when a value is empty, it is more evident.

services/identity/src/service.rs
107–108 ↗(On Diff #13813)

What is the reason for using str slices here? Is it a better solution than using Strings and borrowing them?

112–116 ↗(On Diff #13813)

I'm not sure about this pattern, honestly. Looks like a redundant boilerplate. Why do we use both error! and Err with 2 different messages? Cannot we just return the Err with the more precise message? Is the message from returned status shown in the end? If so, what is the exact reason for additionally using error!?

This revision is now accepted and ready to land.Jun 27 2022, 8:05 AM
services/identity/src/service.rs
107–108 ↗(On Diff #13813)

it's just cleaner since these params are immutable. also usually leads to less clone-ing

112–116 ↗(On Diff #13813)

we should be intentionally opaque about the message we return in Err since that is being propagated to the client. the error log, on the other hand, should be detailed.

113 ↗(On Diff #13813)

yeah, makes sense