-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add spec for MSC2449: Require users to have visibility on an event when submitting reports #1517
Add spec for MSC2449: Require users to have visibility on an event when submitting reports #1517
Conversation
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.
Looks good overall! Just a few nitpicks looking back and forth between the MSC.
summary: Reports an event as inappropriate. You must have permission to | ||
retrieve this event e.g. by being a member in the room for this event. |
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.
This feels a bit hand-wavy. The requirements laid out by MSC2249 are that you must be currently joined to the room that the reported event is in.
The server MUST verify that the user has permission to view the event | ||
before accepting a report. |
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.
Again, this feels looser than what the MSC stated, which is that the reporter must currently be joined to the room that the reported event is in. Perhaps:
The server MUST verify that the user has permission to view the event | |
before accepting a report. | |
The server MUST verify that the user reporting the event is currently | |
joined to the room the event is in before accepting a report. |
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.
further, a changed-in
annotation:
The server MUST verify that the user has permission to view the event | |
before accepting a report. | |
{{< changed-in v="1.7" >}} The server MUST verify that the user | |
reporting the event is currently joined to the room the event is | |
in before accepting a report. |
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.
plus what anoa said - otherwise lgtm, thank you!
The server MUST verify that the user has permission to view the event | ||
before accepting a report. |
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.
further, a changed-in
annotation:
The server MUST verify that the user has permission to view the event | |
before accepting a report. | |
{{< changed-in v="1.7" >}} The server MUST verify that the user | |
reporting the event is currently joined to the room the event is | |
in before accepting a report. |
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.
otherwise lgtm.
The MSC was very clear that the condition was being joined to the room, not visibility of the event - my comments are to try and bring that in line.
It also looks like you have a couple unreviewed threads from @anoadragon453 to look at 😇 (I've added to them as well)
I plan to take over the maintenance of this PR in the author's place. |
Co-authored-by: Travis Ralston <travpc@gmail.com>
now how did that happen
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.
This PR has been updated with changes from review.
it wasn't displaying correctly in the rendered spec otherwise
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.
otherwise lgtm - thanks :)
Co-authored-by: Travis Ralston <travpc@gmail.com>
Oh, apparently I forgot I wasn't working on this...sorry @anoadragon453! |
matrix-org/matrix-spec-proposals#2249
Preview: https://pr1517--matrix-spec-previews.netlify.app