Support getting the device ID explicitly from MAS (#18174)

The context for this is that the Matrix spec allows basically anything
in the device ID. With MSC3861, we're restricting this to strings that
can be represented as scopes.
Whilst this works well for next-gen auth sessions, compatibility/legacy
sessions still can have characters that can't be encoded (mainly spaces)
in them.

To work around that, we added in MAS a behaviour where the device_id is
given as an explicit property of the token introspection response, and
remove it from the scope.
Because we don't expect users to rollout new Synapse and MAS versions in
sync, we needed a way to 'advertise' support for this behaviour: the
easiest way to do that was through an extra header in the introspection
response.

On the longer term, I expect MAS and Synapse to move away from the
introspection endpoint, and instead define a specific API for Synapse ->
MAS communication.

PR on the MAS side:
https://github.com/element-hq/matrix-authentication-service/pull/4067

changelog.d/18174.misc

@@ -0,0 +1 @@
+Support device IDs that can't be represented in a scope when delegating auth to Matrix Authentication Service 0.15.0+.

synapse/api/auth/msc3861_delegated.py

@@ -214,6 +214,9 @@ class MSC3861DelegatedAuth(BaseAuth):
             "Content-Type": "application/x-www-form-urlencoded",
             "User-Agent": str(self._http_client.user_agent, "utf-8"),
             "Accept": "application/json",
+            # Tell MAS that we support reading the device ID as an explicit
+            # value, not encoded in the scope. This is supported by MAS 0.15+
+            "X-MAS-Supports-Device-Id": "1",
         }
 
         args = {"token": token, "token_type_hint": "access_token"}
@@ -409,29 +412,41 @@ class MSC3861DelegatedAuth(BaseAuth):
         else:
             user_id = UserID.from_string(user_id_str)
 
-        # Find device_ids in scope
-        # We only allow a single device_id in the scope, so we find them all in the
-        # scope list, and raise if there are more than one. The OIDC server should be
-        # the one enforcing valid scopes, so we raise a 500 if we find an invalid scope.
-        device_ids = [
-            tok[len(SCOPE_MATRIX_DEVICE_PREFIX) :]
-            for tok in scope
-            if tok.startswith(SCOPE_MATRIX_DEVICE_PREFIX)
-        ]
+        # MAS 0.15+ will give us the device ID as an explicit value for compatibility sessions
+        # If present, we get it from here, if not we get it in thee scope
+        device_id = introspection_result.get("device_id")
+        if device_id is not None:
+            # We got the device ID explicitly, just sanity check that it's a string
+            if not isinstance(device_id, str):
+                raise AuthError(
+                    500,
+                    "Invalid device ID in introspection result",
+                )
+        else:
+            # Find device_ids in scope
+            # We only allow a single device_id in the scope, so we find them all in the
+            # scope list, and raise if there are more than one. The OIDC server should be
+            # the one enforcing valid scopes, so we raise a 500 if we find an invalid scope.
+            device_ids = [
+                tok[len(SCOPE_MATRIX_DEVICE_PREFIX) :]
+                for tok in scope
+                if tok.startswith(SCOPE_MATRIX_DEVICE_PREFIX)
+            ]
 
-        if len(device_ids) > 1:
-            raise AuthError(
-                500,
-                "Multiple device IDs in scope",
-            )
+            if len(device_ids) > 1:
+                raise AuthError(
+                    500,
+                    "Multiple device IDs in scope",
+                )
+
+            device_id = device_ids[0] if device_ids else None
 
-        device_id = device_ids[0] if device_ids else None
         if device_id is not None:
             # Sanity check the device_id
             if len(device_id) > 255 or len(device_id) < 1:
                 raise AuthError(
                     500,
-                    "Invalid device ID in scope",
+                    "Invalid device ID in introspection result",
                 )
 
             # Create the device on the fly if it does not exist

tests/handlers/test_oauth_delegation.py

@@ -380,6 +380,44 @@ class MSC3861OAuthDelegation(HomeserverTestCase):
         )
         self.assertEqual(requester.device_id, DEVICE)
 
+    def test_active_user_with_device_explicit_device_id(self) -> None:
+        """The handler should return a requester with normal user rights and a device ID, given explicitly, as supported by MAS 0.15+"""
+
+        self.http_client.request = AsyncMock(
+            return_value=FakeResponse.json(
+                code=200,
+                payload={
+                    "active": True,
+                    "sub": SUBJECT,
+                    "scope": " ".join([MATRIX_USER_SCOPE]),
+                    "device_id": DEVICE,
+                    "username": USERNAME,
+                },
+            )
+        )
+        request = Mock(args={})
+        request.args[b"access_token"] = [b"mockAccessToken"]
+        request.requestHeaders.getRawHeaders = mock_getRawHeaders()
+        requester = self.get_success(self.auth.get_user_by_req(request))
+        self.http_client.get_json.assert_called_once_with(WELL_KNOWN)
+        self.http_client.request.assert_called_once_with(
+            method="POST", uri=INTROSPECTION_ENDPOINT, data=ANY, headers=ANY
+        )
+        # It should have called with the 'X-MAS-Supports-Device-Id: 1' header
+        self.assertEqual(
+            self.http_client.request.call_args[1]["headers"].getRawHeaders(
+                b"X-MAS-Supports-Device-Id",
+            ),
+            [b"1"],
+        )
+        self._assertParams()
+        self.assertEqual(requester.user.to_string(), "@%s:%s" % (USERNAME, SERVER_NAME))
+        self.assertEqual(requester.is_guest, False)
+        self.assertEqual(
+            get_awaitable_result(self.auth.is_server_admin(requester)), False
+        )
+        self.assertEqual(requester.device_id, DEVICE)
+
     def test_multiple_devices(self) -> None:
         """The handler should raise an error if multiple devices are found in the scope."""