Skip to content
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

Merged
merged 11 commits into from Aug 15, 2023

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented May 9, 2023

@turt2live turt2live added the release-blocker Blocks the next release from happening label May 16, 2023
Copy link
Member

@anoadragon453 anoadragon453 left a 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.

changelogs/client_server/newsfragments/1517.feature Outdated Show resolved Hide resolved
Comment on lines 32 to 33
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.
Copy link
Member

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.

Comment on lines 22 to 23
The server MUST verify that the user has permission to view the event
before accepting a report.
Copy link
Member

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:

Suggested change
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.

Copy link
Member

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:

Suggested change
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.

data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a 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!

data/api/client-server/report_content.yaml Show resolved Hide resolved
Comment on lines 22 to 23
The server MUST verify that the user has permission to view the event
before accepting a report.
Copy link
Member

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:

Suggested change
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.

Copy link
Member

@turt2live turt2live left a 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)

changelogs/client_server/newsfragments/1517.feature Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
@turt2live turt2live removed the release-blocker Blocks the next release from happening label May 24, 2023
@anoadragon453
Copy link
Member

I plan to take over the maintenance of this PR in the author's place.

anoadragon453 and others added 3 commits August 4, 2023 10:13
Copy link
Member

@anoadragon453 anoadragon453 left a 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.

data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise lgtm - thanks :)

content/client-server-api/modules/report_content.md Outdated Show resolved Hide resolved
data/api/client-server/report_content.yaml Outdated Show resolved Hide resolved
Co-authored-by: Travis Ralston <travpc@gmail.com>
@Half-Shot
Copy link
Contributor Author

Oh, apparently I forgot I wasn't working on this...sorry @anoadragon453!

@turt2live turt2live merged commit 1b69e03 into matrix-org:main Aug 15, 2023
10 checks passed
@zecakeh zecakeh mentioned this pull request Aug 21, 2023
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants