Skip to content

[WIP] initial tests for key backup#486

Closed
uhoreg wants to merge 1 commit into
matrix-org:developfrom
uhoreg:e2e_backup
Closed

[WIP] initial tests for key backup#486
uhoreg wants to merge 1 commit into
matrix-org:developfrom
uhoreg:e2e_backup

Conversation

@uhoreg
Copy link
Copy Markdown
Member

@uhoreg uhoreg commented Aug 17, 2018

tests for matrix-org/synapse#2731 (or whatever its successor will end up being)

@uhoreg uhoreg changed the title initial tests for key backup [WIP] initial tests for key backup Aug 17, 2018
@@ -0,0 +1,271 @@
my $fixture = local_user_fixture();

my $current_version; # FIXME: is there a better way of passing the backup version between tests?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is fine.

method => "POST",
uri => "/unstable/room_keys/version",
content => {
algorithm => "m.megolm_backup.v1",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm, shouldn't we use a real algorithm for this? (or is this a real one? it surprises me it doesn't include info about the ciphers being used, like the m.room.encryption algorithm does)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's the algorithm given in the proposal, but I agree, it should probably include some algorithm info

$content->{auth_data} eq "anopaquestring" or
die "Expected auth_data to match submitted data";

# FIXME: check that version matches the version returned above
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm, isn't this just comparing $current_version with $content->{version} or something?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, except that I don't think the version is returned yet, so I need to fix synapse to do that first. ;)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So it turns out that Synapse does return the version, even though it wasn't in the proposal. 🤷‍♂️

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oops. i can imagine that i fixed up some of the thinkos in the proposal whilst doing the implementation, but then given the implementation never got fully finished, the proposal wasn't updated - sorry.


assert_json_keys( $content, "auth_data" );

$content->{algorithm} eq "m.megolm_backup.v1" or
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpicking, but i'd probably factor out these constants in the name of DRY

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually, ignore this, it's more legible at it is.

$content->{first_message_index} == 1 or
die "Expected first message index to match submitted data";

$content->{forwarded_count} == 0 or
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should probably exercise forwarded_count here too - perhaps just put another update attempt in here to check that forwarded keys don't trump non-forwarded ones? (iirc that's how the logic went)

@ara4n
Copy link
Copy Markdown
Member

ara4n commented Aug 17, 2018

lgtm, other than minor feedback!

@uhoreg
Copy link
Copy Markdown
Member Author

uhoreg commented Aug 18, 2018

Reminder to myself before I forget: still need to check the bulk uploading/downloading endpoints, as well as the delete endpoints. Also the the get endpoints without a version specified

@dbkr
Copy link
Copy Markdown
Member

dbkr commented Oct 5, 2018

I've added more tests in #486

@dbkr
Copy link
Copy Markdown
Member

dbkr commented Oct 9, 2018

Superseded by #503

@dbkr dbkr closed this Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants