Page MenuHomePhabricator

[web] Use `signedIdentityKeysBlobSelector` in `TraditionalLoginForm`
ClosedPublic

Authored by atul on Mar 12 2023, 12:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 11:49 AM
Unknown Object (File)
Fri, Mar 8, 6:48 PM
Unknown Object (File)
Fri, Mar 8, 6:48 PM
Unknown Object (File)
Fri, Mar 8, 6:48 PM
Unknown Object (File)
Tue, Mar 5, 11:09 PM
Unknown Object (File)
Tue, Mar 5, 11:09 PM
Unknown Object (File)
Tue, Mar 5, 11:09 PM
Unknown Object (File)
Feb 23 2024, 10:31 AM
Subscribers
None

Details

Summary

Now that we have signedIdentityKeysBlobSelector, we can use it in TraditionalLoginForm and SIWELoginForm (next diff) to simplify things.

Test Plan
  1. Try to log in from web.

Screenshot 2023-03-12 at 5.34.18 PM.png (2×2 px, 810 KB)

  1. Set breakpoint in loginResponder and make sure signedIdentityKeysBlob comes through and is validated as expected.

29b985.png (1×3 px, 770 KB)

  1. Check the cookies table to see if signed_identity_keys column is populated correctly.

691e29.png (542×4 px, 152 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D7047 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Mar 12 2023, 12:57 PM
atul edited the test plan for this revision. (Show Details)
atul edited the test plan for this revision. (Show Details)

fix signedIdentityKeysBlobSelector after testing

ashoat requested changes to this revision.Mar 13 2023, 9:54 AM
ashoat added inline comments.
web/selectors/socket-selectors.js
63–64 ↗(On Diff #23651)

This seems like a really significant change. Tons of questions pop up...

  1. Are we changing the format of this in the database? Do we need a migration?
  2. If I recall correctly, this format needs to be kept consistent in a lot of places. Where else needs to be updated? Do we have a task to track this?

Looking at the MySQL database on my personal keyserver, it appears like "Public" has been included previously, so perhaps this is just you fixing up an error that never got to prod? (I hope so...)

Separately, high-level meta-point: you should really expect your reviewer to call you out for things like this. It will save everyone some time if you annotate changes like this ahead-of-time.

This revision now requires changes to proceed.Mar 13 2023, 9:54 AM
atul requested review of this revision.Mar 13 2023, 10:47 AM
atul added inline comments.
web/selectors/socket-selectors.js
63–64 ↗(On Diff #23651)

Are we changing the format of this in the database? Do we need a migration?

No

If I recall correctly, this format needs to be kept consistent in a lot of places. Where else needs to be updated? Do we have a task to track this?

The keys in Redux CryptoStore are primaryIdentityKeys and notificationIdentityKeys. Those can be changed in a migration to primaryIdentityPublicKeys and notificationIdentityPublicKeys for consistency. I have a diff locally to make this exact change.

Looking at the MySQL database on my personal keyserver, it appears like "Public" has been included previously, so perhaps this is just you fixing up an error that never got to prod? (I hope so...)

Yes, this was an issue that was caught locally and never got to prod. The selector is being used for the first time in this diff and the issue was found in the course of testing thanks to the validators on the keyserver side.

Separately, high-level meta-point: you should really expect your reviewer to call you out for things like this. It will save everyone some time if you annotate changes like this ahead-of-time.

Sure

Cool, glad it never reached prod. Don't care about Redux vs. MySQL consistency (personally)

This revision is now accepted and ready to land.Mar 13 2023, 11:51 AM
This revision was landed with ongoing or failed builds.Mar 14 2023, 9:27 AM
This revision was automatically updated to reflect the committed changes.