Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add service worker for push notifications #5805

Merged
merged 131 commits into from
Jun 25, 2024
Merged

Conversation

Cyperghost
Copy link
Contributor

Closes #3960

@Cyperghost Cyperghost linked an issue Feb 16, 2024 that may be closed by this pull request
com.woltlab.wcf/templates/headIncludeJavaScript.tpl Outdated Show resolved Hide resolved
com.woltlab.wcf/templates/headIncludeJavaScript.tpl Outdated Show resolved Hide resolved
ts/WoltLabSuite/Core/BootstrapFrontend.ts Outdated Show resolved Hide resolved
ts/WoltLabSuite/Core/Notification/ServiceWorker.ts Outdated Show resolved Hide resolved
wcfsetup/setup/db/install.sql Outdated Show resolved Hide resolved
wcfsetup/setup/db/install.sql Outdated Show resolved Hide resolved
$payload = JSON::encode([
'aud' => $serviceWorker->getEndpoint(),
'exp' => TIME_NOW + 43200, // 12h
'sub' => "mailto:" . MAIL_ADMIN_ADDRESS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the email address be rawurlencoded? Thinking about email addresses such as admin+myforum@example.com.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed. The value mailto:admin+myforum@example.com works fine.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also might make sense to drop the reverted dependency-related commits entirely, for two reasons:

  1. Less bloat for the repository, because they will not be part of master after being merged.
  2. Less garbage for the fileDelete.xml generation script to work through. See 04f17cd. That script will add anything to fileDelete.xml that was previously part of the master branch, but no longer is part of it.

@Cyperghost Cyperghost marked this pull request as ready for review February 19, 2024 14:27
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more remarks regarding the cryptography. I did not test any of the code snippets, so please disregard the stuff that does not make sense to you / that you disagree with. It's already much better with the hex2bin/bin2hex gone.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't quite follow the entire cryptographic flow, but that might be because I didn't actually read the underlying specification. That said, it looks reasonable to me now.

You might want to consider running phpcbf and php-cs-fixer on lib/system/service/. Some of the spacing (with regard to empty lines) doesn't match the code style that is generally used and made it a little jarring while attempting to grok the code.

@Cyperghost Cyperghost marked this pull request as draft April 15, 2024 17:00
@Cyperghost
Copy link
Contributor Author

Cyperghost commented Apr 15, 2024

Waiting for composer packages

  • minishlink/web-push version 9.0.0
  • web-token/jwt-library version 4.0.0

…fications

# Conflicts:
#	ts/WoltLabSuite/Core/Notification/Handler.ts
#	wcfsetup/install/files/js/WoltLabSuite/Core/Notification/Handler.js
#	wcfsetup/install/files/lib/system/api/composer.json
#	wcfsetup/install/files/lib/system/api/composer.lock
#	wcfsetup/install/files/lib/system/api/composer/installed.json
#	wcfsetup/install/files/lib/system/api/composer/installed.php
@dtdesign dtdesign mentioned this pull request May 19, 2024
10 tasks
@dtdesign
Copy link
Member

minishlink/web-push

https://github.com/web-push-libs/web-push-php/releases has v9.0.0-rc2 out for a few days already and we can possibly expect a release to arrive in July.

web-token/jwt-library

The version 4.0 appears to be still under development as of now. However, its composer.json requires a minimum PHP version of 8.3 which exceeds our current minimum version of PHP 8.1. @Cyperghost can we somehow backport the necessary changes in the v3.4 tree to work around this?

# Conflicts:
#	wcfsetup/install/files/acp/database/update_com.woltlab.wcf_6.1.php
@Cyperghost Cyperghost marked this pull request as ready for review June 24, 2024 09:18
Cyperghost and others added 3 commits June 24, 2024 14:57
# Conflicts:
#	wcfsetup/install/files/lib/system/api/composer.json
#	wcfsetup/install/files/lib/system/api/composer.lock
#	wcfsetup/install/files/lib/system/api/composer/installed.php
  `web-token/jwt-library` to `3.3.50`
  `spomky-labs/pki-framework` to `1.2.1`
@Cyperghost Cyperghost requested a review from dtdesign June 25, 2024 10:19
  `web-token/jwt-library` to `3.3.50`
  `spomky-labs/pki-framework` to `1.2.1`
@Cyperghost Cyperghost merged commit 4af43aa into master Jun 25, 2024
8 checks passed
@Cyperghost Cyperghost deleted the service-worker-notifications branch June 25, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use service workers for push notifications
4 participants