support admin_email config and pass through into blocking errors, #3687

neilisfragile · 2018-08-13T20:11:43Z

also, return AuthError in all cases (not RegistrationError)

…urn AuthError in all cases

remove comments

…min_email

erikjohnston · 2018-08-15T09:59:24Z

synapse/api/auth.py

-                403, self.hs.config.hs_disabled_message, errcode=Codes.HS_DISABLED
+                403, self.hs.config.hs_disabled_message,
+                errcode=Codes.RESOURCE_LIMIT_EXCEED,
+                admin_uri=self.hs.config.admin_uri,


Looks like you've manged to pull this in from the PR to change the error codes. I'm raising an eyebrow slightly that we're not able to differentiate between the mau/resource stuff and the generic disable HS option.

I'm using https://docs.google.com/document/d/1_CgwkfLznU56fwLUFZXFnKivzVUPjjNsTO0DpCB9ncM/edit#heading=h.g9t44e4usbqd as the base - the proposal is not ratified so could change further (in fact please add comments) - the idea is to be flexible for future limiting, with a human readable explainer in the body - this approach doesn't lend itself to i18n though

I guess we can leave it as is for now and add an option for setting the error code in the config when we disable, if it proves necessary.

erikjohnston · 2018-08-15T10:06:04Z

synapse/api/errors.py

+        self.admin_uri = kwargs.get('admin_uri')
+        self.msg = kwargs.get('msg')
+        self.errcode = kwargs.get('errcode')
+        super(AuthError, self).__init__(*args, errcode=kwargs["errcode"])


So this constructor now looks odd. We should either pass through **kwargs to the super constructor or take errcode as a parameter instead of *args, **kwargs.

Its also worth noting that self.msg and self.errocde are set by SynapseError constructor, so we don't need to set them here.

I'd probably write this as something like:

def __init__(self, code, msg, errcode=Codes.FORBIDDEN, admin_url=None): self.admin_uri = admin_url super(AuthError, self).__init__(code, msg, errcode=errcode)

Sorry, that's brain dead on my part, thanks for the clean up

erikjohnston · 2018-08-15T10:12:44Z

synapse/config/server.py

@@ -82,6 +82,10 @@ def read_config(self, config):
        self.hs_disabled = config.get("hs_disabled", False)
        self.hs_disabled_message = config.get("hs_disabled_message", "")

+        # Admin email to direct users at should their instance become blocked
+        # due to resource constraints
+        self.admin_uri = config.get("admin_uri", None)


Admin email or admin URI?

…rg/synapse into neilj/admin_email

erikjohnston · 2018-08-15T11:50:27Z

changelog.d/3697.misc

@@ -0,0 +1 @@
+update resource limit error codes


We should probably not have two changelogs?

erikjohnston

LGTM, though we need to sort out the change logs

@vojeroen

Features -------- - Add support for the SNI extension to federation TLS connections. Thanks to @vojeroen! ([\#3439](#3439)) - Add /_media/r0/config ([\#3184](#3184)) - speed up /members API and add `at` and `membership` params as per MSC1227 ([\#3568](#3568)) - implement `summary` block in /sync response as per MSC688 ([\#3574](#3574)) - Add lazy-loading support to /messages as per MSC1227 ([\#3589](#3589)) - Add ability to limit number of monthly active users on the server ([\#3633](#3633)) - Support more federation endpoints on workers ([\#3653](#3653)) - Basic support for room versioning ([\#3654](#3654)) - Ability to disable client/server Synapse via conf toggle ([\#3655](#3655)) - Ability to whitelist specific threepids against monthly active user limiting ([\#3662](#3662)) - Add some metrics for the appservice and federation event sending loops ([\#3664](#3664)) - Where server is disabled, block ability for locked out users to read new messages ([\#3670](#3670)) - set admin uri via config, to be used in error messages where the user should contact the administrator ([\#3687](#3687)) - Synapse's presence functionality can now be disabled with the "use_presence" configuration option. ([\#3694](#3694)) - For resource limit blocked users, prevent writing into rooms ([\#3708](#3708)) Bugfixes -------- - Fix occasional glitches in the synapse_event_persisted_position metric ([\#3658](#3658)) - Fix bug on deleting 3pid when using identity servers that don't support unbind API ([\#3661](#3661)) - Make the tests pass on Twisted < 18.7.0 ([\#3676](#3676)) - Don’t ship recaptcha_ajax.js, use it directly from Google ([\#3677](#3677)) - Fixes test_reap_monthly_active_users so it passes under postgres ([\#3681](#3681)) - Fix mau blocking calulation bug on login ([\#3689](#3689)) - Fix missing yield in synapse.storage.monthly_active_users.initialise_reserved_users ([\#3692](#3692)) - Improve HTTP request logging to include all requests ([\#3700](#3700)) - Avoid timing out requests while we are streaming back the response ([\#3701](#3701)) - Support more federation endpoints on workers ([\#3705](#3705), [\#3713](#3713)) - Fix "Starting db txn 'get_all_updated_receipts' from sentinel context" warning ([\#3710](#3710)) - Fix bug where `state_cache` cache factor ignored environment variables ([\#3719](#3719)) - Fix bug in v0.33.3rc1 which caused infinite loops and OOMs ([\#3723](#3723)) - Fix bug introduced in v0.33.3rc1 which made the ToS give a 500 error ([\#3732](#3732)) Deprecations and Removals ------------------------- - The Shared-Secret registration method of the legacy v1/register REST endpoint has been removed. For a replacement, please see [the admin/register API documentation](https://github.com/matrix-org/synapse/blob/master/docs/admin_api/register_api.rst). ([\#3703](#3703)) Internal Changes ---------------- - The test suite now can run under PostgreSQL. ([\#3423](#3423)) - Refactor HTTP replication endpoints to reduce code duplication ([\#3632](#3632)) - Tests now correctly execute on Python 3. ([\#3647](#3647)) - Sytests can now be run inside a Docker container. ([\#3660](#3660)) - Port over enough to Python 3 to allow the sytests to start. ([\#3668](#3668)) - Update docker base image from alpine 3.7 to 3.8. ([\#3669](#3669)) - Rename synapse.util.async to synapse.util.async_helpers to mitigate async becoming a keyword on Python 3.7. ([\#3678](#3678)) - Synapse's tests are now formatted with the black autoformatter. ([\#3679](#3679)) - Implemented a new testing base class to reduce test boilerplate. ([\#3684](#3684)) - Rename MAU prometheus metrics ([\#3690](#3690)) - add new error type ResourceLimit ([\#3707](#3707)) - Logcontexts for replication command handlers ([\#3709](#3709)) - Update admin register API documentation to reference a real user ID. ([\#3712](#3712))

neilisfragile and others added 2 commits August 13, 2018 21:09

support admin_email config and pass through into blocking errors, ret…

f4b4915

…urn AuthError in all cases

set admin email via config

1f24d86

neilisfragile requested a review from a team August 13, 2018 21:44

neilisfragile added 2 commits August 14, 2018 14:55

Update register.py

99ebaed

remove comments

remove blank line

c74c711

neilisfragile mentioned this pull request Aug 14, 2018

Fix Mau off by one errors #3689

Merged

Merge branch 'develop' of github.com:matrix-org/synapse into neilj/ad…

19b433e

…min_email

neilisfragile assigned erikjohnston Aug 15, 2018

neilisfragile added 3 commits August 15, 2018 10:16

replace admin_email with admin_uri for greater flexibility

ab035bd

update error codes for resource limiting

b8429c7

update resource limit error codes

596ce63

erikjohnston suggested changes Aug 15, 2018

View reviewed changes

erikjohnston assigned neilisfragile and unassigned erikjohnston Aug 15, 2018

neilisfragile added 4 commits August 15, 2018 11:27

update error codes

75c663c

update admin email to uri

55afba0

Merge branch 'neilj/update_limits_error_codes' of github.com:matrix-o…

c4eb975

…rg/synapse into neilj/admin_email

clean up AuthError

87a824b

neilisfragile assigned erikjohnston and unassigned neilisfragile Aug 15, 2018

erikjohnston reviewed Aug 15, 2018

View reviewed changes

changelog.d/3697.misc Outdated

@@ -0,0 +1 @@

update resource limit error codes

Copy link

Member

erikjohnston Aug 15, 2018

Choose a reason for hiding this comment

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

We should probably not have two changelogs?

erikjohnston approved these changes Aug 15, 2018

View reviewed changes

remove changelog referencing another PR

39176f2

neilisfragile merged commit 4601129 into develop Aug 15, 2018

neilisfragile deleted the neilj/admin_email branch August 15, 2018 13:52

neilisfragile mentioned this pull request Aug 15, 2018

update limits error codes #3697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support admin_email config and pass through into blocking errors, #3687

support admin_email config and pass through into blocking errors, #3687

neilisfragile commented Aug 13, 2018

erikjohnston Aug 15, 2018

neilisfragile Aug 15, 2018 •

edited

Loading

erikjohnston Aug 15, 2018

erikjohnston Aug 15, 2018

neilisfragile Aug 15, 2018

erikjohnston Aug 15, 2018

erikjohnston Aug 15, 2018

erikjohnston left a comment

support admin_email config and pass through into blocking errors, #3687

support admin_email config and pass through into blocking errors, #3687

Conversation

neilisfragile commented Aug 13, 2018

erikjohnston Aug 15, 2018

Choose a reason for hiding this comment

neilisfragile Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

erikjohnston Aug 15, 2018

Choose a reason for hiding this comment

erikjohnston Aug 15, 2018

Choose a reason for hiding this comment

neilisfragile Aug 15, 2018

Choose a reason for hiding this comment

erikjohnston Aug 15, 2018

Choose a reason for hiding this comment

erikjohnston Aug 15, 2018

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

neilisfragile Aug 15, 2018 •

edited

Loading