diff --git a/UPGRADE.rst b/UPGRADE.rst index 960c2aeb2bdc..2a7ee305bd8e 100644 --- a/UPGRADE.rst +++ b/UPGRADE.rst @@ -75,6 +75,36 @@ for example: wget https://packages.matrix.org/debian/pool/main/m/matrix-synapse-py3/matrix-synapse-py3_1.3.0+stretch1_amd64.deb dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb +Upgrading to v1.24.0 +==================== + +Custom OpenID Connect mapping provider breaking change +------------------------------------------------------ + +This release allows the OpenID Connect mapping provider to perform normalisation +of the localpart of the Matrix ID. This allows for the mapping provider to +specify different algorithms, instead of the [default way](https://matrix.org/docs/spec/appendices#mapping-from-other-character-sets). + +If your Synapse configuration uses a custom mapping provider +(`oidc_config.user_mapping_provider.module` is specified and not equal to +`synapse.handlers.oidc_handler.JinjaOidcMappingProvider`) then you *must* ensure +that `map_user_attributes` of the mapping provider performs some normalisation +of the `localpart` returned. To match previous behaviour you can use the +`map_username_to_mxid_localpart` function provided by Synapse. An example is +shown below: + +.. code-block:: python + + from synapse.types import map_username_to_mxid_localpart + + class MyMappingProvider: + def map_user_attributes(self, userinfo, token): + # ... your custom logic ... + sso_user_id = ... + localpart = map_username_to_mxid_localpart(sso_user_id) + + return {"localpart": localpart} + Upgrading to v1.23.0 ==================== diff --git a/changelog.d/8774.misc b/changelog.d/8774.misc new file mode 100644 index 000000000000..57cca8fee51c --- /dev/null +++ b/changelog.d/8774.misc @@ -0,0 +1 @@ +Add additional error checking for OpenID Connect and SAML mapping providers. diff --git a/docs/sso_mapping_providers.md b/docs/sso_mapping_providers.md index 32b06aa2c570..707dd7397888 100644 --- a/docs/sso_mapping_providers.md +++ b/docs/sso_mapping_providers.md @@ -15,8 +15,15 @@ where SAML mapping providers come into play. SSO mapping providers are currently supported for OpenID and SAML SSO configurations. Please see the details below for how to implement your own. +It is the responsibility of the mapping provider to normalise the SSO attributes +and map them to a valid Matrix ID. The +[specification for Matrix IDs](https://matrix.org/docs/spec/appendices#user-identifiers) +has some information about what is considered valid. Alternately an easy way to +ensure it is valid is to use a Synapse utility function: +`synapse.types.map_username_to_mxid_localpart`. + External mapping providers are provided to Synapse in the form of an external -Python module. You can retrieve this module from [PyPi](https://pypi.org) or elsewhere, +Python module. You can retrieve this module from [PyPI](https://pypi.org) or elsewhere, but it must be importable via Synapse (e.g. it must be in the same virtualenv as Synapse). The Synapse config is then modified to point to the mapping provider (and optionally provide additional configuration for it). diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index be8562d47b63..4bfd8d5617be 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -38,7 +38,12 @@ from synapse.handlers.sso import MappingException from synapse.http.site import SynapseRequest from synapse.logging.context import make_deferred_yieldable -from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart +from synapse.types import ( + JsonDict, + UserID, + contains_invalid_mxid_characters, + map_username_to_mxid_localpart, +) from synapse.util import json_decoder if TYPE_CHECKING: @@ -885,10 +890,12 @@ async def _map_userinfo_to_user( "Retrieved user attributes from user mapping provider: %r", attributes ) - if not attributes["localpart"]: - raise MappingException("localpart is empty") - - localpart = map_username_to_mxid_localpart(attributes["localpart"]) + localpart = attributes["localpart"] + if not localpart: + raise MappingException( + "Error parsing OIDC response: OIDC mapping provider plugin " + "did not return a localpart value" + ) user_id = UserID(localpart, self.server_name).to_string() users = await self.store.get_users_by_id_case_insensitive(user_id) @@ -908,6 +915,11 @@ async def _map_userinfo_to_user( # This mxid is taken raise MappingException("mxid '{}' is already taken".format(user_id)) else: + # Since the localpart is provided via a potentially untrusted module, + # ensure the MXID is valid before registering. + if contains_invalid_mxid_characters(localpart): + raise MappingException("localpart is invalid: %s" % (localpart,)) + # It's the first time this user is logging in and the mapped mxid was # not taken, register the user registered_user_id = await self._registration_handler.register_user( @@ -1076,6 +1088,9 @@ async def map_user_attributes( ) -> UserAttribute: localpart = self._config.localpart_template.render(user=userinfo).strip() + # Ensure only valid characters are included in the MXID. + localpart = map_username_to_mxid_localpart(localpart) + display_name = None # type: Optional[str] if self._config.display_name_template is not None: display_name = self._config.display_name_template.render( diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index aee772239a55..21b2c29c72ce 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -31,6 +31,7 @@ from synapse.module_api import ModuleApi from synapse.types import ( UserID, + contains_invalid_mxid_characters, map_username_to_mxid_localpart, mxid_localpart_allowed_characters, ) @@ -317,6 +318,11 @@ async def _map_saml_response_to_user( "Unable to generate a Matrix ID from the SAML response" ) + # Since the localpart is provided via a potentially untrusted module, + # ensure the MXID is valid before registering. + if contains_invalid_mxid_characters(localpart): + raise MappingException("localpart is invalid: %s" % (localpart,)) + logger.info("Mapped SAML user to local part %s", localpart) registered_user_id = await self._registration_handler.register_user( localpart=localpart, diff --git a/synapse/types.py b/synapse/types.py index 66bb5bac8dc2..3ab6bdbe0626 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -317,14 +317,14 @@ def from_string(cls: Type[DS], s: str) -> DS: ) -def contains_invalid_mxid_characters(localpart): +def contains_invalid_mxid_characters(localpart: str) -> bool: """Check for characters not allowed in an mxid or groupid localpart Args: - localpart (basestring): the localpart to be checked + localpart: the localpart to be checked Returns: - bool: True if there are any naughty characters + True if there are any naughty characters """ return any(c not in mxid_localpart_allowed_characters for c in localpart) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 630e6da808ef..b4fa02acc486 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -12,7 +12,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - import json from urllib.parse import parse_qs, urlparse @@ -24,12 +23,8 @@ from twisted.python.failure import Failure from twisted.web._newclient import ResponseDone -from synapse.handlers.oidc_handler import ( - MappingException, - OidcError, - OidcHandler, - OidcMappingProvider, -) +from synapse.handlers.oidc_handler import OidcError, OidcHandler, OidcMappingProvider +from synapse.handlers.sso import MappingException from synapse.types import UserID from tests.unittest import HomeserverTestCase, override_config @@ -132,14 +127,13 @@ def make_homeserver(self, reactor, clock): config = self.default_config() config["public_baseurl"] = BASE_URL - oidc_config = {} - oidc_config["enabled"] = True - oidc_config["client_id"] = CLIENT_ID - oidc_config["client_secret"] = CLIENT_SECRET - oidc_config["issuer"] = ISSUER - oidc_config["scopes"] = SCOPES - oidc_config["user_mapping_provider"] = { - "module": __name__ + ".TestMappingProvider", + oidc_config = { + "enabled": True, + "client_id": CLIENT_ID, + "client_secret": CLIENT_SECRET, + "issuer": ISSUER, + "scopes": SCOPES, + "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"}, } # Update this config with what's in the default config so that @@ -705,13 +699,13 @@ def test_map_userinfo_to_user(self): def test_map_userinfo_to_existing_user(self): """Existing users can log in with OpenID Connect when allow_existing_users is True.""" store = self.hs.get_datastore() - user4 = UserID.from_string("@test_user_4:test") + user = UserID.from_string("@test_user:test") self.get_success( - store.register_user(user_id=user4.to_string(), password_hash=None) + store.register_user(user_id=user.to_string(), password_hash=None) ) userinfo = { - "sub": "test4", - "username": "test_user_4", + "sub": "test", + "username": "test_user", } token = {} mxid = self.get_success( @@ -719,4 +713,59 @@ def test_map_userinfo_to_existing_user(self): userinfo, token, "user-agent", "10.10.10.10" ) ) - self.assertEqual(mxid, "@test_user_4:test") + self.assertEqual(mxid, "@test_user:test") + + # Register some non-exact matching cases. + user2 = UserID.from_string("@TEST_user_2:test") + self.get_success( + store.register_user(user_id=user2.to_string(), password_hash=None) + ) + user2_caps = UserID.from_string("@test_USER_2:test") + self.get_success( + store.register_user(user_id=user2_caps.to_string(), password_hash=None) + ) + + # Attempting to login without matching a name exactly is an error. + userinfo = { + "sub": "test2", + "username": "TEST_USER_2", + } + e = self.get_failure( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ), + MappingException, + ) + self.assertTrue( + str(e.value).startswith( + "Attempted to login as '@TEST_USER_2:test' but it matches more than one user inexactly:" + ) + ) + + # Logging in when matching a name exactly should work. + user2 = UserID.from_string("@TEST_USER_2:test") + self.get_success( + store.register_user(user_id=user2.to_string(), password_hash=None) + ) + + mxid = self.get_success( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ) + ) + self.assertEqual(mxid, "@TEST_USER_2:test") + + def test_map_userinfo_to_invalid_localpart(self): + """If the mapping provider generates an invalid localpart it should be rejected.""" + userinfo = { + "sub": "test2", + "username": "föö", + } + token = {} + e = self.get_failure( + self.handler._map_userinfo_to_user( + userinfo, token, "user-agent", "10.10.10.10" + ), + MappingException, + ) + self.assertEqual(str(e.value), "localpart is invalid: föö")