Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Ensure that the OIDC remote ID is a string. #8190

Merged
merged 2 commits into from
Aug 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8190.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix logging in via OpenID Connect with a provider that uses integer user IDs.
3 changes: 3 additions & 0 deletions synapse/handlers/oidc_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,9 @@ async def _map_userinfo_to_user(
raise MappingException(
"Failed to extract subject from OIDC response: %s" % (e,)
)
# Some OIDC providers use integer IDs, but Synapse expects external IDs
# to be strings.
remote_user_id = str(remote_user_id)
Copy link
Member

Choose a reason for hiding this comment

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

won't this still cause problems due to numeric userids being reserved for guests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not believe so since the map_user_attributes call below returns what will be used for the localpart of the mxid.

The GitHub example given uses a string there (I think GItHub has an immutable integer ID for each user and then the string ID, e.g. clokep for me).

Pretty much this remote_user_id is only put into the user_external_ids table. I can add a comment to clarify that though.

Copy link
Member

Choose a reason for hiding this comment

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

ohh I see. Ok!

Copy link
Member Author

Choose a reason for hiding this comment

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

I clarified this slightly above (that this is used for the external ID).


logger.info(
"Looking for existing mapping for user %s:%s",
Expand Down
41 changes: 39 additions & 2 deletions tests/handlers/test_oidc.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,17 @@ def deliverBody(self, protocol):
COOKIE_NAME = b"oidc_session"
COOKIE_PATH = "/_synapse/oidc"

MockedMappingProvider = Mock(OidcMappingProvider)

class TestMappingProvider(OidcMappingProvider):
@staticmethod
def parse_config(config):
return

def get_remote_user_id(self, userinfo):
return userinfo["sub"]

async def map_user_attributes(self, userinfo, token):
return {"localpart": userinfo["username"], "display_name": None}


def simple_async_mock(return_value=None, raises=None):
Expand Down Expand Up @@ -123,7 +133,7 @@ def make_homeserver(self, reactor, clock):
oidc_config["issuer"] = ISSUER
oidc_config["scopes"] = SCOPES
oidc_config["user_mapping_provider"] = {
"module": __name__ + ".MockedMappingProvider"
"module": __name__ + ".TestMappingProvider",
}
config["oidc_config"] = oidc_config

Expand Down Expand Up @@ -580,3 +590,30 @@ def test_exchange_code(self):
with self.assertRaises(OidcError) as exc:
yield defer.ensureDeferred(self.handler._exchange_code(code))
self.assertEqual(exc.exception.error, "some_error")

def test_map_userinfo_to_user(self):
"""Ensure that mapping the userinfo returned from a provider to an MXID works properly."""
userinfo = {
"sub": "test_user",
"username": "test_user",
}
# The token doesn't matter with the default user mapping provider.
token = {}
mxid = self.get_success(
self.handler._map_userinfo_to_user(
userinfo, token, "user-agent", "10.10.10.10"
)
)
self.assertEqual(mxid, "@test_user:test")

# Some providers return an integer ID.
userinfo = {
"sub": 1234,
"username": "test_user_2",
}
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")