Page MenuHomePhabricator
Differential D7047

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

Authored by atul on Mar 12 2023, 12:57 PM.
Tags
None
Referenced Files
F3361859: D7047.diff
Sun, Nov 24, 7:50 PM
Unknown Object (File)
Sat, Nov 9, 2:26 AM
Unknown Object (File)
Sat, Nov 9, 1:45 AM
Unknown Object (File)
Sat, Nov 9, 12:42 AM
Unknown Object (File)
Sat, Nov 9, 12:42 AM
Unknown Object (File)
Sat, Nov 9, 12:42 AM
Unknown Object (File)
Sat, Nov 9, 12:42 AM
Unknown Object (File)
Tue, Nov 5, 10:11 PM
View All Files
Subscribers
None

Details

Reviewers
ashoat
Commits
rCOMMe44fc9c0e371: [web] Use `signedIdentityKeysBlobSelector` in `TraditionalLoginForm`
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 created this revision.Mar 12 2023, 12:57 PM
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)
Harbormaster completed remote builds in B17261: Diff 23649.Mar 12 2023, 1:13 PM
atul edited the test plan for this revision. (Show Details)Mar 12 2023, 2:12 PM
atul added a child revision: D7048: [web] Use `signedIdentityKeysBlobSelector` in `SIWELoginForm`.Mar 12 2023, 2:12 PM
atul updated this revision to Diff 23651.Mar 12 2023, 2:30 PM

fix signedIdentityKeysBlobSelector after testing

atul edited the test plan for this revision. (Show Details)Mar 12 2023, 2:38 PM
Harbormaster completed remote builds in B17263: Diff 23651.Mar 12 2023, 2:46 PM
atul removed a reviewer: tomek.Mar 13 2023, 7:34 AM
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

ashoat accepted this revision.Mar 13 2023, 11:51 AM

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
atul updated this revision to Diff 23717.Mar 14 2023, 9:18 AM

rebase and land

This revision was landed with ongoing or failed builds.Mar 14 2023, 9:27 AM
Closed by commit rCOMMe44fc9c0e371: [web] Use `signedIdentityKeysBlobSelector` in `TraditionalLoginForm` (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMMe44fc9c0e371: [web] Use `signedIdentityKeysBlobSelector` in `TraditionalLoginForm`.
Harbormaster completed remote builds in B17317: Diff 23717.Mar 14 2023, 9:33 AM

Revision Contents
Changeset List

PathSize
web/
account/
60 lines
selectors/
11 lines

Diff 23717

View Options

web/account/traditional-login-form.react.js

Loading...
View Options

web/selectors/socket-selectors.js

Loading...