[E2E] Plug-in encryption in client #234
Conversation
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Looking good in general. I'm fine with how encryption is configured here. :)
matrix_client/client.py
Outdated
| self.device_id = response["device_id"] | ||
|
|
||
| if self.encryption: | ||
| if ENCRYPTION_SUPPORT: |
There was a problem hiding this comment.
Can we put this check in __init__ instead? Immediately raise error (rather than logging an error) if instantiating with encryption=True and ENCRYPTION_SUPPORT=False. Also change self.encryption to self._encryption to signal that it isn't something that should be changed at runtime, and then we can drop this check from here entirely.
There was a problem hiding this comment.
Sure, way better design!
matrix_client/client.py
Outdated
| the token) if supplying a token; otherwise, ignored. | ||
| valid_cert_check (bool): Check the homeservers | ||
| certificate on connections? | ||
| encryption (bool): Optional. Wether or not to enable end-to-end encryption |
| @@ -59,6 +59,11 @@ class MatrixClient(object): | |||
| the token) if supplying a token; otherwise, ignored. | |||
There was a problem hiding this comment.
Wait. Why do we have a docstring on both the class and on the __init__ method with subtly different information on arguments? Poking around, most people seem to just not have a docstring on __init__ methods. I'll open an issue to move all info in __init__ docstrings to class docstrings.
There was a problem hiding this comment.
I wondered this too. Also the __init__ docstrings are ignored by Sphinx.
Added a commit to fix this.
matrix_client/client.py
Outdated
| def __init__(self, base_url, token=None, user_id=None, | ||
| valid_cert_check=True, sync_filter_limit=20, | ||
| cache_level=CACHE.ALL, encryption=False): | ||
| cache_level=CACHE.ALL, encryption=False, encryption_conf={}): |
There was a problem hiding this comment.
Default args to methods should almost never be mutable objects. See #236 for example of bugs caused.
| @@ -106,27 +108,6 @@ def global_callback(incoming_event): | |||
| def __init__(self, base_url, token=None, user_id=None, | |||
| valid_cert_check=True, sync_filter_limit=20, | |||
There was a problem hiding this comment.
Not sure what to make of the undocumented sync_filter_limit, which is marked as deprecated in login docstring, so I left it undocumented.
|
Done |
|
Just had a thought, what about the logout method? Since the client attribute set when logging in are not reset when logging out, it is probably OK to keep the same OlmDevice, but it feels a bit weird nonetheless. |
To be honest, I wouldn't worry about it. I know for a fact that logging back in on a |
| self.ephemeral_listeners = [] | ||
| self.device_id = None | ||
| self._encryption = encryption | ||
| self.encryption_conf = encryption_conf or {} |
There was a problem hiding this comment.
tricksy one-liner there. Had to double-check in REPL to make sure it did what I expected.
|
This is good to merge once prereq commits are in. :) |
2 participants
First attempt at trying to integrate e2e into the existing code. No crypto stuff in this one.
With this PR and the later ones I aim at two things:
encryption=Trueto MatrixClient.Not sure about the optional dependency handling in this one, that is why I made it a separate commit. If it's the right way to do it, let me know and I'll squash 'em.
Signed-off-by: Valentin Deniaud <valentin.deniaud@inpt.fr>