Back/webpush #2662
Back/webpush #2662
Conversation
|
You do not signoff your commits, could you please check the DCO CI job? |
|
@provokateurin it may interest you for Neon |
|
We'll check the rough idea later this week, but CI seems pretty red at the moment 🙈 |
Indeed, I'm fixing it. I was waiting for the CI to run PS: I forgot to mention, this feature is part of a grant with NLnet |
p1gp1g
force-pushed
the
back/webpush
branch
2 times, most recently
from
9afa07d to
4da7b5f
Compare
|
It's conflicting in composer.lock so CI is not starting 🙈 |
|
Should be ok now |
|
@nickvergessen Can you run the CI again ? |
|
I will need some help to fix the CI.
|
|
The changes for the android-lib are ready, once this PR is done: nextcloud/android-library@master...p1gp1g:nextcloud-library:feat/webpush |
|
@nickvergessen : Are you OK to run the CI if I push a potential fix for it? (cf. #2662 (comment)) |
|
You were missing some files to set up Mozart correctly, I added them. Let's see what that does to CI. |
| "time": "2025-12-02T00:53:42+00:00" | ||
| }, | ||
| { | ||
| "name": "psr/clock", |
There was a problem hiding this comment.
Something is wrong here.
It might be that your dependency brings in psr/clock or in a different version or something
There was a problem hiding this comment.
it has moved to the packages list, same version: it may be possible another lib use it. Is it a problem since the lib is still listed ?
There was a problem hiding this comment.
The problem is that it should be here, but aparently is not anymore as per Psalm:
Error: lib/BackgroundJob/GenerateUserSettings.php:21:3: MissingDependency: OCP\AppFramework\Utility\ITimeFactory depends on class or interface psr\clock\clockinterface that does not exist (see https://psalm.dev/157)
There was a problem hiding this comment.
Is it ok if I do composer require psr/clock --dev and update the lock, so it will be explicitly included in dev requirement ?
There was a problem hiding this comment.
I'm not sure how that fully works, but psr/clock is part of server's 3rdparty directory already and we should not duplicate it or create a conflict or something.
Maybe you can try if provide solves that?
https://github.com/ChristophWurst/nextcloud_sentry/blob/9e8385fb2c0c170edbedc3d81f3d304f50197cd6/composer.json#L27-L29
There was a problem hiding this comment.
If I understand correctly, we have to add the "provide" to nextcloud/ocp, I can push a test using a temporary repo to override nextcloud/ocp
There was a problem hiding this comment.
I've tested different combinations, with/without provide, in notification app, in ocp, with require/require-dev and nothing works: p1gp1g#1 any idea ?
There was a problem hiding this comment.
I've found the issue: mozart removes the vendor directory after namespacing, and psc/clock is a transitive dependence of the webpush lib
|
Thank you. I think we need to ignore the new vendor directories too? |
The vendor-bin/ part is correctly ignored already: But since this app is shipped with the server it has to be buildable repeatingly, so we need to commit lib/Vendor/ at the end of the pull request work or in a follow up. (Please don't commit it yet, as it makes the PR even more unreadable and not loadable in the GitHub UI) |
|
OK, I see, maybe excluding them from the lint checks then ? |
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Fix index too long for the DB Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
Signed-off-by: sim <[email protected]>
|
The psalm CI was fixed, but there was still some errors. The lint error is fixed now, and I moved the VAPID initialization to fix other CI. There shouldn't be much to do now. The openapi CI is expected to fail because lib/Vendor isn't included in the git repo at this moment |
|
OK, the CI is finally good 👍 |
Are you sure? I think the lib/Vendor is just misleading, the actual mismatch that leads to the error state happens before. Did you try regenerating the openapi specs? |
Yes, the openapi jsons are updated in the PR, and if I run |
Yeah that's okay for now. Unfortunately I'm sick atm, but I hope to be able to review it in more detail later this or next week. |
|
I wish you good recovery. I'll finish UnifiedPush support for Talk android until then :) |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
Why not put |
Because we need repeatable builds and packages, so any shipped code needs to be checked in at the end. |
|
Hey, I wish you recovered well, and a happy new year. This is a gentle bump to find out when you think you'll be able to continue the review ? |
Not fully there yet, but getting there. Thanks a lot.
Yes. |
nickvergessen
left a comment
nickvergessen
left a comment
There was a problem hiding this comment.
After writing the summary I pressed outside of the box and it wiped the text. Sorry if this is shorter now, just can't write it twice.
I reviewed most of the code, but can't follow all the changes in Push.php. Some look like they try to do things that are unrelated to introducing webpush support?
Also we are now checking 2 database tables all the time. Comparing the table structures it should almost be possible to map them to a single one?
- id
- uid
- token
- endpoint = proxyserver
- p256dh = devicepublickey
- auth = pushtokenhash
- apptypes = apptype
- activated = ??
- activation_token = deviceidentifier
Not sure that makes sense, just an idea. Otherwise we should start caching if a user has at least one device in webpush and/or pushhash to save queries where possible.
I'm also missing some general information. Which server is being used by default if a user/device/app would use this? Is it sending data to any 3rd party servers, e.g. unifiedpush.org?
| case NO_SUB = 3; | ||
| } | ||
|
|
||
| #[OpenAPI(scope: 'push')] |
There was a problem hiding this comment.
Nitpick: I think we should use a different scope, as a client either needs to implement and handle the APIs of push or the ones from webpush, but basically not both?
| $result->closeCursor(); | ||
|
|
||
| if (!$row) { | ||
| // In case the user has already a subscription, but inactive or with a different enpoint, pubkey or auth secret |
There was a problem hiding this comment.
| // In case the user has already a subscription, but inactive or with a different enpoint, pubkey or auth secret | |
| // In case the user has already a subscription, but inactive or with a different endpoint, pubkey or auth secret |
| * @param string $endpoint Push Server URL, max 765 chars (RFC8030) | ||
| * @param string $uaPublicKey Public key of the device, uncompress base64url encoded (RFC8291) | ||
| * @param string $auth Authentication tag, base64url encoded (RFC8291) | ||
| * @param string $apptypes comma seperated list of types used to filter incoming notifications - apptypes are alphanum - use "all" to get all notifications, prefix with `-` to exclude (eg. 'all,-talk') |
There was a problem hiding this comment.
Nitpick: Looks like it's "our choice", then let's stick with current naming patterns:
| * @param string $apptypes comma seperated list of types used to filter incoming notifications - apptypes are alphanum - use "all" to get all notifications, prefix with `-` to exclude (eg. 'all,-talk') | |
| * @param string $appTypes comma seperated list of types used to filter incoming notifications - apptypes are alphanum - use "all" to get all notifications, prefix with `-` to exclude (eg. 'all,-talk') |
| * Register a subscription for push notifications | ||
| * | ||
| * @param string $endpoint Push Server URL, max 765 chars (RFC8030) | ||
| * @param string $uaPublicKey Public key of the device, uncompress base64url encoded (RFC8291) |
There was a problem hiding this comment.
UA is user agent? I'd prefer to stick with the naming you use to explain it in the description :P
| * @param string $uaPublicKey Public key of the device, uncompress base64url encoded (RFC8291) | |
| * @param string $devicePublicKey Public key of the device, uncompress base64url encoded (RFC8291) |
| if ( | ||
| !filter_var($endpoint, FILTER_VALIDATE_URL) | ||
| || \strlen($endpoint) > 765 | ||
| || !preg_match('/^https\:\/\//', $endpoint) |
There was a problem hiding this comment.
No need to regex:
| || !preg_match('/^https\:\/\//', $endpoint) | |
| || !str_starts_with($endpoint, 'https://') |
| $device['token'] = (int)$device['token']; | ||
| if (!$this->validateToken($device['token'], $maxAge)) { | ||
| // Token does not exist anymore | ||
| $this->deleteProxyPushToken($device['token']); |
There was a problem hiding this comment.
When the device was inactive for 60 days we shouldn't delete it directly.
It's possible that the device/user is coming back online.
The problem is the comment is misleading by now.
| // The nextcloud application, requested with the proxy push, | ||
| // use to not support `delete-multiple` | ||
| if (!\in_array($app, ['spreed', 'talk', 'admin_notification_talk'], true)) { |
There was a problem hiding this comment.
I don't understand where this is coming from?
| if ($type === IToken::WIPE_TOKEN) { | ||
| // Token does not exist any more, should drop the push device entry | ||
| $this->printInfo('Device token is marked for remote wipe'); | ||
| $this->deletePushToken($tokenId); |
There was a problem hiding this comment.
Should be kept with deleteProxyPushToken
| } catch (InvalidTokenException) { | ||
| // Token does not exist any more, should drop the push device entry | ||
| $this->printInfo('<error>InvalidTokenException is thrown</error>'); | ||
| $this->deletePushToken($tokenId); |
There was a problem hiding this comment.
Should be kept with deleteProxyPushToken
| */ | ||
| protected function webPushCallback(MessageSentReport $report): void { | ||
| if ($report->isSubscriptionExpired()) { | ||
| $this->deleteWebPushTokenByEndpoint($report->getEndpoint()); |
There was a problem hiding this comment.
This permanently removes all the device entries, even if the subscription is continued in the future?
5 participants
This PR adds web push support to send push notifications. Web Push is defined by 3 RFCs: RFC8030 (the requests), RFC8291 (encryption) and RFC8292 (server authorization).
It can then be used by the web application to receive real time notifications, even when nextcloud isn't opened in a tab, and without the notify_push app (which adds a websocket support to nextcloud). This is particularly useful for collaborative work, as nexcloud isn't always opened but we can expect to be notified anyway. This support is in the second PR
This support also allow to get push notifications on Android without the proxy:
Fix #1225
Required for:
nextcloud/android#8684
nextcloud/talk-android#257
Which will also fix:
nextcloud/android#12151 (won't be necessary)
nextcloud/android#11898 (duplicate)
nextcloud/android#5510 (supersede, openpush was a research project, and their website now link to unifiedpush)
nextcloud/android#8800 (another duplicated ?)
nextcloud/android#3333 (UP supports allow using FCM without the proprietary lib)
(There might be other issues)