Properly grandfather data for CAS.

changelog.d/8856.misc

@@ -0,0 +1 @@
+Properly store the mapping of external ID to Matrix ID for CAS users.

synapse/handlers/cas_handler.py

@@ -22,7 +22,7 @@
 from synapse.api.errors import Codes, LoginError
 from synapse.handlers.sso import MappingException, UserAttributes
 from synapse.http.site import SynapseRequest
-from synapse.types import map_username_to_mxid_localpart
+from synapse.types import UserID, map_username_to_mxid_localpart
 
 if TYPE_CHECKING:
     from synapse.app.homeserver import HomeServer
@@ -41,6 +41,7 @@ class CasHandler:
     def __init__(self, hs: "HomeServer"):
         self.hs = hs
         self._hostname = hs.hostname
+        self._store = hs.get_datastore()
         self._auth_handler = hs.get_auth_handler()
         self._registration_handler = hs.get_registration_handler()
 
@@ -267,16 +268,37 @@ async def cas_response_to_user_attributes(failures: int) -> UserAttributes:
             """
             localpart = map_username_to_mxid_localpart(remote_user_id)
 
-            # Append suffix integer if last call to this function failed to produce
-            # a usable mxid.
-            localpart += str(failures) if failures else ""
+            # Due to the grandfathering logic matching any previously registered
+            # mxids it isn't expected for there to be any failures.
+            if failures:
+                raise RuntimeError("CAS is not expected to de-duplicate Matrix IDs")
 
             return UserAttributes(localpart=localpart, display_name=display_name)
 
+        async def grandfather_existing_users() -> Optional[str]:
+            # Since CAS did not used to support storing data into the user_external_ids
+            # tables, we need to attempt to map to existing users.
+            user_id = UserID(
+                map_username_to_mxid_localpart(remote_user_id), self._hostname
+            ).to_string()
+
+            logger.debug(
+                "Looking for existing account based on mapped %s", user_id,
+            )
+
+            users = await self._store.get_users_by_id_case_insensitive(user_id)
+            if users:
+                registered_user_id = list(users.keys())[0]
+                logger.info("Grandfathering mapping to %s", registered_user_id)
+                return 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,
             cas_response_to_user_attributes,
+            grandfather_existing_users,
         )

tests/handlers/test_cas.py

@@ -12,8 +12,6 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 
-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.
@@ -56,50 +54,38 @@ def test_map_cas_user_to_user(self):
         )
         self.assertEqual(mxid, "@test_user:test")
 
-    def test_map_cas_user_to_invalid_localpart(self):
-        """CAS automaps invalid characters to base-64 encoding."""
-        cas_user_id = "föö"
-        display_name = ""
-        mxid = self.get_success(
-            self.handler._map_cas_user_to_matrix_user(
-                cas_user_id, display_name, "user-agent", "10.10.10.10"
-            )
-        )
-        self.assertEqual(mxid, "@f=c3=b6=c3=b6:test")
-
-    def test_map_cas_user_to_user_retries(self):
-        """It can retry generating an MXID if the MXID is already in use."""
+    def test_map_cas_user_to_existing_user(self):
+        """Existing users can log in with CAS 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.
         cas_user_id = "test_user"
         display_name = ""
         mxid = self.get_success(
             self.handler._map_cas_user_to_matrix_user(
                 cas_user_id, display_name, "user-agent", "10.10.10.10"
             )
         )
-        # test_user is already taken, so test_user1 gets registered instead.
-        self.assertEqual(mxid, "@test_user1:test")
+        self.assertEqual(mxid, "@test_user:test")
 
-        # Register all of the potential mxids for a particular CAS 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)
+        # Subsequent calls should map to the same mxid.
+        mxid = self.get_success(
+            self.handler._map_cas_user_to_matrix_user(
+                cas_user_id, display_name, "user-agent", "10.10.10.10"
             )
+        )
+        self.assertEqual(mxid, "@test_user:test")
 
-        # Now attempt to map to a username, this will fail since all potential usernames are taken.
-        cas_user_id = "tester"
-        e = self.get_failure(
+    def test_map_cas_user_to_invalid_localpart(self):
+        """CAS automaps invalid characters to base-64 encoding."""
+        cas_user_id = "föö"
+        display_name = ""
+        mxid = self.get_success(
             self.handler._map_cas_user_to_matrix_user(
                 cas_user_id, display_name, "user-agent", "10.10.10.10"
-            ),
-            MappingException,
-        )
-        self.assertEqual(
-            str(e.value), "Unable to generate a Matrix ID from the SSO response"
+            )
         )
+        self.assertEqual(mxid, "@f=c3=b6=c3=b6:test")