Add support for MSC4560 #2352
No reviewers
Labels
No labels
Abandoned
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
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/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
bob the builder
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
continuwuation/continuwuity!2352
Loading…
Reference in a new issue
No description provided.
Delete branch "ginger/msc4560"
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 pull request adds support for MSC4560 (stripped state requests over federation).
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
@ -0,0 +7,6 @@use crate::{Ruma, router::ClientIdentity};/// # `POST /_matrix/client/unstable/org.continuwuity.msc4560.stripped_state/{room_id_or_alias}`////// Returns a room's stripped state.nit: No link to the MSC?
@ -0,0 +21,9 @@return Err!(Request(TooLarge("Too many vias, please be less indecisive.")));}if body.is_empty() {// hey jimmy, gimme a stripped state with nuthin'return Ok(get_stripped_state::unstable::Response::new(services.globals.server_name().to_owned(),));}This might mislead a client into believing a room exists because it gets a 200 OK, which skips any sort of presence/access checks that may in turn actually mean the room is inaccessible
@ -35,9 +25,6 @@ pub(crate) async fn create_join_event_template_route(State(services): State<crate::State>,body: Ruma<prepare_join_event::v1::Request>,) -> Result<prepare_join_event::v1::Response> {if !services.rooms.metadata.exists(&body.room_id).await {return Err!(Request(NotFound("Room is unknown to this server.")));}Why was this check removed?
It's redundant. If we don't have a room we certainly aren't a resident of it, which is checked immediately afterwards.
@ -114,2 +157,6 @@// We can't sign this, and if we don't sign it our event will fail// the auth check, so we have to bail out here.return Err!(Request(InvalidParam("join_authorized_via_users_server was supplied for a room that is not restricted")));}This isn't true, the field is ignored in this case. There's no need to return an error
Event auth step 5.2.1 runs if
join_authorized_via_users_serveris present, regardless of the room's join rules. We obviously cannot removejoin_authorized_via_users_serverfrom the event without invalidating the signature, so we have two options:I chose option 2 on the grounds that the best course of action is generally not to hand out signatures that we don't need to hand out, even if doing so would currently be harmless.
Oh it DOES I didn't see that, I skipped over to 5.5.3.2, which would have fallen through to the implicit allow. That's dumb, oh well
@ -0,0 +7,6 @@use crate::Ruma;/// # `POST /_matrix/federation/unstable/org.continuwuity.msc4560.stripped_state/{room_id}`////// Returns a room's stripped state.nit: Same thing as the c2s route, no link to the proposal?
@ -0,0 +14,6 @@State(services): State<crate::State>,body: Ruma<get_stripped_state::unstable::Request>,) -> Result<get_stripped_state::unstable::Response> {if body.is_empty() {return Ok(get_stripped_state::unstable::Response::default());}Also doesn't do any sort of access checks that might result in an erroneous OK
@ -240,2 +273,3 @@// to federation.auth_user = self.select_authorising_user(room_id, sender_user, &state_lock).select_authorizing_user(room_id, sender_user, state_lock)👎
@ -809,3 +816,3 @@/// Attempts to find a user who is able to issue an invite in the target/// room.pub async fn select_authorising_user<'a>(pub async fn select_authorizing_user<'a>(was there actually any reason to rename this? Given it is actually spelt with an S in the spec
The zpec uzez a Z in field and error code namez. I don't feel that strongly about it though.
@ -941,0 +936,4 @@.meowlnir_accept_make_join(room_id.to_owned(), user_id.to_owned()).await} else {// Servers can't pass the spam checker.yes they can
(no they can't)
Not with the current spam checker API. I've updated the comment to clarify that.
@ -539,0 +565,8 @@.build_local_stripped_state(room_id, state, num_joined_members).await{if self.user_may_see_summary(querying_user, room_id).await {Accessibility::Accessible((self.services.globals.server_name().to_owned(), state))} else {Accessibility::Inaccessible}Wouldn't it be cheaper to check this before building the local state?
The summary visibility functions assume that the room exists. We have to try to build the summary first so that we can return
NotFoundif it doesn't.@ -539,0 +588,8 @@.build_local_stripped_state(room_id, state, num_joined_members).await{if self.server_may_see_summary(querying_server, room_id).await {Accessibility::Accessible(state)} else {Accessibility::Inaccessible}Wouldn't it be cheaper to check this before building the local state?
(same as other case)
bb3a802c294f231db72aConflicts with main and needs formatting, otherwise lgtm
36b91da5460a5e9ab7acWIP: Add support for MSC4560to Add support for MSC45600a5e9ab7ac138dd61f82