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

Document the event auth rules #1027

Merged
merged 7 commits into from
Oct 20, 2017
Merged

Document the event auth rules #1027

merged 7 commits into from
Oct 20, 2017

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Oct 18, 2017

These are a necessary precursor to state resolution.

These are a necessary precursor to state resolution.
This doesn't seem to be a thing that is actually used.

Also remove unhelpful example which was more lies than truth.

i. If ``sender`` matches ``state_key``, allow.

#. If the ``sender``'s current membership state is not ``joined``, reject.
Copy link
Member

Choose a reason for hiding this comment

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

There's a clause that allows an admin "unbanning" someone if they were banned an the admin has a power level above the "ban_level"

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, fixed in 47005d5.

My understanding of the code is that you can unban someone even if you have PL < ban, provided you have PL >= kick, and the target's PL is <= yours. Seems a bit odd but if it's true we might as well spec it. Can you double-check me?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, apparently I misread the code. Let me try again.

It looks like you have to have PL >= ban and PL >= kick and PL >= the target to unban someone, otherwise you'll get caught at https://github.com/matrix-org/synapse/blob/master/synapse/event_auth.py#L327. Right?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, looks like it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, one more go in 6d038e1. PTAL?

@erikjohnston
Copy link
Member

There's also some checks to ensure that the right signatures are there: https://github.com/matrix-org/synapse/blob/master/synapse/event_auth.py#L47-L68, not sure if they should be included here or not

result in the user being considered joined.

1. If type is ``m.room.create``, allow if and only if has depth 0 and it has no
previous events - *i.e.* it is the first event in the room.
Copy link
Member

Choose a reason for hiding this comment

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

There's an assertion for that room_id domain matches sender domain

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah. I think there's a whole class of checks like this and the signatures which I want to keep separately from the auth rules, because they don't really play a part in state resolution. The line's a bit fuzzy, but I want to punt this one and the signatures to a separate section and a separate PR.

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Other than the comments, I think the only thing missing is the 3pid invite stuff, so yay and lgtm.

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Oct 18, 2017
Use dome definitions to avoid so much repetition, and hopefully make things
clearer.
@richvdh richvdh assigned erikjohnston and unassigned richvdh Oct 19, 2017
@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Oct 19, 2017

#. If the ``join_rule`` is ``public``, allow.

#. Otherwise, reject.
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to handle invites?

Copy link
Member Author

Choose a reason for hiding this comment

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

nnnnyargh

Copy link
Member Author

Choose a reason for hiding this comment

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

@erikjohnston
Copy link
Member

<3

@richvdh
Copy link
Member Author

richvdh commented Oct 20, 2017

that was harder work than it should have been. thanks!

@richvdh richvdh merged commit 5fee8e5 into master Oct 20, 2017
@richvdh richvdh deleted the rav/auth_rules branch October 20, 2017 12:37
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

2 participants