Allow new users to be registered via the admin API even if the monthly active user limit has been reached #7263

dklimpel · 2020-04-11T15:36:27Z

Fixes: #7175
If MAU limit is reached, you can add user with GET /_synapse/admin/v1/register. With PUT /_synapse/admin/v2/users/<user_id> you can only add users if admin user was already active before limit was reached. Admin users seem to be MAU limited.

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.
Pull request includes a sign off
Code style is correct (run the linters)

changelog.d/7263.bugfix

synapse/handlers/register.py

richvdh

ok the code now seems pretty sensible.

The tests, on the other hand, I can't follow :/.

synapse/handlers/register.py

tests/rest/admin/test_user.py

richvdh · 2020-04-29T18:46:23Z

tests/rest/admin/test_user.py

+        Check that a new regular user is created successfully if MAU limit is reached.
+        Admin user was active before creating user.
+        """
+        self.hs.config.registration_shared_secret = None
+
+        handler = self.hs.get_registration_handler()
+
+        self.hs.config.limit_usage_by_mau = True
+        self.hs.config.max_mau_value = 2
+        self.hs.config.mau_trial_days = 0
+
+        def _do_sync_for_user(token):
+            request, channel = self.make_request("GET", "/sync", access_token=token)
+            self.render(request)
+
+            if channel.code != 200:
+                raise HttpResponseException(
+                    channel.code, channel.result["reason"], channel.result["body"]
+                ).to_synapse_error()
+
+        # Sync to set admin user to active
+        _do_sync_for_user(self.admin_user_tok)
+
+        self.store.get_monthly_active_count = Mock(
+            return_value=defer.succeed(self.hs.config.max_mau_value + 1)
+        )
+
+        # Set MAU limit
+        self.get_failure(
+            handler.register_user(localpart="local_part"), ResourceLimitError
+        )
+
+        self.store.get_monthly_active_count = Mock(
+            return_value=defer.succeed(self.hs.config.max_mau_value + 1)
+        )
+        self.get_failure(
+            handler.register_user(localpart="local_part"), ResourceLimitError
+        )
+
+        # Register new user with admin API
+        url = "/_synapse/admin/v2/users/@bob:test"
+
+        # Create user
+        body = json.dumps({"password": "abc123", "admin": False})
+
+        request, channel = self.make_request(
+            "PUT",
+            url,
+            access_token=self.admin_user_tok,
+            content=body.encode(encoding="utf_8"),
+        )
+        self.render(request)
+
+        self.assertEqual(201, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual("@bob:test", channel.json_body["name"])
+        self.assertEqual(False, channel.json_body["admin"])
+
+    def test_create_user_limit_mau_passiv_admin(self):
+        """
+        Check that a new regular user is created successfully if MAU limit is reached.
+        Admin user was not active before creating user and creation fails.
+        """
+        self.hs.config.registration_shared_secret = None
+
+        handler = self.hs.get_registration_handler()
+
+        self.hs.config.limit_usage_by_mau = True
+        self.hs.config.max_mau_value = 2
+        self.hs.config.mau_trial_days = 0
+
+        # Set MAU limit
+        self.store.get_monthly_active_count = Mock(
+            return_value=defer.succeed(self.hs.config.max_mau_value + 1)
+        )
+        self.get_failure(
+            handler.register_user(localpart="local_part"), ResourceLimitError
+        )
+
+        self.store.get_monthly_active_count = Mock(
+            return_value=defer.succeed(self.hs.config.max_mau_value + 1)
+        )
+        self.get_failure(
+            handler.register_user(localpart="local_part"), ResourceLimitError
+        )
+
+        # Register new user with admin API
+        url = "/_synapse/admin/v2/users/@bob:test"
+
+        # Create user
+        body = json.dumps({"password": "abc123", "admin": False})
+
+        request, channel = self.make_request(
+            "PUT",
+            url,
+            access_token=self.admin_user_tok,
+            content=body.encode(encoding="utf_8"),
+        )
+        self.render(request)
+
+        self.assertEqual(500, int(channel.result["code"]), msg=channel.result["body"])


help me out here: what is the difference between these tests and the ones further up at line 327 etc? what is the purpose of calling sync etc?

Perhaps some more comments to explain the test strategy and whwhat we're trying to do would be helpful.

I have added more comments.
The difference is the sync.
If admin did a sync before mau limit is reached, the admin can send POST requests and create new users.
If admin did not a sync before limit is reached, the admin can not create new users, because the admin can not send POST requests. The admin is limited like a normal user.

tests/rest/admin/test_user.py

Update because of changes in "Stop Auth methods from polling the config on every req. (matrix-org#7420)"

clokep · 2020-05-15T19:35:08Z

@dklimpel Is this ready for review again or are you still handling the review comments?

dklimpel · 2020-05-15T20:12:53Z

@clokep It is ready for review.

tests/rest/admin/test_user.py

richvdh · 2020-06-02T14:52:57Z

tests/rest/admin/test_user.py

+    def test_create_user_mau_limit_reached_passiv_admin(self):
+        """
+        Try to create a new regular user if MAU limit is reached.
+        Admin user was not active before creating user.


I'm still a bit baffled by why we need this as well as the previous test. Is it really likely that an inactive admin could be blocked from creating a new user?

The test is presented for completeness. It can be left out. We deal with mau. And it would be possible that an active and passive admin/user get different results.

tests/rest/admin/test_user.py

richvdh

lgtm!

richvdh · 2020-06-05T12:08:26Z

thanks again for your work on this.

@dklimpel

…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))

…y active user limit has been reached (matrix-org#7263)

…dinsic-release-v1.15.x * 'release-v1.15.0' of github.com:matrix-org/synapse: (55 commits) 1.15.0 Fix some attributions Update CHANGES.md 1.15.0rc1 Revert "1.15.0rc1" 1.15.0rc1 Fix bug in account data replication stream. (#7656) Convert the registration handler to async/await. (#7649) Accept device information at the login fallback endpoint. (#7629) Convert user directory handler and related classes to async/await. (#7640) Add an option to disable autojoin for guest accounts (#6637) Clarifications to the admin api documentation (#7647) Update to the stable SSO prefix for UI Auth. (#7630) Fix type information on `assert_*_is_admin` methods (#7645) Remove some unused constants. (#7644) Typo fixes. Allow new users to be registered via the admin API even if the monthly active user limit has been reached (#7263) Add device management to admin API (#7481) Attempt to fix PhoneHomeStatsTestCase.test_performance_100 being flaky. (#7634) Support CS API v0.6.0 (#6585) ...

Sync develop

d813e69

dklimpel force-pushed the admin_api_create_user_mau branch 2 times, most recently from a231639 to 6ca9ff7 Compare April 11, 2020 19:55

Create user with admin API if MAU is reached

8f75ac6

dklimpel force-pushed the admin_api_create_user_mau branch from 6ca9ff7 to 8f75ac6 Compare April 11, 2020 20:01

Sync develop for renew tests

26bddb3

clokep requested a review from a team April 17, 2020 12:30

richvdh changed the title ~~Create user with admin API if MAU limit is reached~~ Allow new users to be registered via the admin API even if the monthly active user limit has been reached Apr 29, 2020

richvdh suggested changes Apr 29, 2020

View reviewed changes

changelog.d/7263.bugfix Outdated Show resolved Hide resolved

synapse/handlers/register.py Outdated Show resolved Hide resolved

dklimpel added 2 commits April 29, 2020 19:27

Add parameter 'by_admin' to register_user

b1ad0f0

Update tests

4c2d3a7

richvdh suggested changes Apr 29, 2020

View reviewed changes

dklimpel added 3 commits May 1, 2020 22:28

Update tests after review

ae0eff0

Merge synapse/develop

75975e0

Update MAU limit tests

b3b347b

Update because of changes in "Stop Auth methods from polling the config on every req. (matrix-org#7420)"

clokep requested a review from richvdh May 18, 2020 13:02

Merge branch 'develop' into admin_api_create_user_mau

9c17aa2

richvdh suggested changes Jun 2, 2020

View reviewed changes

dklimpel added 2 commits June 2, 2020 23:00

Apply suggestions from code review

cf1c9ea

code style

b9d9bec

richvdh self-requested a review June 3, 2020 09:32

richvdh reviewed Jun 5, 2020

View reviewed changes

tests/rest/admin/test_user.py Outdated Show resolved Hide resolved

Update tests/rest/admin/test_user.py

d3b0a82

richvdh approved these changes Jun 5, 2020

View reviewed changes

Update 7263.bugfix

d47b564

richvdh merged commit 908f9e2 into matrix-org:develop Jun 5, 2020

dklimpel deleted the admin_api_create_user_mau branch June 5, 2020 13:59

phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020

Allow new users to be registered via the admin API even if the monthl…

c522610

…y active user limit has been reached (matrix-org#7263)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow new users to be registered via the admin API even if the monthly active user limit has been reached #7263

Allow new users to be registered via the admin API even if the monthly active user limit has been reached #7263

dklimpel commented Apr 11, 2020 •

edited

Loading

richvdh left a comment

richvdh Apr 29, 2020

dklimpel May 1, 2020

clokep commented May 15, 2020

dklimpel commented May 15, 2020

richvdh Jun 2, 2020

dklimpel Jun 2, 2020

richvdh left a comment

richvdh commented Jun 5, 2020

Allow new users to be registered via the admin API even if the monthly active user limit has been reached #7263

Allow new users to be registered via the admin API even if the monthly active user limit has been reached #7263

Conversation

dklimpel commented Apr 11, 2020 • edited Loading

Pull Request Checklist

richvdh left a comment

Choose a reason for hiding this comment

richvdh Apr 29, 2020

Choose a reason for hiding this comment

dklimpel May 1, 2020

Choose a reason for hiding this comment

clokep commented May 15, 2020

dklimpel commented May 15, 2020

richvdh Jun 2, 2020

Choose a reason for hiding this comment

dklimpel Jun 2, 2020

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh commented Jun 5, 2020

dklimpel commented Apr 11, 2020 •

edited

Loading