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 WebPush support for Safari #674

Merged
merged 9 commits into from
Oct 8, 2023

Conversation

blighj
Copy link
Contributor

@blighj blighj commented Nov 20, 2022

PR for #673
Stores the full endpoint url for web push given by the browser, in the registration_id of the model, instead of just the token, and use exactly what the browser gave us when we send the message. Still supports existing registration_id/browser combo's but rasies a warning.

Also:

  • Adds tests for web push
  • Update docs for configuring web push
  • Handle 404/410 when regeistering and deactivate device
  • Remove pywebpush from APNS' options.extras_require in setup.cfg

The new docs address issues/questions raised in #647, #658, #665
The PR was inspired/informed by #487, #558, #602, #603, #604, #605

@simonkern
Copy link
Contributor

tores the full endpoint url for web push given by the browser, in the registration_id of the model, instead of just the token, and use exactly what the browser gave us when we send the message.

I also thought about this aspect some time ago when I added push support and think this is a really good idea. This could also remove the need to extract the browser type via javascript, because we then already have the url of the endpoint we need to send the push to stored in the WebPushDevice object.

@simonkern
Copy link
Contributor

iOS 16.4 just landed. What's the current status here? Are there still things that need to be done?

@blighj
Copy link
Contributor Author

blighj commented Mar 28, 2023

From my prespective, as a first time contributor, this is ready to go.
I assume the next step is a code review.

@simonkern
Copy link
Contributor

simonkern commented Mar 29, 2023

I am not a maintainer, but I could take this for a test spin. Do you know who could do a release? Maybe @alenzeinolov or @jamaalscarlett can help out?

return results
except WebPushException as e:
if e.response is not None and e.response.status_code in [404, 410]:
results["failure"] = 1

Choose a reason for hiding this comment

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

I get an error here because the exception comes from line 34, but results is defined on line 41. It should be enough to just switch them around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent point, commit added to address it.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining the two status codes and why you are disabling the device

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about?

Suggested change
results["failure"] = 1
# if we find that the status from the vendor was 404 (invalide url) or 410 (Gone) we should no
# longer try to send messages to this device.
# https://web.dev/push-notifications-common-issues-and-reporting-bugs/#http-status-codes
results["failure"] = 1

@ammarjmhor10
Copy link

Hi, is there any update please?

@mamal72
Copy link

mamal72 commented Apr 28, 2023

Hey guys. Any updates on this? Can we do one more push and have it released? Thanks in advance.

@azmeuk azmeuk mentioned this pull request Jul 24, 2023
@azmeuk azmeuk mentioned this pull request Aug 3, 2023
- ``WP_PRIVATE_KEY``: Absolute path to your private certificate file: os.path.join(BASE_DIR, "private_key.pem")
- ``WP_CLAIMS``: Dictionary with the same sub info like claims file: {'sub': "mailto: [email protected]"}
- ``WP_ERROR_TIMEOUT``: The timeout on WebPush POSTs. (Optional)
- ``WP_POST_URL``: A dictionary (key per browser supported) with the full url that webpush notifications will be POSTed to. (Optional)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this line removed?

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 MR is essentially deprectating the old approach. Instead the full endpoint that the browser returns is stored in the registration_id of the model, instead of just the token, and the code uses exactly what the browser gave us when we send the message. Rather than composing a url based on what is in WP_POST_URL.

As a result I thought it was sensible to remove the WP_POST_URL from the docs as it would confuse new users.


.. code-block:: python

pip install py-vapid (Only for generating key)
Copy link
Member

Choose a reason for hiding this comment

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

You are missing the

pip install pywebpush

line

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 part of the docs is explaining how to generate the keys needed. It's my undestanding that you only need py-vapid for that part.
You could essentially do that part on your local machine, and then deploy your keys to production where the library is used in your application.

push_notifications/webpush.py Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@azmeuk azmeuk added the webpush label Aug 18, 2023
@azmeuk
Copy link
Member

azmeuk commented Aug 25, 2023

I tested this branch on several browsers, including Safari on iOS and I confirm this works!

@azmeuk
Copy link
Member

azmeuk commented Aug 25, 2023

For what it worth, instead of manually using py-vapid, I could do the key generation dynamically with:

from py_vapid import b64urlencode
from py_vapid import Vapid02 as Vapid
from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives.asymmetric import ec


def vapid_public_key(path):
    vapid = Vapid.from_file(path)
    pk = vapid.public_key.public_bytes(
        serialization.Encoding.X962, serialization.PublicFormat.UncompressedPoint
    )
    return b64urlencode(pk)


def generate_private_key(path):
    private_key = ec.generate_private_key(ec.SECP256R1, default_backend())
    private_pem = private_key.private_bytes(
        encoding=serialization.Encoding.PEM,
        format=serialization.PrivateFormat.PKCS8,
        encryption_algorithm=serialization.NoEncryption()
    )
    with open(path, "wb") as file:
        file.write(private_pem)
        file.close()

generate_private_key can be used if the private key does not exist, vapid_public_key can be used to get the base64 export of the public key and insert it dynamically in the templates.

README.rst Outdated Show resolved Hide resolved
@jamaalscarlett
Copy link
Member

@blighj once you address @azmeuk I think this is good to merge. Nice work!

@jamaalscarlett
Copy link
Member

@blighj looks like there are some newly failing tests

James Bligh and others added 7 commits September 30, 2023 19:26
Declare results variable at the start of the block
Co-authored-by: Cuyler Stuwe <[email protected]>
Co-authored-by: James Bligh <[email protected]>
• Added example code to register WP device
• Fixed issue where call to extract UserAgent didn't include UA
• Added examples on how to send a Web Push message
README.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

Merging #674 (99fca51) into master (53525ca) will increase coverage by 0.84%.
Report is 4 commits behind head on master.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
+ Coverage   68.67%   69.51%   +0.84%     
==========================================
  Files          26       26              
  Lines        1111     1122      +11     
  Branches      243      245       +2     
==========================================
+ Hits          763      780      +17     
+ Misses        310      304       -6     
  Partials       38       38              
Files Coverage Δ
push_notifications/conf/app.py 75.15% <100.00%> (ø)
push_notifications/migrations/0001_initial.py 100.00% <ø> (ø)
...otifications/migrations/0002_auto_20160106_0850.py 100.00% <ø> (ø)
push_notifications/migrations/0003_wnsdevice.py 100.00% <ø> (ø)
push_notifications/migrations/0004_fcm.py 100.00% <ø> (ø)
...ush_notifications/migrations/0005_applicationid.py 100.00% <ø> (ø)
...ush_notifications/migrations/0006_webpushdevice.py 100.00% <ø> (ø)
push_notifications/webpush.py 100.00% <100.00%> (+33.33%) ⬆️
push_notifications/fields.py 64.06% <50.00%> (ø)
push_notifications/models.py 78.04% <0.00%> (ø)
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor Author

@blighj blighj Sep 30, 2023

Choose a reason for hiding this comment

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

@blighj looks like there are some newly failing tests

The tests are passing now, this file was failing, it is really outside the scope of this PR. My diagnosis is that a string is more bytes in py3 than the tests were expecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done more digging and I see clearly now why the test was failing, the serilaizer is tied to the model, the registration_id in the APNSDevice has a max_length of 200, and the test was creating a string of lenght 400 ("aE"*200).

So the update I did is appropriate.
@jamaalscarlett This MR is good to go now.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to re-review and hopefully get this merged today. Great work.

@jamaalscarlett jamaalscarlett merged commit b719521 into jazzband:master Oct 8, 2023
7 checks passed
@blighj
Copy link
Contributor Author

blighj commented Oct 8, 2023

:) Thank you kindly
I think you could now safely close PR #487
And the issues #457, #526, #629, #647, #657, and #665

a release to version 3.1.0 would be much appreciated too

@blighj blighj deleted the webpush_safari branch October 8, 2023 06:42
50-Course pushed a commit that referenced this pull request Oct 3, 2024
* Add WebPush support for Safari

* Update webpush.py based on review

Declare results variable at the start of the block

* Fix typo in warning

Co-authored-by: Cuyler Stuwe <[email protected]>

* Update README.rst

Co-authored-by: James Bligh <[email protected]>

* Fix mailto: space

* Expanded documentation for Web Push (#558)

• Added example code to register WP device
• Fixed issue where call to extract UserAgent didn't include UA
• Added examples on how to send a Web Push message

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Attempt to fix tests

* Update README.rst

---------

Co-authored-by: James Bligh <[email protected]>
Co-authored-by: Cuyler Stuwe <[email protected]>
Co-authored-by: Éloi Rivard <[email protected]>
Co-authored-by: Neil Littlejohns <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants