Add additional type hints to SSO and registration code #8784

clokep · 2020-11-19T19:36:38Z

Sets the HomeServer type on CasHandler.__init__ and then fixes some errors that come about from that.
Adds type hints to the register handler.
Fixes a bug in the CAS, OIDC, and SAML handler when registering a new user (the user-agent / IP address were given to register_user in the wrong format). I don't expect this to have an observable impact since you need to be using SSO + a spam checker which checks for valid registrations by user-agent / IP address. I suspect these two things together is extremely rare, if not non-existent.

clokep · 2020-11-19T20:11:22Z

synapse/handlers/cas_handler.py

@@ -218,7 +226,7 @@ async def handle_ticket(
                registered_user_id = await self._registration_handler.register_user(
                    localpart=localpart,
                    default_display_name=user_display_name,
-                    user_agent_ips=(user_agent, ip_address),
+                    user_agent_ips=[(user_agent, ip_address)],


This fix is made to the CAS, OIDC, and SAML handlers and is a bug from #8034.

clokep · 2020-11-19T20:12:07Z

synapse/handlers/register.py

+            # If a default display name is not given, generate one.
+            generate_display_name = default_display_name is None


This fixes a bug where if this went multiple loops only the initial loop would update the display name, which seems incorrect.

clokep · 2020-11-19T20:12:31Z

synapse/handlers/register.py

+            # If a default display name is not given, generate one.
+            generate_display_name = default_display_name is None
+            # This breaks on successful registration *or* errors after 10 failures.
+            while True:


Change this to a loop which breaks simplifies the types of user (it is now always a UserID).

clokep · 2020-11-19T20:13:29Z

synapse/handlers/register.py

+                    # If an invite is required, there must be a auto-join user ID.
+                    assert self.hs.config.registration.auto_join_user_id


This is asserted in the config code but mypy doesn't know that.

clokep · 2020-11-19T20:14:00Z

synapse/handlers/register.py

@@ -553,42 +567,42 @@ def check_registration_ratelimit(self, address):

        self.ratelimiter.ratelimit(address)

-    def register_with_store(
+    async def register_with_store(


Instead of returning an awaitable I made this async, which we decided was a clearer pattern to use.

clokep · 2020-11-19T20:14:49Z

synapse/handlers/saml_handler.py

@@ -39,7 +39,7 @@
 from synapse.util.iterutils import chunk_seq

 if TYPE_CHECKING:
-    import synapse.server
+    from synapse.server import HomeServer


This is just for consistency.

erikjohnston · 2020-11-20T10:17:59Z

changelog.d/8784.misc

@@ -0,0 +1 @@
+Add additional type hints in the registration and SSO handlers.


Should this be a bugfix now?

Good call! I'll re-word this to talk about the bug being fixed.

I updated this although I don't love my wording...

clokep · 2020-11-20T18:09:06Z

@erikjohnston Unfortunately this needed further changes due to the failing sytest. I've updated the CAS code to be more similar to SAML/OIDC, which actually create a user during UI auth flow if it doesn't exist anymore. This makes little sense, but having them be consistent seems reasonable to me.

Stale due to additional changes

@angdraug

Synapse 1.24.0 (2020-12-09) =========================== Due to the two security issues highlighted below, server administrators are encouraged to update Synapse. We are not aware of these vulnerabilities being exploited in the wild. Security advisory ----------------- The following issues are fixed in v1.23.1 and v1.24.0. - There is a denial of service attack ([CVE-2020-26257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26257)) against the federation APIs in which future events will not be correctly sent to other servers over federation. This affects all servers that participate in open federation. (Fixed in [#8776](matrix-org/synapse#8776)). - Synapse may be affected by OpenSSL [CVE-2020-1971](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1971). Synapse administrators should ensure that they have the latest versions of the cryptography Python package installed. To upgrade Synapse along with the cryptography package: * Administrators using the [`matrix.org` Docker image](https://hub.docker.com/r/matrixdotorg/synapse/) or the [Debian/Ubuntu packages from `matrix.org`](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#matrixorg-packages) should ensure that they have version 1.24.0 or 1.23.1 installed: these images include the updated packages. * Administrators who have [installed Synapse from source](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#installing-from-source) should upgrade the cryptography package within their virtualenv by running: ```sh <path_to_virtualenv>/bin/pip install 'cryptography>=3.3' ``` * Administrators who have installed Synapse from distribution packages should consult the information from their distributions. Internal Changes ---------------- - Add a maximum version for pysaml2 on Python 3.5. ([\#8898](matrix-org/synapse#8898)) Synapse 1.24.0rc2 (2020-12-04) ============================== Bugfixes -------- - Fix a regression in v1.24.0rc1 which failed to allow SAML mapping providers which were unable to redirect users to an additional page. ([\#8878](matrix-org/synapse#8878)) Internal Changes ---------------- - Add support for the `prometheus_client` newer than 0.9.0. Contributed by Jordan Bancino. ([\#8875](matrix-org/synapse#8875)) Synapse 1.24.0rc1 (2020-12-02) ============================== Features -------- - Add admin API for logging in as a user. ([\#8617](matrix-org/synapse#8617)) - Allow specification of the SAML IdP if the metadata returns multiple IdPs. ([\#8630](matrix-org/synapse#8630)) - Add support for re-trying generation of a localpart for OpenID Connect mapping providers. ([\#8801](matrix-org/synapse#8801), [\#8855](matrix-org/synapse#8855)) - Allow the `Date` header through CORS. Contributed by Nicolas Chamo. ([\#8804](matrix-org/synapse#8804)) - Add a config option, `push.group_by_unread_count`, which controls whether unread message counts in push notifications are defined as "the number of rooms with unread messages" or "total unread messages". ([\#8820](matrix-org/synapse#8820)) - Add `force_purge` option to delete-room admin api. ([\#8843](matrix-org/synapse#8843)) Bugfixes -------- - Fix a bug where appservices may be sent an excessive amount of read receipts and presence. Broke in v1.22.0. ([\#8744](matrix-org/synapse#8744)) - Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body. ([\#8776](matrix-org/synapse#8776)) - Fix a bug where synctl could spawn duplicate copies of a worker. Contributed by Waylon Cude. ([\#8798](matrix-org/synapse#8798)) - Allow per-room profiles to be used for the server notice user. ([\#8799](matrix-org/synapse#8799)) - Fix a bug where logging could break after a call to SIGHUP. ([\#8817](matrix-org/synapse#8817)) - Fix `register_new_matrix_user` failing with "Bad Request" when trailing slash is included in server URL. Contributed by @angdraug. ([\#8823](matrix-org/synapse#8823)) - Fix a minor long-standing bug in login, where we would offer the `password` login type if a custom auth provider supported it, even if password login was disabled. ([\#8835](matrix-org/synapse#8835)) - Fix a long-standing bug which caused Synapse to require unspecified parameters during user-interactive authentication. ([\#8848](matrix-org/synapse#8848)) - Fix a bug introduced in v1.20.0 where the user-agent and IP address reported during user registration for CAS, OpenID Connect, and SAML were of the wrong form. ([\#8784](matrix-org/synapse#8784)) Improved Documentation ---------------------- - Clarify the usecase for a msisdn delegate. Contributed by Adrian Wannenmacher. ([\#8734](matrix-org/synapse#8734)) - Remove extraneous comma from JSON example in User Admin API docs. ([\#8771](matrix-org/synapse#8771)) - Update `turn-howto.md` with troubleshooting notes. ([\#8779](matrix-org/synapse#8779)) - Fix the example on how to set the `Content-Type` header in nginx for the Client Well-Known URI. ([\#8793](matrix-org/synapse#8793)) - Improve the documentation for the admin API to list all media in a room with respect to encrypted events. ([\#8795](matrix-org/synapse#8795)) - Update the formatting of the `push` section of the homeserver config file to better align with the [code style guidelines](https://github.com/matrix-org/synapse/blob/develop/docs/code_style.md#configuration-file-format). ([\#8818](matrix-org/synapse#8818)) - Improve documentation how to configure prometheus for workers. ([\#8822](matrix-org/synapse#8822)) - Update example prometheus console. ([\#8824](matrix-org/synapse#8824)) Deprecations and Removals ------------------------- - Remove old `/_matrix/client/*/admin` endpoints which were deprecated since Synapse 1.20.0. ([\#8785](matrix-org/synapse#8785)) - Disable pretty printing JSON responses for curl. Users who want pretty-printed output should use [jq](https://stedolan.github.io/jq/) in combination with curl. Contributed by @tulir. ([\#8833](matrix-org/synapse#8833)) Internal Changes ---------------- - Simplify the way the `HomeServer` object caches its internal attributes. ([\#8565](matrix-org/synapse#8565), [\#8851](matrix-org/synapse#8851)) - Add an example and documentation for clock skew to the SAML2 sample configuration to allow for clock/time difference between the homserver and IdP. Contributed by @localguru. ([\#8731](matrix-org/synapse#8731)) - Generalise `RoomMemberHandler._locally_reject_invite` to apply to more flows than just invite. ([\#8751](matrix-org/synapse#8751)) - Generalise `RoomStore.maybe_store_room_on_invite` to handle other, non-invite membership events. ([\#8754](matrix-org/synapse#8754)) - Refactor test utilities for injecting HTTP requests. ([\#8757](matrix-org/synapse#8757), [\#8758](matrix-org/synapse#8758), [\#8759](matrix-org/synapse#8759), [\#8760](matrix-org/synapse#8760), [\#8761](matrix-org/synapse#8761), [\#8777](matrix-org/synapse#8777)) - Consolidate logic between the OpenID Connect and SAML code. ([\#8765](matrix-org/synapse#8765)) - Use `TYPE_CHECKING` instead of magic `MYPY` variable. ([\#8770](matrix-org/synapse#8770)) - Add a commandline script to sign arbitrary json objects. ([\#8772](matrix-org/synapse#8772)) - Minor log line improvements for the SSO mapping code used to generate Matrix IDs from SSO IDs. ([\#8773](matrix-org/synapse#8773)) - Add additional error checking for OpenID Connect and SAML mapping providers. ([\#8774](matrix-org/synapse#8774), [\#8800](matrix-org/synapse#8800)) - Add type hints to HTTP abstractions. ([\#8806](matrix-org/synapse#8806), [\#8812](matrix-org/synapse#8812)) - Remove unnecessary function arguments and add typing to several membership replication classes. ([\#8809](matrix-org/synapse#8809)) - Optimise the lookup for an invite from another homeserver when trying to reject it. ([\#8815](matrix-org/synapse#8815)) - Add tests for `password_auth_provider`s. ([\#8819](matrix-org/synapse#8819)) - Drop redundant database index on `event_json`. ([\#8845](matrix-org/synapse#8845)) - Simplify `uk.half-shot.msc2778.login.application_service` login handler. ([\#8847](matrix-org/synapse#8847)) - Refactor `password_auth_provider` support code. ([\#8849](matrix-org/synapse#8849)) - Add missing `ordering` to background database updates. ([\#8850](matrix-org/synapse#8850)) - Allow for specifying a room version when creating a room in unit tests via `RestHelper.create_room_as`. ([\#8854](matrix-org/synapse#8854))

Fix some types in the CAS code.

37fb198

clokep force-pushed the clokep/cas-types branch from 22dfaae to 37fb198 Compare November 19, 2020 19:38

clokep added 2 commits November 19, 2020 15:05

Add additional types to registration handler.

28ad9bf

Fix a bug with generating display names.

2e83da8

clokep changed the title ~~Fix some types in the CAS code.~~ Add additional type hints to SSO and registration code Nov 19, 2020

clokep added 2 commits November 19, 2020 15:09

Remove unnecessary assertion.

7ade914

Update newsfragment.

e62f4dc

clokep commented Nov 19, 2020

View reviewed changes

Update comment.

2819f83

clokep commented Nov 19, 2020

View reviewed changes

clokep requested a review from a team November 19, 2020 20:15

erikjohnston previously approved these changes Nov 20, 2020

View reviewed changes

Re-structure CAS code to be more similar to SAML/OIDC.

3b9ce2a

Update the changelog.

781e280

clokep requested a review from erikjohnston November 23, 2020 13:46

erikjohnston approved these changes Nov 23, 2020

View reviewed changes

clokep merged commit 6fde6aa into develop Nov 23, 2020

clokep deleted the clokep/cas-types branch November 23, 2020 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional type hints to SSO and registration code #8784

Add additional type hints to SSO and registration code #8784

clokep commented Nov 19, 2020 •

edited

Loading

clokep Nov 19, 2020

clokep Nov 19, 2020

clokep Nov 19, 2020

clokep Nov 19, 2020

clokep Nov 19, 2020

clokep Nov 19, 2020

erikjohnston Nov 20, 2020

clokep Nov 20, 2020

clokep Nov 20, 2020

clokep commented Nov 20, 2020

		# If a default display name is not given, generate one.
		generate_display_name = default_display_name is None

		# If an invite is required, there must be a auto-join user ID.
		assert self.hs.config.registration.auto_join_user_id

		@@ -0,0 +1 @@
		Add additional type hints in the registration and SSO handlers.

Add additional type hints to SSO and registration code #8784

Add additional type hints to SSO and registration code #8784

Conversation

clokep commented Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Nov 20, 2020

clokep commented Nov 19, 2020 •

edited

Loading