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
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun requested review of this revision.Jun 27 2022, 1:06 AM
tomek added inline comments.
services/identity/src/service.rs
113

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

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

112–116

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

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

112–116

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

yeah, makes sense