Make sydent.hs_federation pass mypy --strict
#432
Conversation
It has no business being a nested function---doesn't capture anything
sort imports
DMRobertson
force-pushed
the
dmr/mypy-hs-federation
branch
from
09eebea to
38c20e2
Compare
Turn and face `git blame`
sydent/hs_federation/types.py
Outdated
| class VerifyKey(TypedDict): | ||
| key: str |
There was a problem hiding this comment.
Is it easier if we define these classes in the order that lets them build up so we can remove all the quotes? (Personally I find reading it that way easier too.)
There was a problem hiding this comment.
Personal preference I think. No strong opinions here. I'll take another look at this once I've considered where we want to use attrs.
sydent/hs_federation/verifier.py
Outdated
| For example, `X-Matrix origin=origin.example.com,key="ed25519:key1",sig="ABCDEF..."` | ||
| taken from | ||
| https://spec.matrix.org/unstable/server-server-api/#request-authentication . | ||
| TODO Is there a more authoritative reference for the X-Matrix scheme? |
There was a problem hiding this comment.
What is more authoritative than the spec?
There was a problem hiding this comment.
That particular link gives an example of the X-Matrix auth scheme, but it's not formally defined there. I couldn't see anywhere else in the server-server API that defined it.
There was a problem hiding this comment.
There's a small paragraph that vaguely describes it. I think that is the definition of it, it just could be improved.
There was a problem hiding this comment.
best to link to the stable spec, otherwise the links can go out of date: https://matrix.org/docs/spec/server_server/r0.1.4#request-authentication
but yes, that is the authoritative reference, such as it is.
| @@ -171,65 +182,77 @@ async def authenticate_request( | |||
|
|
|||
| :return: The origin of the server whose signature was validated | |||
| """ | |||
| json_request = { | |||
There was a problem hiding this comment.
I think the type annotation for content in this function is wrong.
I think it should be a JsonDict, judging by the callsite.
Unsure if this is a net improvement
| @@ -0,0 +1,34 @@ | |||
| from typing import Dict | |||
| if "key" not in server_keys[key_name]: | ||
| logger.warning("Ignoring key %s with no 'key'") | ||
| continue |
There was a problem hiding this comment.
This got moved into _getKeysForServer, essentially. (I think?)
There was a problem hiding this comment.
That's right. On lines 100--109 of verifier.py, I wanted to ensure that I only had something of type VerifyKeys after verifying that it was of the correct type and structure.
(attrs and cattrs might make this easier one day?)
| from sydent.types import JsonDict | ||
|
|
||
|
|
||
| class VerifyKey(TypedDict): |
There was a problem hiding this comment.
Just sort of curious why this class doesn't require the @attr decorator to use what looks like attr-powered class creation. I am sure there is something terribly simple I am missing here :)
There was a problem hiding this comment.
The syntax is similar (much like stdlib's dataclasses) but this is mainly a fancy type annotation rather than a class with actual runtime behaviour.
PEP 589 has the motivation, but in a nutshell: TypedDict is a means to add some level of type checking to code that passes dictionaries around with a fixed set of keys. I think the goal is to add type checking without having to rewrite your application or library to use attr.s, dataclasses, namedtuples.
Note that TypedDict (more strictly: the subclass of TypedDict that I create) is just used as annotation here: there's no runtime effect. (I think you can call VerifyKey(...), but it behaves as if you called dict(...) directly.)
There was a problem hiding this comment.
Does that make sense? Happy to expand or reword either way!
This was a bit more involved!