Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removal: Remove experimental support for MSC3852 #17640

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+1 −39
Open

Removal: Remove experimental support for MSC3852 #17640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
326fc0b
Removal: Remove experimental support for MSC3852
rahulporuri Aug 30, 2024
60875dc
Add changelog
rahulporuri Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/17640.removal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove experimental support for MSC 3852.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically MSC's are written without a space in-between "MSC" and the number:

Suggested change
Remove experimental support for MSC 3852.
Remove experimental support for [MSC3852](https://github.com/matrix-org/matrix-spec-proposals/pull/3852).

7 changes: 0 additions & 7 deletions docs/admin_api/user_admin_api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,15 +857,13 @@ A response body like the following is returned:
"device_id": "QBUAZIFURK",
"display_name": "android",
"last_seen_ip": "1.2.3.4",
"last_seen_user_agent": "Mozilla/5.0 (X11; Linux x86_64; rv:103.0) Gecko/20100101 Firefox/103.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, removing this field would be a backwards-incompatible change to the Admin API.

While this field was added to the Admin API in matrix-org/synapse#13549, it was not connected to the experimental_features.msc3852_enabled config option. But rather, always returned and not considered experimental.

It'd be best to keep this field in the Admin API. I could also see it as having been useful for moderation purposes.

"last_seen_ts": 1474491775024,
"user_id": "<user_id>"
},
{
"device_id": "AUIECTSRND",
"display_name": "ios",
"last_seen_ip": "1.2.3.5",
"last_seen_user_agent": "Mozilla/5.0 (X11; Linux x86_64; rv:103.0) Gecko/20100101 Firefox/103.0",
"last_seen_ts": 1474491775025,
"user_id": "<user_id>"
}
Expand All @@ -892,8 +890,6 @@ The following fields are returned in the JSON response body:
Absent if no name has been set.
- `last_seen_ip` - The IP address where this device was last seen.
(May be a few minutes out of date, for efficiency reasons).
- `last_seen_user_agent` - The user agent of the device when it was last seen.
(May be a few minutes out of date, for efficiency reasons).
- `last_seen_ts` - The timestamp (in milliseconds since the unix epoch) when this
devices was last seen. (May be a few minutes out of date, for efficiency reasons).
- `user_id` - Owner of device.
Expand Down Expand Up @@ -972,7 +968,6 @@ A response body like the following is returned:
"device_id": "<device_id>",
"display_name": "android",
"last_seen_ip": "1.2.3.4",
"last_seen_user_agent": "Mozilla/5.0 (X11; Linux x86_64; rv:103.0) Gecko/20100101 Firefox/103.0",
"last_seen_ts": 1474491775024,
"user_id": "<user_id>"
}
Expand All @@ -994,8 +989,6 @@ The following fields are returned in the JSON response body:
Absent if no name has been set.
- `last_seen_ip` - The IP address where this device was last seen.
(May be a few minutes out of date, for efficiency reasons).
- `last_seen_user_agent` - The user agent of the device when it was last seen.
(May be a few minutes out of date, for efficiency reasons).
- `last_seen_ts` - The timestamp (in milliseconds since the unix epoch) when this
devices was last seen. (May be a few minutes out of date, for efficiency reasons).
- `user_id` - Owner of device.
Expand Down
3 changes: 0 additions & 3 deletions synapse/config/experimental.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,9 +350,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# MSC3848: Introduce errcodes for specific event sending failures
self.msc3848_enabled: bool = experimental.get("msc3848_enabled", False)

# MSC3852: Expose last seen user agent field on /_matrix/client/v3/devices.
self.msc3852_enabled: bool = experimental.get("msc3852_enabled", False)

# MSC3866: M_USER_AWAITING_APPROVAL error code
raw_msc3866_config = experimental.get("msc3866", {})
self.msc3866 = MSC3866Config(**raw_msc3866_config)
Expand Down
2 changes: 0 additions & 2 deletions synapse/handlers/device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ def __init__(self, hs: "HomeServer"):
self._auth_handler = hs.get_auth_handler()
self._event_sources = hs.get_event_sources()
self.server_name = hs.hostname
self._msc3852_enabled = hs.config.experimental.msc3852_enabled
self._query_appservices_for_keys = (
hs.config.experimental.msc3984_appservice_key_query
)
Expand Down Expand Up @@ -1131,7 +1130,6 @@ def _update_device_from_client_ips(
ip = client_ips.get((device["user_id"], device["device_id"]))
device.update(
{
"last_seen_user_agent": ip.user_agent if ip else None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave this in, but keep the following code in the client endpoints:

        for device in devices:
            last_seen_user_agent = device["last_seen_user_agent"]
            del device["last_seen_user_agent"]

such that the field isn't exposed to clients, but remains exposed to the Admin API.

"last_seen_ts": ip.last_seen if ip else None,
"last_seen_ip": ip.ip if ip else None,
}
Expand Down
25 changes: 0 additions & 25 deletions synapse/rest/client/devices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,26 +61,13 @@ def __init__(self, hs: "HomeServer"):
self.hs = hs
self.auth = hs.get_auth()
self.device_handler = hs.get_device_handler()
self._msc3852_enabled = hs.config.experimental.msc3852_enabled

async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
requester = await self.auth.get_user_by_req(request, allow_guest=True)
devices = await self.device_handler.get_devices_by_user(
requester.user.to_string()
)

# If MSC3852 is disabled, then the "last_seen_user_agent" field will be
# removed from each device. If it is enabled, then the field name will
# be replaced by the unstable identifier.
#
# When MSC3852 is accepted, this block of code can just be removed to
# expose "last_seen_user_agent" to clients.
for device in devices:
last_seen_user_agent = device["last_seen_user_agent"]
del device["last_seen_user_agent"]
if self._msc3852_enabled:
device["org.matrix.msc3852.last_seen_user_agent"] = last_seen_user_agent

return 200, {"devices": devices}


Expand Down Expand Up @@ -148,7 +135,6 @@ def __init__(self, hs: "HomeServer"):
assert isinstance(handler, DeviceHandler)
self.device_handler = handler
self.auth_handler = hs.get_auth_handler()
self._msc3852_enabled = hs.config.experimental.msc3852_enabled
self._msc3861_oauth_delegation_enabled = hs.config.experimental.msc3861.enabled

async def on_GET(
Expand All @@ -161,17 +147,6 @@ async def on_GET(
if device is None:
raise NotFoundError("No device found")

# If MSC3852 is disabled, then the "last_seen_user_agent" field will be
# removed from each device. If it is enabled, then the field name will
# be replaced by the unstable identifier.
#
# When MSC3852 is accepted, this block of code can just be removed to
# expose "last_seen_user_agent" to clients.
last_seen_user_agent = device["last_seen_user_agent"]
del device["last_seen_user_agent"]
if self._msc3852_enabled:
device["org.matrix.msc3852.last_seen_user_agent"] = last_seen_user_agent

return 200, device

class DeleteBody(RequestBodyModel):
Expand Down
1 change: 0 additions & 1 deletion tests/handlers/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,6 @@ def test_devices(self) -> None:
self.assertEqual(args[0][0]["user_id"], self.user2)
self.assertIn("device_id", args[0][0])
self.assertIsNone(args[0][0]["display_name"])
self.assertIsNone(args[0][0]["last_seen_user_agent"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep this in.

self.assertIsNone(args[0][0]["last_seen_ts"])
self.assertIsNone(args[0][0]["last_seen_ip"])

Expand Down
1 change: 0 additions & 1 deletion tests/rest/admin/test_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1379,7 +1379,6 @@ def _validate_attributes_of_device_response(self, response: JsonDict) -> None:
self.assertEqual(response["device_id"], self.other_user_device_id)
self.assertEqual(response["display_name"], self.other_user_device_display_name)
self.assertEqual(response["last_seen_ip"], self.other_user_client_ip)
self.assertEqual(response["last_seen_user_agent"], self.other_user_user_agent)
self.assertIsInstance(response["last_seen_ts"], int)
self.assertGreater(response["last_seen_ts"], 0)

Expand Down
Loading