Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
MSC3083: Restricting room membership based on membership in other rooms #3083
MSC3083: Restricting room membership based on membership in other rooms #3083
Changes from 54 commits
d5633d1
dfcc467
c81947a
4fc5acf
13e3f18
36b19fb
2919e57
fab5eaa
5afe23a
590b7a4
cbc4515
4eeb27f
0f49611
c7ab867
c1eb461
41dd06d
e81686c
8a3ad47
1d1d356
7061e19
5a58af6
f3e7fba
ed679c7
bfa0dfe
39b9a9d
91c7612
39fdfa3
3bab6bd
8e0b001
0b49932
b4296ef
e5305a7
6d041d4
69aec55
42a34de
808bb1b
87f9938
182c806
1be4019
76333ee
5f2240a
3037232
2c65a03
b9204cc
d95200f
dc945a4
2012466
db40a1c
81a588e
b41a1a3
290f903
ffb9095
7caff82
55b99d2
48c1d9d
88a9404
c0b7f07
77422e2
3885a94
d9cae9b
d128869
2e7db4a
72ffbfe
31a9b2a
db089aa
9699aa8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After sleeping on this, I wonder if it makes more sense to use a generic
M_TRY_ANOTHER_HOMESERVER
error instead. This could then get re-used in the future if necessary. Why this is happening could be included in the error message.Does that sound more useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably best to keep the error code explicit, which allows us more flexibility with how we handle the situation in future. 🤷♂️ though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a close call, but i'd also veer towards being explicit.
M_CANNOT_ALLOW
is awkward and clumsy and doesn't match withM_UNABLE_TO_AUTHORISE_JOIN
though. What aboutM_UNABLE_TO_GRANT_JOIN
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thoughts! I've updated the
M_CANNOT_ALLOW
toM_UNABLE_TO_GRANT_JOIN
, which sounds an awful lot likeM_UNABLE_AUTHORISE_JOIN
, but... 🤷There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for future spec PR writer: update this (and other links) to point at stable versions for historical reasons. It currently redirects to the unstable version due to lack of release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that https://matrix.org/docs/spec/#complete-list-of-room-versions links to https://matrix.org/docs/spec/rooms/v1.html which eventually redirects to https://spec.matrix.org/unstable/rooms/v1/#authorization-rules.
So I don't think there's something better to link to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, this is effectively a note to self. No action needed on your part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It came up during implementation that it was unclear if this should be done unconditionally or only if the event has extra signatures added, etc.
I'm not sure this necessarily needs to be defined here, the requesting server should most likely just use it if it is returned (as long as the event itself is unmodified).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to do this conditionally in the implementation and leave it to another MSC to always sign them.