Refactor OIDC tests to better mimic an actual OIDC provider #13910

sandhose · 2022-09-26T16:22:25Z

This implements a fake OIDC server, which intercepts calls to the HTTP client.
It makes the tests more accurate, by covering more internal methods.

One particular example was the ID token validation, which was mocked before.
This helped me uncover a LIE we had in our dependencies: we actually need at least authlib 0.15.1, and not just >=0.14.0 (which looks fine for packagers, according to repology)
I initially did this, because I needed to actually test the OIDC Backchannel Logout implementation in #11414

Instead of constantly mocking the internal methods of the OIDC handler, it now mocks HTTP responses Signed-off-by: Quentin Gliech <quenting@element.io>

sandhose

Left a few notes to help with review

sandhose · 2022-09-26T16:23:34Z

pyproject.toml

@@ -192,7 +192,7 @@ psycopg2 = { version = ">=2.8", markers = "platform_python_implementation != 'Py
 psycopg2cffi = { version = ">=2.8", markers = "platform_python_implementation == 'PyPy'", optional = true }
 psycopg2cffi-compat = { version = "==1.1", markers = "platform_python_implementation == 'PyPy'", optional = true }
 pysaml2 = { version = ">=4.5.0", optional = true }
-authlib = { version = ">=0.14.0", optional = true }


So that was a LIE.

synapse/handlers/oidc.py

sandhose · 2022-09-26T16:26:21Z

tests/federation/test_federation_client.py

@@ -230,21 +227,3 @@ def _get_pdu_once(self) -> EventBase:
        self.assertEqual(remote_pdu.internal_metadata.outlier, False)

        return remote_pdu
-
-
-def _mock_response(resp: JsonDict):


Because we already had a FakeResponse class and I enhanced it, I took the opportunity to replace this here

sandhose · 2022-09-26T16:27:30Z

tests/handlers/test_oidc.py

+        client_redirect_url: str = "http://client/redirect",
+        scope: str = "openid",
+        with_sid: bool = False,
+    ) -> Tuple[SynapseRequest, FakeAuthorizationGrant]:


Even though we're not using the returned grant here, it is useful for the backchannel logout tests

sandhose · 2022-09-26T16:28:38Z

tests/handlers/test_oidc.py

-        self.provider._exchange_code = simple_async_mock(return_value=token)  # type: ignore[assignment]
-        self.provider._parse_id_token = simple_async_mock(return_value=userinfo)  # type: ignore[assignment]
-        self.provider._fetch_userinfo = simple_async_mock(return_value=userinfo)  # type: ignore[assignment]


So all of those are not mocked anymore!

tests/test_utils/__init__.py

sandhose · 2022-09-26T16:35:13Z

tests/test_utils/__init__.py

+
+    @property
+    def phrase(self):
+        return RESPONSES.get(self.code, b"Unknown Status")


Instead of having to set an HTTP phrase explicitly, we just get it from the RESPONSES dict, which maps HTTP status codes to status phrases

tests/test_utils/oidc.py

clokep · 2022-09-28T14:19:05Z

tests/test_utils/__init__.py

-    # HTTP response phrase (eg b'OK' for a 200)
-    phrase = attr.ib(type=bytes)


Do we not use this? I'm quite confused by the changes in this file -- they seem unrelated to the rest of the PR.

Oh, I see this got moved below.

Still this refactoring seems like it could be split out for a separate review.

tests/test_utils/oidc.py

…r-oidc-tests

sandhose · 2022-10-24T14:26:34Z

Looks like deb building in CI is broken because of an old version of authlib... even though the logs says it installs 1.1.0! And I can't reproduce the issue locally, poetry run python ./scripts-dev/build_debian_packages.py just works 🤔

EDIT: nevermind, I was on the wrong branch, I can reproduce locally

clokep

I think this largely looks good, most of my comments are fairly cursory.

tests/test_utils/__init__.py

tests/test_utils/oidc.py

tests/handlers/test_oidc.py

tests/rest/client/test_auth.py

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

clokep

A couple minor renames, but looks good! 👍

tests/rest/client/utils.py

@asymmetric

Synapse 1.71.0 (2022-11-08) =========================== Please note that, as announced in the release notes for Synapse 1.69.0, legacy Prometheus metric names are now disabled by default. They will be removed altogether in Synapse 1.73.0. If not already done, server administrators should update their dashboards and alerting rules to avoid using the deprecated metric names. See the [upgrade notes](https://matrix-org.github.io/synapse/v1.71/upgrade.html#upgrading-to-v1710) for more details. **Note:** in line with our [deprecation policy](https://matrix-org.github.io/synapse/latest/deprecation_policy.html) for platform dependencies, this will be the last release to support PostgreSQL 10, which reaches upstream end-of-life on November 10th, 2022. Future releases of Synapse will require PostgreSQL 11+. No significant changes since 1.71.0rc2. Synapse 1.71.0rc2 (2022-11-04) ============================== Improved Documentation ---------------------- - Document the changes to monthly active user metrics due to deprecation of legacy Prometheus metric names. ([\matrix-org#14358](matrix-org#14358), [\matrix-org#14360](matrix-org#14360)) Deprecations and Removals ------------------------- - Disable legacy Prometheus metric names by default. They can still be re-enabled for now, but they will be removed altogether in Synapse 1.73.0. ([\matrix-org#14353](matrix-org#14353)) Internal Changes ---------------- - Run unit tests against Python 3.11. ([\matrix-org#13812](matrix-org#13812)) Synapse 1.71.0rc1 (2022-11-01) ============================== Features -------- - Support back-channel logouts from OpenID Connect providers. ([\matrix-org#11414](matrix-org#11414)) - Allow use of Postgres and SQLlite full-text search operators in search queries. ([\matrix-org#11635](matrix-org#11635), [\matrix-org#14310](matrix-org#14310), [\matrix-org#14311](matrix-org#14311)) - Implement [MSC3664](matrix-org/matrix-spec-proposals#3664), Pushrules for relations. Contributed by Nico. ([\matrix-org#11804](matrix-org#11804)) - Improve aesthetics of HTML templates. Note that these changes do not retroactively apply to templates which have been [customised](https://matrix-org.github.io/synapse/latest/templates.html#templates) by server admins. ([\matrix-org#13652](matrix-org#13652)) - Enable write-ahead logging for SQLite installations. Contributed by [@asymmetric](https://github.com/asymmetric). ([\matrix-org#13897](matrix-org#13897)) - Show erasure status when [listing users](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#query-user-account) in the Admin API. ([\matrix-org#14205](matrix-org#14205)) - Provide a specific error code when a `/sync` request provides a filter which doesn't represent a JSON object. ([\matrix-org#14262](matrix-org#14262)) Bugfixes -------- - Fix a long-standing bug where the `update_synapse_database` script could not be run with multiple databases. Contributed by @thefinn93 @ Beeper. ([\matrix-org#13422](matrix-org#13422)) - Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name` and have `max_avatar_size` and/or `allowed_avatar_mimetypes` configuration. Contributed by @ashfame. ([\matrix-org#13927](matrix-org#13927)) - Check appservice user interest against the local users instead of all users in the room to align with [MSC3905](matrix-org/matrix-spec-proposals#3905). ([\matrix-org#13958](matrix-org#13958)) - Fix a long-standing bug where Synapse would accidentally include extra information in the response to [`PUT /_matrix/federation/v2/invite/{roomId}/{eventId}`](https://spec.matrix.org/v1.4/server-server-api/#put_matrixfederationv2inviteroomideventid). ([\matrix-org#14064](matrix-org#14064)) - Fix a bug introduced in Synapse 1.64.0 where presence updates could be missing from `/sync` responses. ([\matrix-org#14243](matrix-org#14243)) - Fix a bug introduced in Synapse 1.60.0 which caused an error to be logged when Synapse received a SIGHUP signal if debug logging was enabled. ([\matrix-org#14258](matrix-org#14258)) - Prevent history insertion ([MSC2716](matrix-org/matrix-spec-proposals#2716)) during an partial join ([MSC3706](matrix-org/matrix-spec-proposals#3706)). ([\matrix-org#14291](matrix-org#14291)) - Fix a bug introduced in Synapse 1.34.0 where device names would be returned via a federation user key query request when `allow_device_name_lookup_over_federation` was set to `false`. ([\matrix-org#14304](matrix-org#14304)) - Fix a bug introduced in Synapse 0.34.0 where logs could include error spam when background processes are measured as taking a negative amount of time. ([\matrix-org#14323](matrix-org#14323)) - Fix a bug introduced in Synapse 1.70.0 where clients were unable to PUT new [dehydrated devices](matrix-org/matrix-spec-proposals#2697). ([\matrix-org#14336](matrix-org#14336)) Improved Documentation ---------------------- - Explain how to disable the use of [`trusted_key_servers`](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#trusted_key_servers). ([\matrix-org#13999](matrix-org#13999)) - Add workers settings to [configuration manual](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#individual-worker-configuration). ([\matrix-org#14086](matrix-org#14086)) - Correct the name of the config option [`encryption_enabled_by_default_for_room_type`](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#encryption_enabled_by_default_for_room_type). ([\matrix-org#14110](matrix-org#14110)) - Update docstrings of `SynapseError` and `FederationError` to bettter describe what they are used for and the effects of using them are. ([\matrix-org#14191](matrix-org#14191)) Internal Changes ---------------- - Remove unused `@lru_cache` decorator. ([\matrix-org#13595](matrix-org#13595)) - Save login tokens in database and prevent login token reuse. ([\matrix-org#13844](matrix-org#13844)) - Refactor OIDC tests to better mimic an actual OIDC provider. ([\matrix-org#13910](matrix-org#13910)) - Fix type annotation causing import time error in the Complement forking launcher. ([\matrix-org#14084](matrix-org#14084)) - Refactor [MSC3030](matrix-org/matrix-spec-proposals#3030) `/timestamp_to_event` endpoint to loop over federation destinations with standard pattern and error handling. ([\matrix-org#14096](matrix-org#14096)) - Add initial power level event to batch of bulk persisted events when creating a new room. ([\matrix-org#14228](matrix-org#14228)) - Refactor `/key/` endpoints to use `RestServlet` classes. ([\matrix-org#14229](matrix-org#14229)) - Switch to using the `matrix-org/backend-meta` version of `triage-incoming` for new issues in CI. ([\matrix-org#14230](matrix-org#14230)) - Build wheels on macos 11, not 10.15. ([\matrix-org#14249](matrix-org#14249)) - Add debugging to help diagnose lost device list updates. ([\matrix-org#14268](matrix-org#14268)) - Add Rust cache to CI for `trial` runs. ([\matrix-org#14287](matrix-org#14287)) - Improve type hinting of `RawHeaders`. ([\matrix-org#14303](matrix-org#14303)) - Use Poetry 1.2.0 in the Twisted Trunk CI job. ([\matrix-org#14305](matrix-org#14305)) <details> <summary>Dependency updates</summary> Runtime: - Bump anyhow from 1.0.65 to 1.0.66. ([\matrix-org#14278](matrix-org#14278)) - Bump jinja2 from 3.0.3 to 3.1.2. ([\matrix-org#14271](matrix-org#14271)) - Bump prometheus-client from 0.14.0 to 0.15.0. ([\matrix-org#14274](matrix-org#14274)) - Bump psycopg2 from 2.9.4 to 2.9.5. ([\matrix-org#14331](matrix-org#14331)) - Bump pysaml2 from 7.1.2 to 7.2.1. ([\matrix-org#14270](matrix-org#14270)) - Bump sentry-sdk from 1.5.11 to 1.10.1. ([\matrix-org#14330](matrix-org#14330)) - Bump serde from 1.0.145 to 1.0.147. ([\matrix-org#14277](matrix-org#14277)) - Bump serde_json from 1.0.86 to 1.0.87. ([\matrix-org#14279](matrix-org#14279)) Tooling and CI: - Bump black from 22.3.0 to 22.10.0. ([\matrix-org#14328](matrix-org#14328)) - Bump flake8-bugbear from 21.3.2 to 22.9.23. ([\matrix-org#14042](matrix-org#14042)) - Bump peaceiris/actions-gh-pages from 3.8.0 to 3.9.0. ([\matrix-org#14276](matrix-org#14276)) - Bump peaceiris/actions-mdbook from 1.1.14 to 1.2.0. ([\matrix-org#14275](matrix-org#14275)) - Bump setuptools-rust from 1.5.1 to 1.5.2. ([\matrix-org#14273](matrix-org#14273)) - Bump twine from 3.8.0 to 4.0.1. ([\matrix-org#14332](matrix-org#14332)) - Bump types-opentracing from 2.4.7 to 2.4.10. ([\matrix-org#14133](matrix-org#14133)) - Bump types-requests from 2.28.11 to 2.28.11.2. ([\matrix-org#14272](matrix-org#14272)) </details>

sandhose added 2 commits September 26, 2022 18:12

Refactor OIDC tests to better mimic an actual OIDC provider

db30e73

Instead of constantly mocking the internal methods of the OIDC handler, it now mocks HTTP responses Signed-off-by: Quentin Gliech <quenting@element.io>

Newsfile.

99a3929

sandhose commented Sep 26, 2022

View reviewed changes

sandhose marked this pull request as ready for review September 26, 2022 16:39

sandhose requested a review from a team as a code owner September 26, 2022 16:39

clokep self-assigned this Sep 28, 2022

clokep reviewed Sep 28, 2022

View reviewed changes

tests/test_utils/oidc.py Outdated Show resolved Hide resolved

clokep reviewed Sep 28, 2022

View reviewed changes

tests/test_utils/oidc.py Outdated Show resolved Hide resolved

sandhose added 2 commits October 24, 2022 15:10

Merge remote-tracking branch 'upstream/develop' into quenting/refacto…

10a423c

…r-oidc-tests

Rename FakeOidcProvider -> FakeOidcServer + docstrings

a973c91

sandhose requested a review from clokep October 24, 2022 13:34

Comment about the token endpoint not checking auth in the FakeOidcServer

731c683

sandhose added 2 commits October 24, 2022 16:44

Fix tests with newer authlib versions

cf16d0a

Suggestion from code review

7c981b4

clokep reviewed Oct 25, 2022

View reviewed changes

Apply suggestions from code review

3233581

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

sandhose requested a review from clokep October 25, 2022 13:18

clokep approved these changes Oct 25, 2022

View reviewed changes

tests/rest/client/utils.py Outdated Show resolved Hide resolved

Rename provider -> fake_server

f225a91

clokep enabled auto-merge (squash) October 25, 2022 13:52

clokep merged commit 9192d74 into matrix-org:develop Oct 25, 2022

sandhose deleted the quenting/refactor-oidc-tests branch October 25, 2022 14:34

sandhose mentioned this pull request Oct 26, 2022

Support OIDC backchannel logouts #11414

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor OIDC tests to better mimic an actual OIDC provider #13910

Refactor OIDC tests to better mimic an actual OIDC provider #13910

sandhose commented Sep 26, 2022 •

edited

Loading

sandhose left a comment

sandhose Sep 26, 2022

sandhose Sep 26, 2022

sandhose Sep 26, 2022

sandhose Sep 26, 2022

sandhose Sep 26, 2022

clokep Sep 28, 2022

clokep Sep 28, 2022

sandhose commented Oct 24, 2022 •

edited

Loading

clokep left a comment •

edited

Loading

clokep left a comment

		# HTTP response phrase (eg b'OK' for a 200)
		phrase = attr.ib(type=bytes)

Refactor OIDC tests to better mimic an actual OIDC provider #13910

Refactor OIDC tests to better mimic an actual OIDC provider #13910

Conversation

sandhose commented Sep 26, 2022 • edited Loading

sandhose left a comment

Choose a reason for hiding this comment

sandhose Sep 26, 2022

Choose a reason for hiding this comment

sandhose Sep 26, 2022

Choose a reason for hiding this comment

sandhose Sep 26, 2022

Choose a reason for hiding this comment

sandhose Sep 26, 2022

Choose a reason for hiding this comment

sandhose Sep 26, 2022

Choose a reason for hiding this comment

clokep Sep 28, 2022

Choose a reason for hiding this comment

clokep Sep 28, 2022

Choose a reason for hiding this comment

sandhose commented Oct 24, 2022 • edited Loading

clokep left a comment • edited Loading

Choose a reason for hiding this comment

clokep left a comment

Choose a reason for hiding this comment

sandhose commented Sep 26, 2022 •

edited

Loading

sandhose commented Oct 24, 2022 •

edited

Loading

clokep left a comment •

edited

Loading