feat: Update policy server implementation to match latest spec #1487
No reviewers
Labels
No labels
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
5 participants
Notifications
Due date
Depends on
#1656 refactor: Switch to upstream Ruma
continuwuation/continuwuity
Reference
continuwuation/continuwuity!1487
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "nex/feat/policy-servers-2-electric-boogaloo"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request updates the MSC4284 policy server implementation to better match the now stabilised MSC.
Related Synapse pull request: https://github.com/element-hq/synapse/pull/19503
m.room.policyschemaWill also add:
/.well-known/matrix/policy_serverSupport for the stable and unstable endpoints, hopefully with some built-in checks (for example to enforce DAG "correctness") and/or the option to pass through to another servicePull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
feat: Update policy server implementation to be closer to stable MSC4284to WIP: feat: Update policy server implementation to be closer to stable MSC42846c96945b0a02ab2daa57Gonna drop the well-known and inline support from this PR to reduce the scope and get it merged sooner. Will probably add later.
9ebafaaabc77e769d3e1WIP: feat: Update policy server implementation to be closer to stable MSC4284to feat: Update policy server implementation to be closer to stable MSC4284b4128b58136748645033feat: Update policy server implementation to be closer to stable MSC4284to feat: Update policy server implementation to match latest specb43296d5b038d8eb1a174d13533cbb88cf46822cThis PR is ready for merge, but is blocked pending some issues with the widely deployed policy server policyserv (currently some rooms, including all of the Matrix Foundation rooms, are unusable if they are using policyserv if this PR is applied). Will be merged by June 1st at the latest.
@ -90,0 +108,4 @@trace!("no policy server configured");} else {error!("failed to load policy server event: {e}");// TODO: Should this fail closed?This TODO allows rooms with an invalid policy server to continue to be used if the event is invalid in some way (e.g. redacted). It might be necessary to create a "redacted" PS config struct so that we can explicitly check if the event actually has its fields present, since you disable the PS by removing the
via(but doing so causes ruma to cry).Blocked on https://github.com/matrix-org/matrix-spec/issues/2362 (see previous comment). Blocking any upcoming release as the current implementation still uses legacy checks, which policy servers are not going to keep around forever.
4a9a7b131462fa4f2693Unblocking this early as it's preventing further improvements to more critical parts of the codebase and we can't work around it much longer
@ -72,1 +92,3 @@return Ok(true);) -> Result<()> {let ps = match pdu.event_type().with_state_key("").0 {| StateEventType::RoomPolicy => return Ok(()),what's the purpose of this early return?
(m.room.policy, "")events are excluded from policy server checks and thus are always permitted.@ -90,0 +109,4 @@trace!("no policy server configured");} else {error!("failed to load policy server event: {e}");// TODO: Should this fail closed?this branch can only be hit if an internal database error occurred. generally I'm in favor of panicking when that happens because it indicates that something is very broken
This branch can be hit if the policy server configuration is malformed (for example, a literal
nullvalue forvia). The spec says that such events should be treated as if the policy server is disabled, but doing so means we can't distinguish from actual database errors: https://github.com/ruma/ruma/issues/2490Is it not possible to check if the error is a deserialization faulure?
i forgot you can do that
@ -156,0 +262,4 @@);// TODO: select between this sleep and shutdown signalsleep(saturated).await;if !self.services.server.running() {is this check necessary? I would assume that the executor threads for incoming requests would just be killed when the server exits
Ongoing requests are allowed to complete while new requests are refused during a graceful shutdown
I believe the sleep does not get interrupted either, hence the desire to select between the shutdown signal and the sleep finishing, so that it can be interrupted. Otherwise a malicious policy server could serve us 429s with the max retry-after time and have us spinning for several minutes after a shutdown is signalled
2167891aed6c215bd35043d7eb2568to78ff1c5136event_idis removed before policy-checking event 09582df6f4fdb25f4aadeb829c2951