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

Replace pyjwt with authlib in org.matrix.login.jwt #13011

Merged

Conversation

aytchell
Copy link
Contributor

@aytchell aytchell commented Jun 9, 2022

As discussed in #12830 here is the PR for replacing PyJWT with Authlib

Contained changes

Beneath the actual code changes I have

  • adapted the documentation in jwt.md
  • changed comments pointing to the list of supported algorithms
  • added a small dev-script for creating JWTs locally (which seems to be included in pyjwt but not in authlib)
  • Changed the expected error messages in some unit tests
  • removed pyjwt from pyproject.toml and ran poetry lock --no-update

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@aytchell aytchell requested a review from a team as a code owner June 9, 2022 15:01
aytchell added 5 commits June 9, 2022 17:49
Due to the move from PyJWT to authlib the error messages have changed.
Instead of mapping messages in the code just to please the tests I
changed the expücted error messages in the unit tests.
Seems like PyJWT provides a script to create JWTs from the command line.
Authlib does not provide such a script so I added this tiny script as a
replacement.

It doesn't parse parameters nor writes files but since this feature is
only required by developer while doing some local tests I'd say that's
OK.
Signed-off-by: Hannes Lerchl <[email protected]>
@aytchell aytchell force-pushed the aytchell/replace_pyjwt_with_authlib branch from 8f1183f to 5a7e7b0 Compare June 9, 2022 15:50
@aytchell
Copy link
Contributor Author

aytchell commented Jun 9, 2022

I've seen that tests are failing because pyproject.toml and poetry.lock are out of sync.

poetry is kind of brand new to me and I don't want to fiddle around with dependency versions of a big project. So I reverted my change to pyproject.toml and leave it to someone more experience to remove the pyjwt entry in there.

@aytchell
Copy link
Contributor Author

Ugh, sorry for all the red test runs. I'm currently figuring out the behaviour of the build bot.
The test Tests / trial (3.10) (pull_request) fails with builtins.ModuleNotFoundError: No module named 'authlib'

Does it not do poetry install --extras all? (and if not - are there no tests for oidc?)
Or do I have to enable this somehow in login_tests?

@DMRobertson
Copy link
Contributor

Does it not do poetry install --extras all

It does for most jobs, but not all: because some people will be running Synapse without optional dependencies like authlib. It's still marked as optional in pyproject.toml:

authlib = { version = ">=0.14.0", optional = true }

synapse/rest/client/login.py Show resolved Hide resolved
@DMRobertson
Copy link
Contributor

I've seen that tests are failing because pyproject.toml and poetry.lock are out of sync.

Got a link? I think there's a bug in the beta version of poetry that we use where it can spuriously give out these warning messages in CI; I think this might be a false positive.

@aytchell
Copy link
Contributor Author

I've seen that tests are failing because pyproject.toml and poetry.lock are out of sync.

Got a link? I think there's a bug in the beta version of poetry that we use where it can spuriously give out
these warning messages in CI; I think this might be a false positive.

I was talking about the first version of my PR.

I removed pyjwt from pyproject.toml and did poetry lock which then caused poetry to "go wild" and update versions from a lot of other libs. So I did commit the change in pyproject.toml but not the changed poetry.lock which then was disliked by the build bot.

So long story short: I removed all code which makes use of pyjwt but did not remove the dependency entries in pyproject.toml nor poetry.lock

@DMRobertson
Copy link
Contributor

I removed pyjwt from pyproject.toml and did poetry lock which then caused poetry to "go wild" and update versions from a lot of other libs.

You were almost there: the invocation you want is poetry lock --no-update. This might be of interest: https://github.com/matrix-org/synapse/blob/master/docs/development/dependencies.md#remove-a-dependency

(I think this is missing from the docs site, which I will fix imminently.)

aytchell added 2 commits June 10, 2022 16:23
In case a server hoster doesn't use oidc or jwt there should be no need
to install authlib. So for such cases the imports need to 'come later'.
Updated poetry.lock file with --no-update

Signed-off-by: Hannes Lerchl <[email protected]>
@aytchell aytchell force-pushed the aytchell/replace_pyjwt_with_authlib branch from 8da6d15 to b4e56a1 Compare June 11, 2022 11:15
@DMRobertson DMRobertson requested a review from a team June 12, 2022 18:06
@DMRobertson DMRobertson dismissed their stale review June 12, 2022 18:07

changes made

@aytchell
Copy link
Contributor Author

There are red tests in the complement suite called TestRoomReceiptsReadMarkers/....

I tried to reproduce these failures locally by following the instructions at https://matrix-org.github.io/synapse/latest/development/contributing_guide.html?highlight=complement#run-the-integration-tests-complement
and I even ran only the failing test multiple times in a row - without "success" (tests are green on my machine).

Since I don't know, how my changes could affect read markers I checked other PRs and found, that #13025 (type annotations) has also failing "read marker" tests. Does anyone has some hints how I could locally reproduce this test failure? Or is this a "known flaky" test?

@squahtx
Copy link
Contributor

squahtx commented Jun 13, 2022

There are red tests in the complement suite called TestRoomReceiptsReadMarkers/....

That particular test is known to be flaky and will be fixed by matrix-org/complement#393

@DMRobertson
Copy link
Contributor

I'm pretty sure the remaining test failures is another known flake.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Many thanks for this. I don't follow all the details, so what follows is a mixture of questions and requested changes.

docs/sample_config.yaml Outdated Show resolved Hide resolved
docs/usage/configuration/config_documentation.md Outdated Show resolved Hide resolved
synapse/rest/client/login.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
synapse/rest/client/login.py Show resolved Hide resolved
synapse/rest/client/login.py Show resolved Hide resolved
scripts-dev/build_custom_jwt.py Outdated Show resolved Hide resolved
poetry.lock Show resolved Hide resolved
docs/jwt.md Outdated Show resolved Hide resolved
@DMRobertson DMRobertson self-assigned this Jun 14, 2022
@DMRobertson DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 14, 2022
@DMRobertson DMRobertson self-requested a review June 15, 2022 09:42
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks, nearly there!

synapse/rest/client/login.py Show resolved Hide resolved
synapse/rest/client/login.py Show resolved Hide resolved
synapse/rest/client/login.py Show resolved Hide resolved
synapse/rest/client/login.py Show resolved Hide resolved
synapse/rest/client/login.py Show resolved Hide resolved
Comment on lines 45 to 50
try:
import jwt

jwt # To stop unused lint.
except ImportError:
raise ConfigError(MISSING_JWT)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is still outstanding.

@aytchell
Copy link
Contributor Author

I think this is still outstanding.

Correct. I've already added this to my local branch; it now reads from authlib.jose import JsonWebToken and the error message states to install it with pip install synapse[jwt] (as mentioned in the documentation).

I've prepared a version which is merged with the latest develop as well as a version which is rebased atop of the latest develop.

Which is the preferred one to procede with?

@DMRobertson
Copy link
Contributor

I've prepared a version which is merged with the latest develop as well as a version which is rebased atop of the latest develop.

Which is the preferred one to procede with?

I mean to cover this here, but to be explicit: the one merged with develop please!

@aytchell
Copy link
Contributor Author

Oh. Thanks. I missed this comment.
Pushed it

@DMRobertson DMRobertson self-requested a review June 15, 2022 12:52
aytchell added 3 commits June 15, 2022 15:00
The pyjwt based version of the jwt login flow contained a check (like
this) for PyJWT. Since authlib is an optional dependency I add this
check when `jwt_config` is configured.
@aytchell aytchell force-pushed the aytchell/replace_pyjwt_with_authlib branch from 71a2589 to 395997e Compare June 15, 2022 13:02
@aytchell
Copy link
Contributor Author

Hmm. Having pedantic checks is a good thing for a project ;-)
Applied another fix

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM! I'm going to ask for a quick second opinion from the Synapse team though, just because I'm not massively familiar with OAuth2.

@DMRobertson DMRobertson requested a review from a team June 15, 2022 13:47
@aytchell
Copy link
Contributor Author

Thanks a lot for guiding me through this process

)
except jwt.PyJWTError as e:
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

@sandhose pointed out that this is a pretty broad exception clause. Can you narrow this to a JOSE-specific exception?

Perhaps JoseError from here? https://github.com/lepture/authlib/blob/master/authlib/jose/errors.py

Copy link
Contributor

Choose a reason for hiding this comment

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

(That will mean that any other exception gets properly flagged as an application error rather than causing a 403 to the requester.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and pushed it

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Sorry, one final change and this is ready to go

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@DMRobertson DMRobertson enabled auto-merge (squash) June 15, 2022 15:16
@DMRobertson
Copy link
Contributor

I think this conflicts with Erik's recent change to poetry.lock in #13063. I'll fixup and merge

@aytchell
Copy link
Contributor Author

Merci beaucoup

@DMRobertson DMRobertson merged commit 7d99414 into matrix-org:develop Jun 15, 2022
Fizzadar added a commit to beeper/synapse-legacy-fork that referenced this pull request Aug 23, 2022
Synapse 1.62.0 (2022-07-05)
===========================

No significant changes since 1.62.0rc3.

Authors of spam-checker plugins should consult the [upgrade notes](https://github.com/matrix-org/synapse/blob/release-v1.62/docs/upgrade.md#upgrading-to-v1620) to learn about the enriched signatures for spam checker callbacks, which are supported with this release of Synapse.

Synapse 1.62.0rc3 (2022-07-04)
==============================

Bugfixes
--------

- Update the version of the [ldap3 plugin](https://github.com/matrix-org/matrix-synapse-ldap3/) included in the `matrixdotorg/synapse` DockerHub images and the Debian packages hosted on `packages.matrix.org` to 0.2.1. This fixes [a bug](matrix-org/matrix-synapse-ldap3#163) with usernames containing uppercase characters. ([\matrix-org#13156](matrix-org#13156))
- Fix a bug introduced in Synapse 1.62.0rc1 affecting unread counts for users on small servers. ([\matrix-org#13168](matrix-org#13168))

Synapse 1.62.0rc2 (2022-07-01)
==============================

Bugfixes
--------

- Fix unread counts for users on large servers. Introduced in v1.62.0rc1. ([\matrix-org#13140](matrix-org#13140))
- Fix DB performance when deleting old push notifications. Introduced in v1.62.0rc1. ([\matrix-org#13141](matrix-org#13141))

Synapse 1.62.0rc1 (2022-06-28)
==============================

Features
--------

- Port the spam-checker API callbacks to a new, richer API. This is part of an ongoing change to let spam-checker modules inform users of the reason their event or operation is rejected. ([\matrix-org#12857](matrix-org#12857), [\matrix-org#13047](matrix-org#13047))
- Allow server admins to customise the response of the `/.well-known/matrix/client` endpoint. ([\matrix-org#13035](matrix-org#13035))
- Add metrics measuring the CPU and DB time spent in state resolution. ([\matrix-org#13036](matrix-org#13036))
- Speed up fetching of device list changes in `/sync` and `/keys/changes`. ([\matrix-org#13045](matrix-org#13045), [\matrix-org#13098](matrix-org#13098))
- Improve URL previews for sites which only provide Twitter Card metadata, e.g. LWN.net. ([\matrix-org#13056](matrix-org#13056))

Bugfixes
--------

- Update [MSC3786](matrix-org/matrix-spec-proposals#3786) implementation to check `state_key`. ([\matrix-org#12939](matrix-org#12939))
- Fix a bug introduced in Synapse 1.58 where Synapse would not report full version information when installed from a git checkout. This is a best-effort affair and not guaranteed to be stable. ([\matrix-org#12973](matrix-org#12973))
- Fix a bug introduced in Synapse 1.60 where Synapse would fail to start if the `sqlite3` module was not available. ([\matrix-org#12979](matrix-org#12979))
- Fix a bug where non-standard information was required when requesting the `/hierarchy` API over federation. Introduced
  in Synapse v1.41.0. ([\matrix-org#12991](matrix-org#12991))
- Fix a long-standing bug which meant that rate limiting was not restrictive enough in some cases. ([\matrix-org#13018](matrix-org#13018))
- Fix a bug introduced in Synapse 1.58 where profile requests for a malformed user ID would ccause an internal error. Synapse now returns 400 Bad Request in this situation. ([\matrix-org#13041](matrix-org#13041))
- Fix some inconsistencies in the event authentication code. ([\matrix-org#13087](matrix-org#13087), [\matrix-org#13088](matrix-org#13088))
- Fix a long-standing bug where room directory requests would cause an internal server error if given a malformed room alias. ([\matrix-org#13106](matrix-org#13106))

Improved Documentation
----------------------

- Add documentation for how to configure Synapse with Workers using Docker Compose. Includes example worker config and docker-compose.yaml. Contributed by @Thumbscrew. ([\matrix-org#12737](matrix-org#12737))
- Ensure the [Poetry cheat sheet](https://matrix-org.github.io/synapse/develop/development/dependencies.html) is available in the online documentation. ([\matrix-org#13022](matrix-org#13022))
- Mention removed community/group worker endpoints in upgrade.md. Contributed by @olmari. ([\matrix-org#13023](matrix-org#13023))
- Add instructions for running Complement with `gotestfmt`-formatted output locally. ([\matrix-org#13073](matrix-org#13073))
- Update OpenTracing docs to reference the configuration manual rather than the configuration file. ([\matrix-org#13076](matrix-org#13076))
- Update information on downstream Debian packages. ([\matrix-org#13095](matrix-org#13095))
- Remove documentation for the Delete Group Admin API which no longer exists. ([\matrix-org#13112](matrix-org#13112))

Deprecations and Removals
-------------------------

- Remove the unspecced `DELETE /directory/list/room/{roomId}` endpoint, which hid rooms from the [public room directory](https://spec.matrix.org/v1.3/client-server-api/#listing-rooms). Instead, `PUT` to the same URL with a visibility of `"private"`. ([\matrix-org#13123](matrix-org#13123))

Internal Changes
----------------

- Add tests for cancellation of `GET /rooms/$room_id/members` and `GET /rooms/$room_id/state` requests. ([\matrix-org#12674](matrix-org#12674))
- Report login failures due to unknown third party identifiers in the same way as failures due to invalid passwords. This prevents an attacker from using the error response to determine if the identifier exists. Contributed by Daniel Aloni. ([\matrix-org#12738](matrix-org#12738))
- Merge the Complement testing Docker images into a single, multi-purpose image. ([\matrix-org#12881](matrix-org#12881), [\matrix-org#13075](matrix-org#13075))
- Simplify the database schema for `event_edges`. ([\matrix-org#12893](matrix-org#12893))
- Clean up the test code for client disconnection. ([\matrix-org#12929](matrix-org#12929))
- Remove code generating comments in configuration. ([\matrix-org#12941](matrix-org#12941))
- Add `Cross-Origin-Resource-Policy: cross-origin` header to content repository's thumbnail and download endpoints. ([\matrix-org#12944](matrix-org#12944))
- Replace noop background updates with `DELETE` delta. ([\matrix-org#12954](matrix-org#12954), [\matrix-org#13050](matrix-org#13050))
- Use lower isolation level when inserting read receipts to avoid serialization errors. Contributed by Nick @ Beeper. ([\matrix-org#12957](matrix-org#12957))
- Reduce the amount of state we pull from the DB. ([\matrix-org#12963](matrix-org#12963))
- Enable testing against PostgreSQL databases in Complement CI. ([\matrix-org#12965](matrix-org#12965), [\matrix-org#13034](matrix-org#13034))
- Fix an inaccurate comment. ([\matrix-org#12969](matrix-org#12969))
- Remove the `delete_device` method and always call `delete_devices`. ([\matrix-org#12970](matrix-org#12970))
- Use a GitHub form for issues rather than a hard-to-read, easy-to-ignore template. ([\matrix-org#12982](matrix-org#12982))
- Move [MSC3715](matrix-org/matrix-spec-proposals#3715) behind an experimental config flag. ([\matrix-org#12984](matrix-org#12984))
- Add type hints to tests. ([\matrix-org#12985](matrix-org#12985), [\matrix-org#13099](matrix-org#13099))
- Refactor macaroon tokens generation and move the unsubscribe link in notification emails to `/_synapse/client/unsubscribe`. ([\matrix-org#12986](matrix-org#12986))
- Fix documentation for running complement tests. ([\matrix-org#12990](matrix-org#12990))
- Faster joins: add issue links to the TODO comments in the code. ([\matrix-org#13004](matrix-org#13004))
- Reduce DB usage of `/sync` when a large number of unread messages have recently been sent in a room. ([\matrix-org#13005](matrix-org#13005), [\matrix-org#13096](matrix-org#13096), [\matrix-org#13118](matrix-org#13118))
- Replaced usage of PyJWT with methods from Authlib in `org.matrix.login.jwt`. Contributed by Hannes Lerchl. ([\matrix-org#13011](matrix-org#13011))
- Modernize the `contrib/graph/` scripts. ([\matrix-org#13013](matrix-org#13013))
- Remove redundant `room_version` parameters from event auth functions. ([\matrix-org#13017](matrix-org#13017))
- Decouple `synapse.api.auth_blocking.AuthBlocking` from `synapse.api.auth.Auth`. ([\matrix-org#13021](matrix-org#13021))
- Add type annotations to `synapse.storage.databases.main.devices`. ([\matrix-org#13025](matrix-org#13025))
- Set default `sync_response_cache_duration` to two minutes. ([\matrix-org#13042](matrix-org#13042))
- Rename CI test runs. ([\matrix-org#13046](matrix-org#13046))
- Increase timeout of complement CI test runs. ([\matrix-org#13048](matrix-org#13048))
- Refactor entry points so that they all have a `main` function. ([\matrix-org#13052](matrix-org#13052))
- Refactor the Dockerfile-workers configuration script to use Jinja2 templates in Synapse workers' Supervisord blocks. ([\matrix-org#13054](matrix-org#13054))
- Add headers to individual options in config documentation to allow for linking. ([\matrix-org#13055](matrix-org#13055))
- Make Complement CI logs easier to read. ([\matrix-org#13057](matrix-org#13057), [\matrix-org#13058](matrix-org#13058), [\matrix-org#13069](matrix-org#13069))
- Don't instantiate modules with keyword arguments. ([\matrix-org#13060](matrix-org#13060))
- Fix type checking errors against Twisted trunk. ([\matrix-org#13061](matrix-org#13061))
- Allow MSC3030 `timestamp_to_event` calls from anyone on world-readable rooms. ([\matrix-org#13062](matrix-org#13062))
- Add a CI job to check that schema deltas are in the correct folder. ([\matrix-org#13063](matrix-org#13063))
- Avoid rechecking event auth rules which are independent of room state. ([\matrix-org#13065](matrix-org#13065))
- Reduce the duplication of code that invokes the rate limiter. ([\matrix-org#13070](matrix-org#13070))
- Add a Subject Alternative Name to the certificate generated for Complement tests. ([\matrix-org#13071](matrix-org#13071))
- Add more tests for room upgrades. ([\matrix-org#13074](matrix-org#13074))
- Pin dependencies maintained by matrix.org to [semantic version](https://semver.org/) bounds. ([\matrix-org#13082](matrix-org#13082))
- Correctly report prometheus DB stats for `get_earliest_token_for_stats`. ([\matrix-org#13085](matrix-org#13085))
- Fix a long-standing bug where a finished logging context would be re-started when Synapse failed to persist an event from federation. ([\matrix-org#13089](matrix-org#13089))
- Simplify the alias deletion logic as an application service. ([\matrix-org#13093](matrix-org#13093))
- Add type annotations to `tests.test_server`. ([\matrix-org#13124](matrix-org#13124))
dotlambda added a commit to dotlambda/nixpkgs that referenced this pull request Feb 10, 2023
gador pushed a commit to gador/nixpkgs that referenced this pull request Feb 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants