Cleanups to the OpenID Connect integration (#7628)

docs, default configs, comments. Nothing very significant.

4 files changed, 125 insertions, 83 deletions

diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index 586038078f..e24dd637bc 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -55,7 +55,6 @@ class OIDCConfig(Config):
         self.oidc_token_endpoint = oidc_config.get("token_endpoint")
         self.oidc_userinfo_endpoint = oidc_config.get("userinfo_endpoint")
         self.oidc_jwks_uri = oidc_config.get("jwks_uri")
-        self.oidc_subject_claim = oidc_config.get("subject_claim", "sub")
         self.oidc_skip_verification = oidc_config.get("skip_verification", False)
 
         ump_config = oidc_config.get("user_mapping_provider", {})
@@ -86,92 +85,119 @@ class OIDCConfig(Config):
 
     def generate_config_section(self, config_dir_path, server_name, **kwargs):
         return """\
-        # Enable OpenID Connect for registration and login. Uses authlib.
+        # OpenID Connect integration. The following settings can be used to make Synapse
+        # use an OpenID Connect Provider for authentication, instead of its internal
+        # password database.
+        #
+        # See https://github.com/matrix-org/synapse/blob/master/openid.md.
         #
         oidc_config:
-            # enable OpenID Connect. Defaults to false.
-            #
-            #enabled: true
-
-            # use the OIDC discovery mechanism to discover endpoints. Defaults to true.
-            #
-            #discover: true
-
-            # the OIDC issuer. Used to validate tokens and discover the providers endpoints. Required.
-            #
-            #issuer: "https://accounts.example.com/"
-
-            # oauth2 client id to use. Required.
-            #
-            #client_id: "provided-by-your-issuer"
-
-            # oauth2 client secret to use. Required.
-            #
-            #client_secret: "provided-by-your-issuer"
-
-            # auth method to use when exchanging the token.
-            # Valid values are "client_secret_basic" (default), "client_secret_post" and "none".
-            #
-            #client_auth_method: "client_secret_basic"
-
-            # list of scopes to ask. This should include the "openid" scope. Defaults to ["openid"].
-            #
-            #scopes: ["openid"]
-
-            # the oauth2 authorization endpoint. Required if provider discovery is disabled.
+          # Uncomment the following to enable authorization against an OpenID Connect
+          # server. Defaults to false.
+          #
+          #enabled: true
+
+          # Uncomment the following to disable use of the OIDC discovery mechanism to
+          # discover endpoints. Defaults to true.
+          #
+          #discover: false
+
+          # the OIDC issuer. Used to validate tokens and (if discovery is enabled) to
+          # discover the provider's endpoints.
+          #
+          # Required if 'enabled' is true.
+          #
+          #issuer: "https://accounts.example.com/"
+
+          # oauth2 client id to use.
+          #
+          # Required if 'enabled' is true.
+          #
+          #client_id: "provided-by-your-issuer"
+
+          # oauth2 client secret to use.
+          #
+          # Required if 'enabled' is true.
+          #
+          #client_secret: "provided-by-your-issuer"
+
+          # auth method to use when exchanging the token.
+          # Valid values are 'client_secret_basic' (default), 'client_secret_post' and
+          # 'none'.
+          #
+          #client_auth_method: client_secret_post
+
+          # list of scopes to request. This should normally include the "openid" scope.
+          # Defaults to ["openid"].
+          #
+          #scopes: ["openid", "profile"]
+
+          # the oauth2 authorization endpoint. Required if provider discovery is disabled.
+          #
+          #authorization_endpoint: "https://accounts.example.com/oauth2/auth"
+
+          # the oauth2 token endpoint. Required if provider discovery is disabled.
+          #
+          #token_endpoint: "https://accounts.example.com/oauth2/token"
+
+          # the OIDC userinfo endpoint. Required if discovery is disabled and the
+          # "openid" scope is not requested.
+          #
+          #userinfo_endpoint: "https://accounts.example.com/userinfo"
+
+          # URI where to fetch the JWKS. Required if discovery is disabled and the
+          # "openid" scope is used.
+          #
+          #jwks_uri: "https://accounts.example.com/.well-known/jwks.json"
+
+          # Uncomment to skip metadata verification. Defaults to false.
+          #
+          # Use this if you are connecting to a provider that is not OpenID Connect
+          # compliant.
+          # Avoid this in production.
+          #
+          #skip_verification: true
+
+          # An external module can be provided here as a custom solution to mapping
+          # attributes returned from a OIDC provider onto a matrix user.
+          #
+          user_mapping_provider:
+            # The custom module's class. Uncomment to use a custom module.
+            # Default is {mapping_provider!r}.
             #
-            #authorization_endpoint: "https://accounts.example.com/oauth2/auth"
-
-            # the oauth2 token endpoint. Required if provider discovery is disabled.
-            #
-            #token_endpoint: "https://accounts.example.com/oauth2/token"
-
-            # the OIDC userinfo endpoint. Required if discovery is disabled and the "openid" scope is not asked.
+            # See https://github.com/matrix-org/synapse/blob/master/docs/sso_mapping_providers.md#openid-mapping-providers
+            # for information on implementing a custom mapping provider.
             #
-            #userinfo_endpoint: "https://accounts.example.com/userinfo"
+            #module: mapping_provider.OidcMappingProvider
 
-            # URI where to fetch the JWKS. Required if discovery is disabled and the "openid" scope is used.
+            # Custom configuration values for the module. This section will be passed as
+            # a Python dictionary to the user mapping provider module's `parse_config`
+            # method.
             #
-            #jwks_uri: "https://accounts.example.com/.well-known/jwks.json"
-
-            # skip metadata verification. Defaults to false.
-            # Use this if you are connecting to a provider that is not OpenID Connect compliant.
-            # Avoid this in production.
+            # The examples below are intended for the default provider: they should be
+            # changed if using a custom provider.
             #
-            #skip_verification: false
-
+            config:
+              # name of the claim containing a unique identifier for the user.
+              # Defaults to `sub`, which OpenID Connect compliant providers should provide.
+              #
+              #subject_claim: "sub"
 
-            # An external module can be provided here as a custom solution to mapping
-            # attributes returned from a OIDC provider onto a matrix user.
-            #
-            user_mapping_provider:
-              # The custom module's class. Uncomment to use a custom module.
-              # Default is {mapping_provider!r}.
+              # Jinja2 template for the localpart of the MXID.
+              #
+              # When rendering, this template is given the following variables:
+              #   * user: The claims returned by the UserInfo Endpoint and/or in the ID
+              #     Token
+              #
+              # This must be configured if using the default mapping provider.
               #
-              #module: mapping_provider.OidcMappingProvider
+              localpart_template: "{{{{ user.preferred_username }}}}"
 
-              # Custom configuration values for the module. Below options are intended
-              # for the built-in provider, they should be changed if using a custom
-              # module. This section will be passed as a Python dictionary to the
-              # module's `parse_config` method.
+              # Jinja2 template for the display name to set on first login.
               #
-              # Below is the config of the default mapping provider, based on Jinja2
-              # templates. Those templates are used to render user attributes, where the
-              # userinfo object is available through the `user` variable.
+              # If unset, no displayname will be set.
               #
-              config:
-                # name of the claim containing a unique identifier for the user.
-                # Defaults to `sub`, which OpenID Connect compliant providers should provide.
-                #
-                #subject_claim: "sub"
-
-                # Jinja2 template for the localpart of the MXID
-                #
-                localpart_template: "{{{{ user.preferred_username }}}}"
-
-                # Jinja2 template for the display name to set on first login. Optional.
-                #
-                #display_name_template: "{{{{ user.given_name }}}} {{{{ user.last_name }}}}"
+              #display_name_template: "{{{{ user.given_name }}}} {{{{ user.last_name }}}}"
         """.format(
             mapping_provider=DEFAULT_USER_MAPPING_PROVIDER
         )
diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py
index 38ec256984..d0a19751e8 100644
--- a/synapse/config/saml2_config.py
+++ b/synapse/config/saml2_config.py
@@ -218,6 +218,8 @@ class SAML2Config(Config):
 
     def generate_config_section(self, config_dir_path, server_name, **kwargs):
         return """\
+        ## Single sign-on integration ##
+
         # Enable SAML2 for registration and login. Uses pysaml2.
         #
         # At least one of `sp_config` or `config_path` must be set in this section to
diff --git a/synapse/config/sso.py b/synapse/config/sso.py
index aff642f015..73b7296399 100644
--- a/synapse/config/sso.py
+++ b/synapse/config/sso.py
@@ -61,7 +61,8 @@ class SSOConfig(Config):
 
     def generate_config_section(self, **kwargs):
         return """\
-        # Additional settings to use with single-sign on systems such as SAML2 and CAS.
+        # Additional settings to use with single-sign on systems such as OpenID Connect,
+        # SAML2 and CAS.
         #
         sso:
             # A list of client URLs which are whitelisted so that the user does not
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 4ba8c7fda5..9c08eb5399 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -37,6 +37,7 @@ from twisted.web.client import readBody
 from synapse.config import ConfigError
 from synapse.http.server import finish_request
 from synapse.http.site import SynapseRequest
+from synapse.logging.context import make_deferred_yieldable
 from synapse.push.mailer import load_jinja2_templates
 from synapse.server import HomeServer
 from synapse.types import UserID, map_username_to_mxid_localpart
@@ -99,7 +100,6 @@ class OidcHandler:
             hs.config.oidc_client_auth_method,
         )  # type: ClientAuth
         self._client_auth_method = hs.config.oidc_client_auth_method  # type: str
-        self._subject_claim = hs.config.oidc_subject_claim
         self._provider_metadata = OpenIDProviderMetadata(
             issuer=hs.config.oidc_issuer,
             authorization_endpoint=hs.config.oidc_authorization_endpoint,
@@ -310,6 +310,10 @@ class OidcHandler:
         received in the callback to exchange it for a token. The call uses the
         ``ClientAuth`` to authenticate with the client with its ID and secret.
 
+        See:
+           https://tools.ietf.org/html/rfc6749#section-3.2
+           https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint
+
         Args:
             code: The authorization code we got from the callback.
 
@@ -362,7 +366,7 @@ class OidcHandler:
             code=response.code, phrase=response.phrase.decode("utf-8")
         )
 
-        resp_body = await readBody(response)
+        resp_body = await make_deferred_yieldable(readBody(response))
 
         if response.code >= 500:
             # In case of a server error, we should first try to decode the body
@@ -484,6 +488,7 @@ class OidcHandler:
                 claims_params=claims_params,
             )
         except ValueError:
+            logger.info("Reloading JWKS after decode error")
             jwk_set = await self.load_jwks(force=True)  # try reloading the jwks
             claims = jwt.decode(
                 token["id_token"],
@@ -592,6 +597,9 @@ class OidcHandler:
         # The provider might redirect with an error.
         # In that case, just display it as-is.
         if b"error" in request.args:
+            # error response from the auth server. see:
+            #  https://tools.ietf.org/html/rfc6749#section-4.1.2.1
+            #  https://openid.net/specs/openid-connect-core-1_0.html#AuthError
             error = request.args[b"error"][0].decode()
             description = request.args.get(b"error_description", [b""])[0].decode()
 
@@ -605,8 +613,11 @@ class OidcHandler:
             self._render_error(request, error, description)
             return
 
+        # otherwise, it is presumably a successful response. see:
+        #   https://tools.ietf.org/html/rfc6749#section-4.1.2
+
         # Fetch the session cookie
-        session = request.getCookie(SESSION_COOKIE_NAME)
+        session = request.getCookie(SESSION_COOKIE_NAME)  # type: Optional[bytes]
         if session is None:
             logger.info("No session cookie found")
             self._render_error(request, "missing_session", "No session cookie found")
@@ -654,7 +665,7 @@ class OidcHandler:
             self._render_error(request, "invalid_request", "Code parameter is missing")
             return
 
-        logger.info("Exchanging code")
+        logger.debug("Exchanging code")
         code = request.args[b"code"][0].decode()
         try:
             token = await self._exchange_code(code)
@@ -663,10 +674,12 @@ class OidcHandler:
             self._render_error(request, e.error, e.error_description)
             return
 
+        logger.debug("Successfully obtained OAuth2 access token")
+
         # 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:
-            logger.info("Fetching userinfo")
+            logger.debug("Fetching userinfo")
             try:
                 userinfo = await self._fetch_userinfo(token)
             except Exception as e:
@@ -674,7 +687,7 @@ class OidcHandler:
                 self._render_error(request, "fetch_error", str(e))
                 return
         else:
-            logger.info("Extracting userinfo from id_token")
+            logger.debug("Extracting userinfo from id_token")
             try:
                 userinfo = await self._parse_id_token(token, nonce=nonce)
             except Exception as e:
@@ -750,7 +763,7 @@ class OidcHandler:
         return macaroon.serialize()
 
     def _verify_oidc_session_token(
-        self, session: str, state: str
+        self, session: bytes, state: str
     ) -> Tuple[str, str, Optional[str]]:
         """Verifies and extract an OIDC session token.