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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
May 25, 2020
Merged

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

merged 3 commits into from
May 25, 2020

Conversation

lucavb
Copy link
Contributor

@lucavb lucavb commented Aug 1, 2019 •
edited by turt2live
Loading

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
@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Aug 1, 2019
@turt2live turt2live self-requested a review August 1, 2019 15:08
uhoreg
uhoreg reviewed Aug 1, 2019
View reviewed changes
Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up.

proposals/2209-auth-rules-other-keys-in-m.room.power.levels.md Outdated Show resolved Hide resolved
proposals/2209-auth-rules-other-keys-in-m.room.power.levels.md Outdated Show resolved Hide resolved
proposals/2209-auth-rules-other-keys-in-m.room.power.levels.md Outdated Show resolved Hide resolved
turt2live
turt2live approved these changes Aug 1, 2019
View reviewed changes
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.

seems sensible to me, thanks!

proposals/2209-auth-rules-other-keys-in-m.room.power.levels.md Outdated Show resolved Hide resolved
@turt2live turt2live added the unassigned-room-version Remove this label when things get versioned. label Aug 1, 2019
KitsuneRal
KitsuneRal approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

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

proposals/2209-auth-rules-other-keys-in-m.room.power.levels.md Outdated Show resolved Hide resolved
@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@turt2live turt2live self-requested a review May 12, 2020 22:15
@turt2live
Copy link
Member

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

@mscbot
Copy link
Collaborator

mscbot commented May 12, 2020 •
edited by erikjohnston
Loading

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.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels May 12, 2020
@turt2live turt2live removed their request for review May 12, 2020 22:28
@turt2live turt2live added requires-room-version An idea which will require a bump in room version and removed proposal-in-review labels May 12, 2020
@clokep clokep mentioned this pull request May 14, 2020
Closed
@richvdh richvdh requested review from richvdh and removed request for richvdh May 15, 2020 14:18
@clokep clokep mentioned this pull request May 18, 2020
Merged
6 tasks
@turt2live turt2live mentioned this pull request May 19, 2020
Merged
@mscbot
Copy link
Collaborator

mscbot commented May 20, 2020

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

@mscbot mscbot removed the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label May 20, 2020
@mscbot mscbot added the final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. label May 20, 2020
@turt2live turt2live self-assigned this May 21, 2020
turt2live added a commit that referenced this pull request May 21, 2020
@turt2live
Incorporate MSC2209 (notifications auth rules)
74c51b0 
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
Merged
@turt2live
Copy link
Member

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

@turt2live turt2live removed requires-room-version An idea which will require a bump in room version unassigned-room-version Remove this label when things get versioned. labels May 21, 2020
@mscbot
Copy link
Collaborator

mscbot commented May 25, 2020

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

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels May 25, 2020
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed finished-final-comment-period labels May 25, 2020
@turt2live turt2live merged commit e9e9366 into matrix-org:master May 25, 2020
@ara4n ara4n removed the spec-pr-in-review A proposal which has been PR'd against the spec and is in review label Jun 15, 2020
@richvdh
Copy link
Member

richvdh commented Jul 29, 2020

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

@richvdh richvdh added the merged A proposal whose PR has merged into the spec! label Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uhoreg uhoreg uhoreg left review comments

@turt2live turt2live turt2live approved these changes

@KitsuneRal KitsuneRal KitsuneRal approved these changes

Assignees

@turt2live turt2live

Labels
kind:core MSC which is critical to the protocol's success merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

notifications keys in m.room.power_levels events can be set to above the sender's power level
7 participants
@lucavb @turt2live @mscbot @richvdh @uhoreg @KitsuneRal @ara4n