From 3c22997f8424b645a5ad6911220aaffc9161e14c Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 9 Jun 2020 00:27:39 +0200
Subject: [PATCH 01/16] Add config option for always using userinfo endpoint

---
 synapse/config/oidc_config.py    | 6 ++++++
 synapse/handlers/oidc_handler.py | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index e24dd637bc65..19f80e25b1ce 100644
--- a/synapse/config/oidc_config.py
+++ b/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_uses_userinfo = oidc_config.get("uses_userinfo", False)
 
         ump_config = oidc_config.get("user_mapping_provider", {})
         ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER)
@@ -158,6 +159,11 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           #
           #skip_verification: true
 
+          # Always use userinfo endpoint. Required for providers that don't include user
+          # information in the token response, e.g. Gitlab.
+          #
+          #uses_userinfo: false
+
           # An external module can be provided here as a custom solution to mapping
           # attributes returned from a OIDC provider onto a matrix user.
           #
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 9c08eb53994b..3180b94789b9 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/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._uses_userinfo_config = hs.config.oidc_uses_userinfo  # type: bool
         self._client_auth = ClientAuth(
             hs.config.oidc_client_id,
             hs.config.oidc_client_secret,
@@ -224,8 +225,7 @@ def _uses_userinfo(self) -> bool:
         ``access_token`` with the ``userinfo_endpoint``.
         """
 
-        # Maybe that should be user-configurable and not inferred?
-        return "openid" not in self._scopes
+        return self._uses_userinfo_config or "openid" not in self._scopes
 
     async def load_metadata(self) -> OpenIDProviderMetadata:
         """Load and validate the provider metadata.

From 1a40dd2eea7d20d5ccc3524ba16bff4a6f1db4c3 Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 9 Jun 2020 00:34:45 +0200
Subject: [PATCH 02/16] Add and adjust sample config to match code style

---
 docs/sample_config.yaml       | 5 +++++
 synapse/config/oidc_config.py | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index b06394a2bd67..5b3cd8637b55 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1601,6 +1601,11 @@ oidc_config:
   #
   #skip_verification: true
 
+  # Always use userinfo endpoint. Required for providers that don't include
+  # user information in the token response, e.g. Gitlab.
+  #
+  #uses_userinfo: true
+
   # An external module can be provided here as a custom solution to mapping
   # attributes returned from a OIDC provider onto a matrix user.
   #
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index 19f80e25b1ce..b56b2d93570e 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -159,10 +159,10 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           #
           #skip_verification: true
 
-          # Always use userinfo endpoint. Required for providers that don't include user
-          # information in the token response, e.g. Gitlab.
+          # Always use userinfo endpoint. Required for providers that don't include
+          # user information in the token response, e.g. Gitlab.
           #
-          #uses_userinfo: false
+          #uses_userinfo: true
 
           # An external module can be provided here as a custom solution to mapping
           # attributes returned from a OIDC provider onto a matrix user.

From d1b73aec1cdca0156071b0bb8dbd2fb51947f93b Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 9 Jun 2020 00:44:19 +0200
Subject: [PATCH 03/16] Add changelog

---
 changelog.d/7658.feature | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/7658.feature

diff --git a/changelog.d/7658.feature b/changelog.d/7658.feature
new file mode 100644
index 000000000000..6a3875bd7d39
--- /dev/null
+++ b/changelog.d/7658.feature
@@ -0,0 +1 @@
+Add config option for always using userinfo endpoint for OpenID Connect with Gitlab. Contributed by Benjamin Koch.

From 099ed8dd033e7d6fe39fc441ad2d1548cad6fb2e Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 30 Jun 2020 21:19:00 +0000
Subject: [PATCH 04/16] Revert "Add and adjust sample config to match code
 style"

This reverts commit 1a40dd2eea7d20d5ccc3524ba16bff4a6f1db4c3.
---
 docs/sample_config.yaml       | 5 -----
 synapse/config/oidc_config.py | 6 +++---
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 5b3cd8637b55..b06394a2bd67 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1601,11 +1601,6 @@ oidc_config:
   #
   #skip_verification: true
 
-  # Always use userinfo endpoint. Required for providers that don't include
-  # user information in the token response, e.g. Gitlab.
-  #
-  #uses_userinfo: true
-
   # An external module can be provided here as a custom solution to mapping
   # attributes returned from a OIDC provider onto a matrix user.
   #
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index b56b2d93570e..19f80e25b1ce 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -159,10 +159,10 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           #
           #skip_verification: true
 
-          # Always use userinfo endpoint. Required for providers that don't include
-          # user information in the token response, e.g. Gitlab.
+          # Always use userinfo endpoint. Required for providers that don't include user
+          # information in the token response, e.g. Gitlab.
           #
-          #uses_userinfo: true
+          #uses_userinfo: false
 
           # An external module can be provided here as a custom solution to mapping
           # attributes returned from a OIDC provider onto a matrix user.

From 960dfc11e2b5cf01728d30052e99612c720c4fe0 Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 30 Jun 2020 21:19:16 +0000
Subject: [PATCH 05/16] Revert "Add config option for always using userinfo
 endpoint"

This reverts commit 3c22997f8424b645a5ad6911220aaffc9161e14c.
---
 synapse/config/oidc_config.py    | 6 ------
 synapse/handlers/oidc_handler.py | 4 ++--
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index 19f80e25b1ce..e24dd637bc65 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -56,7 +56,6 @@ 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_uses_userinfo = oidc_config.get("uses_userinfo", False)
 
         ump_config = oidc_config.get("user_mapping_provider", {})
         ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER)
@@ -159,11 +158,6 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           #
           #skip_verification: true
 
-          # Always use userinfo endpoint. Required for providers that don't include user
-          # information in the token response, e.g. Gitlab.
-          #
-          #uses_userinfo: false
-
           # An external module can be provided here as a custom solution to mapping
           # attributes returned from a OIDC provider onto a matrix user.
           #
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 3180b94789b9..9c08eb53994b 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -94,7 +94,6 @@ 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._uses_userinfo_config = hs.config.oidc_uses_userinfo  # type: bool
         self._client_auth = ClientAuth(
             hs.config.oidc_client_id,
             hs.config.oidc_client_secret,
@@ -225,7 +224,8 @@ def _uses_userinfo(self) -> bool:
         ``access_token`` with the ``userinfo_endpoint``.
         """
 
-        return self._uses_userinfo_config or "openid" not in self._scopes
+        # Maybe that should be user-configurable and not inferred?
+        return "openid" not in self._scopes
 
     async def load_metadata(self) -> OpenIDProviderMetadata:
         """Load and validate the provider metadata.

From 3bf5c6597bb7a7a0788f23be792bb70e5f7c51eb Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 30 Jun 2020 22:17:13 +0000
Subject: [PATCH 06/16] add option user_profile_method for OpenID Connect

This has been suggested as a replacement for uses_userinfo:
see https://github.com/matrix-org/synapse/pull/7658#pullrequestreview-428855439
---
 synapse/config/oidc_config.py    |  1 +
 synapse/handlers/oidc_handler.py | 48 ++++++++++++++++++++++----------
 tests/handlers/test_oidc.py      | 38 ++++++++++++++++++-------
 3 files changed, 62 insertions(+), 25 deletions(-)

diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index e24dd637bc65..b93a6c628073 100644
--- a/synapse/config/oidc_config.py
+++ b/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)
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 9c08eb53994b..4cd5c0afa6b4 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/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,28 @@ 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:
+            #TODO We should probably check this earlier.
+            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 +263,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, 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 +291,7 @@ async def load_jwks(self, force: bool = False) -> JWKS:
                     ]
                 }
         """
-        if self._uses_userinfo:
+        if self._uses_userinfo(token):
             # We're not using jwt signing, return an empty jwk set
             return {"keys": []}
 
@@ -438,6 +449,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 +496,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 +507,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 +696,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)
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 1bb25ab6842c..dae95b9bbf27 100644
--- a/tests/handlers/test_oidc.py
+++ b/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,28 @@ 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 +441,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(

From cc63f82475a85a98d347fd0ac41a0c3f3cbd737d Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 30 Jun 2020 22:28:28 +0000
Subject: [PATCH 07/16] fix style errors

---
 synapse/handlers/oidc_handler.py | 1 -
 tests/handlers/test_oidc.py      | 5 +----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 4cd5c0afa6b4..1a7a0d059b87 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -233,7 +233,6 @@ def _uses_userinfo(self, token) -> bool:
         elif self._user_profile_method == "auto":
             return "id_token" not in token
         else:
-            #TODO We should probably check this earlier.
             raise ValueError(
                 'user_profile_method must be one of "userinfo_endpoint", "id_token", or "auto"'
             )
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index dae95b9bbf27..edfd49daf482 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -287,10 +287,7 @@ def test_validate_config(self):
         h._validate_metadata()
         h._user_profile_method = "invalid"
         self.assertRaisesRegex(
-            ValueError,
-            "user_profile_method must be one of",
-            h._uses_userinfo,
-            token1
+            ValueError, "user_profile_method must be one of", h._uses_userinfo, token1
         )
 
         h._user_profile_method = "userinfo_endpoint"

From 450bee6cde924ee260a881408dd4ac00fe5ff967 Mon Sep 17 00:00:00 2001
From: Benjamin Koch <bbbsnowball@gmail.com>
Date: Tue, 30 Jun 2020 22:59:22 +0000
Subject: [PATCH 08/16] undo breaking change to signature of public method
 load_jwks

---
 synapse/handlers/oidc_handler.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 1a7a0d059b87..406505c83804 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -262,7 +262,7 @@ async def load_metadata(self) -> OpenIDProviderMetadata:
 
         return self._provider_metadata
 
-    async def load_jwks(self, token, 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
@@ -290,7 +290,7 @@ async def load_jwks(self, token, force: bool = False) -> JWKS:
                     ]
                 }
         """
-        if self._uses_userinfo(token):
+        if token is not None and self._uses_userinfo(token):
             # We're not using jwt signing, return an empty jwk set
             return {"keys": []}
 

From 951d580f81a2431badbf5f03044dcc8bc773d394 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 22 Sep 2020 17:19:10 -0400
Subject: [PATCH 09/16] Revert 'auto' back to the previous behavior.

---
 synapse/handlers/oidc_handler.py | 50 ++++++++++++--------------------
 tests/handlers/test_oidc.py      | 35 +++++++---------------
 2 files changed, 28 insertions(+), 57 deletions(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 21b6f6812d5e..f06a866cd82f 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -196,11 +196,11 @@ def _validate_metadata(self):
                     % (m["response_types_supported"],)
                 )
 
-        # If the openid scope was not requested, we need a userinfo endpoint to fetch user infos
-        if self._user_profile_method == "userinfo_endpoint":
+        # Ensure there's a userinfo endpoint to fetch from if it is requred.
+        if self._uses_userinfo:
             if m.get("userinfo_endpoint") is None:
                 raise ValueError(
-                    'provider has no "userinfo_endpoint", even though it is required because user_profile_method requests it'
+                    'provider has no "userinfo_endpoint", even though it is required'
                 )
         else:
             # If we're not using userinfo, we need a valid jwks to validate the ID token
@@ -210,27 +210,20 @@ def _validate_metadata(self):
                 else:
                     raise ValueError('"jwks_uri" must be set')
 
-    def _uses_userinfo(self, token) -> bool:
+    @property
+    def _uses_userinfo(self) -> bool:
         """Returns True if the ``userinfo_endpoint`` should be used.
 
-        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``.
+        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``.
         """
 
-        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"'
-            )
+        return (
+            "openid" not in self._scopes
+            or self._user_profile_method == "userinfo_endpoint"
+        )
 
     async def load_metadata(self) -> OpenIDProviderMetadata:
         """Load and validate the provider metadata.
@@ -257,7 +250,7 @@ async def load_metadata(self) -> OpenIDProviderMetadata:
 
         return self._provider_metadata
 
-    async def load_jwks(self, token=None, force: bool = False) -> JWKS:
+    async def load_jwks(self, 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
@@ -285,7 +278,7 @@ async def load_jwks(self, token=None, force: bool = False) -> JWKS:
                     ]
                 }
         """
-        if token is not None and self._uses_userinfo(token):
+        if self._uses_userinfo:
             # We're not using jwt signing, return an empty jwk set
             return {"keys": []}
 
@@ -443,13 +436,6 @@ 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"])]},
@@ -490,7 +476,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(token)
+        jwk_set = await self.load_jwks()
         try:
             claims = jwt.decode(
                 token["id_token"],
@@ -501,7 +487,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(token, force=True)  # try reloading the jwks
+            jwk_set = await self.load_jwks(force=True)  # try reloading the jwks
             claims = jwt.decode(
                 token["id_token"],
                 key=jwk_set,
@@ -690,7 +676,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(token):
+        if self._uses_userinfo:
             logger.debug("Fetching userinfo")
             try:
                 userinfo = await self._fetch_userinfo(token)
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 5478ff796c1d..89ec5fcb31bb 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -195,30 +195,29 @@ def test_no_discovery(self):
     @defer.inlineCallbacks
     def test_load_jwks(self):
         """JWKS loading is done once (then cached) if used."""
-        token = {"id_token": "dummy"}
-        jwks = yield defer.ensureDeferred(self.handler.load_jwks(token))
+        jwks = yield defer.ensureDeferred(self.handler.load_jwks())
         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(token))
+        yield defer.ensureDeferred(self.handler.load_jwks())
         self.http_client.get_json.assert_not_called()
 
         # …unless forced
         self.http_client.reset_mock()
-        yield defer.ensureDeferred(self.handler.load_jwks(token, force=True))
+        yield defer.ensureDeferred(self.handler.load_jwks(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(token, force=True))
+                yield defer.ensureDeferred(self.handler.load_jwks(force=True))
 
         # Return empty key set if JWKS are not used
-        self.handler._user_profile_method = "userinfo_endpoint"
+        self.handler._scopes = []  # not asking the openid scope
         self.http_client.get_json.reset_mock()
-        jwks = yield defer.ensureDeferred(self.handler.load_jwks(token, force=True))
+        jwks = yield defer.ensureDeferred(self.handler.load_jwks(force=True))
         self.http_client.get_json.assert_not_called()
         self.assertEqual(jwks, {"keys": []})
 
@@ -282,25 +281,11 @@ def test_validate_config(self):
             )
 
         # Tests for configs that the userinfo endpoint
-        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.assertFalse(h._uses_userinfo)
+        h._scopes = []  # do not request the openid scope
+        self.assertTrue(h._uses_userinfo)
         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}
         ):
@@ -458,7 +443,7 @@ def test_callback(self):
         self.handler._fetch_userinfo.reset_mock()
 
         # With userinfo fetching
-        self.handler._user_profile_method = "userinfo_endpoint"
+        self.handler._scopes = []  # do not ask the "openid" scope
         yield defer.ensureDeferred(self.handler.handle_oidc_callback(request))
 
         self.handler._auth_handler.complete_sso_login.assert_called_once_with(

From b50e1b2c576baa9769e222627d03f9bac3c02278 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 22 Sep 2020 17:28:12 -0400
Subject: [PATCH 10/16] Add documentation.

---
 docs/sample_config.yaml       | 8 ++++++++
 synapse/config/oidc_config.py | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index fb04ff283dee..69bdff597dfb 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1689,6 +1689,14 @@ oidc_config:
   #
   #skip_verification: true
 
+  # Whether to fetch the user profile from the userinfo endpoint. Valid
+  # values are:
+  #
+  # Defaults to "auto", which fetches the userinfo endpoint if "openid" is included
+  # in `scopes`. Uncomment the following to always fetch the userinfo endpoint.
+  #
+  #user_profile_method: "userinfo_endpoint"
+
   # An external module can be provided here as a custom solution to mapping
   # attributes returned from a OIDC provider onto a matrix user.
   #
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index 15a04ff31f7c..04921442cd5a 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -159,6 +159,14 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           #
           #skip_verification: true
 
+          # Whether to fetch the user profile from the userinfo endpoint. Valid
+          # values are:
+          #
+          # Defaults to "auto", which fetches the userinfo endpoint if "openid" is included
+          # in `scopes`. Uncomment the following to always fetch the userinfo endpoint.
+          #
+          #user_profile_method: "userinfo_endpoint"
+
           # An external module can be provided here as a custom solution to mapping
           # attributes returned from a OIDC provider onto a matrix user.
           #

From 7e5477cc57eae14cf868513acb5c8fb8c56d66bf Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 22 Sep 2020 17:43:31 -0400
Subject: [PATCH 11/16] Add tests.

---
 tests/handlers/test_oidc.py | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 89ec5fcb31bb..dd8def230d5f 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -280,9 +280,15 @@ def test_validate_config(self):
                 h._validate_metadata,
             )
 
-        # Tests for configs that the userinfo endpoint
+        # Tests for configs that require the userinfo endpoint
         self.assertFalse(h._uses_userinfo)
-        h._scopes = []  # do not request the openid scope
+        self.assertEqual(h._user_profile_method, "auto")
+        h._user_profile_method = "userinfo_endpoint"
+        self.assertTrue(h._uses_userinfo)
+
+        # Revert the profile method and do not request the "openid" scope.
+        h._user_profile_method = "auto"
+        h._scopes = []
         self.assertTrue(h._uses_userinfo)
         self.assertRaisesRegex(ValueError, "userinfo_endpoint", h._validate_metadata)
 

From afe06cf827157ff15f3de838052d83cb9853940d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 28 Sep 2020 14:23:46 -0400
Subject: [PATCH 12/16] Add some documentation.

---
 docs/openid.md | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/docs/openid.md b/docs/openid.md
index 70b37f858bd8..48736819995a 100644
--- a/docs/openid.md
+++ b/docs/openid.md
@@ -238,13 +238,36 @@ Synapse config:
 
 ```yaml
 oidc_config:
-   enabled: true
-   issuer: "https://id.twitch.tv/oauth2/"
-   client_id: "your-client-id" # TO BE FILLED
-   client_secret: "your-client-secret" # TO BE FILLED
-   client_auth_method: "client_secret_post"
-   user_mapping_provider:
-     config:
-       localpart_template: '{{ user.preferred_username }}'
-       display_name_template: '{{ user.name }}'
+  enabled: true
+  issuer: "https://id.twitch.tv/oauth2/"
+  client_id: "your-client-id" # TO BE FILLED
+  client_secret: "your-client-secret" # TO BE FILLED
+  client_auth_method: "client_secret_post"
+  user_mapping_provider:
+    config:
+      localpart_template: "{{ user.preferred_username }}"
+      display_name_template: "{{ user.name }}"
+```
+
+### GitLab
+
+1. Create a [new application](https://gitlab.com/profile/applications).
+2. Add the `read_user` and `openid` scopes.
+3. Add this Callback URL: `[synapse public baseurl]/_synapse/oidc/callback`
+
+Synapse config:
+
+```yaml
+oidc_config:
+  enabled: true
+  issuer: "https://gitlab.com/"
+  client_id: "your-client-id" # TO BE FILLED
+  client_secret: "your-client-secret" # TO BE FILLED
+  client_auth_method: "client_secret_post"
+  scopes: ["openid", "read_user"]
+  user_profile_method: "userinfo_endpoint"
+  user_mapping_provider:
+    config:
+      localpart_template: '{{ user.nickname }}'
+      display_name_template: '{{ user.name }}'
 ```

From ea9891e5c5f1212caf13c31dc9f85c7c6e916e9d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Tue, 29 Sep 2020 13:49:47 -0400
Subject: [PATCH 13/16] Fix typo.

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
---
 synapse/handlers/oidc_handler.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 7d6289fa38dc..246395e73e86 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -197,7 +197,7 @@ def _validate_metadata(self):
                     % (m["response_types_supported"],)
                 )
 
-        # Ensure there's a userinfo endpoint to fetch from if it is requred.
+        # Ensure there's a userinfo endpoint to fetch from if it is required.
         if self._uses_userinfo:
             if m.get("userinfo_endpoint") is None:
                 raise ValueError(

From af375d247a15d5450059578465441b9680173fdb Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 29 Sep 2020 13:54:09 -0400
Subject: [PATCH 14/16] Finish comment.

---
 docs/sample_config.yaml       | 2 +-
 synapse/config/oidc_config.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index 29f30d9b1ee4..ef55137f1605 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -1690,7 +1690,7 @@ oidc_config:
   #skip_verification: true
 
   # Whether to fetch the user profile from the userinfo endpoint. Valid
-  # values are:
+  # values are: "auto" or "userinfo_endpoint".
   #
   # Defaults to "auto", which fetches the userinfo endpoint if "openid" is included
   # in `scopes`. Uncomment the following to always fetch the userinfo endpoint.
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index 07e0754caae4..456a921db413 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -161,7 +161,7 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
           #skip_verification: true
 
           # Whether to fetch the user profile from the userinfo endpoint. Valid
-          # values are:
+          # values are: "auto" or "userinfo_endpoint".
           #
           # Defaults to "auto", which fetches the userinfo endpoint if "openid" is included
           # in `scopes`. Uncomment the following to always fetch the userinfo endpoint.

From eec75082d59cd4ff819d0030a7e695b6d929a9f3 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Wed, 30 Sep 2020 13:21:15 -0400
Subject: [PATCH 15/16] Clarify the changelog.

---
 changelog.d/7658.feature | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelog.d/7658.feature b/changelog.d/7658.feature
index 6a3875bd7d39..17094a67bac0 100644
--- a/changelog.d/7658.feature
+++ b/changelog.d/7658.feature
@@ -1 +1 @@
-Add config option for always using userinfo endpoint for OpenID Connect with Gitlab. Contributed by Benjamin Koch.
+Add a configuration option for always using "userinfo endpoint" for OpenID Connect. This fixes support for some identity providers, e.g. GitLab. Contributed by Benjamin Koch.

From 65414799c19d1e21b9b4d47410f08377fa4156ae Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Wed, 30 Sep 2020 13:21:47 -0400
Subject: [PATCH 16/16] Add a missing word.

---
 changelog.d/7658.feature | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelog.d/7658.feature b/changelog.d/7658.feature
index 17094a67bac0..fbf345988d35 100644
--- a/changelog.d/7658.feature
+++ b/changelog.d/7658.feature
@@ -1 +1 @@
-Add a configuration option for always using "userinfo endpoint" for OpenID Connect. This fixes support for some identity providers, e.g. GitLab. Contributed by Benjamin Koch.
+Add a configuration option for always using the "userinfo endpoint" for OpenID Connect. This fixes support for some identity providers, e.g. GitLab. Contributed by Benjamin Koch.