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

MSC4163: Make ACLs apply to EDUs #4163

Merged
merged 13 commits into from
Aug 5, 2024
Merged

MSC4163: Make ACLs apply to EDUs #4163

merged 13 commits into from
Aug 5, 2024

Conversation

Kladki
Copy link
Contributor

@Kladki Kladki commented Jul 6, 2024 •
edited
Loading

Rendered

Implementations:

Signed-off-by: Matthias Ahouansou matthias@ahouansou.cz

FCP tickyboxes

@Kladki
Proposal to make ACLs apply to ACLs
cc7d6e6 
Signed-off-by: Matthias Ahouansou <matthias@ahouansou.cz>
@Kladki
fix spelling mistakes
481381e
@tulir tulir added proposal A matrix spec change proposal s2s Server-to-Server API (federation) kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Jul 7, 2024
tulir
tulir reviewed Jul 7, 2024
View reviewed changes
proposals/4163-make-acls-apply-to-edus.md
Copy link
Member

@tulir tulir Jul 7, 2024 •
edited by clokep
Loading

Choose a reason for hiding this comment

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

Implementation requirements:

  • Server

Copy link
Member

Choose a reason for hiding this comment

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

See server implementations in thread @ https://github.com/matrix-org/matrix-spec-proposals/pull/4163/files#r1667727322

@Kladki
make behaviour more closely align with current ACL conventions, by in…
a55cab1 
…stead considering the origin to the event

in practice this doesn't make a significant difference, due to the fact that servers should already be rejecting EDUs from servers which are not the same as the server of the user ID to prevent impersonation
@Kladki
simplify recipt logic, making it apply to all possible recipts
e30a503
TheArcaneBrony

This comment was marked as outdated.

Sign in to view

@clokep
Copy link
Member

clokep commented Jul 10, 2024

I think this is pretty straightforward, has an implementation in conduwuit and makes sense from what ACLs are meant to do.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Jul 10, 2024 •
edited
Loading

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

Concerns:

  • general clarity

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 proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jul 10, 2024
turt2live
turt2live approved these changes Jul 10, 2024
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.

generally looks good to me, thank you!

proposals/4163-make-acls-apply-to-edus.md Outdated Show resolved Hide resolved
proposals/4163-make-acls-apply-to-edus.md Outdated Show resolved Hide resolved
proposals/4163-make-acls-apply-to-edus.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

@mscbot concern general clarity

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Jul 10, 2024
anoadragon453
anoadragon453 approved these changes Jul 17, 2024
View reviewed changes
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.

Small wording nits. But otherwise this lgtm, thanks!

proposals/4163-make-acls-apply-to-edus.md Outdated Show resolved Hide resolved
proposals/4163-make-acls-apply-to-edus.md Outdated Show resolved Hide resolved
@turt2live turt2live self-requested a review July 23, 2024 19:25
@turt2live
Copy link
Member

@Kladki you'll also need to sign-off your changes before we're able to accept this. Currently only 1/12 commits are signed off. It may be best to sign off using a comment or the PR description.

@turt2live
Copy link
Member

@mscbot resolve general clarity

@mscbot
Copy link
Collaborator

mscbot commented Jul 30, 2024

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

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. unresolved-concerns This proposal has at least one outstanding concern labels Jul 30, 2024
@mscbot
Copy link
Collaborator

mscbot commented Aug 4, 2024

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 Aug 4, 2024
@anoadragon453 anoadragon453 merged commit cf4cdf2 into matrix-org:main Aug 5, 2024
1 check passed
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Aug 5, 2024
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Oct 1, 2024
@turt2live
Copy link
Member

I haven't verified the implementation on this, but presumably someone has if it was accepted back in August.

@girlbossceo
Copy link

They do work, it was initially added to conduwuit as part of a moderation need and confirmed it stopped the problem since then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clokep clokep clokep left review comments

@turt2live turt2live turt2live approved these changes

@richvdh richvdh richvdh left review comments

@tulir tulir tulir left review comments

@TheArcaneBrony TheArcaneBrony TheArcaneBrony left review comments

@girlbossceo girlbossceo girlbossceo left review comments

@anoadragon453 anoadragon453 anoadragon453 approved these changes

Assignees
No one assigned
Labels
kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal s2s Server-to-Server API (federation) spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec
Projects
Spec Core Team Backlog
Status: Requires spec writing
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Kladki @clokep @mscbot @turt2live @girlbossceo @anoadragon453 @richvdh @tulir @TheArcaneBrony