[lib][keyserver] Introduce `SIWEAuthRequest` and `siweAuthRequestInputValidator`
ClosedPublic
Actions

Authored by atul on Dec 24 2022, 7:57 PM.

Details

Reviewers

ashoat
tomek
kamil

Commits

rCOMM39232bd23691: [lib][keyserver] Introduce `SIWEAuthRequest` and `siweAuthRequestInputValidator`

Summary

SIWEAuthRequest defines the interface of the siwe_auth endpoint. This interface is validated on the keyserver by siweAuthRequestInputValidator.

NOTE: D5603 included a source field in siweAuthRequestInputValidator, but I don't believe it's necessary or relevant.

Depends on D6024

Test Plan

Tested implicitly by subsequent diffs.

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D6025 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Dec 24 2022, 7:57 PM

atul edited the summary of this revision. (Show Details)Dec 24 2022, 7:59 PM

atul added a parent revision: D6024: [keyserver] Introduce basic scaffolding for `siwe_auth` endpoint.

atul added a child revision: D6026: [lib] Introduce `SIWEAuthServerCall` and use as argument for `siweAuth()`.

atul edited the summary of this revision. (Show Details)Dec 24 2022, 8:02 PM

Harbormaster completed remote builds in B14735: Diff 20099.Dec 24 2022, 8:11 PM

atul requested review of this revision.Dec 24 2022, 8:11 PM

atul added inline comments.Dec 25 2022, 12:01 AM

lib/types/siwe-types.js
14 ↗	(On Diff #20099)	Going to remove this `address` field in a subsequent diff. There's no reason for it and it's redundant with the `address` field in the `message`. It's also useless because whereas the `address` field in the `message` is "signed," the `address` field in `SIWEAuthRequest` is not.

Regarding the LogInActionSource, I believe we added those to log_in for @kamil's work on the Privacy Policy stuff. While I'm not sure it's needed anymore for that work, it might be useful to know what part of the UI a SIWE request is coming from in the future. That said, we only have one place in the UI for a SIWE request today, and we won't be able to build any sort of "auto-log-in" feature for SIWE... so maybe there's no point to tracking the source, since there will only be one? Probably something we can come back to later.

lib/types/siwe-types.js
14 ↗	(On Diff #20099)	Confused why introduce it now if you're gonna remove it later

This revision is now accepted and ready to land.Dec 25 2022, 7:21 PM

atul added inline comments.Dec 25 2022, 7:23 PM

lib/types/siwe-types.js
14 ↗	(On Diff #20099)	I didn't realize until later in the stack... I just included it to match what was already there in the codebase/Derek's diffs. I could rebase, but figured we want to go as quick as possible so lazily adding a diff later seemed better?

ashoat added inline comments.Dec 25 2022, 7:28 PM

lib/types/siwe-types.js
14 ↗	(On Diff #20099)	Not trying to slow you down but it should only take 5min to rebase to an earlier commit, fix it up, and run `arc diff`

In D6025#181127, @ashoat wrote:

Regarding the LogInActionSource, I believe we added those to log_in for @kamil's work on the Privacy Policy stuff. While I'm not sure it's needed anymore for that work, it might be useful to know what part of the UI a SIWE request is coming from in the future. That said, we only have one place in the UI for a SIWE request today, and we won't be able to build any sort of "auto-log-in" feature for SIWE... so maybe there's no point to tracking the source, since there will only be one? Probably something we can come back to later.

Specifically regarding "Probably something we can come back to later," is this something we should create a Linear task for? Or is it something that'll come up naturally if/when we hypothetically have multiple places in UI for SIWE request?

remove address

This revision was landed with ongoing or failed builds.Dec 27 2022, 12:02 AM

Closed by commit rCOMM39232bd23691: [lib][keyserver] Introduce `SIWEAuthRequest` and `siweAuthRequestInputValidator` (authored by atul). · Explain Why

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMM39232bd23691: [lib][keyserver] Introduce `SIWEAuthRequest` and `siweAuthRequestInputValidator`.

Harbormaster completed remote builds in B14755: Diff 20125.Dec 27 2022, 12:14 AM

In D6025#181127, @ashoat wrote:

Regarding the LogInActionSource, I believe we added those to log_in for @kamil's work on the Privacy Policy stuff. While I'm not sure it's needed anymore for that work, it might be useful to know what part of the UI a SIWE request is coming from in the future. That said, we only have one place in the UI for a SIWE request today, and we won't be able to build any sort of "auto-log-in" feature for SIWE... so maybe there's no point to tracking the source, since there will only be one? Probably something we can come back to later.

Yes, it was added to the initial version of Policy stuff, but we current logic when we use modal not auth forms so the source is not needed anymore.

In D6025#181212, @atul wrote:

Specifically regarding "Probably something we can come back to later," is this something we should create a Linear task for? Or is it something that'll come up naturally if/when we hypothetically have multiple places in UI for SIWE request?

From my perspective, accepting the policies thing was the only reason for passing the login source to the keyserver, so probably it'll come up naturally. Still, maybe there is a different need I'm not familiar with.

Thanks for asking, don't think we need a Linear task for this right now since it's entirely hypothetical. My commentary above was probably unnecessary