/synapse

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

Already on GitHub? Sign in to your account

Make the device id on e2e key upload optional #956

Merged
merged 4 commits into from Jul 27, 2016

Conversation

Reviewers
No reviews
Assignees

@NegativeMjark NegativeMjark

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@richvdh @NegativeMjark
@richvdh
Member

richvdh commented Jul 27, 2016

We should now be able to get our device_id from the access_token, so the device_id on the upload request is optional. Where it is supplied, we should check that it matches.

Also make sure that the device_id is registered in the devices table, for both new e2e keys and those already in the database.

Finally, delete e2e keys from the database when we delete a device.

richvdh added some commits Jul 26, 2016

@richvdh
Make the device id on e2e key upload optional 
We should now be able to get our device_id from the access_token, so the
device_id on the upload request is optional. Where it is supplied, we should
check that it matches.

For active access_tokens without an associated device_id, we ought to register
the device in the devices table.

Also update the table on upgrade so that all of the existing e2e keys are
associated with real devices.
2e3d90d
@richvdh
Delete e2e keys on device delete

Some checks were not successful

3 errored and 3 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #356 origin/rav/check_device_id_on_key_upload failed in 1 min 27 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #1192 origin/rav/check_device_id_on_key_upload failed in 1 min 6 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #1217 origin/rav/check_device_id_on_key_upload failed in 5 min 34 sec
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #1247 origin/rav/check_device_id_on_key_upload succeeded in 36 sec
Details
@matrixbot
Unit Tests (Commit) — Build #1284 origin/rav/check_device_id_on_key_upload succeeded in 2 min 28 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
d47115f
@richvdh
SQL syntax fix

Some checks were not successful

3 errored, 3 failing, and 4 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #357 origin/rav/check_device_id_on_key_upload failed in 6 min 57 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #1193 origin/rav/check_device_id_on_key_upload failed in 6 min 40 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #1218 origin/rav/check_device_id_on_key_upload failed in 5 min 56 sec
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #1248 origin/rav/check_device_id_on_key_upload succeeded in 34 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #1285 origin/rav/check_device_id_on_key_upload succeeded in 2 min 30 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
26cb0ef
@richvdh
Member

richvdh commented Jul 27, 2016

matrixbot: retest this please

@richvdh richvdh assigned NegativeMjark Jul 27, 2016

@richvdh richvdh referenced this pull request in matrix-org/sytest Jul 27, 2016

Merged

Update sytests to match e2e key upload changes #271

@NegativeMjark NegativeMjark and 1 other commented on an outdated diff Jul 27, 2016

View full outdated diff
synapse/rest/client/v2_alpha/keys.py
@@ -51,23 +54,47 @@ class KeyUploadServlet(RestServlet):
},
}
"""
- PATTERNS = client_v2_patterns("/keys/upload/(?P<device_id>[^/]*)", releases=())
+ PATTERNS = client_v2_patterns("/keys/upload(/(?P<device_id>[^/]+))?$",
+ releases=(), v2_alpha=False)
@NegativeMjark

NegativeMjark Jul 27, 2016

Contributor

Might want to update sytest to use unstable rather than v2_alpha?

@NegativeMjark

NegativeMjark Jul 27, 2016

Contributor

I see that you already have.

@NegativeMjark

NegativeMjark Jul 27, 2016

Contributor

Is there a reason to remove the v2_alpha path?

@richvdh

richvdh Jul 27, 2016

Member

just that it's kinda BS, and Vector is using unstable.

@NegativeMjark

NegativeMjark Jul 27, 2016

Contributor

Is Vector the only thing using that API? I though Tor had got a client using the e2e stuff.

@richvdh

richvdh Jul 27, 2016

Member

yeah he does. https://github.com/torhve/weechat-matrix-protocol-script/blob/master/matrix.lua

and I see he's using v2_alpha

FUCK IT

this is why we shouldn't publish apis on v2_alpha until they are stable.
@NegativeMjark
Contributor

NegativeMjark commented Jul 27, 2016

matrixbot: retest this please

@NegativeMjark NegativeMjark and 1 other commented on an outdated diff Jul 27, 2016

View full outdated diff
synapse/rest/client/v2_alpha/keys.py
body = parse_json_object_from_request(request)
+ if device_id is not None:
+ # passing the device_id here is deprecated; however, we allow it
+ # for now for compatibility with older clients. But if a device_id
+ # was given here and in the auth, they must match.
@NegativeMjark

NegativeMjark Jul 27, 2016

Contributor

Why do they need to match? Do they match in vector now, or will vector need to be updated for this to work?

@richvdh

richvdh Jul 27, 2016

Member

It seems to be silly to allow you to upload keys for a different device.

And sadly this does need fixing in vector. I'm trying to fix vector, but that is making me cry too.

@NegativeMjark

NegativeMjark Jul 27, 2016

Contributor

Might it be worth holding off adding this check until vector is updated? Or are we concluding that it's okay to break it on the old vector?

@richvdh

richvdh Jul 27, 2016

Member

yeah and it's gonna break Tor's stuff too. argh fuckery I hate my life.
@richvdh
key upload tweaks 
1. Add v2_alpha URL back in, since things seem to be using it.

2. Don't reject the request if the device_id in the upload request fails to
   match that in the access_token.

Some checks were not successful

3 failing and 7 successful checks
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #1253 origin/rav/check_device_id_on_key_upload succeeded in 32 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Dendron (Commit) — Build #362 origin/rav/check_device_id_on_key_upload succeeded in 7 min 28 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #1198 origin/rav/check_device_id_on_key_upload succeeded in 7 min 26 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #1223 origin/rav/check_device_id_on_key_upload succeeded in 5 min 34 sec
Details
@matrixbot
Unit Tests (Commit) — Build #1290 origin/rav/check_device_id_on_key_upload succeeded in 2 min 28 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
ccec25e
@NegativeMjark
Contributor

NegativeMjark commented Jul 27, 2016

LGTM

@richvdh richvdh merged commit 40e5396 into develop Jul 27, 2016
7 of 10 checks passed

7 of 10 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
Flake8 + Packaging (Commit) Build #1253 origin/rav/check_device_id_on_key_upload succeeded in 32 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Dendron (Commit) Build #362 origin/rav/check_device_id_on_key_upload succeeded in 7 min 28 sec
Details
Sytest Postgres (Commit) Build #1198 origin/rav/check_device_id_on_key_upload succeeded in 7 min 26 sec
Details
Sytest SQLite (Commit) Build #1223 origin/rav/check_device_id_on_key_upload succeeded in 5 min 34 sec
Details
Unit Tests (Commit) Build #1290 origin/rav/check_device_id_on_key_upload succeeded in 2 min 28 sec
Details
Unit Tests (Merged PR) Build finished.
Details

@richvdh richvdh deleted the rav/check_device_id_on_key_upload branch Oct 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment