Prefix idp_id with "oidc-" (#9189)

... to avoid clashes with other SSO mechanisms

4 files changed, 35 insertions, 9 deletions

diff --git a/changelog.d/9189.misc b/changelog.d/9189.misc
new file mode 100644
index 0000000000..9a5740aac2
--- /dev/null
+++ b/changelog.d/9189.misc
@@ -0,0 +1 @@
+Add an `oidc-` prefix to any `idp_id`s which are given in the `oidc_providers` configuration.
diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index b49a5da8cc..87bfe22237 100644
--- a/docs/sample_config.yaml
+++ b/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
diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py
index 8cb0c42f36..d58a83be7f 100644
--- a/synapse/config/oidc_config.py
+++ b/synapse/config/oidc_config.py
@@ -69,7 +69,9 @@ class OIDCConfig(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.
@@ -155,13 +157,16 @@ class OIDCConfig(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"
@@ -185,8 +190,8 @@ class OIDCConfig(Config):
 
           # 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 @@ OIDC_PROVIDER_CONFIG_SCHEMA = {
     "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:
diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py
index 2d25490374..2672ce24c6 100644
--- a/tests/rest/client/v1/test_login.py
+++ b/tests/rest/client/v1/test_login.py
@@ -446,7 +446,7 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
         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)