Page MenuHomePhabricator

[keyserver] update claim username responder to take username and password
ClosedPublic

Authored by varun on May 31 2024, 2:01 PM.
Tags
None
Referenced Files
F3351598: D12258.diff
Sat, Nov 23, 2:44 AM
Unknown Object (File)
Mon, Nov 18, 1:38 PM
Unknown Object (File)
Tue, Nov 12, 10:00 AM
Unknown Object (File)
Tue, Nov 12, 9:56 AM
Unknown Object (File)
Tue, Nov 12, 8:33 AM
Unknown Object (File)
Tue, Nov 12, 7:32 AM
Unknown Object (File)
Tue, Nov 12, 1:13 AM
Unknown Object (File)
Thu, Nov 7, 8:42 PM
Subscribers

Details

Summary

logged out clients will be anonymous viewers to the keyserver, so we'll need a username and password to attest that the user is who they say they are

Test Plan

tested in following diff by calling the responder and then using the message from the keyserver to successfully authenticate with identity service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun added inline comments.
lib/actions/user-actions.js
215 ↗(On Diff #40812)

this was timing out pretty consistently on staging so i increased the timeout threshold

varun published this revision for review.May 31 2024, 2:12 PM
ashoat added inline comments.
keyserver/src/endpoints.js
463 ↗(On Diff #40812)

It looks like this might cause all callsites to this endpoint on existing clients to fail. Are any existing clients using this endpoint?

keyserver/src/responders/user-responders.js
964–967 ↗(On Diff #40812)

Reduce indentation on these three lines

979 ↗(On Diff #40812)

If !userRow.hash, should we throw invalid_parameters instead, since this account wasn't required originally with the authoritative keyserver, and can't be reserved?

lib/actions/user-actions.js
215 ↗(On Diff #40812)

Should we increase it further? Isn't our default timeout longer than this? Should we just remove this override?

225 ↗(On Diff #40812)

We could probably avoid spreading here if we wrap the declaration with Object.freeze, but in my comment above I suggest removing this override entirely

This revision is now accepted and ready to land.Jun 1 2024, 12:08 PM
keyserver/src/endpoints.js
463 ↗(On Diff #40812)

no there are no existing clients using this endpoint

keyserver/src/responders/user-responders.js
979 ↗(On Diff #40812)

that makes sense

lib/actions/user-actions.js
215 ↗(On Diff #40812)

yeah it can just be removed. default timeout is 10 seconds i believe