feat(push): Add MSC4174 web push #2122
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 assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
continuwuation/continuwuity!2122
Loading…
Reference in a new issue
No description provided.
Delete branch "eleboucher/continuwuity:feat/msc4174-webpush"
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 implement the MSC4174, tested e2e with complement
Fixes: #1849
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
Re: Requested for review
I haven't the Rust knowledge to confidently say the code is good, but it seems alright.
I've spun up a Continuwuity on localhost, but haven't actually tested web push yet. I'll need to implement it in Nexus for this, and that means waiting for Tulir to implement needed stuff on Gomuks too, so it may take a little time.
I requested your review because you mentioned you were going to test this locally ahead of merge, so any issues you find will be valuable to hear as part of the code review process itself :)
i will also try to implement the part on sable
Mostly just code style concerns, I don't see anything functionally wrong here (checks out with passing complement tests)
@ -484,0 +530,4 @@Err!(Request(NotFound("No pusher with this app ID exists.")))},| ActivationOutcome::UnknownToken =>Err(activation_token_error(&UNKNOWN_ACTIVATION_TOKEN, "Unknown activation token.")),Couldn't this just be inlined into an
Err!()macro call? rather than calling an entire function?@ -484,0 +531,4 @@},| ActivationOutcome::UnknownToken =>Err(activation_token_error(&UNKNOWN_ACTIVATION_TOKEN, "Unknown activation token.")),| ActivationOutcome::Expired => Err(activation_token_error(Same here
I see this was marked as resolved, but I don't see a new commit? Same goes for the other resolved conversations.
damn i must be blind or stupid
@ -484,0 +539,4 @@}static EXPIRED_ACTIVATION_TOKEN: LazyLock<ErrorKind> =LazyLock::new(|| custom_error_kind("M_EXPIRED_ACTIVATION_TOKEN"));Shouldn't the unstable prefix be used in these error codes?
i'm following the spec here
@ -484,0 +543,4 @@static UNKNOWN_ACTIVATION_TOKEN: LazyLock<ErrorKind> =LazyLock::new(|| custom_error_kind("M_UNKNOWN_ACTIVATION_TOKEN"));fn custom_error_kind(errcode: &str) -> ErrorKind {This seems like an odd way to define the custom error, any reason not to just add it to the
ErrorKindenum itself?@ -1582,1 +1582,4 @@/// Allow encrypted Web Push pushers (MSC4174).#[serde(default)]pub allow_webpush: bool,Since
webpush_vapid_contacthas to be set to enable the feature, and it defaults to unset, I think this allow flag might be redundant in this case@ -0,0 +1,36 @@pub mod v3 {nit: Should be
unstableinstead ofv3until the proposal is stable@ -0,0 +1,614 @@use std::{collections::HashMap, sync::Arc, time::Duration};This file is rather quite large/dense, it might be worth splitting the
Vapidimpl,Serviceimpl, and utilities (funcs and consts) into separate files for easier maintainability.I'd also recommend attaching docstrings to the function defs and ideally some comments dotted around to explain what the server is doing to help someone who might be looking at this code having never seen actual webpush stuff (e.g. me) to understand what is going on
Blocked on https://github.com/matrix-org/matrix-spec-proposals/pull/4174#discussion_r3761648776.
5a2f881a26877b7d71a6877b7d71a6c6c7587955c6c7587955c234f001f7Could this follow https://github.com/matrix-org/matrix-spec-proposals/pull/4174#discussion_r3786568663 for now? Until it's written into the MSC, or until the MSC is in spec and therefore using stable identifiers?
@Henry-Hiles wrote in #2122 (comment):
Done let's get this merge now :D
c234f001f71833c6b35eTBH i'm not certain it'd get merged until the MSC itself is updated to have unstable prefixes, but I can at least test this now :)
I'll try to get an impl going in Nexus sometime soon.
1833c6b35e9fc2bbe0e79fc2bbe0e7abc6101a2fOnce https://github.com/matrix-org/matrix-spec-proposals/pull/4174/changes#r3800536083 is applied to the MSC, this can be unblocked.
abc6101a2fd7236bb279Hi, the MSC now has an unstable prefix, so it should be ready to be implemented.
Does this PR already follow https://github.com/matrix-org/matrix-spec-proposals/pull/4174/changes#diff-b2f2ad49aabe11774709efbdd9f7d3537ed2ed223b0a868d69e9db814dd18887R219?
EDIT: It appears to.
d7236bb279c4adfb3e76@ -1333,1 +1333,4 @@# Allow encrypted Web Push pushers (MSC4174).# VAPID contact URI (mailto: or https:).#webpush_vapid_contact =we have built-in contact page, so this can be automatic! :>
done
What no, we meant the "about" web page that displays support contacts - email isn't set by default
5ecbdf201cbbddc2b22fbbddc2b22f84aae2b5c0Vapid info is not returned on capabilities request:
84aae2b5c057888e15f557888e15f5fcaf1a2130fcaf1a21307ca69e93117ca69e9311fd70f57f23fd70f57f23d08a5e2f65d08a5e2f65ab100561bdI have a minimal implementation going in Nexus (currently just printing out events when you get a notif), seems to work perfectly including VAPID/encryption!
ab100561bde5ac66defce5ac66defc4b61d980724b61d9807264f5a236e164f5a236e10fcab89136Hi, this has merge conflicts, could it be rebased?
25aadb1c0089b9ef7a49Looks fine to me, there's some stuff I'd improve (like unifying the backoff system with server health tracking in some way) but to prevent this PR being blocked any longer I'm fine with merging as-is and picking up another time.
89b9ef7a497d0dbe3dca7d0dbe3dcabb5af28e2dHi, I think you messed up a rebase, there are two seperate commits in here of the same name.
90252e40084d704ab26f4d704ab26fed1e634579