diff options
| -rw-r--r-- | UPGRADE.rst | 30 | ||||
| -rw-r--r-- | changelog.d/8774.misc | 1 | ||||
| -rw-r--r-- | docs/sso_mapping_providers.md | 9 | ||||
| -rw-r--r-- | synapse/handlers/oidc_handler.py | 25 | ||||
| -rw-r--r-- | synapse/handlers/saml_handler.py | 6 | ||||
| -rw-r--r-- | synapse/types.py | 6 | ||||
| -rw-r--r-- | tests/handlers/test_oidc.py | 89 |
7 files changed, 137 insertions, 29 deletions
diff --git a/UPGRADE.rst b/UPGRADE.rst index 7c19cf2a70..4de1bb5841 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 0000000000..57cca8fee5 --- /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 32b06aa2c5..707dd73978 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 be8562d47b..4bfd8d5617 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -38,7 +38,12 @@ from synapse.handlers._base import BaseHandler 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 @@ class OidcHandler(BaseHandler): "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 @@ class OidcHandler(BaseHandler): # 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 @@ class JinjaOidcMappingProvider(OidcMappingProvider[JinjaOidcMappingConfig]): ) -> 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 9bf430b656..5d9b555b13 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -31,6 +31,7 @@ from synapse.http.site import SynapseRequest from synapse.module_api import ModuleApi from synapse.types import ( UserID, + contains_invalid_mxid_characters, map_username_to_mxid_localpart, mxid_localpart_allowed_characters, ) @@ -318,6 +319,11 @@ class SamlHandler(BaseHandler): "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 66bb5bac8d..3ab6bdbe06 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -317,14 +317,14 @@ mxid_localpart_allowed_characters = set( ) -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 630e6da808..b4fa02acc4 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 @@ import pymacaroons 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 @@ class OidcHandlerTestCase(HomeserverTestCase): 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 @@ class OidcHandlerTestCase(HomeserverTestCase): 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 @@ class OidcHandlerTestCase(HomeserverTestCase): 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öö") |