fix: Allow device ID to be optional when iterating over /messages #757

Merged
Jade merged 3 commits from nex/fix-appservices-history into main 2025-04-18 13:01:10 +00:00
Owner

This fixes #738 by allowing the device ID to be optional, which (as far as I can tell) is only the case for appservices.

I've tested this and I can check history using the Meowlnir appservice:

 nex@ivy  ~/GolandProjects/meowlnir  ↱ nexy7574/protections  curl -H 'Authorization: Bearer <omitted>' https://continuwuity-dev.nexy7574.co.uk/_matrix/client/v3/rooms/\!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk/messages\?user_id\=@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk\&dir\=f | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4096    0  4096    0     0   240k      0 --:--:-- --:--:-- --:--:--  250k
{
  "start": "-9223372036854775808",
  "end": "1004",
  "chunk": [
    {
      "content": {
        "room_version": "11"
      },
      "event_id": "$rEueTAtRtxdNfwknBCWZW6XW3tYNlZEe8PDgJHdCbY0",
      "origin_server_ts": 1744732909706,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "",
      "type": "m.room.create",
      "unsigned": {
        "age": 1936187
      }
    },
    {
      "content": {
        "displayname": "nex 🏳‍⚧",
        "is_direct": false,
        "membership": "join"
      },
      "event_id": "$MfC67VVyFblPMVnnNtUopDavTtYbsa7kjXBJLDX581c",
      "origin_server_ts": 1744732909711,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "@nex:continuwuity-dev.nexy7574.co.uk",
      "type": "m.room.member",
      "unsigned": {
        "age": 1936182
      }
    },
    {
      "content": {
        "ban": 50,
        "events": {
          "m.poll.response": 0,
          "m.room.encryption": 100,
          "m.room.history_visibility": 100,
          "m.room.power_levels": 100,
          "m.room.server_acl": 100,
          "m.room.tombstone": 100,
          "org.matrix.msc3381.poll.response": 0
        },
        "events_default": 0,
        "invite": 0,
        "kick": 50,
        "notifications": {
          "room": 50
        },
        "redact": 50,
        "state_default": 50,
        "users": {
          "@nex:continuwuity-dev.nexy7574.co.uk": 100
        },
        "users_default": 0
      },
      "event_id": "$tlJlnGydRuLT2il3zOVtIsrc1r5z7d4v-OsM5WkTu5U",
      "origin_server_ts": 1744732909714,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "",
      "type": "m.room.power_levels",
      "unsigned": {
        "age": 1936179
      }
    },
    {
      "content": {
        "join_rule": "invite"
      },
      "event_id": "$fvp_h0FpUgi8Pvllhz7wESOWYBDdnvkqTttJEjcb0bE",
      "origin_server_ts": 1744732909717,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "",
      "type": "m.room.join_rules",
      "unsigned": {
        "age": 1936176
      }
    },
    {
      "content": {
        "history_visibility": "shared"
      },
      "event_id": "$VJShJi8UyXWYzWwGdYIJS-LphSCViMSB_QdROcTtnRw",
      "origin_server_ts": 1744732909719,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "",
      "type": "m.room.history_visibility",
      "unsigned": {
        "age": 1936174
      }
    },
    {
      "content": {
        "guest_access": "can_join"
      },
      "event_id": "$fx87WB9TWBo8TUbumqZECChDR3VETM9NZCntRWm5vIA",
      "origin_server_ts": 1744732909721,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "",
      "type": "m.room.guest_access",
      "unsigned": {
        "age": 1936172
      }
    },
    {
      "content": {
        "guest_access": "can_join"
      },
      "event_id": "$06pIHzq11TaaIwWgXv3aq8ksFzGf20IspnJGrG1167Y",
      "origin_server_ts": 1744732909723,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "",
      "type": "m.room.guest_access",
      "unsigned": {
        "age": 1936170,
        "prev_content": {
          "guest_access": "can_join"
        },
        "prev_sender": "@nex:continuwuity-dev.nexy7574.co.uk",
        "replaces_state": "$fx87WB9TWBo8TUbumqZECChDR3VETM9NZCntRWm5vIA"
      }
    },
    {
      "content": {
        "name": "lab"
      },
      "event_id": "$eOdpkIfl_qZ8z4eE8UHV07mhUkRqUeiXyMk-BklMpjo",
      "origin_server_ts": 1744732909725,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "",
      "type": "m.room.name",
      "unsigned": {
        "age": 1936168
      }
    },
    {
      "content": {
        "displayname": "test",
        "is_direct": false,
        "membership": "invite"
      },
      "event_id": "$rwLI_9a-fs1d6LnZ46ff0vaocH4V3_x5mqSwmPZ7urA",
      "origin_server_ts": 1744734584319,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@nex:continuwuity-dev.nexy7574.co.uk",
      "state_key": "@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk",
      "type": "m.room.member",
      "unsigned": {
        "age": 261574
      }
    },
    {
      "content": {
        "displayname": "test",
        "membership": "join"
      },
      "event_id": "$AvGwpheH3Rl0HBSbZ4i30L5XvMPHIMkdCXEJ5rbDHp8",
      "origin_server_ts": 1744734616777,
      "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk",
      "sender": "@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk",
      "state_key": "@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk",
      "type": "m.room.member",
      "unsigned": {
        "age": 229116,
        "prev_content": {
          "displayname": "test",
          "is_direct": false,
          "membership": "invite"
        },
        "prev_sender": "@nex:continuwuity-dev.nexy7574.co.uk",
        "replaces_state": "$rwLI_9a-fs1d6LnZ46ff0vaocH4V3_x5mqSwmPZ7urA"
      }
    }
  ]
}
This fixes #738 by allowing the device ID to be optional, which (as far as I can tell) is only the case for appservices. I've tested this and I can check history using the Meowlnir appservice: <details> ``` nex@ivy  ~/GolandProjects/meowlnir  ↱ nexy7574/protections  curl -H 'Authorization: Bearer <omitted>' https://continuwuity-dev.nexy7574.co.uk/_matrix/client/v3/rooms/\!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk/messages\?user_id\=@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk\&dir\=f | jq % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 4096 0 4096 0 0 240k 0 --:--:-- --:--:-- --:--:-- 250k { "start": "-9223372036854775808", "end": "1004", "chunk": [ { "content": { "room_version": "11" }, "event_id": "$rEueTAtRtxdNfwknBCWZW6XW3tYNlZEe8PDgJHdCbY0", "origin_server_ts": 1744732909706, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "", "type": "m.room.create", "unsigned": { "age": 1936187 } }, { "content": { "displayname": "nex 🏳‍⚧", "is_direct": false, "membership": "join" }, "event_id": "$MfC67VVyFblPMVnnNtUopDavTtYbsa7kjXBJLDX581c", "origin_server_ts": 1744732909711, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "@nex:continuwuity-dev.nexy7574.co.uk", "type": "m.room.member", "unsigned": { "age": 1936182 } }, { "content": { "ban": 50, "events": { "m.poll.response": 0, "m.room.encryption": 100, "m.room.history_visibility": 100, "m.room.power_levels": 100, "m.room.server_acl": 100, "m.room.tombstone": 100, "org.matrix.msc3381.poll.response": 0 }, "events_default": 0, "invite": 0, "kick": 50, "notifications": { "room": 50 }, "redact": 50, "state_default": 50, "users": { "@nex:continuwuity-dev.nexy7574.co.uk": 100 }, "users_default": 0 }, "event_id": "$tlJlnGydRuLT2il3zOVtIsrc1r5z7d4v-OsM5WkTu5U", "origin_server_ts": 1744732909714, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "", "type": "m.room.power_levels", "unsigned": { "age": 1936179 } }, { "content": { "join_rule": "invite" }, "event_id": "$fvp_h0FpUgi8Pvllhz7wESOWYBDdnvkqTttJEjcb0bE", "origin_server_ts": 1744732909717, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "", "type": "m.room.join_rules", "unsigned": { "age": 1936176 } }, { "content": { "history_visibility": "shared" }, "event_id": "$VJShJi8UyXWYzWwGdYIJS-LphSCViMSB_QdROcTtnRw", "origin_server_ts": 1744732909719, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "", "type": "m.room.history_visibility", "unsigned": { "age": 1936174 } }, { "content": { "guest_access": "can_join" }, "event_id": "$fx87WB9TWBo8TUbumqZECChDR3VETM9NZCntRWm5vIA", "origin_server_ts": 1744732909721, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "", "type": "m.room.guest_access", "unsigned": { "age": 1936172 } }, { "content": { "guest_access": "can_join" }, "event_id": "$06pIHzq11TaaIwWgXv3aq8ksFzGf20IspnJGrG1167Y", "origin_server_ts": 1744732909723, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "", "type": "m.room.guest_access", "unsigned": { "age": 1936170, "prev_content": { "guest_access": "can_join" }, "prev_sender": "@nex:continuwuity-dev.nexy7574.co.uk", "replaces_state": "$fx87WB9TWBo8TUbumqZECChDR3VETM9NZCntRWm5vIA" } }, { "content": { "name": "lab" }, "event_id": "$eOdpkIfl_qZ8z4eE8UHV07mhUkRqUeiXyMk-BklMpjo", "origin_server_ts": 1744732909725, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "", "type": "m.room.name", "unsigned": { "age": 1936168 } }, { "content": { "displayname": "test", "is_direct": false, "membership": "invite" }, "event_id": "$rwLI_9a-fs1d6LnZ46ff0vaocH4V3_x5mqSwmPZ7urA", "origin_server_ts": 1744734584319, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@nex:continuwuity-dev.nexy7574.co.uk", "state_key": "@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk", "type": "m.room.member", "unsigned": { "age": 261574 } }, { "content": { "displayname": "test", "membership": "join" }, "event_id": "$AvGwpheH3Rl0HBSbZ4i30L5XvMPHIMkdCXEJ5rbDHp8", "origin_server_ts": 1744734616777, "room_id": "!SUAwlr2OqA4oYKq05F:continuwuity-dev.nexy7574.co.uk", "sender": "@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk", "state_key": "@as.meowlnir.test:continuwuity-dev.nexy7574.co.uk", "type": "m.room.member", "unsigned": { "age": 229116, "prev_content": { "displayname": "test", "is_direct": false, "membership": "invite" }, "prev_sender": "@nex:continuwuity-dev.nexy7574.co.uk", "replaces_state": "$rwLI_9a-fs1d6LnZ46ff0vaocH4V3_x5mqSwmPZ7urA" } } ] } ``` </details>
nex added the
Bug
Matrix/Appservices
labels 2025-04-15 16:40:03 +00:00
nex added 1 commit 2025-04-15 16:40:04 +00:00
requested review from Owners 2025-04-15 16:40:12 +00:00
Jade requested changes 2025-04-15 19:16:11 +00:00
Dismissed
Jade left a comment
Owner

Looks mostly good to me, just some code style stuff to fix

Looks mostly good to me, just some code style stuff to fix
@ -69,3 +64,2 @@
debug_assert!(IGNORED_MESSAGE_TYPES.is_sorted(), "IGNORED_MESSAGE_TYPES is not sorted");
let sender = body.sender();
let (sender_user, sender_device) = sender;
let sender_user = body.sender_user();
Owner

What's the difference here?

What's the difference here?
Author
Owner

the important part is the line below - (user, device) = sender panics if there's no device. Splitting it out allows device to be a reference without expecting it to exist

the important part is the line below - `(user, device) = sender` panics if there's no device. Splitting it out allows device to be a reference without expecting it to exist
Owner

I see, because sender() calls sender_device() which unwraps:

	#[inline]
	pub(crate) fn sender_device(&self) -> &DeviceId {
		self.sender_device
			.as_deref()
			.expect("user must be authenticated and device identified")
	}
I see, because `sender()` calls `sender_device()` which unwraps: ```rs #[inline] pub(crate) fn sender_device(&self) -> &DeviceId { self.sender_device .as_deref() .expect("user must be authenticated and device identified") } ```
Author
Owner

Precisely yeah. That's what was causing the issue here (and presumably will cause the same issue elsewhere, however I checked and this isn't referenced anywhere else that appservices are likely to hit)

Precisely yeah. That's what was causing the issue here (and presumably will cause the same issue elsewhere, however I checked and this isn't referenced anywhere else that appservices are likely to hit)
Jade marked this conversation as resolved
@ -139,0 +136,4 @@
} else {
lazy_loading::Context {
user_id: sender_user,
device_id: <&DeviceId>::from(""),
Owner

Instead of doing an if statement, use a method like unwrap_or_default

Instead of doing an if statement, use a method like [unwrap_or_default](https://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap_or_default)
Jade marked this conversation as resolved
@ -8,3 +8,3 @@
use std::sync::OnceLock;
static BRANDING: &str = "conduwuit";
static BRANDING: &str = "continuwuity";
Owner

Strictly should be a separate commit, but I don't really care

Strictly should be a separate commit, but I don't really care
Author
Owner

I can pull that out and push it in later, but it seems a bit silly making a whole new PR for it and might as well shoehorn it in this one

I can pull that out and push it in later, but it seems a bit silly making a whole new PR for it and might as well shoehorn it in this one
Owner

You can have multiple commits in a PR! As said, dw about it but it's not something to make a habit of.

You can have multiple commits in a PR! As said, dw about it but it's not something to make a habit of.
Jade marked this conversation as resolved
nex added 1 commit 2025-04-15 19:50:38 +00:00
Jade added 1 commit 2025-04-15 20:09:19 +00:00
nex closed this pull request 2025-04-15 20:33:23 +00:00
Author
Owner

did not mean to do that whoopsies

did not mean to do that whoopsies
Jade reopened this pull request 2025-04-15 20:34:50 +00:00
Owner

Undeleted :)

Undeleted :)
nex added 1 commit 2025-04-15 20:35:11 +00:00
Author
Owner

I have been writing software to a professional standard for half a decade and I can be trusted with git, I swear

I have been writing software to a professional standard for half a decade and I can be trusted with git, I swear
Owner

image
What's the mystery commit with no content?

![image](/attachments/e1fff248-4cb7-4a6b-81d0-65eebc3f7178) What's the mystery commit with no content?
Author
Owner

Couldn't push my local copy without it for whatever god forsaken reason, do you want me to rewrite history?

Couldn't push my local copy without it for whatever god forsaken reason, do you want me to rewrite history?
Jade added 1 commit 2025-04-15 20:42:20 +00:00
Owner

nah, I'll do that on merge

nah, I'll do that on merge
Owner

Latest commit is just a fancy way of writing:

sender_device.map(|id| id.as_ref()).unwrap_or_else(|| <&DeviceId>::from(""))

as demanded by clippy btw.
The as_ref was done implicitly in earlier code, but type inference isn't quite good enough for it to work through the unwrap_or_else.

Latest commit is just a fancy way of writing: ```rs sender_device.map(|id| id.as_ref()).unwrap_or_else(|| <&DeviceId>::from("")) ``` as demanded by clippy btw. The as_ref was done implicitly in earlier code, but type inference isn't quite good enough for it to work through the unwrap_or_else.
Owner

But anyway down to +- 5 from +23 -18!

But anyway down to +- 5 from +23 -18!
Jade force-pushed nex/fix-appservices-history from e9a1bb7f68 to 41a5def0ac 2025-04-16 12:38:59 +00:00 Compare
Jade added 1 commit 2025-04-16 12:51:50 +00:00
In the previous commit, app services would all appear to be the same
device when accessing the same user. This sets the device ID to be the
appservice ID when available to avoid possible clobbering.
Jade added 1 commit 2025-04-16 12:52:55 +00:00
Jade force-pushed nex/fix-appservices-history from 7b1d8d56f6 to b77590e072 2025-04-16 13:01:36 +00:00 Compare
Jade force-pushed nex/fix-appservices-history from b77590e072 to 4911fc9951 2025-04-17 12:22:14 +00:00 Compare
Jade approved these changes 2025-04-18 12:59:14 +00:00
Jade left a comment
Owner

It's been running on my instances for a couple of days now, no problems.

It's been running on my instances for a couple of days now, no problems.
Jade force-pushed nex/fix-appservices-history from 4911fc9951 to dc599db19c 2025-04-18 13:00:41 +00:00 Compare
Jade merged commit dc599db19c into main 2025-04-18 13:01:10 +00:00
Jade deleted branch nex/fix-appservices-history 2025-04-18 13:01:10 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: continuwuation/continuwuity#757
No description provided.