Don't throw exception on m.login.jwt automatic user creation#7585
Don't throw exception on m.login.jwt automatic user creation#7585clokep merged 4 commits intomatrix-org:developfrom
Conversation
|
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 <olof@ethup.se>
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 <olof@ethup.se>
|
Looking at the failed test, it errors with |
I kicked off the test run again and it seems to have passed. 😄 |
clokep
left a comment
There was a problem hiding this comment.
Looks good! (And thanks for fixing the spelling error.) Did you see if it would be possible to add a test for this flow?
Signed-off-by: Olof Johansson <olof@ethup.se>
|
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 <olof@ethup.se>
|
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. 👍 |
|
Also looks like #7604 got filed, which is essentially this same issue. |
…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))
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! :)