Add config option for always using userinfo endpoint

changelog.d/7658.feature

@@ -0,0 +1 @@
+Add config option for always using userinfo endpoint for OpenID Connect with Gitlab. Contributed by Benjamin Koch.

synapse/config/oidc_config.py

@@ -56,6 +56,7 @@ def read_config(self, config, **kwargs):
         self.oidc_userinfo_endpoint = oidc_config.get("userinfo_endpoint")
         self.oidc_jwks_uri = oidc_config.get("jwks_uri")
         self.oidc_skip_verification = oidc_config.get("skip_verification", False)
+        self.oidc_user_profile_method = oidc_config.get("user_profile_method", "auto")
 
         ump_config = oidc_config.get("user_mapping_provider", {})
         ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER)

synapse/handlers/oidc_handler.py

@@ -94,6 +94,7 @@ class OidcHandler:
     def __init__(self, hs: HomeServer):
         self._callback_url = hs.config.oidc_callback_url  # type: str
         self._scopes = hs.config.oidc_scopes  # type: List[str]
+        self._user_profile_method = hs.config.oidc_user_profile_method  # type: str
         self._client_auth = ClientAuth(
             hs.config.oidc_client_id,
             hs.config.oidc_client_secret,
@@ -201,10 +202,10 @@ def _validate_metadata(self):
                 )
 
         # If the openid scope was not requested, we need a userinfo endpoint to fetch user infos
-        if self._uses_userinfo:
+        if self._user_profile_method == "userinfo_endpoint":
             if m.get("userinfo_endpoint") is None:
                 raise ValueError(
-                    'provider has no "userinfo_endpoint", even though it is required because the "openid" scope is not requested'
+                    'provider has no "userinfo_endpoint", even though it is required because user_profile_method requests it'
                 )
         else:
             # If we're not using userinfo, we need a valid jwks to validate the ID token
@@ -214,18 +215,27 @@ def _validate_metadata(self):
                 else:
                     raise ValueError('"jwks_uri" must be set')
 
-    @property
-    def _uses_userinfo(self) -> bool:
+    def _uses_userinfo(self, token) -> bool:
         """Returns True if the ``userinfo_endpoint`` should be used.
 
-        This is based on the requested scopes: if the scopes include
-        ``openid``, the provider should give use an ID token containing the
-        user informations. If not, we should fetch them using the
-        ``access_token`` with the ``userinfo_endpoint``.
+        This is based on user_profile_method: if it is ``id_token``,
+        the provider should use an ID token containing the
+        user informations. If it is ``userinfo_endpoint``, we should
+        fetch them using the ``access_token`` with the ``userinfo_endpoint``.
+        If it is ``auto``, we use the ``id_token`` if present in the token
+        response, and fallback to the ``userinfo_endpoint``.
         """
 
-        # Maybe that should be user-configurable and not inferred?
-        return "openid" not in self._scopes
+        if self._user_profile_method == "userinfo_endpoint":
+            return True
+        elif self._user_profile_method == "id_token":
+            return False
+        elif self._user_profile_method == "auto":
+            return "id_token" not in token
+        else:
+            raise ValueError(
+                'user_profile_method must be one of "userinfo_endpoint", "id_token", or "auto"'
+            )
 
     async def load_metadata(self) -> OpenIDProviderMetadata:
         """Load and validate the provider metadata.
@@ -252,7 +262,7 @@ async def load_metadata(self) -> OpenIDProviderMetadata:
 
         return self._provider_metadata
 
-    async def load_jwks(self, force: bool = False) -> JWKS:
+    async def load_jwks(self, token=None, force: bool = False) -> JWKS:
         """Load the JSON Web Key Set used to sign ID tokens.
 
         If we're not using the ``userinfo_endpoint``, user infos are extracted
@@ -280,7 +290,7 @@ async def load_jwks(self, force: bool = False) -> JWKS:
                     ]
                 }
         """
-        if self._uses_userinfo:
+        if token is not None and self._uses_userinfo(token):
             # We're not using jwt signing, return an empty jwk set
             return {"keys": []}
 
@@ -438,6 +448,13 @@ async def _fetch_userinfo(self, token: Token) -> UserInfo:
         """
         metadata = await self.load_metadata()
 
+        # We do check this in load_metadata but we have to check again because
+        # load_metadata cannot handle the "auto" case.
+        if metadata.get("userinfo_endpoint") is None:
+            raise ValueError(
+                'provider has no "userinfo_endpoint", even though it is required because user_profile_method requests it'
+            )
+
         resp = await self._http_client.get_json(
             metadata["userinfo_endpoint"],
             headers={"Authorization": ["Bearer {}".format(token["access_token"])]},
@@ -478,7 +495,7 @@ async def _parse_id_token(self, token: Token, nonce: str) -> UserInfo:
 
         # Try to decode the keys in cache first, then retry by forcing the keys
         # to be reloaded
-        jwk_set = await self.load_jwks()
+        jwk_set = await self.load_jwks(token)
         try:
             claims = jwt.decode(
                 token["id_token"],
@@ -489,7 +506,7 @@ async def _parse_id_token(self, token: Token, nonce: str) -> UserInfo:
             )
         except ValueError:
             logger.info("Reloading JWKS after decode error")
-            jwk_set = await self.load_jwks(force=True)  # try reloading the jwks
+            jwk_set = await self.load_jwks(token, force=True)  # try reloading the jwks
             claims = jwt.decode(
                 token["id_token"],
                 key=jwk_set,
@@ -678,7 +695,7 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
 
         # Now that we have a token, get the userinfo, either by decoding the
         # `id_token` or by fetching the `userinfo_endpoint`.
-        if self._uses_userinfo:
+        if self._uses_userinfo(token):
             logger.debug("Fetching userinfo")
             try:
                 userinfo = await self._fetch_userinfo(token)

tests/handlers/test_oidc.py

@@ -185,29 +185,30 @@ def test_no_discovery(self):
     @defer.inlineCallbacks
     def test_load_jwks(self):
         """JWKS loading is done once (then cached) if used."""
-        jwks = yield defer.ensureDeferred(self.handler.load_jwks())
+        token = {"id_token": "dummy"}
+        jwks = yield defer.ensureDeferred(self.handler.load_jwks(token))
         self.http_client.get_json.assert_called_once_with(JWKS_URI)
         self.assertEqual(jwks, {"keys": []})
 
         # subsequent calls should be cached…
         self.http_client.reset_mock()
-        yield defer.ensureDeferred(self.handler.load_jwks())
+        yield defer.ensureDeferred(self.handler.load_jwks(token))
         self.http_client.get_json.assert_not_called()
 
         # …unless forced
         self.http_client.reset_mock()
-        yield defer.ensureDeferred(self.handler.load_jwks(force=True))
+        yield defer.ensureDeferred(self.handler.load_jwks(token, force=True))
         self.http_client.get_json.assert_called_once_with(JWKS_URI)
 
         # Throw if the JWKS uri is missing
         with self.metadata_edit({"jwks_uri": None}):
             with self.assertRaises(RuntimeError):
-                yield defer.ensureDeferred(self.handler.load_jwks(force=True))
+                yield defer.ensureDeferred(self.handler.load_jwks(token, force=True))
 
         # Return empty key set if JWKS are not used
-        self.handler._scopes = []  # not asking the openid scope
+        self.handler._user_profile_method = "userinfo_endpoint"
         self.http_client.get_json.reset_mock()
-        jwks = yield defer.ensureDeferred(self.handler.load_jwks(force=True))
+        jwks = yield defer.ensureDeferred(self.handler.load_jwks(token, force=True))
         self.http_client.get_json.assert_not_called()
         self.assertEqual(jwks, {"keys": []})
 
@@ -271,11 +272,25 @@ def test_validate_config(self):
             )
 
         # Tests for configs that the userinfo endpoint
-        self.assertFalse(h._uses_userinfo)
-        h._scopes = []  # do not request the openid scope
-        self.assertTrue(h._uses_userinfo)
+        token1 = {}
+        token2 = {"id_token": "dummy"}
+        self.assertTrue(h._uses_userinfo(token1))
+        self.assertFalse(h._uses_userinfo(token2))
+        h._validate_metadata()
+        h._user_profile_method = "userinfo_endpoint"
+        self.assertTrue(h._uses_userinfo(token1))
+        self.assertTrue(h._uses_userinfo(token2))
         self.assertRaisesRegex(ValueError, "userinfo_endpoint", h._validate_metadata)
+        h._user_profile_method = "id_token"
+        self.assertFalse(h._uses_userinfo(token1))
+        self.assertFalse(h._uses_userinfo(token2))
+        h._validate_metadata()
+        h._user_profile_method = "invalid"
+        self.assertRaisesRegex(
+            ValueError, "user_profile_method must be one of", h._uses_userinfo, token1
+        )
 
+        h._user_profile_method = "userinfo_endpoint"
         with self.metadata_edit(
             {"userinfo_endpoint": USERINFO_ENDPOINT, "jwks_uri": None}
         ):
@@ -423,7 +438,7 @@ def test_callback(self):
         self.handler._fetch_userinfo.reset_mock()
 
         # With userinfo fetching
-        self.handler._scopes = []  # do not ask the "openid" scope
+        self.handler._user_profile_method = "userinfo_endpoint"
         yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(