From 3f73a5a66e83c079ed4942298600cd38d39521e9 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 18 Nov 2020 14:08:43 -0500
Subject: [PATCH 01/17] Add basic SAML tests.

---
 changelog.d/8800.misc       |   1 +
 tests/handlers/test_saml.py | 127 ++++++++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)
 create mode 100644 changelog.d/8800.misc
 create mode 100644 tests/handlers/test_saml.py

diff --git a/changelog.d/8800.misc b/changelog.d/8800.misc
new file mode 100644
index 000000000000..7824e367362b
--- /dev/null
+++ b/changelog.d/8800.misc
@@ -0,0 +1 @@
+Add tests for SAML integration.
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
new file mode 100644
index 000000000000..0667ba2e89d3
--- /dev/null
+++ b/tests/handlers/test_saml.py
@@ -0,0 +1,127 @@
+#  Copyright 2020 The Matrix.org Foundation C.I.C.
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+from mock import Mock
+
+import attr
+
+from synapse.handlers.saml_handler import SamlHandler
+from synapse.handlers.sso import MappingException
+
+from tests.unittest import HomeserverTestCase
+
+# These are a few constants that are used as config parameters in the tests.
+BASE_URL = "https://synapse/"
+
+
+@attr.s
+class FakeAuthnResponse:
+    ava = attr.ib(type=dict)
+
+
+class TestMappingProvider:
+    def __init__(self, config, module):
+        pass
+
+    @staticmethod
+    def parse_config(config):
+        return
+
+    @staticmethod
+    def get_saml_attributes(config):
+        return {"uid"}, {"displayName"}
+
+    def get_remote_user_id(self, saml_response, client_redirect_url):
+        return saml_response.ava["uid"]
+
+    def saml_response_to_user_attributes(
+        self, saml_response, failures, client_redirect_url
+    ):
+        return {"mxid_localpart": saml_response.ava["username"], "displayname": None}
+
+
+class SamlHandlerTestCase(HomeserverTestCase):
+    def make_homeserver(self, reactor, clock):
+
+        self.http_client = Mock(spec=["get_json"])
+        self.http_client.user_agent = "Synapse Test"
+
+        config = self.default_config()
+        config["public_baseurl"] = BASE_URL
+        saml_config = {
+            "sp_config": {"metadata": {}},
+            # Disable grandfathering.
+            "grandfathered_mxid_source_attribute": None,
+            "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"},
+        }
+        config["saml2_config"] = saml_config
+
+        hs = self.setup_test_homeserver(
+            http_client=self.http_client,
+            proxied_http_client=self.http_client,
+            config=config,
+        )
+
+        self.handler = SamlHandler(hs)
+
+        return hs
+
+    def test_map_saml_response_to_user(self):
+        """Ensure that mapping the SAML response returned from a provider to an MXID works properly."""
+        saml_response = FakeAuthnResponse({"uid": "test_user", "username": "test_user"})
+        # The redirect_url doesn't matter with the default user mapping provider.
+        redirect_url = ""
+        mxid = self.get_success(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user:test")
+
+        # Some providers return an integer ID.
+        saml_response = FakeAuthnResponse({"uid": 1234, "username": "test_user_2"})
+        mxid = self.get_success(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user_2:test")
+
+        # Test if the mxid is already taken
+        store = self.hs.get_datastore()
+        self.get_success(
+            store.register_user(user_id="@test_user_3:test", password_hash=None)
+        )
+        saml_response = FakeAuthnResponse({"uid": "test3", "username": "test_user_3"})
+        e = self.get_failure(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(
+            str(e.value), "Unable to generate a Matrix ID from the SAML response"
+        )
+
+    def test_map_saml_response_to_invalid_localpart(self):
+        """If the mapping provider generates an invalid localpart it should be rejected."""
+        saml_response = FakeAuthnResponse({"uid": "test2", "username": "föö"})
+        redirect_url = ""
+        e = self.get_failure(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(str(e.value), "localpart is invalid: föö")

From 3949b5890f12ec1eae8734012b2923025744d834 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 24 Nov 2020 08:43:48 -0500
Subject: [PATCH 02/17] Install xmlsec1 for pysaml2.

---
 .buildkite/scripts/test_old_deps.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.buildkite/scripts/test_old_deps.sh b/.buildkite/scripts/test_old_deps.sh
index cdb77b556ca6..9905c4bc4f3c 100755
--- a/.buildkite/scripts/test_old_deps.sh
+++ b/.buildkite/scripts/test_old_deps.sh
@@ -6,7 +6,7 @@
 set -ex
 
 apt-get update
-apt-get install -y python3.5 python3.5-dev python3-pip libxml2-dev libxslt-dev zlib1g-dev tox
+apt-get install -y python3.5 python3.5-dev python3-pip libxml2-dev libxslt-dev xmlsec1 zlib1g-dev tox
 
 export LANG="C.UTF-8"
 

From bb06478cbe12ed085a884f1e9784ffe32ab13214 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 30 Nov 2020 08:08:13 -0500
Subject: [PATCH 03/17] Remove some bits of the tests that were copied from
 OIDC.

---
 tests/handlers/test_saml.py | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 0667ba2e89d3..325741ba6041 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -89,21 +89,12 @@ def test_map_saml_response_to_user(self):
         )
         self.assertEqual(mxid, "@test_user:test")
 
-        # Some providers return an integer ID.
-        saml_response = FakeAuthnResponse({"uid": 1234, "username": "test_user_2"})
-        mxid = self.get_success(
-            self.handler._map_saml_response_to_user(
-                saml_response, redirect_url, "user-agent", "10.10.10.10"
-            )
-        )
-        self.assertEqual(mxid, "@test_user_2:test")
-
         # Test if the mxid is already taken
         store = self.hs.get_datastore()
         self.get_success(
-            store.register_user(user_id="@test_user_3:test", password_hash=None)
+            store.register_user(user_id="@test_user_2:test", password_hash=None)
         )
-        saml_response = FakeAuthnResponse({"uid": "test3", "username": "test_user_3"})
+        saml_response = FakeAuthnResponse({"uid": "test2", "username": "test_user_2"})
         e = self.get_failure(
             self.handler._map_saml_response_to_user(
                 saml_response, redirect_url, "user-agent", "10.10.10.10"
@@ -116,7 +107,7 @@ def test_map_saml_response_to_user(self):
 
     def test_map_saml_response_to_invalid_localpart(self):
         """If the mapping provider generates an invalid localpart it should be rejected."""
-        saml_response = FakeAuthnResponse({"uid": "test2", "username": "föö"})
+        saml_response = FakeAuthnResponse({"uid": "test", "username": "föö"})
         redirect_url = ""
         e = self.get_failure(
             self.handler._map_saml_response_to_user(

From 003b25e9ffed2e3129e928555f91ea8688bbf436 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 30 Nov 2020 08:17:43 -0500
Subject: [PATCH 04/17] Add an additional test for generating in-use usernames.

---
 synapse/handlers/saml_handler.py |  2 +-
 tests/handlers/test_oidc.py      |  2 +-
 tests/handlers/test_saml.py      | 53 +++++++++++++++++++++++---------
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 34db10ffe43e..7ffad7d8af22 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -265,7 +265,7 @@ async def saml_response_to_remapped_user_attributes(
             return UserAttributes(
                 localpart=result.get("mxid_localpart"),
                 display_name=result.get("displayname"),
-                emails=result.get("emails"),
+                emails=result.get("emails", []),
             )
 
         with (await self._mapping_lock.queue(self._auth_provider_id)):
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index e880d32be687..c5a07c15c733 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -832,7 +832,7 @@ def test_map_userinfo_to_user_retries(self):
         # test_user is already taken, so test_user1 gets registered instead.
         self.assertEqual(mxid, "@test_user1:test")
 
-        # Register all of the potential users for a particular username.
+        # Register all of the potential usernames for a particular username.
         self.get_success(
             store.register_user(user_id="@tester:test", password_hash=None)
         )
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 325741ba6041..313bf2fc9e92 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -48,7 +48,8 @@ def get_remote_user_id(self, saml_response, client_redirect_url):
     def saml_response_to_user_attributes(
         self, saml_response, failures, client_redirect_url
     ):
-        return {"mxid_localpart": saml_response.ava["username"], "displayname": None}
+        localpart = saml_response.ava["username"] + (str(failures) if failures else "")
+        return {"mxid_localpart": localpart, "displayname": None}
 
 
 class SamlHandlerTestCase(HomeserverTestCase):
@@ -75,6 +76,9 @@ def make_homeserver(self, reactor, clock):
 
         self.handler = SamlHandler(hs)
 
+        # Reduce the number of attempts when generating MXIDs.
+        self.handler._sso_handler._MAP_USERNAME_RETRIES = 3
+
         return hs
 
     def test_map_saml_response_to_user(self):
@@ -89,30 +93,51 @@ def test_map_saml_response_to_user(self):
         )
         self.assertEqual(mxid, "@test_user:test")
 
-        # Test if the mxid is already taken
-        store = self.hs.get_datastore()
-        self.get_success(
-            store.register_user(user_id="@test_user_2:test", password_hash=None)
-        )
-        saml_response = FakeAuthnResponse({"uid": "test2", "username": "test_user_2"})
+    def test_map_saml_response_to_invalid_localpart(self):
+        """If the mapping provider generates an invalid localpart it should be rejected."""
+        saml_response = FakeAuthnResponse({"uid": "test", "username": "föö"})
+        redirect_url = ""
         e = self.get_failure(
             self.handler._map_saml_response_to_user(
                 saml_response, redirect_url, "user-agent", "10.10.10.10"
             ),
             MappingException,
         )
-        self.assertEqual(
-            str(e.value), "Unable to generate a Matrix ID from the SAML response"
-        )
+        self.assertEqual(str(e.value), "localpart is invalid: föö")
 
-    def test_map_saml_response_to_invalid_localpart(self):
-        """If the mapping provider generates an invalid localpart it should be rejected."""
-        saml_response = FakeAuthnResponse({"uid": "test", "username": "föö"})
+    def test_map_userinfo_to_user_retries(self):
+        """The mapping provider can retry generating an MXID if the MXID is already in use."""
+        store = self.hs.get_datastore()
+        self.get_success(
+            store.register_user(user_id="@test_user:test", password_hash=None)
+        )
+        saml_response = FakeAuthnResponse({"uid": "test", "username": "test_user"})
         redirect_url = ""
+        mxid = self.get_success(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            )
+        )
+        # test_user is already taken, so test_user1 gets registered instead.
+        self.assertEqual(mxid, "@test_user1:test")
+
+        # Register all of the potential usernames for a particular username.
+        self.get_success(
+            store.register_user(user_id="@tester:test", password_hash=None)
+        )
+        for i in range(1, 3):
+            self.get_success(
+                store.register_user(user_id="@tester%d:test" % i, password_hash=None)
+            )
+
+        # Now attempt to map to a username, this will fail since all potential usernames are taken.
+        saml_response = FakeAuthnResponse({"uid": "tester", "username": "tester"})
         e = self.get_failure(
             self.handler._map_saml_response_to_user(
                 saml_response, redirect_url, "user-agent", "10.10.10.10"
             ),
             MappingException,
         )
-        self.assertEqual(str(e.value), "localpart is invalid: föö")
+        self.assertEqual(
+            str(e.value), "Unable to generate a Matrix ID from the SSO response"
+        )

From 91792568c4bce845779980505bc64121b89bfab5 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 12:38:30 -0500
Subject: [PATCH 05/17] Fix a left-over OIDC term.

---
 tests/handlers/test_saml.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 313bf2fc9e92..3873da9b5493 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -105,7 +105,7 @@ def test_map_saml_response_to_invalid_localpart(self):
         )
         self.assertEqual(str(e.value), "localpart is invalid: föö")
 
-    def test_map_userinfo_to_user_retries(self):
+    def test_map_saml_response_to_user_retries(self):
         """The mapping provider can retry generating an MXID if the MXID is already in use."""
         store = self.hs.get_datastore()
         self.get_success(

From 1de016a9b508f5a230b416a17d95369cc0ce3b1d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 12:47:41 -0500
Subject: [PATCH 06/17] Update comments.

---
 tests/handlers/test_oidc.py | 2 +-
 tests/handlers/test_saml.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index c5a07c15c733..0ca95e9b0c9a 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -832,7 +832,7 @@ def test_map_userinfo_to_user_retries(self):
         # test_user is already taken, so test_user1 gets registered instead.
         self.assertEqual(mxid, "@test_user1:test")
 
-        # Register all of the potential usernames for a particular username.
+        # Register all of the potential mxids for a particular OIDC username.
         self.get_success(
             store.register_user(user_id="@tester:test", password_hash=None)
         )
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 3873da9b5493..5d60b4677fa9 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -121,7 +121,7 @@ def test_map_saml_response_to_user_retries(self):
         # test_user is already taken, so test_user1 gets registered instead.
         self.assertEqual(mxid, "@test_user1:test")
 
-        # Register all of the potential usernames for a particular username.
+        # Register all of the potential mxids for a particular SAML username.
         self.get_success(
             store.register_user(user_id="@tester:test", password_hash=None)
         )

From 6a581821d740afb24e856fe3f2648a7447aeb2a4 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 12:48:16 -0500
Subject: [PATCH 07/17] Remove unused HTTP code.

---
 tests/handlers/test_saml.py | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 5d60b4677fa9..828b83e16a9b 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -12,8 +12,6 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 
-from mock import Mock
-
 import attr
 
 from synapse.handlers.saml_handler import SamlHandler
@@ -54,10 +52,6 @@ def saml_response_to_user_attributes(
 
 class SamlHandlerTestCase(HomeserverTestCase):
     def make_homeserver(self, reactor, clock):
-
-        self.http_client = Mock(spec=["get_json"])
-        self.http_client.user_agent = "Synapse Test"
-
         config = self.default_config()
         config["public_baseurl"] = BASE_URL
         saml_config = {

From ef2e79f2d112b41fa9b58837ba7a949f7dda20b2 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 12:51:05 -0500
Subject: [PATCH 08/17] Use singletons for handlers.

---
 tests/handlers/test_oidc.py | 9 +++++----
 tests/handlers/test_saml.py | 8 +++-----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 0ca95e9b0c9a..003c49885f98 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -23,7 +23,7 @@
 from twisted.python.failure import Failure
 from twisted.web._newclient import ResponseDone
 
-from synapse.handlers.oidc_handler import OidcError, OidcHandler, OidcMappingProvider
+from synapse.handlers.oidc_handler import OidcError, OidcMappingProvider
 from synapse.handlers.sso import MappingException
 from synapse.types import UserID
 
@@ -155,13 +155,14 @@ def make_homeserver(self, reactor, clock):
             config=config,
         )
 
-        self.handler = OidcHandler(hs)
+        self.handler = hs.get_oidc_handler()
+        sso_handler = hs.get_sso_handler()
         # Mock the render error method.
         self.render_error = Mock(return_value=None)
-        self.handler._sso_handler.render_error = self.render_error
+        sso_handler.render_error = self.render_error
 
         # Reduce the number of attempts when generating MXIDs.
-        self.handler._sso_handler._MAP_USERNAME_RETRIES = 3
+        sso_handler._MAP_USERNAME_RETRIES = 3
 
         return hs
 
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 828b83e16a9b..603362b73e47 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -14,7 +14,6 @@
 
 import attr
 
-from synapse.handlers.saml_handler import SamlHandler
 from synapse.handlers.sso import MappingException
 
 from tests.unittest import HomeserverTestCase
@@ -63,15 +62,14 @@ def make_homeserver(self, reactor, clock):
         config["saml2_config"] = saml_config
 
         hs = self.setup_test_homeserver(
-            http_client=self.http_client,
-            proxied_http_client=self.http_client,
             config=config,
         )
 
-        self.handler = SamlHandler(hs)
+        self.handler = hs.get_saml_handler()
 
         # Reduce the number of attempts when generating MXIDs.
-        self.handler._sso_handler._MAP_USERNAME_RETRIES = 3
+        sso_handler = hs.get_sso_handler()
+        sso_handler._MAP_USERNAME_RETRIES = 3
 
         return hs
 

From 1ab9ad13e273ff9fc83d2c3b65cef4822b91585d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 12:56:01 -0500
Subject: [PATCH 09/17] Use default_config method.

---
 tests/handlers/test_oidc.py | 18 ++++++++++--------
 tests/handlers/test_saml.py | 11 ++++++-----
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 003c49885f98..7135b0ed7259 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -127,13 +127,8 @@ async def get_json(url):
 
 
 class OidcHandlerTestCase(HomeserverTestCase):
-    def make_homeserver(self, reactor, clock):
-
-        self.http_client = Mock(spec=["get_json"])
-        self.http_client.get_json.side_effect = get_json
-        self.http_client.user_agent = "Synapse Test"
-
-        config = self.default_config()
+    def default_config(self):
+        config = super().default_config()
         config["public_baseurl"] = BASE_URL
         oidc_config = {
             "enabled": True,
@@ -149,10 +144,17 @@ def make_homeserver(self, reactor, clock):
         oidc_config.update(config.get("oidc_config", {}))
         config["oidc_config"] = oidc_config
 
+        return config
+
+    def make_homeserver(self, reactor, clock):
+
+        self.http_client = Mock(spec=["get_json"])
+        self.http_client.get_json.side_effect = get_json
+        self.http_client.user_agent = "Synapse Test"
+
         hs = self.setup_test_homeserver(
             http_client=self.http_client,
             proxied_http_client=self.http_client,
-            config=config,
         )
 
         self.handler = hs.get_oidc_handler()
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 603362b73e47..79fd47036f40 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -50,8 +50,8 @@ def saml_response_to_user_attributes(
 
 
 class SamlHandlerTestCase(HomeserverTestCase):
-    def make_homeserver(self, reactor, clock):
-        config = self.default_config()
+    def default_config(self):
+        config = super().default_config()
         config["public_baseurl"] = BASE_URL
         saml_config = {
             "sp_config": {"metadata": {}},
@@ -61,9 +61,10 @@ def make_homeserver(self, reactor, clock):
         }
         config["saml2_config"] = saml_config
 
-        hs = self.setup_test_homeserver(
-            config=config,
-        )
+        return config
+
+    def make_homeserver(self, reactor, clock):
+        hs = self.setup_test_homeserver()
 
         self.handler = hs.get_saml_handler()
 

From 837bbf9e239f392c03cb814248948c38e8eec17d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Tue, 1 Dec 2020 13:12:18 -0500
Subject: [PATCH 10/17] Update changelog.

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

diff --git a/changelog.d/8800.misc b/changelog.d/8800.misc
index 7824e367362b..57cca8fee51c 100644
--- a/changelog.d/8800.misc
+++ b/changelog.d/8800.misc
@@ -1 +1 @@
-Add tests for SAML integration.
+Add additional error checking for OpenID Connect and SAML mapping providers.

From 6b1ac9a3964c6f454e6daf749b08d831d0114e8d Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 13:29:32 -0500
Subject: [PATCH 11/17] Lint.

---
 tests/handlers/test_oidc.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 7135b0ed7259..073cee2c3f24 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -153,8 +153,7 @@ def make_homeserver(self, reactor, clock):
         self.http_client.user_agent = "Synapse Test"
 
         hs = self.setup_test_homeserver(
-            http_client=self.http_client,
-            proxied_http_client=self.http_client,
+            http_client=self.http_client, proxied_http_client=self.http_client,
         )
 
         self.handler = hs.get_oidc_handler()

From 6b77f3e8655dd9920dd4f26ba329bd04779d09fc Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 13:30:58 -0500
Subject: [PATCH 12/17] Remove an unused parameter.

---
 tests/handlers/test_oidc.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 073cee2c3f24..c9807a7b73b8 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -152,9 +152,7 @@ def make_homeserver(self, reactor, clock):
         self.http_client.get_json.side_effect = get_json
         self.http_client.user_agent = "Synapse Test"
 
-        hs = self.setup_test_homeserver(
-            http_client=self.http_client, proxied_http_client=self.http_client,
-        )
+        hs = self.setup_test_homeserver(proxied_http_client=self.http_client)
 
         self.handler = hs.get_oidc_handler()
         sso_handler = hs.get_sso_handler()

From 57cd3b9b3b698af638f2dcb00fd3db0df0254168 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 12:44:55 -0500
Subject: [PATCH 13/17] Add a test.

---
 tests/handlers/test_oidc.py |  8 ++++++++
 tests/handlers/test_saml.py | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index c9807a7b73b8..d485af52fd04 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -731,6 +731,14 @@ def test_map_userinfo_to_existing_user(self):
         )
         self.assertEqual(mxid, "@test_user:test")
 
+        # Subsequent calls should map to the same mxid.
+        mxid = self.get_success(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user:test")
+
         # Note that a second SSO user can be mapped to the same Matrix ID. (This
         # requires a unique sub, but something that maps to the same matrix ID,
         # in this case we'll just use the same username. A more realistic example
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 79fd47036f40..e1e13a5faffc 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -16,7 +16,7 @@
 
 from synapse.handlers.sso import MappingException
 
-from tests.unittest import HomeserverTestCase
+from tests.unittest import HomeserverTestCase, override_config
 
 # These are a few constants that are used as config parameters in the tests.
 BASE_URL = "https://synapse/"
@@ -59,6 +59,10 @@ def default_config(self):
             "grandfathered_mxid_source_attribute": None,
             "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"},
         }
+
+        # Update this config with what's in the default config so that
+        # override_config works as expected.
+        saml_config.update(config.get("saml2_config", {}))
         config["saml2_config"] = saml_config
 
         return config
@@ -86,6 +90,34 @@ def test_map_saml_response_to_user(self):
         )
         self.assertEqual(mxid, "@test_user:test")
 
+    @override_config({"saml2_config": {"grandfathered_mxid_source_attribute": "mxid"}})
+    def test_map_saml_response_to_existing_user(self):
+        """Existing users can log in with SAML account."""
+        store = self.hs.get_datastore()
+        self.get_success(
+            store.register_user(user_id="@test_user:test", password_hash=None)
+        )
+
+        # Map a user via SSO.
+        saml_response = FakeAuthnResponse(
+            {"uid": "tester", "mxid": ["test_user"], "username": "test_user"}
+        )
+        redirect_url = ""
+        mxid = self.get_success(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user:test")
+
+        # Subsequent calls should map to the same mxid.
+        mxid = self.get_success(
+            self.handler._map_saml_response_to_user(
+                saml_response, redirect_url, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@test_user:test")
+
     def test_map_saml_response_to_invalid_localpart(self):
         """If the mapping provider generates an invalid localpart it should be rejected."""
         saml_response = FakeAuthnResponse({"uid": "test", "username": "föö"})

From d80f070c8b0151f5c6a6aa9af8ba96643a88b1a1 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 13:20:40 -0500
Subject: [PATCH 14/17] Fix mapping of legacy SAML users.

---
 changelog.d/8855.feature         |  1 +
 synapse/handlers/oidc_handler.py | 31 +++++++++++++++--
 synapse/handlers/saml_handler.py |  9 ++---
 synapse/handlers/sso.py          | 60 ++++++++++----------------------
 4 files changed, 54 insertions(+), 47 deletions(-)
 create mode 100644 changelog.d/8855.feature

diff --git a/changelog.d/8855.feature b/changelog.d/8855.feature
new file mode 100644
index 000000000000..77f7fe4e5d51
--- /dev/null
+++ b/changelog.d/8855.feature
@@ -0,0 +1 @@
+Add support for re-trying generation of a localpart for OpenID Connect mapping providers.
diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 78c4e94a9d1d..759478a66234 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -39,7 +39,7 @@
 from synapse.handlers.sso import MappingException, UserAttributes
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import make_deferred_yieldable
-from synapse.types import JsonDict, map_username_to_mxid_localpart
+from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -898,13 +898,40 @@ async def oidc_response_to_user_attributes(failures: int) -> UserAttributes:
 
             return UserAttributes(**attributes)
 
+        async def grandfather_existing_users() -> Optional[str]:
+            if self._allow_existing_users:
+                # If allowing existing users we want to generate a single localpart
+                # and attempt to match it.
+                failures = 0
+                attributes = await oidc_response_to_user_attributes(failures)
+
+                user_id = UserID(attributes.localpart, self.server_name).to_string()
+                users = await self.store.get_users_by_id_case_insensitive(user_id)
+                if users:
+                    # If an existing matrix ID is returned, then use it.
+                    if len(users) == 1:
+                        previously_registered_user_id = next(iter(users))
+                    elif user_id in users:
+                        previously_registered_user_id = user_id
+                    else:
+                        # Do not attempt to continue generating Matrix IDs.
+                        raise MappingException(
+                            "Attempted to login as '{}' but it matches more than one user inexactly: {}".format(
+                                user_id, users
+                            )
+                        )
+
+                    return previously_registered_user_id
+
+            return None
+
         return await self._sso_handler.get_mxid_from_sso(
             self._auth_provider_id,
             remote_user_id,
             user_agent,
             ip_address,
             oidc_response_to_user_attributes,
-            self._allow_existing_users,
+            grandfather_existing_users,
         )
 
 
diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py
index 7ffad7d8af22..76d4169fe2e6 100644
--- a/synapse/handlers/saml_handler.py
+++ b/synapse/handlers/saml_handler.py
@@ -268,7 +268,7 @@ async def saml_response_to_remapped_user_attributes(
                 emails=result.get("emails", []),
             )
 
-        with (await self._mapping_lock.queue(self._auth_provider_id)):
+        async def grandfather_existing_users() -> Optional[str]:
             # backwards-compatibility hack: see if there is an existing user with a
             # suitable mapping from the uid
             if (
@@ -290,17 +290,18 @@ async def saml_response_to_remapped_user_attributes(
                 if users:
                     registered_user_id = list(users.keys())[0]
                     logger.info("Grandfathering mapping to %s", registered_user_id)
-                    await self.store.record_user_external_id(
-                        self._auth_provider_id, remote_user_id, registered_user_id
-                    )
                     return registered_user_id
 
+            return None
+
+        with (await self._mapping_lock.queue(self._auth_provider_id)):
             return await self._sso_handler.get_mxid_from_sso(
                 self._auth_provider_id,
                 remote_user_id,
                 user_agent,
                 ip_address,
                 saml_response_to_remapped_user_attributes,
+                grandfather_existing_users,
             )
 
     def expire_sessions(self):
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index d963082210f0..f42b90e1bcfe 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -116,7 +116,7 @@ async def get_mxid_from_sso(
         user_agent: str,
         ip_address: str,
         sso_to_matrix_id_mapper: Callable[[int], Awaitable[UserAttributes]],
-        allow_existing_users: bool = False,
+        grandfather_existing_users: Optional[Callable[[], Awaitable[Optional[str]]]],
     ) -> str:
         """
         Given an SSO ID, retrieve the user ID for it and possibly register the user.
@@ -125,6 +125,10 @@ async def get_mxid_from_sso(
         if it has that matrix ID is returned regardless of the current mapping
         logic.
 
+        If a callable is provided for grandfathering users, it is called and can
+        potentially return a matrix ID to use. If it does, the SSO ID is linked to
+        this matrix ID for subsequent calls.
+
         The mapping function is called (potentially multiple times) to generate
         a localpart for the user.
 
@@ -132,17 +136,6 @@ async def get_mxid_from_sso(
         given user-agent and IP address and the SSO ID is linked to this matrix
         ID for subsequent calls.
 
-        If allow_existing_users is true the mapping function is only called once
-        and results in:
-
-            1. The use of a previously registered matrix ID. In this case, the
-               SSO ID is linked to the matrix ID. (Note it is possible that
-               other SSO IDs are linked to the same matrix ID.)
-            2. An unused localpart, in which case the user is registered (as
-               discussed above).
-            3. An error if the generated localpart matches multiple pre-existing
-               matrix IDs. Generally this should not happen.
-
         Args:
             auth_provider_id: A unique identifier for this SSO provider, e.g.
                 "oidc" or "saml".
@@ -152,8 +145,9 @@ async def get_mxid_from_sso(
             sso_to_matrix_id_mapper: A callable to generate the user attributes.
                 The only parameter is an integer which represents the amount of
                 times the returned mxid localpart mapping has failed.
-            allow_existing_users: True if the localpart returned from the
-                mapping provider can be linked to an existing matrix ID.
+            grandfather_existing_users: A callable which can return an previously
+                existing matrix ID. The SSO ID is then linked to the returned
+                matrix ID.
 
         Returns:
              The user ID associated with the SSO response.
@@ -171,6 +165,16 @@ async def get_mxid_from_sso(
         if previously_registered_user_id:
             return previously_registered_user_id
 
+        # Check for grandfathering of users.
+        if grandfather_existing_users:
+            previously_registered_user_id = await grandfather_existing_users()
+            if previously_registered_user_id:
+                # Future logins should also match this user ID.
+                await self.store.record_user_external_id(
+                    auth_provider_id, remote_user_id, previously_registered_user_id
+                )
+                return previously_registered_user_id
+
         # Otherwise, generate a new user.
         for i in range(self._MAP_USERNAME_RETRIES):
             try:
@@ -194,33 +198,7 @@ async def get_mxid_from_sso(
 
             # Check if this mxid already exists
             user_id = UserID(attributes.localpart, self.server_name).to_string()
-            users = await self.store.get_users_by_id_case_insensitive(user_id)
-            # Note, if allow_existing_users is true then the loop is guaranteed
-            # to end on the first iteration: either by matching an existing user,
-            # raising an error, or registering a new user. See the docstring for
-            # more in-depth an explanation.
-            if users and allow_existing_users:
-                # If an existing matrix ID is returned, then use it.
-                if len(users) == 1:
-                    previously_registered_user_id = next(iter(users))
-                elif user_id in users:
-                    previously_registered_user_id = user_id
-                else:
-                    # Do not attempt to continue generating Matrix IDs.
-                    raise MappingException(
-                        "Attempted to login as '{}' but it matches more than one user inexactly: {}".format(
-                            user_id, users
-                        )
-                    )
-
-                # Future logins should also match this user ID.
-                await self.store.record_user_external_id(
-                    auth_provider_id, remote_user_id, previously_registered_user_id
-                )
-
-                return previously_registered_user_id
-
-            elif not users:
+            if not await self.store.get_users_by_id_case_insensitive(user_id):
                 # This mxid is free
                 break
         else:

From 063fd8b3ddda089df1254f9f3617d8ee276bd659 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Dec 2020 14:30:22 -0500
Subject: [PATCH 15/17] Do not duplicate an attribute from BaseHandler.

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

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 759478a66234..a0a4dbe826db 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -117,7 +117,6 @@ def __init__(self, hs: "HomeServer"):
         self._http_client = hs.get_proxied_http_client()
         self._auth_handler = hs.get_auth_handler()
         self._registration_handler = hs.get_registration_handler()
-        self._server_name = hs.config.server_name  # type: str
         self._macaroon_secret_key = hs.config.macaroon_secret_key
 
         # identifier for the external_ids table
@@ -737,7 +736,7 @@ def _generate_oidc_session_token(
             A signed macaroon token with the session information.
         """
         macaroon = pymacaroons.Macaroon(
-            location=self._server_name, identifier="key", key=self._macaroon_secret_key,
+            location=self.server_name, identifier="key", key=self._macaroon_secret_key,
         )
         macaroon.add_first_party_caveat("gen = 1")
         macaroon.add_first_party_caveat("type = session")

From 4fa146eb12491d609646c32f12ed17cd2a866da7 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 2 Dec 2020 07:17:27 -0500
Subject: [PATCH 16/17] Revert "Do not duplicate an attribute from
 BaseHandler."

This reverts commit 063fd8b3ddda089df1254f9f3617d8ee276bd659.
---
 synapse/handlers/oidc_handler.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index a0a4dbe826db..759478a66234 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -117,6 +117,7 @@ def __init__(self, hs: "HomeServer"):
         self._http_client = hs.get_proxied_http_client()
         self._auth_handler = hs.get_auth_handler()
         self._registration_handler = hs.get_registration_handler()
+        self._server_name = hs.config.server_name  # type: str
         self._macaroon_secret_key = hs.config.macaroon_secret_key
 
         # identifier for the external_ids table
@@ -736,7 +737,7 @@ def _generate_oidc_session_token(
             A signed macaroon token with the session information.
         """
         macaroon = pymacaroons.Macaroon(
-            location=self.server_name, identifier="key", key=self._macaroon_secret_key,
+            location=self._server_name, identifier="key", key=self._macaroon_secret_key,
         )
         macaroon.add_first_party_caveat("gen = 1")
         macaroon.add_first_party_caveat("type = session")

From 6e0c95caaf0e81c70f8be060d972778323e46391 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 2 Dec 2020 07:18:03 -0500
Subject: [PATCH 17/17] Handle review comment.

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

diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py
index 759478a66234..55c437789049 100644
--- a/synapse/handlers/oidc_handler.py
+++ b/synapse/handlers/oidc_handler.py
@@ -902,8 +902,7 @@ async def grandfather_existing_users() -> Optional[str]:
             if self._allow_existing_users:
                 # If allowing existing users we want to generate a single localpart
                 # and attempt to match it.
-                failures = 0
-                attributes = await oidc_response_to_user_attributes(failures)
+                attributes = await oidc_response_to_user_attributes(failures=0)
 
                 user_id = UserID(attributes.localpart, self.server_name).to_string()
                 users = await self.store.get_users_by_id_case_insensitive(user_id)