Add restore_with_access_token to the FFI auth service. #866
Conversation
There was a problem hiding this comment.
So this is one of those things that scares me quite a bit. Namely if we're logging in with an access token, that token will have a device ID attached to it and this device ID might have already some E2EE keys attached to it.
If the device already has some E2EE keys attached to it and we locally aren't the owners of those keys, we're going to try to upload and attach another pair of E2EE keys to the device.
This can easily lead to broken E2EE setups where we can't decrypt other peoples messages, nor are they able to decrypt ours.
One way to ensure that the device isn't already taken is to send out a /keys/query request, and see if there are any device keys attached to this device ID.
On the other hand, since this is mostly a WIP thing while OIDC gets developed and is only touching the bindings I'm inclined to let it slide. Please just add a big warning or TODO about this.
|
In my opinion this could be improved (as in less hacky) and then reused for #859, because the same logic needed here will be required for implementing it. The only thing that would change after it's implemented is that this method wouldn't need to be public anymore. By "hacky" I mean that we shouldn't have to create a temporary client with a fake user ID and device ID just to be able to make a call to |
|
The direction OIDC is going in is to not generate the device ID on the server and instead leave it to the clients to do. In fact, it looks like this PR was just merged: matrix-org/matrix-authentication-service#285 I'll find out more info about this. |
|
Ah I didn't know about that, it's good to know. |
|
@zecakeh More information on this:
|
pixlwave
force-pushed
the
doug/login-access-token
branch
from
2141808 to
535d230
Compare
pixlwave
changed the title
I've moved the device ID creation out to the app (using it to define the login scope as mentioned above) and have updated this PR to have a I've also made some small tweaks to use the existing discovery client for the |
| .map_err(AuthenticationError::from)?; | ||
|
|
||
| // Restore the client using the session. | ||
| client |
There was a problem hiding this comment.
Do you want to modify the original client behind the mutex as well, or is that not important?
There was a problem hiding this comment.
Hmmm that's an interesting point. I don't see it as important as the service should be finished with when this succeeds. I think its fine for now but maybe we'll need to revisit this in the future 👍
pixlwave
force-pushed
the
doug/client-path
branch
from
0c847b5 to
1eadd0b
Compare
pixlwave
force-pushed
the
doug/login-access-token
branch
from
535d230 to
bda7db3
Compare
Add a device ID parameter to restore_with_access_token
pixlwave
force-pushed
the
doug/login-access-token
branch
from
bda7db3 to
a951a27
Compare
This PR is likely a temporary addition until #859 is implemented to allow for element-hq/element-x-ios#137 to be completed with the proof of concept implementation.
The new method first creates an in-memory client to perform a
/whoamiand get the user ID and device ID. Then it creates a second client with a store path set and sets this one up for real.Forked from #860 so would need to wait for that to be merged first.