fix: improve appservice authentication and thread pagination UX #923
No reviewers
Labels
No labels
Bug
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/MSC
Matrix/Media
Meta
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
To-Merge
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!923
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tom/fixes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR addresses authentication issues affecting appservice users and improves the thread pagination experience.
Appservice Authentication (#813): Appservices were failing to authenticate because their
sender_localpart
user was never created in the database. This fix ensures the user account is created during appservice startup and registration, and automatically reactivates the account if it was previously deactivated. Fixes #813.Token Collision Prevention: Prevents access tokens from being shared between users and appservices, which could cause authentication ambiguity. On startup, any existing collisions are resolved in favour of appservices, and new collisions are prevented during token generation. Token lookups are also optimised using concurrent queries (adapted from tuwunel).
Thread Pagination: Replaces
PduCount
pagination tokens with persistentShortEventId
tokens, but retain backwards-compatibility for both types. The thread root event is now included when paginating to the beginning of a thread, duplicate events at boundaries are fixed, and token format is consistent across/relations
and/messages
endpoints.I'm currently running
forgejo.ellis.link/continuwuation/continuwuity:branch-tom-threads
and confirmed threading works great - the attached screenshot shows it's correctly telling the client there are no more events before the top one, and including the "hey" root message from outside the thread that the first in-thread reply was responding to.I'm currently using appservices without an issue, but may need someone who originally experienced the issue in #813 to replicate and confirm if this has resolved their issues.
OK, lotta comments here. Good news - they're mostly code quality!
@ -64,0 +64,4 @@
/// Parse a pagination token, trying ShortEventId first, then falling back to
/// PduCount
async fn parse_pagination_token(
_services: &Services,
if it needs access to services and isn't a top level API request, it should be a service, but it doesn't, so it doesn't need these unused params
Agreed! I've removed the unused parameters and extracted to a shared utils module 🙂
@ -64,0 +69,4 @@
token: Option<&str>,
default: PduCount,
) -> Result<PduCount> {
let Some(token) = token else {
this shouldn't be a part of this function. use unwrap_or* functions
I didn't see that, thanks - I've rewritten to use
.map(parse_token).transpose()?.unwrap_or_else(||...)
so defaults are handled at call sites 👌@ -88,2 +117,2 @@
.transpose()?
.unwrap_or_else(|| match body.dir {
let from: PduCount =
parse_pagination_token(&services, room_id, body.from.as_deref(), match body.dir {
applying the feedback on the referenced function, you'd be able to do this like the original code, but with
.map(parse_pagination_token)
instead.Good shout, both files now use the
.map(parse_token).transpose()?
pattern - much cleaner! 😄@ -93,2 +122,3 @@
.await?;
let to: Option<PduCount> = body.to.as_deref().map(str::parse).flat_ok();
let to: Option<PduCount> = if let Some(to_str) = body.to.as_deref() {
see above
Ditto!
@ -22,1 +26,4 @@
/// Parse a pagination token, trying ShortEventId first, then falling back to
/// PduCount
async fn parse_pagination_token(
duplicated code
Yeah, sorry... I've created
src/api/client/utils.rs
with a sharedparse_pagination_token
andcount_to_token
so am reusing those instead of duplicating now.@ -23,0 +53,4 @@
}
/// Convert a PduCount to a token string (using the underlying ShortEventId)
fn count_to_token(count: PduCount) -> String {
duplicated code
Ditto
@ -116,3 +150,1 @@
| Direction::Forward => PduCount::min(),
| Direction::Backward => PduCount::max(),
});
let start: PduCount = parse_pagination_token(services, room_id, from, match dir {
see above
Ditto
@ -119,2 +154,3 @@
.await?;
let to: Option<PduCount> = to.map(str::parse).flat_ok();
let to: Option<PduCount> = if let Some(to_str) = to {
see above
Ditto!
@ -162,0 +232,4 @@
| Direction::Forward => events.last(),
| Direction::Backward => events.first(),
}
.map(|(count, _)| count_to_token(*count))
for reference, could be
.map(at!(0))
.map(count_to_token)
, it's just a matter of preferenceAs above 🙂
@ -162,0 +238,4 @@
};
// Build the response chunk with thread root if needed
let chunk: Vec<_> = if let Some(root) = root_event {
Default behaviour should be written once. Don't worry if that's too difficult though.
I wasn't sure how to do this before, but have had a go at simplifying to use
root_event.into_iter()...chain(...)
pattern - hopefully this looks right to you?@ -345,0 +355,4 @@
.map_ok(Token::Appservice);
pin_mut!(user_token, appservice_token);
match select_ok([Left(user_token), Right(appservice_token)]).await {
I'm hoping that this will only return an err if both futures error.
Exactly that, yeah - I've added a comment to confirm this is intentional 👌
@ -56,0 +57,4 @@
if let Ok((user_id, device_id)) = self
.services
.users
.find_from_token(®istration.as_token)
What might be missing from the overarching logic here is checking that appservices don't conflict with each other?
I can't believe I missed that! I've added a collision check at startup, and also made it so you can't configure a new appservice with the same token - it allows appservices with the same name through, so you're allowed to update the config of an appservice without it demanding the token change each time 😅
@ -114,3 +183,3 @@
}
pub async fn find_from_token(&self, token: &str) -> Option<RegistrationInfo> {
pub async fn find_from_token(&self, token: &str) -> Result<RegistrationInfo> {
I'm guessing this is Jason code to use select_ok, but it still feels wrong :(
I replied in Matrix then remembered I can reply here! 😅
The user
find_from_token
was written to useResult
so it seemed to make sense to align the two so we can incorporate the "check both together" method rather than "check Option for appservice then get Result for user".I can switch to Option for both instead, it just seemed a lot cleaner than trying to convert at the point we're receiving two different types of outputs from the two functions - what do you think?
(in the meantime, I've added a brief doc comment explaining why Result is used!)
Yeah I get it.
@ -64,3 +65,3 @@
let mut current = ArrayVec::<u8, 16>::new();
current.extend(target.to_be_bytes());
current.extend(from.saturating_inc(dir).into_unsigned().to_be_bytes());
current.extend(from_unsigned.to_be_bytes());
Mind explaining this change? not sure what's going on here
Oh yeah, it was a fun one... the old saturating_inc could skip events at min/max boundaries. It was making a mess of paging through threads, so now we query from the exact position and filter excludes it explicitly - I've also added a comment to clarify what's going on.
@ -410,0 +417,4 @@
.await
.is_ok()
{
let new_token = utils::random_string(32);
Uh, I think handling the token regeneration here could result in unexpected behaviour if in future somewhere attempts to set a specific token for a user. Either return an error and have the caller match on it to retry, or don't take the token as an arg (always generate one). You could combine the best of both with a helper function that handles regeneration.
I'm happy to refactor this either way, though I think it's a risk to even entertain the possibility that users provide a custom token?
AFAIK, the only reason we do it for appservices is that it's provided by the admin and part of the YAML confg snippet given to the admin command, so generating the token and providing it back afterwards would be worse UX.
I'll put together a function for generating a guaranteed unique token though, so the flow can use that function, and that codepath might be useful for something else in the future 🙂
I've split this into separate functions -
generate_unique_token()
for creating tokens, thenset_token()
now fails on collision rather than silently regenerating. Hopefully that's ok?e56a7ab115
077eac688e
077eac688e
8e98d533f6
8e98d533f6
971a82425a
971a82425a
8b295b1e22
8b295b1e22
583cb924f1