Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Element-R: implement cancellation of verification requests #3505

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jun 23, 2023

Based on #3504

The final part of element-hq/element-web#25319

Notes: Support for interactive device verification in Element-R.


Here's what your changelog entry will look like:

✨ Features

  • Support for interactive device verification in Element-R. (#3505).

@richvdh richvdh force-pushed the rav/element-r/28_cancel_request branch from 2294b8f to 96173bc Compare June 23, 2023 15:56
@richvdh richvdh marked this pull request as ready for review June 23, 2023 16:06
@richvdh richvdh requested review from a team as code owners June 23, 2023 16:06
Copy link

@artcodespace artcodespace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments are small ones, nothing blocking.


/** the method picked in the .start event */
public get chosenMethod(): string | null {
const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RustSdkCryptos.Qr | RustSdkCryptoJs.Sas | undefined union appears a couple of times in this class. Do you think it would warrant becoming a type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, though there's also an argument that it should be the rust-sdk's job to declare the type; or better yet, to define the return type of getVerification correctly so that application code doesn't need to do this sort of annoying type assertion (but that's hard because of limitations in the rust tooling).

This code is actually from #3490, so I'll leave it alone for now, in any case. Will consider adding a type if I find myself writing another copy of the union.

},
confirm: async (): Promise<void> => {
const requests: Array<OutgoingRequest> = await this.inner.confirm();
for (const m of requests) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a bone question. Why m?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erm, good question. m for message, maybe? Though again, #3490 rather than new in this PR.

function onChange() {
expect(request.phase).toEqual(VerificationPhase.Started);
expect(request.otherPartySupportsMethod("m.sas.v1")).toBe(true);
expect(request.chosenMethod).toEqual("m.sas.v1");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be toBe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly? I'm not sure it makes any difference, or if there is any particluar convention here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only asking as using toEqual suggests to me some sort of object comparison is going on (comparing keys and values I mean) whereas toBe suggests comparison of primitives. It makes no difference to the output here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we don't normally write method.is("m.sas.v1") for doing string comparison, so I'm not convinced that toBe is, in fact, more natural for string comparison.

Also, I come from a Python world, where you definitely need assertEqual, and not assertIs:

>>> a = "foo"
>>> a += "bar"
>>> b = "foobar"
>>> a == b
True
>>> a is b
False

// initially there should be no verifications in progress
{
const requests = aliceClient.getCrypto()!.getVerificationRequestsToDeviceInProgress(TEST_USER_ID);
expect(requests.length).toEqual(0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a toHaveLength matcher available if you prefer how that reads

spec/integ/crypto/verification.spec.ts Show resolved Hide resolved
@richvdh richvdh changed the base branch from develop to rav/element-r/27_refactor_integ_test June 26, 2023 11:31
Base automatically changed from rav/element-r/27_refactor_integ_test to develop June 26, 2023 15:04
@richvdh richvdh changed the title Element-R: implement cancellation of verifcation requests Element-R: implement cancellation of verification requests Jun 26, 2023
@richvdh richvdh force-pushed the rav/element-r/28_cancel_request branch from 1398ac0 to 85e0ddf Compare June 26, 2023 15:23
@richvdh richvdh added this pull request to the merge queue Jun 26, 2023
Merged via the queue into develop with commit d1dec4c Jun 26, 2023
@richvdh richvdh deleted the rav/element-r/28_cancel_request branch June 26, 2023 17:11
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Feb 22, 2024
* The Browserify artifact is being deprecated, scheduled for removal in the October 10th release cycle. ([\matrix-org#3189](matrix-org#3189)).
* ElementR: Add `CryptoApi#bootstrapSecretStorage` ([\matrix-org#3483](matrix-org#3483)). Contributed by @florianduros.
* Deprecate `MatrixClient.findVerificationRequestDMInProgress`, `MatrixClient.getVerificationRequestsToDeviceInProgress`, and `MatrixClient.requestVerification`, in favour of methods in `CryptoApi`. ([\matrix-org#3474](matrix-org#3474)).
* Introduce a new `Crypto.VerificationRequest` interface, and deprecate direct access to the old `VerificationRequest` class. Also deprecate some related classes that were exported from `src/crypto/verification/request/VerificationRequest` ([\matrix-org#3449](matrix-org#3449)).
* OIDC: navigate to authorization endpoint ([\matrix-org#3499](matrix-org#3499)). Contributed by @kerryarchibald.
* Support for interactive device verification in Element-R. ([\matrix-org#3505](matrix-org#3505)).
* Support for interactive device verification in Element-R. ([\matrix-org#3508](matrix-org#3508)).
* Support for interactive device verification in Element-R. ([\matrix-org#3490](matrix-org#3490)). Fixes element-hq/element-web#25316.
* Element-R: Store cross signing keys in secret storage ([\matrix-org#3498](matrix-org#3498)). Contributed by @florianduros.
* OIDC: add dynamic client registration util function ([\matrix-org#3481](matrix-org#3481)). Contributed by @kerryarchibald.
* Add getLastUnthreadedReceiptFor utility to Thread delegating to the underlying Room ([\matrix-org#3493](matrix-org#3493)).
* ElementR: Add `rust-crypto#createRecoveryKeyFromPassphrase` implementation ([\matrix-org#3472](matrix-org#3472)). Contributed by @florianduros.
* Aggregate relations regardless of whether event fits into the timeline ([\matrix-org#3496](matrix-org#3496)). Fixes element-hq/element-web#25596.
* Fix bug where switching media caused media in subsequent calls to fail ([\matrix-org#3489](matrix-org#3489)).
* Fix: remove polls from room state on redaction ([\matrix-org#3475](matrix-org#3475)). Fixes element-hq/element-web#25573. Contributed by @kerryarchibald.
* Fix export type `GeneratedSecretStorageKey` ([\matrix-org#3479](matrix-org#3479)). Contributed by @florianduros.
* Close IDB database before deleting it to prevent spurious unexpected close errors ([\matrix-org#3478](matrix-org#3478)). Fixes element-hq/element-web#25597.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants