MSC4150: m.allow recommendation for moderation policy lists #4150
Conversation
Johennes
changed the title
Johennes
force-pushed
the
johannes/m-allow-recommendation
branch
from
a505b31 to
1533fbe
Compare
Signed-off-by: Johannes Marbach <[email protected]>
Johennes
force-pushed
the
johannes/m-allow-recommendation
branch
from
1533fbe to
3928fa1
Compare
There was a problem hiding this comment.
Implementation requirements:
- Publisher of new recommendation
- Consumer of new recommendation
There was a problem hiding this comment.
Doesn't mjolnir satisfy both of these already?
There was a problem hiding this comment.
Very possibly, but the implementations would need to be reviewed. Can the original PRs rather than commits be linked on this thread please?
There was a problem hiding this comment.
Draupnir publishes the org.matrix.mjolnir.allow policies in the appservice admin room to designate the users that have access here via this command.
Draupnir consumes them here to enforce access upon invitation of the appservice bot via the same access code as mjolnir.
However, this implementation exists explicitly with the intent that org.matrix.mjolnir.allow is only used to describe an entity that is allowed to participate to begin with. That's to say the absence of a policy rule with an allow recommendation for a given entity type requires you to infer that there is a policy rule for that entity type with an entity field that has the glob value "*". Which is a long winded way of saying this is explicitly not an an implementation of a recommendation that performs anti-ban and is not the opposite to m.ban. This is intended to be the same as allow from the m.room.server_acl algorithm.
[Edit: this comment was written before Travis's reply loaded for me when I refreshed the page, whoops. This is intended to provide context for the MSC]
There was a problem hiding this comment.
Can the original PRs be linked
The original PR won't be useful, the work was done as part of mjolnir for all in this PR matrix-org/mjolnir#364 1. Mjolnir for all works by using policy lists to describe a set of users who have access to their own personal hosted Mjolnir bot. This policy list is managed with policies that have the org.matrix.mjolnir.allow recommendation. The specific commit that introduced the access control mechanism is here but that likely won't be useful matrix-org/mjolnir@074a16b. Mjolnir for all didn't have the commands to manage this list but Draupnir does and the PR to do that is the-draupnir-project/Draupnir#47 (which probably is reviewable - but please note that it will be cleaner and easier to look at Draupnir's code directly, as there have been fundamental changes to Draupnir in this time that significantly improve code quality).
That being said, I don't think an appeal to see a PR makes sense. It surely isn't the intention of SCT or other reviewers to review the actual code2? Would it make more sense to ask for documentation that describes the feature?
And for clarity I'm just trying to provide as much context for Mjolnir & Draupnir here, I don't know or care if they meet the requirements of implementation.
Footnotes
-
Which itself has its own heavy context, and it is a shame that it is missing from the PR itself. ↩
-
https://spec.matrix.org/proposals/#implementing-a-proposal ↩
There was a problem hiding this comment.
@turt2live do you think the mjolnir implementation is sufficient to move this proposal ahead?
There was a problem hiding this comment.
I haven't reviewed the implementations, sorry.
turt2live
added
proposal
| When both `m.ban` and `m.allow` recommendations are used in the same policy list, confusion could arise if | ||
| two rules with opposing recommendations match against an entity. Given that the spec doesn't currently | ||
| impose _any_ logic for how policy lists are to be interpreted, it could well leave this question as an | ||
| implementation detail, too. | ||
|
|
||
| However, if a resolution is indeed desired, the spec could mandate that `m.ban` rules override `m.allow` | ||
| rules but not the other way around. This matches both [Mjolnir's implementation] and the way | ||
| [server access control lists] are evaluated. |
There was a problem hiding this comment.
I think that we would need to explicitly choose one or the other here to prevent a situation where in future there are distinct clients that implemented them differently, and then some innocent person gets caught in a mess when they mix them without context.
Separately, anti-ban is risky and there are other ways of "undoing" a ban (by redacting the policy or replacing the state event with a blank one - which is what Mjolnir and Draupnir do). In the future, I would like consumers of policy lists to also be able to annotate policies with other policies1 2. This would be a way of ignoring/undoing a policy from a list that you are consuming but do not have write access to, which is safer than marking an entity as immune (which is what anti-ban would be).
Please also see the details I wrote up for how this is implemented in Mjolnir and Draupnir #4150 (comment)
Footnotes
There was a problem hiding this comment.
I think I agree but I wonder if then we'd need to turn the spec's current suggesting language on how to interpret policy lists into a prescription? It'd feel slightly off if we were to mandate how m.ban and m.allow interact when the concrete semantics of both are deliberately left as implementation details.
Rendered
In line with matrix-org/matrix-spec#1700, the following disclosure applies:
I am a Systems Architect at gematik, Software Engineer at Unomed, Matrix community member and former Element employee. This proposal was written and published with my gematik hat on.