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

MSC2209: Alter auth rules to check notifications in m.room.power_levels #2209

Merged
merged 3 commits into from May 25, 2020

Conversation

@lucavb
Copy link
Contributor

@lucavb lucavb commented Aug 1, 2019

The key notifications was added to the m.room.power_levels event after the finalisation of the auth rules specified in room version 1. This leads to the fact, that this dictionary is not subject to the same validation as other dictionaries in the event, such as users or events. This especially means that Alice is able to alter any entry within the dictionary including ones, that are above her own power level, which is inconsistent with the behaviour for the other two dictionaries.

m.room.power_levels
room version 1

rendered

Related

Fixes #2198

Signed-off-by: Luca Becker luca.becker@me.com

@lucavb lucavb changed the title MSC2198: Alter auth rules to check notifications in m.room.power_levels MSC2209: Alter auth rules to check notifications in m.room.power_levels Aug 1, 2019
Copy link
Member

@uhoreg uhoreg left a comment

Thanks for writing this up.

Loading

Loading
Loading
Loading
Copy link
Member

@turt2live turt2live left a comment

seems sensible to me, thanks!

Loading

Loading
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Aside from a formatting nitpick it looks a no-brainer to merge.

Loading

Loading
@turt2live turt2live added this to Review stages in Client-server r0.6 proposals Aug 26, 2019
@turt2live turt2live self-requested a review May 12, 2020
@turt2live
Copy link
Member

@turt2live turt2live commented May 12, 2020

Looks like this is ready to go. In this particular case, an implementation proof probably isn't needed given the clear description of what happens and how it is fixed - if others feel differently, please raise a concern.

@mscbot fcp merge

Loading

@mscbot
Copy link
Collaborator

@mscbot mscbot commented May 12, 2020

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

Loading

@mscbot
Copy link
Collaborator

@mscbot mscbot commented May 20, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

Loading

@turt2live turt2live self-assigned this May 21, 2020
turt2live added a commit that referenced this issue May 21, 2020
MSC: #2209

The changes are slightly difficult to word without dumping the text in and playing a game of spot the difference, so we now use our pre-existing pygments support to render a representation of the difference. The difference is shown in markdown-like format instead of RST for ease of understanding. It's also not rendered HTML for largely complexity reasons.
@turt2live turt2live mentioned this pull request May 21, 2020
@turt2live
Copy link
Member

@turt2live turt2live commented May 21, 2020

Spec PR (for when this leaves FCP): #2563

Loading

@mscbot
Copy link
Collaborator

@mscbot mscbot commented May 25, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

Loading

@turt2live turt2live merged commit e9e9366 into matrix-org:master May 25, 2020
7 checks passed
Loading
@richvdh
Copy link
Member

@richvdh richvdh commented Jul 29, 2020

@ara4n this has been merged to the spec, so the correct label is 'merged'.

Loading

@richvdh richvdh added the merged label Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

7 participants