-
-
Notifications
You must be signed in to change notification settings - Fork 833
Show a progress bar while migrating from legacy crypto #12104
Conversation
6a24076
to
835fc53
Compare
Proposal I think this should make it clear that it's a one-off. |
d3b7d88
to
3abfd00
Compare
Proposal2 |
Now updated |
c45fe14
to
afd52ca
Compare
afd52ca
to
9b29ee9
Compare
9b29ee9
to
6155a2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - a few minor comments and questions.
function getPickleAdditionalData(userId, deviceId) { | ||
const additionalData = new Uint8Array(userId.length + deviceId.length + 1); | ||
for (let i = 0; i < userId.length; i++) { | ||
additionalData[i] = userId.charCodeAt(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is all the data ASCII? We appear to be looping through UTF-16 code units and inserting them into 8-bit character data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, this whole method was a c&p from BasePlatform.ts
. To answer this specific point: hopefully!
The data in question are a user ID and a device ID. The user ID is effectively made up by playwright; currently it is always @user:localhost
. (Also, https://spec.matrix.org/v1.9/appendices/#user-identifiers mandates ascii for user IDs.)
The device ID is made up by the homeserver being used for the tests. They lack a grammar (matrix-org/matrix-spec#174), but Synapse always uses ASCII IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds OK then, but maybe worth a comment.
for (let i = 0; i < userId.length; i++) { | ||
additionalData[i] = userId.charCodeAt(i); | ||
} | ||
additionalData[userId.length] = 124; // "|" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd marginally prefer "|".charCodeAt(0)
but OK with this.
} | ||
additionalData[userId.length] = 124; // "|" | ||
for (let i = 0; i < deviceId.length; i++) { | ||
additionalData[userId.length + 1 + i] = deviceId.charCodeAt(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be easier to concatenate the strings and then loop through one string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you'd think. I can only say again: this was cargo-culted.
yield decoded.charCodeAt(i); | ||
} | ||
}; | ||
const decoded = Uint8Array.from(itFunc()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about UTF-16 code units going into an 8-bit array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but I guess these are from a base64-encoded string, so they are guaranteed ASCII.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. this bit was cargo-culted from decodeBase64
in the js-sdk.
Weirdly, there is no 1.11.56, so it apparently was skipped? Changes in [1.11.57](https://github.com/element-hq/element-web/releases/tag/v1.11.57) (2024-01-31) ================================================================================================== ## 🦖 Deprecations * Deprecate welcome bot `welcome_user_id` support ([#26885](element-hq/element-web#26885)). Contributed by @t3chguy. ## ✨ Features * Expose apps/widgets ([#12071](matrix-org/matrix-react-sdk#12071)). Contributed by @charlynguyen. * Enable the rust-crypto labs button ([#12114](matrix-org/matrix-react-sdk#12114)). Contributed by @richvdh. * Show a progress bar while migrating from legacy crypto ([#12104](matrix-org/matrix-react-sdk#12104)). Contributed by @richvdh. * Update Twemoji to Jdecked v15.0.3 ([#12147](matrix-org/matrix-react-sdk#12147)). Contributed by @t3chguy. * Change Quick Settings icon ([#12141](matrix-org/matrix-react-sdk#12141)). Contributed by @florianduros. * Use Compound tooltips more widely ([#12128](matrix-org/matrix-react-sdk#12128)). Contributed by @t3chguy. ## 🐛 Bug Fixes * Fix OIDC bugs due to amnesiac stores forgetting OIDC issuer \& other data ([#12166](matrix-org/matrix-react-sdk#12166)). Contributed by @t3chguy. * Fix account management link for delegated auth OIDC setups ([#12144](matrix-org/matrix-react-sdk#12144)). Contributed by @t3chguy. * Fix Safari IME support ([#11016](matrix-org/matrix-react-sdk#11016)). Contributed by @SuperKenVery. * Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12127](matrix-org/matrix-react-sdk#12127)). * Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12126](matrix-org/matrix-react-sdk#12126)). Contributed by @t3chguy. * Fix 1F97A and 1F979 in Twemoji COLR font ([#12177](matrix-org/matrix-react-sdk#12177)). ## ✨ Features * Expose apps/widgets ([#12071](matrix-org/matrix-react-sdk#12071)). Contributed by @charlynguyen. * Enable the rust-crypto labs button ([#12114](matrix-org/matrix-react-sdk#12114)). Contributed by @richvdh. * Show a progress bar while migrating from legacy crypto ([#12104](matrix-org/matrix-react-sdk#12104)). Contributed by @richvdh. * Update Twemoji to Jdecked v15.0.3 ([#12147](matrix-org/matrix-react-sdk#12147)). Contributed by @t3chguy. * Change Quick Settings icon ([#12141](matrix-org/matrix-react-sdk#12141)). Contributed by @florianduros. * Use Compound tooltips more widely ([#12128](matrix-org/matrix-react-sdk#12128)). Contributed by @t3chguy. ## 🐛 Bug Fixes * Fix OIDC bugs due to amnesiac stores forgetting OIDC issuer \& other data ([#12166](matrix-org/matrix-react-sdk#12166)). Contributed by @t3chguy. * Fix account management link for delegated auth OIDC setups ([#12144](matrix-org/matrix-react-sdk#12144)). Contributed by @t3chguy. * Fix Safari IME support ([#11016](matrix-org/matrix-react-sdk#11016)). Contributed by @SuperKenVery. * Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12127](matrix-org/matrix-react-sdk#12127)). * Fix Stickerpicker layout crossing multiple CSS stacking contexts ([#12126](matrix-org/matrix-react-sdk#12126)). Contributed by @t3chguy. * Fix 1F97A and 1F979 in Twemoji COLR font ([#12177](matrix-org/matrix-react-sdk#12177)). ## ✨ Features * Use jitsi-lobby in video channel (video rooms) ([#26879](element-hq/element-web#26879)). Contributed by @toger5.
Fixes element-hq/element-web#26773.
Requires matrix-org/matrix-js-sdk#3982
Screenshot:
Here's what your changelog entry will look like:
✨ Features