Prefix idp_id with "oidc-" (#9189)

... to avoid clashes with other SSO mechanisms

Author: richvdh

changelog.d/9189.misc

@@ -0,0 +1 @@
+Add an `oidc-` prefix to any `idp_id`s which are given in the `oidc_providers` configuration.

docs/sample_config.yaml

@@ -1728,7 +1728,9 @@ saml2_config:
 #
 #   idp_icon: An optional icon for this identity provider, which is presented
 #       by identity picker pages. If given, must be an MXC URI of the format
-#       mxc://<server-name>/<media-id>
+#       mxc://<server-name>/<media-id>. (An easy way to obtain such an MXC URI
+#       is to upload an image to an (unencrypted) room and then copy the "url"
+#       from the source of the event.)
 #
 #   discover: set to 'false' to disable the use of the OIDC discovery mechanism
 #       to discover endpoints. Defaults to true.
@@ -1814,13 +1816,16 @@ saml2_config:
 #
 # For backwards compatibility, it is also possible to configure a single OIDC
 # provider via an 'oidc_config' setting. This is now deprecated and admins are
-# advised to migrate to the 'oidc_providers' format.
+# advised to migrate to the 'oidc_providers' format. (When doing that migration,
+# use 'oidc' for the idp_id to ensure that existing users continue to be
+# recognised.)
 #
 oidc_providers:
   # Generic example
   #
   #- idp_id: my_idp
   #  idp_name: "My OpenID provider"
+  #  idp_icon: "mxc://example.com/mediaid"
   #  discover: false
   #  issuer: "https://accounts.example.com/"
   #  client_id: "provided-by-your-issuer"
@@ -1844,8 +1849,8 @@ oidc_providers:
 
   # For use with Github
   #
-  #- idp_id: google
-  #  idp_name: Google
+  #- idp_id: github
+  #  idp_name: Github
   #  discover: false
   #  issuer: "https://github.com/"
   #  client_id: "your-client-id" # TO BE FILLED

synapse/config/oidc_config.py

@@ -69,7 +69,9 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
         #
         #   idp_icon: An optional icon for this identity provider, which is presented
         #       by identity picker pages. If given, must be an MXC URI of the format
-        #       mxc://<server-name>/<media-id>
+        #       mxc://<server-name>/<media-id>. (An easy way to obtain such an MXC URI
+        #       is to upload an image to an (unencrypted) room and then copy the "url"
+        #       from the source of the event.)
         #
         #   discover: set to 'false' to disable the use of the OIDC discovery mechanism
         #       to discover endpoints. Defaults to true.
@@ -155,13 +157,16 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
         #
         # For backwards compatibility, it is also possible to configure a single OIDC
         # provider via an 'oidc_config' setting. This is now deprecated and admins are
-        # advised to migrate to the 'oidc_providers' format.
+        # advised to migrate to the 'oidc_providers' format. (When doing that migration,
+        # use 'oidc' for the idp_id to ensure that existing users continue to be
+        # recognised.)
         #
         oidc_providers:
           # Generic example
           #
           #- idp_id: my_idp
           #  idp_name: "My OpenID provider"
+          #  idp_icon: "mxc://example.com/mediaid"
           #  discover: false
           #  issuer: "https://accounts.example.com/"
           #  client_id: "provided-by-your-issuer"
@@ -185,8 +190,8 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
 
           # For use with Github
           #
-          #- idp_id: google
-          #  idp_name: Google
+          #- idp_id: github
+          #  idp_name: Github
           #  discover: false
           #  issuer: "https://github.com/"
           #  client_id: "your-client-id" # TO BE FILLED
@@ -210,6 +215,8 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
     "type": "object",
     "required": ["issuer", "client_id", "client_secret"],
     "properties": {
+        # TODO: fix the maxLength here depending on what MSC2528 decides
+        #   remember that we prefix the ID given here with `oidc-`
         "idp_id": {"type": "string", "minLength": 1, "maxLength": 128},
         "idp_name": {"type": "string"},
         "idp_icon": {"type": "string"},
@@ -335,6 +342,8 @@ def _parse_oidc_config_dict(
     # enforce those limits now.
     # TODO: factor out this stuff to a generic function
     idp_id = oidc_config.get("idp_id", "oidc")
+
+    # TODO: update this validity check based on what MSC2858 decides.
     valid_idp_chars = set(string.ascii_lowercase + string.digits + "-._")
 
     if any(c not in valid_idp_chars for c in idp_id):
@@ -348,6 +357,17 @@ def _parse_oidc_config_dict(
             "idp_id must start with a-z", config_path + ("idp_id",),
         )
 
+    # prefix the given IDP with a prefix specific to the SSO mechanism, to avoid
+    # clashes with other mechs (such as SAML, CAS).
+    #
+    # We allow "oidc" as an exception so that people migrating from old-style
+    # "oidc_config" format (which has long used "oidc" as its idp_id) can migrate to
+    # a new-style "oidc_providers" entry without changing the idp_id for their provider
+    # (and thereby invalidating their user_external_ids data).
+
+    if idp_id != "oidc":
+        idp_id = "oidc-" + idp_id
+
     # MSC2858 also specifies that the idp_icon must be a valid MXC uri
     idp_icon = oidc_config.get("idp_icon")
     if idp_icon is not None:

tests/rest/client/v1/test_login.py

@@ -446,7 +446,7 @@ def test_multi_sso_redirect(self):
         p.feed(channel.result["body"].decode("utf-8"))
         p.close()
 
-        self.assertCountEqual(p.radios["idp"], ["cas", "oidc", "idp1", "saml"])
+        self.assertCountEqual(p.radios["idp"], ["cas", "oidc", "oidc-idp1", "saml"])
 
         self.assertEqual(p.hiddens["redirectUrl"], TEST_CLIENT_REDIRECT_URL)