Use the same helper for submitting state events on room creation and on state endpoint #1799
No reviewers
Labels
No labels
Abandoned
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
bob the builder
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!1799
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "eve/ackduck:room_creation_events"
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?
On room creation, A list of initial state events may be provided that should be submitted to the room after creation. These events should be treated as any other state event and submitted to the same checks.
With this change, state events submitted on room creation will be submitted through the same helper as those through the usual endpoint.
The submission helper is moved to the timeline service to make it available everywhere. This will be useful for implementing #903.
This pull request...
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
420d2173e0210b7e95b7210b7e95b76656a2b1136656a2b1139cea8d2080seems sensible, just some comments R.E. making some opportunistic style changes
@ -0,0 +20,4 @@#[implement(super::Service)]#[allow(clippy::too_many_arguments)]pub async fn send_state_event_for_key_helper(I think if you're attaching it to the service,
_helperisn't necessary.send_state_eventis shorter and probably more memorable imo@ -0,0 +56,4 @@}#[implement(super::Service)]async fn allowed_to_send_state_event(I think this should be called
assert_allowed_to_send_state_event, since its sole function is propagating an error if the change is forbidden@ -0,0 +63,4 @@state_key: &str,json: &mut Raw<AnyStateEventContent>,) -> Result {match event_type {Forgejo forgot to include this comment apparently but I think it'd be a good idea to split these match arms into their own functions to make this function less complex cognitively
Would that be better ? Yeah it's a big match statement, but it's very intuitive to understand what it does. I feel like splitting it into functions would just make it more difficult to read the code by adding more indirection to the code, without really making it more clear than it already is.
I disagree. I think maintaining the match arms but having them all point to functions is easier to follow, since there's clear(er) boundaries. This might just be my dyslexia influencing my preferences, but I find the match statement really hard to follow.
Something like this is what I (personally) prefer:
I would also argue the simple arms should go into their own functions for style consistency but that'd be overly pedantic of me
It's very easy to jump to function definitions, I don't think it makes it any more difficult.
I tend to flatten the code when moving it into a function out of a match arm, preferring to
return falseexplicitly whenever such a condition is encountered rather than nesting a bunch of statements. For example:pub(super) fn verify_policy_signature(via: &ServerName,ps_key: &Base64<Standard, Vec<u8>>,pdu_json: &CanonicalJsonObject,redaction_rules: &RedactionRules,) -> bool {#[cfg(debug_assertions)]{let pretty = serde_json::to_string(pdu_json).unwrap();trace!(data=%pretty, "Preparing to check policy server signature");};let Some(canonical_json) = redact(pdu_json.clone(), redaction_rules, None).ok().and_then(|r| to_canonical_object(r).ok())else {debug_warn!("Failed to redact event");return false;};let Some(signature) = extract_signature(pdu_json, via, POLICY_SERVER_KEY_ID_ED25519) else {debug!("No (valid) policy server signature present on event");return false;};trace!(%signature, "Verifying policy server signature");let Ok(canonical_str) = to_canonical_json_string_for_signing(&canonical_json) else {debug_warn!("Could not convert canonical json object into string");return false;};verify_canonical_json_bytes(&SigningKeyAlgorithm::Ed25519,ps_key.as_bytes(),signature.as_bytes(),canonical_str.as_bytes(),).inspect_err(|e| debug_error!("Policy server verification failed: {e}")).is_ok()}This means the "happy path" is always as leftmost as possible and is generally easier to read.
Again though, "easy to read/follow" is subjective, and it's entirely plausible this is just an issue exclusive to me. If you disagree let me know and I'm happy to mark this as resolved
Fair enough. I've split the match statement. You're right I do think it's more readable this way now that I look at it.
9cea8d20805e47223ab9Finally getting back to this, sorry been busy.
No worries!
5e47223ab9a73be5450dcan we get the clippy lints fixed ? looks good otherwise
a73be5450da72d318c1aa72d318c1aeea0a79300eea0a79300848da5be3b848da5be3b97545455d297545455d2a7892b296f