Make the device id on e2e key upload optional #956
Conversation
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.
|
matrixbot: retest this please |
| @@ -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) | |||
There was a problem hiding this comment.
Might want to update sytest to use unstable rather than v2_alpha?
There was a problem hiding this comment.
I see that you already have.
There was a problem hiding this comment.
Is there a reason to remove the v2_alpha path?
There was a problem hiding this comment.
just that it's kinda BS, and Vector is using unstable.
There was a problem hiding this comment.
Is Vector the only thing using that API? I though Tor had got a client using the e2e stuff.
There was a problem hiding this comment.
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.
|
matrixbot: retest this please |
| 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. |
There was a problem hiding this comment.
Why do they need to match? Do they match in vector now, or will vector need to be updated for this to work?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yeah and it's gonna break Tor's stuff too. argh fuckery I hate my life.
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.
|
LGTM |
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.