From ad5d2e7ec0850f84ff74a137ca53b701925778d8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Mar 2021 13:51:24 +0000 Subject: Pull up appservice login deprecation notice --- CHANGES.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 238eb8a4ed..860797daf9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,13 @@ Synapse 1.30.0rc1 (2021-03-16) ============================== +Note that this release deprecates the ability for appservices to +call `POST /_matrix/client/r0/register` without the body parameter `type`. Appservice +developers should use a `type` value of `m.login.application_service` as +per [the spec](https://matrix.org/docs/spec/application_service/r0.1.2#server-admin-style-permissions). +In future releases, calling this endpoint with an access token - but without a `m.login.application_service` +type - will fail. + Features -------- @@ -68,12 +75,6 @@ Internal Changes - Prevent attempting to bundle aggregations for state events in /context APIs. ([\#9619](https://github.com/matrix-org/synapse/issues/9619)) -Removal warning ---------------- - -Note that this release deprecates the ability for appservices to call `POST /_matrix/client/r0/register` without the body parameter `type`. Appservice developers should use a `type` value of `m.login.application_service` as per the spec. In future releases, calling this endpoint with an access token but -without a valid type will fail. - Synapse 1.29.0 (2021-03-08) =========================== -- cgit 1.5.1 From e3bc0e6f7c0f9e847f817538e490ffee489469b5 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Mar 2021 14:32:49 +0000 Subject: Changelog typo --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 860797daf9..d3e8fbae34 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -25,7 +25,7 @@ Bugfixes - Fix long-standing bug when generating thumbnails for some images with transparency: `TypeError: cannot unpack non-iterable int object`. ([\#9473](https://github.com/matrix-org/synapse/issues/9473)) - Purge chain cover indexes for events that were purged prior to Synapse v1.29.0. ([\#9542](https://github.com/matrix-org/synapse/issues/9542), [\#9583](https://github.com/matrix-org/synapse/issues/9583)) - Fix bug where federation requests were not correctly retried on 5xx responses. ([\#9567](https://github.com/matrix-org/synapse/issues/9567)) -- Re-Activating account with admin API when local passwords are disabled. ([\#9587](https://github.com/matrix-org/synapse/issues/9587)) +- Fix re-activating an account via the admin API when local passwords are disabled. ([\#9587](https://github.com/matrix-org/synapse/issues/9587)) - Fix a bug introduced in Synapse 1.20 which caused incoming federation transactions to stack up, causing slow recovery from outages. ([\#9597](https://github.com/matrix-org/synapse/issues/9597)) - Fix a bug introduced in v1.28.0 where the OpenID Connect callback endpoint could error with a `MacaroonInitException`. ([\#9620](https://github.com/matrix-org/synapse/issues/9620)) - Fix Internal Server Error on `GET /_synapse/client/saml2/authn_response` request. ([\#9623](https://github.com/matrix-org/synapse/issues/9623)) -- cgit 1.5.1 From 8000cf131592b6edcded65ef4be20b8ac0f1bfd3 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 16 Mar 2021 16:44:25 +0100 Subject: Return m.change_password.enabled=false if local database is disabled (#9588) Instead of if the user does not have a password hash. This allows a SSO user to add a password to their account, but only if the local password database is configured. --- changelog.d/9588.bugfix | 1 + synapse/handlers/auth.py | 13 +++++++++ synapse/rest/client/v2_alpha/capabilities.py | 23 ++++++++-------- tests/rest/client/v2_alpha/test_capabilities.py | 36 ++++++++++++++++++++++--- 4 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 changelog.d/9588.bugfix diff --git a/changelog.d/9588.bugfix b/changelog.d/9588.bugfix new file mode 100644 index 0000000000..b8d6140565 --- /dev/null +++ b/changelog.d/9588.bugfix @@ -0,0 +1 @@ +Fix the `/capabilities` endpoint to return `m.change_password` as disabled if the local password database is not used for authentication. Contributed by @dklimpel. diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index fb5f8118f0..badac8c26c 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -886,6 +886,19 @@ class AuthHandler(BaseHandler): ) return result + def can_change_password(self) -> bool: + """Get whether users on this server are allowed to change or set a password. + + Both `config.password_enabled` and `config.password_localdb_enabled` must be true. + + Note that any account (even SSO accounts) are allowed to add passwords if the above + is true. + + Returns: + Whether users on this server are allowed to change or set a password + """ + return self._password_enabled and self._password_localdb_enabled + def get_supported_login_types(self) -> Iterable[str]: """Get a the login types supported for the /login API diff --git a/synapse/rest/client/v2_alpha/capabilities.py b/synapse/rest/client/v2_alpha/capabilities.py index 76879ac559..44ccf10ed4 100644 --- a/synapse/rest/client/v2_alpha/capabilities.py +++ b/synapse/rest/client/v2_alpha/capabilities.py @@ -13,12 +13,18 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +from typing import TYPE_CHECKING, Tuple from synapse.api.room_versions import KNOWN_ROOM_VERSIONS from synapse.http.servlet import RestServlet +from synapse.http.site import SynapseRequest +from synapse.types import JsonDict from ._base import client_patterns +if TYPE_CHECKING: + from synapse.server import HomeServer + logger = logging.getLogger(__name__) @@ -27,21 +33,16 @@ class CapabilitiesRestServlet(RestServlet): PATTERNS = client_patterns("/capabilities$") - def __init__(self, hs): - """ - Args: - hs (synapse.server.HomeServer): server - """ + def __init__(self, hs: "HomeServer"): super().__init__() self.hs = hs self.config = hs.config self.auth = hs.get_auth() - self.store = hs.get_datastore() + self.auth_handler = hs.get_auth_handler() - async def on_GET(self, request): - requester = await self.auth.get_user_by_req(request, allow_guest=True) - user = await self.store.get_user_by_id(requester.user.to_string()) - change_password = bool(user["password_hash"]) + async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + await self.auth.get_user_by_req(request, allow_guest=True) + change_password = self.auth_handler.can_change_password() response = { "capabilities": { @@ -58,5 +59,5 @@ class CapabilitiesRestServlet(RestServlet): return 200, response -def register_servlets(hs, http_server): +def register_servlets(hs: "HomeServer", http_server): CapabilitiesRestServlet(hs).register(http_server) diff --git a/tests/rest/client/v2_alpha/test_capabilities.py b/tests/rest/client/v2_alpha/test_capabilities.py index e808339fb3..287a1a485c 100644 --- a/tests/rest/client/v2_alpha/test_capabilities.py +++ b/tests/rest/client/v2_alpha/test_capabilities.py @@ -18,6 +18,7 @@ from synapse.rest.client.v1 import login from synapse.rest.client.v2_alpha import capabilities from tests import unittest +from tests.unittest import override_config class CapabilitiesTestCase(unittest.HomeserverTestCase): @@ -33,6 +34,7 @@ class CapabilitiesTestCase(unittest.HomeserverTestCase): hs = self.setup_test_homeserver() self.store = hs.get_datastore() self.config = hs.config + self.auth_handler = hs.get_auth_handler() return hs def test_check_auth_required(self): @@ -56,7 +58,7 @@ class CapabilitiesTestCase(unittest.HomeserverTestCase): capabilities["m.room_versions"]["default"], ) - def test_get_change_password_capabilities(self): + def test_get_change_password_capabilities_password_login(self): localpart = "user" password = "pass" user = self.register_user(localpart, password) @@ -66,10 +68,36 @@ class CapabilitiesTestCase(unittest.HomeserverTestCase): capabilities = channel.json_body["capabilities"] self.assertEqual(channel.code, 200) - - # Test case where password is handled outside of Synapse self.assertTrue(capabilities["m.change_password"]["enabled"]) - self.get_success(self.store.user_set_password_hash(user, None)) + + @override_config({"password_config": {"localdb_enabled": False}}) + def test_get_change_password_capabilities_localdb_disabled(self): + localpart = "user" + password = "pass" + user = self.register_user(localpart, password) + access_token = self.get_success( + self.auth_handler.get_access_token_for_user_id( + user, device_id=None, valid_until_ms=None + ) + ) + + channel = self.make_request("GET", self.url, access_token=access_token) + capabilities = channel.json_body["capabilities"] + + self.assertEqual(channel.code, 200) + self.assertFalse(capabilities["m.change_password"]["enabled"]) + + @override_config({"password_config": {"enabled": False}}) + def test_get_change_password_capabilities_password_disabled(self): + localpart = "user" + password = "pass" + user = self.register_user(localpart, password) + access_token = self.get_success( + self.auth_handler.get_access_token_for_user_id( + user, device_id=None, valid_until_ms=None + ) + ) + channel = self.make_request("GET", self.url, access_token=access_token) capabilities = channel.json_body["capabilities"] -- cgit 1.5.1 From dd5e5dc1d6c88a3532d25f18cfc312d8bc813473 Mon Sep 17 00:00:00 2001 From: Hubbe Date: Tue, 16 Mar 2021 17:46:07 +0200 Subject: Add SSO attribute requirements for OIDC providers (#9609) Allows limiting who can login using OIDC via the claims made from the IdP. --- changelog.d/9609.feature | 1 + docs/sample_config.yaml | 24 +++++++ synapse/config/oidc_config.py | 40 +++++++++++- synapse/handlers/oidc_handler.py | 13 ++++ tests/handlers/test_oidc.py | 132 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 changelog.d/9609.feature diff --git a/changelog.d/9609.feature b/changelog.d/9609.feature new file mode 100644 index 0000000000..f3b6342069 --- /dev/null +++ b/changelog.d/9609.feature @@ -0,0 +1 @@ +Logins using OpenID Connect can require attributes on the `userinfo` response in order to login. Contributed by Hubbe King. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 7de000f4a4..a9f59e39f7 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1873,6 +1873,24 @@ saml2_config: # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. # +# It is possible to configure Synapse to only allow logins if certain attributes +# match particular values in the OIDC userinfo. The requirements can be listed under +# `attribute_requirements` as shown below. All of the listed attributes must +# match for the login to be permitted. Additional attributes can be added to +# userinfo by expanding the `scopes` section of the OIDC config to retrieve +# additional information from the OIDC provider. +# +# If the OIDC claim is a list, then the attribute must match any value in the list. +# Otherwise, it must exactly match the value of the claim. Using the example +# below, the `family_name` claim MUST be "Stephensson", but the `groups` +# claim MUST contain "admin". +# +# attribute_requirements: +# - attribute: family_name +# value: "Stephensson" +# - attribute: groups +# value: "admin" +# # See https://github.com/matrix-org/synapse/blob/master/docs/openid.md # for information on how to configure these options. # @@ -1905,6 +1923,9 @@ oidc_providers: # localpart_template: "{{ user.login }}" # display_name_template: "{{ user.name }}" # email_template: "{{ user.email }}" + # attribute_requirements: + # - attribute: userGroup + # value: "synapseUsers" # For use with Keycloak # @@ -1914,6 +1935,9 @@ oidc_providers: # client_id: "synapse" # client_secret: "copy secret generated in Keycloak UI" # scopes: ["openid", "profile"] + # attribute_requirements: + # - attribute: groups + # value: "admin" # For use with Github # diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 2bfb537c15..eab042a085 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -15,11 +15,12 @@ # limitations under the License. from collections import Counter -from typing import Iterable, Mapping, Optional, Tuple, Type +from typing import Iterable, List, Mapping, Optional, Tuple, Type import attr from synapse.config._util import validate_config +from synapse.config.sso import SsoAttributeRequirement from synapse.python_dependencies import DependencyException, check_requirements from synapse.types import Collection, JsonDict from synapse.util.module_loader import load_module @@ -191,6 +192,24 @@ class OIDCConfig(Config): # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. # + # It is possible to configure Synapse to only allow logins if certain attributes + # match particular values in the OIDC userinfo. The requirements can be listed under + # `attribute_requirements` as shown below. All of the listed attributes must + # match for the login to be permitted. Additional attributes can be added to + # userinfo by expanding the `scopes` section of the OIDC config to retrieve + # additional information from the OIDC provider. + # + # If the OIDC claim is a list, then the attribute must match any value in the list. + # Otherwise, it must exactly match the value of the claim. Using the example + # below, the `family_name` claim MUST be "Stephensson", but the `groups` + # claim MUST contain "admin". + # + # attribute_requirements: + # - attribute: family_name + # value: "Stephensson" + # - attribute: groups + # value: "admin" + # # See https://github.com/matrix-org/synapse/blob/master/docs/openid.md # for information on how to configure these options. # @@ -223,6 +242,9 @@ class OIDCConfig(Config): # localpart_template: "{{{{ user.login }}}}" # display_name_template: "{{{{ user.name }}}}" # email_template: "{{{{ user.email }}}}" + # attribute_requirements: + # - attribute: userGroup + # value: "synapseUsers" # For use with Keycloak # @@ -232,6 +254,9 @@ class OIDCConfig(Config): # client_id: "synapse" # client_secret: "copy secret generated in Keycloak UI" # scopes: ["openid", "profile"] + # attribute_requirements: + # - attribute: groups + # value: "admin" # For use with Github # @@ -329,6 +354,10 @@ OIDC_PROVIDER_CONFIG_SCHEMA = { }, "allow_existing_users": {"type": "boolean"}, "user_mapping_provider": {"type": ["object", "null"]}, + "attribute_requirements": { + "type": "array", + "items": SsoAttributeRequirement.JSON_SCHEMA, + }, }, } @@ -465,6 +494,11 @@ def _parse_oidc_config_dict( jwt_header=client_secret_jwt_key_config["jwt_header"], jwt_payload=client_secret_jwt_key_config.get("jwt_payload", {}), ) + # parse attribute_requirements from config (list of dicts) into a list of SsoAttributeRequirement + attribute_requirements = [ + SsoAttributeRequirement(**x) + for x in oidc_config.get("attribute_requirements", []) + ] return OidcProviderConfig( idp_id=idp_id, @@ -488,6 +522,7 @@ def _parse_oidc_config_dict( allow_existing_users=oidc_config.get("allow_existing_users", False), user_mapping_provider_class=user_mapping_provider_class, user_mapping_provider_config=user_mapping_provider_config, + attribute_requirements=attribute_requirements, ) @@ -577,3 +612,6 @@ class OidcProviderConfig: # the config of the user mapping provider user_mapping_provider_config = attr.ib() + + # required attributes to require in userinfo to allow login/registration + attribute_requirements = attr.ib(type=List[SsoAttributeRequirement]) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 6d8551a6d6..bc3630e9e9 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -280,6 +280,7 @@ class OidcProvider: self._config = provider self._callback_url = hs.config.oidc_callback_url # type: str + self._oidc_attribute_requirements = provider.attribute_requirements self._scopes = provider.scopes self._user_profile_method = provider.user_profile_method @@ -859,6 +860,18 @@ class OidcProvider: ) # otherwise, it's a login + logger.debug("Userinfo for OIDC login: %s", userinfo) + + # Ensure that the attributes of the logged in user meet the required + # attributes by checking the userinfo against attribute_requirements + # In order to deal with the fact that OIDC userinfo can contain many + # types of data, we wrap non-list values in lists. + if not self._sso_handler.check_required_attributes( + request, + {k: v if isinstance(v, list) else [v] for k, v in userinfo.items()}, + self._oidc_attribute_requirements, + ): + return # Call the mapper to register/login the user try: diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 5e9c9c2e88..c7796fb837 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -989,6 +989,138 @@ class OidcHandlerTestCase(HomeserverTestCase): self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) self.assertRenderedError("mapping_error", "localpart is invalid: ") + @override_config( + { + "oidc_config": { + **DEFAULT_CONFIG, + "attribute_requirements": [{"attribute": "test", "value": "foobar"}], + } + } + ) + def test_attribute_requirements(self): + """The required attributes must be met from the OIDC userinfo response.""" + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + + # userinfo lacking "test": "foobar" attribute should fail. + userinfo = { + "sub": "tester", + "username": "tester", + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": "foobar" attribute should succeed. + userinfo = { + "sub": "tester", + "username": "tester", + "test": "foobar", + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + + # check that the auth handler got called as expected + auth_handler.complete_sso_login.assert_called_once_with( + "@tester:test", "oidc", ANY, ANY, None, new_user=True + ) + + @override_config( + { + "oidc_config": { + **DEFAULT_CONFIG, + "attribute_requirements": [{"attribute": "test", "value": "foobar"}], + } + } + ) + def test_attribute_requirements_contains(self): + """Test that auth succeeds if userinfo attribute CONTAINS required value""" + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + # userinfo with "test": ["foobar", "foo", "bar"] attribute should succeed. + userinfo = { + "sub": "tester", + "username": "tester", + "test": ["foobar", "foo", "bar"], + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + + # check that the auth handler got called as expected + auth_handler.complete_sso_login.assert_called_once_with( + "@tester:test", "oidc", ANY, ANY, None, new_user=True + ) + + @override_config( + { + "oidc_config": { + **DEFAULT_CONFIG, + "attribute_requirements": [{"attribute": "test", "value": "foobar"}], + } + } + ) + def test_attribute_requirements_mismatch(self): + """ + Test that auth fails if attributes exist but don't match, + or are non-string values. + """ + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + # userinfo with "test": "not_foobar" attribute should fail + userinfo = { + "sub": "tester", + "username": "tester", + "test": "not_foobar", + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": ["foo", "bar"] attribute should fail + userinfo = { + "sub": "tester", + "username": "tester", + "test": ["foo", "bar"], + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": False attribute should fail + # this is largely just to ensure we don't crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": False, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": None attribute should fail + # a value of None breaks the OIDC spec, but it's important to not crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": None, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": 1 attribute should fail + # this is largely just to ensure we don't crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": 1, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": 3.14 attribute should fail + # this is largely just to ensure we don't crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": 3.14, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + def _generate_oidc_session_token( self, state: str, -- cgit 1.5.1 From 27d2820c33d94cd99aea128b6ade76a7de838c3d Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Tue, 16 Mar 2021 19:19:27 +0100 Subject: Enable flake8-bugbear, but disable most checks. (#9499) * Adds B00 to ignored checks. * Fixes remaining issues. --- changelog.d/9499.misc | 1 + setup.cfg | 3 ++- setup.py | 1 + synapse/app/__init__.py | 4 +++- synapse/config/key.py | 6 +++++- synapse/config/metrics.py | 4 +++- synapse/config/oidc_config.py | 4 +++- synapse/config/repository.py | 4 +++- synapse/config/saml2_config.py | 4 +++- synapse/config/tracer.py | 4 +++- synapse/crypto/context_factory.py | 2 +- tests/unittest.py | 2 +- 12 files changed, 29 insertions(+), 10 deletions(-) create mode 100644 changelog.d/9499.misc diff --git a/changelog.d/9499.misc b/changelog.d/9499.misc new file mode 100644 index 0000000000..1513017a10 --- /dev/null +++ b/changelog.d/9499.misc @@ -0,0 +1 @@ +Introduce bugbear to the test suite and fix some of it's lint violations. \ No newline at end of file diff --git a/setup.cfg b/setup.cfg index 5e301c2cd7..920868df20 100644 --- a/setup.cfg +++ b/setup.cfg @@ -18,7 +18,8 @@ ignore = # E203: whitespace before ':' (which is contrary to pep8?) # E731: do not assign a lambda expression, use a def # E501: Line too long (black enforces this for us) -ignore=W503,W504,E203,E731,E501 +# B00: Subsection of the bugbear suite (TODO: add in remaining fixes) +ignore=W503,W504,E203,E731,E501,B00 [isort] line_length = 88 diff --git a/setup.py b/setup.py index bbd9e7862a..b834e4e55b 100755 --- a/setup.py +++ b/setup.py @@ -99,6 +99,7 @@ CONDITIONAL_REQUIREMENTS["lint"] = [ "isort==5.7.0", "black==20.8b1", "flake8-comprehensions", + "flake8-bugbear", "flake8", ] diff --git a/synapse/app/__init__.py b/synapse/app/__init__.py index 4a9b0129c3..d1a2cd5e19 100644 --- a/synapse/app/__init__.py +++ b/synapse/app/__init__.py @@ -22,7 +22,9 @@ logger = logging.getLogger(__name__) try: python_dependencies.check_requirements() except python_dependencies.DependencyException as e: - sys.stderr.writelines(e.message) + sys.stderr.writelines( + e.message # noqa: B306, DependencyException.message is a property + ) sys.exit(1) diff --git a/synapse/config/key.py b/synapse/config/key.py index de964dff13..350ff1d665 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -404,7 +404,11 @@ def _parse_key_servers(key_servers, federation_verify_certificates): try: jsonschema.validate(key_servers, TRUSTED_KEY_SERVERS_SCHEMA) except jsonschema.ValidationError as e: - raise ConfigError("Unable to parse 'trusted_key_servers': " + e.message) + raise ConfigError( + "Unable to parse 'trusted_key_servers': {}".format( + e.message # noqa: B306, jsonschema.ValidationError.message is a valid attribute + ) + ) for server in key_servers: server_name = server["server_name"] diff --git a/synapse/config/metrics.py b/synapse/config/metrics.py index dfd27e1523..2b289f4208 100644 --- a/synapse/config/metrics.py +++ b/synapse/config/metrics.py @@ -56,7 +56,9 @@ class MetricsConfig(Config): try: check_requirements("sentry") except DependencyException as e: - raise ConfigError(e.message) + raise ConfigError( + e.message # noqa: B306, DependencyException.message is a property + ) self.sentry_dsn = config["sentry"].get("dsn") if not self.sentry_dsn: diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index eab042a085..747ab9a7fe 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -42,7 +42,9 @@ class OIDCConfig(Config): try: check_requirements("oidc") except DependencyException as e: - raise ConfigError(e.message) from e + raise ConfigError( + e.message # noqa: B306, DependencyException.message is a property + ) from e # check we don't have any duplicate idp_ids now. (The SSO handler will also # check for duplicates when the REST listeners get registered, but that happens diff --git a/synapse/config/repository.py b/synapse/config/repository.py index 69d9de5a43..061c4ec83f 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -176,7 +176,9 @@ class ContentRepositoryConfig(Config): check_requirements("url_preview") except DependencyException as e: - raise ConfigError(e.message) + raise ConfigError( + e.message # noqa: B306, DependencyException.message is a property + ) if "url_preview_ip_range_blacklist" not in config: raise ConfigError( diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py index 4b494f217f..6db9cb5ced 100644 --- a/synapse/config/saml2_config.py +++ b/synapse/config/saml2_config.py @@ -76,7 +76,9 @@ class SAML2Config(Config): try: check_requirements("saml2") except DependencyException as e: - raise ConfigError(e.message) + raise ConfigError( + e.message # noqa: B306, DependencyException.message is a property + ) self.saml2_enabled = True diff --git a/synapse/config/tracer.py b/synapse/config/tracer.py index 0c1a854f09..727a1e7008 100644 --- a/synapse/config/tracer.py +++ b/synapse/config/tracer.py @@ -39,7 +39,9 @@ class TracerConfig(Config): try: check_requirements("opentracing") except DependencyException as e: - raise ConfigError(e.message) + raise ConfigError( + e.message # noqa: B306, DependencyException.message is a property + ) # The tracer is enabled so sanitize the config diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index 14b21796d9..4ca13011e5 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -219,7 +219,7 @@ class SSLClientConnectionCreator: # ... and we also gut-wrench a '_synapse_tls_verifier' attribute into the # tls_protocol so that the SSL context's info callback has something to # call to do the cert verification. - setattr(tls_protocol, "_synapse_tls_verifier", self._verifier) + tls_protocol._synapse_tls_verifier = self._verifier return connection diff --git a/tests/unittest.py b/tests/unittest.py index ca7031c724..224f037ce1 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -140,7 +140,7 @@ class TestCase(unittest.TestCase): try: self.assertEquals(attrs[key], getattr(obj, key)) except AssertionError as e: - raise (type(e))(e.message + " for '.%s'" % key) + raise (type(e))("Assert error for '.{}':".format(key)) from e def assert_dict(self, required, actual): """Does a partial assert of a dict. -- cgit 1.5.1 From b449af0379db871945f32a572883d47b5c9018a3 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 17 Mar 2021 07:14:39 -0400 Subject: Add type hints to the room member handler. (#9631) --- changelog.d/9631.misc | 1 + synapse/handlers/register.py | 4 ++-- synapse/handlers/room_member.py | 4 ++++ synapse/handlers/room_member_worker.py | 10 ++++++++-- synapse/server.py | 4 ++-- 5 files changed, 17 insertions(+), 6 deletions(-) create mode 100644 changelog.d/9631.misc diff --git a/changelog.d/9631.misc b/changelog.d/9631.misc new file mode 100644 index 0000000000..35338cd332 --- /dev/null +++ b/changelog.d/9631.misc @@ -0,0 +1 @@ +Add additional type hints to the Homeserver object. diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 1abc8875cb..d7f226d589 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -437,10 +437,10 @@ class RegistrationHandler(BaseHandler): if RoomAlias.is_valid(r): ( - room_id, + room, remote_room_hosts, ) = await room_member_handler.lookup_room_alias(room_alias) - room_id = room_id.to_string() + room_id = room.to_string() else: raise SynapseError( 400, "%s was not legal room ID or room alias" % (r,) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 1660921306..4d20ed8357 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -155,6 +155,10 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): """ raise NotImplementedError() + @abc.abstractmethod + async def forget(self, user: UserID, room_id: str) -> None: + raise NotImplementedError() + def ratelimit_invite(self, room_id: Optional[str], invitee_user_id: str): """Ratelimit invites by room and by target user. diff --git a/synapse/handlers/room_member_worker.py b/synapse/handlers/room_member_worker.py index 108730a7a1..d75506c75e 100644 --- a/synapse/handlers/room_member_worker.py +++ b/synapse/handlers/room_member_worker.py @@ -14,7 +14,7 @@ # limitations under the License. import logging -from typing import List, Optional, Tuple +from typing import TYPE_CHECKING, List, Optional, Tuple from synapse.api.errors import SynapseError from synapse.handlers.room_member import RoomMemberHandler @@ -25,11 +25,14 @@ from synapse.replication.http.membership import ( ) from synapse.types import Requester, UserID +if TYPE_CHECKING: + from synapse.app.homeserver import HomeServer + logger = logging.getLogger(__name__) class RoomMemberWorkerHandler(RoomMemberHandler): - def __init__(self, hs): + def __init__(self, hs: "HomeServer"): super().__init__(hs) self._remote_join_client = ReplRemoteJoin.make_client(hs) @@ -83,3 +86,6 @@ class RoomMemberWorkerHandler(RoomMemberHandler): await self._notify_change_client( user_id=target.to_string(), room_id=room_id, change="left" ) + + async def forget(self, target: UserID, room_id: str) -> None: + raise RuntimeError("Cannot forget rooms on workers.") diff --git a/synapse/server.py b/synapse/server.py index 48ac87a124..dd4ee7dd3c 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -96,7 +96,7 @@ from synapse.handlers.room import ( RoomShutdownHandler, ) from synapse.handlers.room_list import RoomListHandler -from synapse.handlers.room_member import RoomMemberMasterHandler +from synapse.handlers.room_member import RoomMemberHandler, RoomMemberMasterHandler from synapse.handlers.room_member_worker import RoomMemberWorkerHandler from synapse.handlers.search import SearchHandler from synapse.handlers.set_password import SetPasswordHandler @@ -630,7 +630,7 @@ class HomeServer(metaclass=abc.ABCMeta): return ThirdPartyEventRules(self) @cache_in_self - def get_room_member_handler(self): + def get_room_member_handler(self) -> RoomMemberHandler: if self.config.worker_app: return RoomMemberWorkerHandler(self) return RoomMemberMasterHandler(self) -- cgit 1.5.1 From 567f88f835a55d2241cc129ac44b8b0dcedfa6e2 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 17 Mar 2021 12:33:18 +0000 Subject: Prep work for removing `outlier` from `internal_metadata` (#9411) * Populate `internal_metadata.outlier` based on `events` table Rather than relying on `outlier` being in the `internal_metadata` column, populate it based on the `events.outlier` column. * Move `outlier` out of InternalMetadata._dict Ultimately, this will allow us to stop writing it to the database. For now, we have to grandfather it back in so as to maintain compatibility with older versions of Synapse. --- changelog.d/9411.misc | 1 + synapse/events/__init__.py | 9 ++++++--- synapse/events/utils.py | 2 ++ synapse/replication/http/federation.py | 3 +++ synapse/replication/http/send_event.py | 4 +++- synapse/storage/databases/main/events.py | 19 +++++++++++++++++-- synapse/storage/databases/main/events_worker.py | 5 ++++- 7 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 changelog.d/9411.misc diff --git a/changelog.d/9411.misc b/changelog.d/9411.misc new file mode 100644 index 0000000000..c3e6cfa5f1 --- /dev/null +++ b/changelog.d/9411.misc @@ -0,0 +1 @@ +Preparatory steps for removing redundant `outlier` data from `event_json.internal_metadata` column. diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index 3ec4120f85..8f6b955d17 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -98,7 +98,7 @@ class DefaultDictProperty(DictProperty): class _EventInternalMetadata: - __slots__ = ["_dict", "stream_ordering"] + __slots__ = ["_dict", "stream_ordering", "outlier"] def __init__(self, internal_metadata_dict: JsonDict): # we have to copy the dict, because it turns out that the same dict is @@ -108,7 +108,10 @@ class _EventInternalMetadata: # the stream ordering of this event. None, until it has been persisted. self.stream_ordering = None # type: Optional[int] - outlier = DictProperty("outlier") # type: bool + # whether this event is an outlier (ie, whether we have the state at that point + # in the DAG) + self.outlier = False + out_of_band_membership = DictProperty("out_of_band_membership") # type: bool send_on_behalf_of = DictProperty("send_on_behalf_of") # type: str recheck_redaction = DictProperty("recheck_redaction") # type: bool @@ -129,7 +132,7 @@ class _EventInternalMetadata: return dict(self._dict) def is_outlier(self) -> bool: - return self._dict.get("outlier", False) + return self.outlier def is_out_of_band_membership(self) -> bool: """Whether this is an out of band membership, like an invite or an invite diff --git a/synapse/events/utils.py b/synapse/events/utils.py index 7ca5c9940a..5022e0fcb3 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -54,6 +54,8 @@ def prune_event(event: EventBase) -> EventBase: event.internal_metadata.stream_ordering ) + pruned_event.internal_metadata.outlier = event.internal_metadata.outlier + # Mark the event as redacted pruned_event.internal_metadata.redacted = True diff --git a/synapse/replication/http/federation.py b/synapse/replication/http/federation.py index 8af53b4f28..82ea3b895f 100644 --- a/synapse/replication/http/federation.py +++ b/synapse/replication/http/federation.py @@ -40,6 +40,7 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): // containing the event "event_format_version": .., // 1,2,3 etc: the event format version "internal_metadata": { .. serialized internal_metadata .. }, + "outlier": true|false, "rejected_reason": .., // The event.rejected_reason field "context": { .. serialized event context .. }, }], @@ -84,6 +85,7 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): "room_version": event.room_version.identifier, "event_format_version": event.format_version, "internal_metadata": event.internal_metadata.get_dict(), + "outlier": event.internal_metadata.is_outlier(), "rejected_reason": event.rejected_reason, "context": serialized_context, } @@ -116,6 +118,7 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): event = make_event_from_dict( event_dict, room_ver, internal_metadata, rejected_reason ) + event.internal_metadata.outlier = event_payload["outlier"] context = EventContext.deserialize( self.storage, event_payload["context"] diff --git a/synapse/replication/http/send_event.py b/synapse/replication/http/send_event.py index 8fa104c8d3..a4c5b44292 100644 --- a/synapse/replication/http/send_event.py +++ b/synapse/replication/http/send_event.py @@ -40,6 +40,7 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): // containing the event "event_format_version": .., // 1,2,3 etc: the event format version "internal_metadata": { .. serialized internal_metadata .. }, + "outlier": true|false, "rejected_reason": .., // The event.rejected_reason field "context": { .. serialized event context .. }, "requester": { .. serialized requester .. }, @@ -79,7 +80,6 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): ratelimit (bool) extra_users (list(UserID)): Any extra users to notify about event """ - serialized_context = await context.serialize(event, store) payload = { @@ -87,6 +87,7 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): "room_version": event.room_version.identifier, "event_format_version": event.format_version, "internal_metadata": event.internal_metadata.get_dict(), + "outlier": event.internal_metadata.is_outlier(), "rejected_reason": event.rejected_reason, "context": serialized_context, "requester": requester.serialize(), @@ -108,6 +109,7 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): event = make_event_from_dict( event_dict, room_ver, internal_metadata, rejected_reason ) + event.internal_metadata.outlier = content["outlier"] requester = Requester.deserialize(self.store, content["requester"]) context = EventContext.deserialize(self.storage, content["context"]) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index cd1ceac50e..98dac19a95 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -1270,8 +1270,10 @@ class PersistEventsStore: logger.exception("") raise + # update the stored internal_metadata to update the "outlier" flag. + # TODO: This is unused as of Synapse 1.31. Remove it once we are happy + # to drop backwards-compatibility with 1.30. metadata_json = json_encoder.encode(event.internal_metadata.get_dict()) - sql = "UPDATE event_json SET internal_metadata = ? WHERE event_id = ?" txn.execute(sql, (metadata_json, event.event_id)) @@ -1319,6 +1321,19 @@ class PersistEventsStore: d.pop("redacted_because", None) return d + def get_internal_metadata(event): + im = event.internal_metadata.get_dict() + + # temporary hack for database compatibility with Synapse 1.30 and earlier: + # store the `outlier` flag inside the internal_metadata json as well as in + # the `events` table, so that if anyone rolls back to an older Synapse, + # things keep working. This can be removed once we are happy to drop support + # for that + if event.internal_metadata.is_outlier(): + im["outlier"] = True + + return im + self.db_pool.simple_insert_many_txn( txn, table="event_json", @@ -1327,7 +1342,7 @@ class PersistEventsStore: "event_id": event.event_id, "room_id": event.room_id, "internal_metadata": json_encoder.encode( - event.internal_metadata.get_dict() + get_internal_metadata(event) ), "json": json_encoder.encode(event_dict(event)), "format_version": event.format_version, diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index c04e162ccc..952d4969b2 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -799,6 +799,7 @@ class EventsWorkerStore(SQLBaseStore): rejected_reason=rejected_reason, ) original_ev.internal_metadata.stream_ordering = row["stream_ordering"] + original_ev.internal_metadata.outlier = row["outlier"] event_map[event_id] = original_ev @@ -905,7 +906,8 @@ class EventsWorkerStore(SQLBaseStore): ej.json, ej.format_version, r.room_version, - rej.reason + rej.reason, + e.outlier FROM events AS e JOIN event_json AS ej USING (event_id) LEFT JOIN rooms r ON r.room_id = e.room_id @@ -929,6 +931,7 @@ class EventsWorkerStore(SQLBaseStore): "room_version_id": row[5], "rejected_reason": row[6], "redactions": [], + "outlier": row[7], } # check for redactions -- cgit 1.5.1 From ad721fc559b4af6140852adf58c93ae6ab0bf6b5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 17 Mar 2021 13:20:08 +0000 Subject: Fix bad naming of storage function (#9637) We had two functions named `get_forward_extremities_for_room` and `get_forward_extremeties_for_room` that took different paramters. We rename one of them to avoid confusion. --- changelog.d/9637.misc | 1 + synapse/handlers/device.py | 2 +- synapse/handlers/sync.py | 6 ++++-- synapse/storage/databases/main/event_federation.py | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 changelog.d/9637.misc diff --git a/changelog.d/9637.misc b/changelog.d/9637.misc new file mode 100644 index 0000000000..90a27d9f8f --- /dev/null +++ b/changelog.d/9637.misc @@ -0,0 +1 @@ +Rename storage function to fix spelling and not conflict with another functions name. diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index df3cdc8fba..6aa3f73eee 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -166,7 +166,7 @@ class DeviceWorkerHandler(BaseHandler): # Fetch the current state at the time. try: - event_ids = await self.store.get_forward_extremeties_for_room( + event_ids = await self.store.get_forward_extremities_for_room_at_stream_ordering( room_id, stream_ordering=stream_ordering ) except errors.StoreError: diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index f50257cd57..7b723ead58 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1979,8 +1979,10 @@ class SyncHandler: logger.info("User joined room after current token: %s", room_id) - extrems = await self.store.get_forward_extremeties_for_room( - room_id, event_pos.stream + extrems = ( + await self.store.get_forward_extremities_for_room_at_stream_ordering( + room_id, event_pos.stream + ) ) users_in_room = await self.state.get_current_users_in_room(room_id, extrems) if user_id in users_in_room: diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py index 332193ad1c..a956be491a 100644 --- a/synapse/storage/databases/main/event_federation.py +++ b/synapse/storage/databases/main/event_federation.py @@ -793,7 +793,7 @@ class EventFederationWorkerStore(EventsWorkerStore, SignatureWorkerStore, SQLBas return int(min_depth) if min_depth is not None else None - async def get_forward_extremeties_for_room( + async def get_forward_extremities_for_room_at_stream_ordering( self, room_id: str, stream_ordering: int ) -> List[str]: """For a given room_id and stream_ordering, return the forward -- cgit 1.5.1