Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add type hints to various handlers. #9223

Merged
merged 11 commits into from
Jan 26, 2021
8 changes: 8 additions & 0 deletions mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ files =
synapse/handlers/_base.py,
synapse/handlers/account_data.py,
synapse/handlers/account_validity.py,
synapse/handlers/acme.py,
synapse/handlers/acme_issuing_service.py,
synapse/handlers/admin.py,
synapse/handlers/appservice.py,
synapse/handlers/auth.py,
Expand Down Expand Up @@ -194,3 +196,9 @@ ignore_missing_imports = True

[mypy-hiredis]
ignore_missing_imports = True

[mypy-josepy.*]
ignore_missing_imports = True

[mypy-txacme.*]
ignore_missing_imports = True
12 changes: 7 additions & 5 deletions synapse/handlers/acme.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# limitations under the License.

import logging
from typing import TYPE_CHECKING

import twisted
import twisted.internet.error
Expand All @@ -22,6 +23,9 @@

from synapse.app import check_bind_error

if TYPE_CHECKING:
from synapse.app.homeserver import HomeServer

logger = logging.getLogger(__name__)

ACME_REGISTER_FAIL_ERROR = """
Expand All @@ -35,12 +39,12 @@


class AcmeHandler:
def __init__(self, hs):
def __init__(self, hs: "HomeServer"):
self.hs = hs
self.reactor = hs.get_reactor()
self._acme_domain = hs.config.acme_domain

async def start_listening(self):
async def start_listening(self) -> None:
from synapse.handlers import acme_issuing_service

# Configure logging for txacme, if you need to debug
Expand Down Expand Up @@ -85,7 +89,7 @@ async def start_listening(self):
logger.error(ACME_REGISTER_FAIL_ERROR)
raise

async def provision_certificate(self):
async def provision_certificate(self) -> None:

logger.warning("Reprovisioning %s", self._acme_domain)

Expand All @@ -110,5 +114,3 @@ async def provision_certificate(self):
except Exception:
logger.exception("Failed saving!")
raise

return True
Copy link
Member Author

@clokep clokep Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is just awaited and the return type is never used.

27 changes: 19 additions & 8 deletions synapse/handlers/acme_issuing_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
imported conditionally.
"""
import logging
from typing import Dict, Iterable, List

import attr
import pem
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives import serialization
from josepy import JWKRSA
Expand All @@ -36,20 +38,27 @@
from zope.interface import implementer

from twisted.internet import defer
from twisted.internet.interfaces import IReactorTCP
from twisted.python.filepath import FilePath
from twisted.python.url import URL
from twisted.web.resource import IResource

logger = logging.getLogger(__name__)


def create_issuing_service(reactor, acme_url, account_key_file, well_known_resource):
def create_issuing_service(
reactor: IReactorTCP,
acme_url: str,
account_key_file: str,
well_known_resource: IResource,
) -> AcmeIssuingService:
"""Create an ACME issuing service, and attach it to a web Resource

Args:
reactor: twisted reactor
acme_url (str): URL to use to request certificates
account_key_file (str): where to store the account key
well_known_resource (twisted.web.IResource): web resource for .well-known.
acme_url: URL to use to request certificates
account_key_file: where to store the account key
well_known_resource: web resource for .well-known.
we will attach a child resource for "acme-challenge".

Returns:
Expand Down Expand Up @@ -83,18 +92,20 @@ class ErsatzStore:
A store that only stores in memory.
"""

certs = attr.ib(default=attr.Factory(dict))
certs = attr.ib(type=Dict[bytes, List[bytes]], default=attr.Factory(dict))

def store(self, server_name, pem_objects):
def store(
self, server_name: bytes, pem_objects: Iterable[pem.AbstractPEMObject]
) -> defer.Deferred:
self.certs[server_name] = [o.as_bytes() for o in pem_objects]
return defer.succeed(None)


def load_or_create_client_key(key_file):
def load_or_create_client_key(key_file: str) -> JWKRSA:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should be writing stubs for libraries. Since we ignore the lib in mypy.ini it'll be treated as Any, which is distinctly unhelpful, and perhaps even worse than not having a type hint at all?

We do this in other places though, so happy to leave this as is and discuss the broader issue separately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would make sense to do as a separate step? 🤷 The ACME stuff in particular is deprecated I think, so maybe not worthwhile to write stubs there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I just know that we sometimes get confused at why mypy isn't picking up obvious bugs because of this sort of thing, but that can be dealt with separately

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #9228.

"""Load the ACME account key from a file, creating it if it does not exist.

Args:
key_file (str): name of the file to use as the account key
key_file: name of the file to use as the account key
"""
# this is based on txacme.endpoint.load_or_create_client_key, but doesn't
# hardcode the 'client.key' filename
Expand Down