This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Don't throw exception on m.login.jwt automatic user creation #7585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nope, botched up the changelog thing. I'm on it. |
When a previously non-existent user is authenticated with m.login.jwt
(after the token has been validated), the user should be created. But a
bug made checking if the user exists reset the claimed username to None,
and we would see generic TypeError exceptions when we asked synapse to
process it further:
...
File "/usr/lib/python3/dist-packages/synapse/rest/client/v1/login.py", line 393, in do_jwt_login
result = await self._complete_login(
File "/usr/lib/python3/dist-packages/synapse/rest/client/v1/login.py", line 338, in _complete_login
localpart=UserID.from_string(user_id).localpart
File "/usr/lib/python3/dist-packages/synapse/types.py", line 171, in from_string
if len(s) < 1 or s[0:1] != cls.SIGIL:
TypeError: object of type 'NoneType' has no len()
This regression was introduced in 541f1b9 (part of the v1.6.0 release
of synapse); it only affects auth flows where _complete_login is called
with create_non_existant_user=True, which turns out is only the
m.login.jwt auth flow.
Signed-off-by: Olof Johansson <[email protected]>
The parameter is for an internal method with no use outside of the module (as confirmed by grepping for the method name in the repository). Signed-off-by: Olof Johansson <[email protected]>
|
Looking at the failed test, it errors with |
I kicked off the test run again and it seems to have passed. 😄 |
clokep
reviewed
May 29, 2020
Signed-off-by: Olof Johansson <[email protected]>
|
I haven't looked at writing unit tests for this flow, no. Would love to give it a try. A new pull request for that would be better, right? |
|
I've written some unit tests, but as they are based on this change, I'm holding off on opening the pull request until this lands. If you like to take a peek the tests are available at https://github.com/olof/synapse/tree/jwt_unit_tests Edit: note that I will be rebasing that branch once this pull request lands. |
You can just include them in this PR! No need to make it a separate one. |
Signed-off-by: Olof Johansson <[email protected]>
|
Alright, tests added in this pull requests. Do I need to adapt the changelog, or that's ok as is? |
|
The changelog looks good. I'll take another look at the code soon! 👍 |
|
Neat. This seem sane! I ran the tests without the fixes and two of the tests failed, so that's perfect. 👍 |
clokep
approved these changes
Jun 1, 2020
|
Also looks like #7604 got filed, which is essentially this same issue. |
babolivier
added a commit
that referenced
this pull request
Jun 10, 2020
…rg-hotfixes Synapse 1.15.0rc1 (2020-06-09) ============================== Features -------- - Advertise support for Client-Server API r0.6.0 and remove related unstable feature flags. ([\#6585](#6585)) - Add an option to disable autojoining rooms for guest accounts. ([\#6637](#6637)) - For SAML authentication, add the ability to pass email addresses to be added to new users' accounts via SAML attributes. Contributed by Christopher Cooper. ([\#7385](#7385)) - Add admin APIs to allow server admins to manage users' devices. Contributed by @dklimpel. ([\#7481](#7481)) - Add support for generating thumbnails for WebP images. Previously, users would see an empty box instead of preview image. ([\#7586](#7586)) - Support the standardized `m.login.sso` user-interactive authentication flow. ([\#7630](#7630)) Bugfixes -------- - Allow new users to be registered via the admin API even if the monthly active user limit has been reached. Contributed by @dkimpel. ([\#7263](#7263)) - Fix email notifications not being enabled for new users when created via the Admin API. ([\#7267](#7267)) - Fix str placeholders in an instance of `PrepareDatabaseException`. Introduced in Synapse v1.8.0. ([\#7575](#7575)) - Fix a bug in automatic user creation during first time login with `m.login.jwt`. Regression in v1.6.0. Contributed by @olof. ([\#7585](#7585)) - Fix a bug causing the cross-signing keys to be ignored when resyncing a device list. ([\#7594](#7594)) - Fix metrics failing when there is a large number of active background processes. ([\#7597](#7597)) - Fix bug where returning rooms for a group would fail if it included a room that the server was not in. ([\#7599](#7599)) - Fix duplicate key violation when persisting read markers. ([\#7607](#7607)) - Prevent an entire iteration of the device list resync loop from failing if one server responds with a malformed result. ([\#7609](#7609)) - Fix exceptions when fetching events from a remote host fails. ([\#7622](#7622)) - Make `synctl restart` start synapse if it wasn't running. ([\#7624](#7624)) - Pass device information through to the login endpoint when using the login fallback. ([\#7629](#7629)) - Advertise the `m.login.token` login flow when OpenID Connect is enabled. ([\#7631](#7631)) - Fix bug in account data replication stream. ([\#7656](#7656)) Improved Documentation ---------------------- - Update the OpenBSD installation instructions. ([\#7587](#7587)) - Advertise Python 3.8 support in `setup.py`. ([\#7602](#7602)) - Add a link to `#synapse:matrix.org` in the troubleshooting section of the README. ([\#7603](#7603)) - Clarifications to the admin api documentation. ([\#7647](#7647)) Internal Changes ---------------- - Convert the identity handler to async/await. ([\#7561](#7561)) - Improve query performance for fetching state from a PostgreSQL database. ([\#7567](#7567)) - Speed up processing of federation stream RDATA rows. ([\#7584](#7584)) - Add comment to systemd example to show postgresql dependency. ([\#7591](#7591)) - Refactor `Ratelimiter` to limit the amount of expensive config value accesses. ([\#7595](#7595)) - Convert groups handlers to async/await. ([\#7600](#7600)) - Clean up exception handling in `SAML2ResponseResource`. ([\#7614](#7614)) - Check that all asynchronous tasks succeed and general cleanup of `MonthlyActiveUsersTestCase` and `TestMauLimit`. ([\#7619](#7619)) - Convert `get_user_id_by_threepid` to async/await. ([\#7620](#7620)) - Switch to upstream `dh-virtualenv` rather than our fork for Debian package builds. ([\#7621](#7621)) - Update CI scripts to check the number in the newsfile fragment. ([\#7623](#7623)) - Check if the localpart of a Matrix ID is reserved for guest users earlier in the registration flow, as well as when responding to requests to `/register/available`. ([\#7625](#7625)) - Minor cleanups to OpenID Connect integration. ([\#7628](#7628)) - Attempt to fix flaky test: `PhoneHomeStatsTestCase.test_performance_100`. ([\#7634](#7634)) - Fix typos of `m.olm.curve25519-aes-sha2` and `m.megolm.v1.aes-sha2` in comments, test files. ([\#7637](#7637)) - Convert user directory, state deltas, and stats handlers to async/await. ([\#7640](#7640)) - Remove some unused constants. ([\#7644](#7644)) - Fix type information on `assert_*_is_admin` methods. ([\#7645](#7645)) - Convert registration handler to async/await. ([\#7649](#7649))
phil-flex
pushed a commit
to phil-flex/synapse
that referenced
this pull request
Jun 16, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a previously non-existent user is authenticated with m.login.jwt (after the token has been validated), the user should be created. But a bug made checking if the user exists reset the claimed username to None, and we would see generic TypeError exceptions when we asked synapse to
process it further:
This regression was introduced in 541f1b9 (part of the v1.6.0 release of synapse); it only affects auth flows where _complete_login is called with create_non_existant_user=True, which turns out is only the m.login.jwt auth flow.
This PR also includes a commit that changes the parameter
create_non_existant_usertocreate_non_existent_user, because... proactive confusion avoidance?Pull Request Checklist
[✓] on all checkboxes, as far as I can judge, but first time contributor here, so review my change with diligence! :)