Fix webpush vapid_claims from one request bleeding into others

changelog.d/180.bugfix

@@ -0,0 +1,8 @@
+Fix vapid_claims from one webpush request bleeding into others
+
+The vapid_claims dict was reused across requests, but pywebpush actually modifies this dictionary.
+This made the pushkin usable with only a single pusher for a single user, any other pushers
+would be rejected by their gateway, at least if from a different provider, as the aud claim
+(the origin of the endpoint url) didn't match the server name, but would likely even fail for 
+a single provider as the expiry time wasn't refreshed.
+The resut was the pushkey would be rejected, removing the pusher.

sygnal/webpushpushkin.py

@@ -15,7 +15,9 @@
 import json
 import logging
 import os.path
+import time
 from io import BytesIO
+from urllib.parse import urlparse
 
 from prometheus_client import Gauge, Histogram
 from py_vapid import Vapid, VapidException
@@ -104,16 +106,15 @@ def __init__(self, name, sygnal, config):
  self.vapid_private_key = Vapid.from_file(private_key_file=privkey_filename)
  except VapidException as e:
  raise PushkinSetupException("invalid 'vapid_private_key' file") from e
- vapid_contact_email = self.get_config("vapid_contact_email")
- if not vapid_contact_email:
+ self.vapid_contact_email = self.get_config("vapid_contact_email")
+ if not self.vapid_contact_email:
  raise PushkinSetupException("'vapid_contact_email' not set in config")
- self.vapid_claims = {"sub": "mailto:{}".format(vapid_contact_email)}
 
  async def _dispatch_notification_unlimited(self, n, device, context):
  p256dh = device.pushkey
  if not isinstance(device.data, dict):
  logger.warn(
- "device.data is not a dict for pushkey %s, rejecting pushkey", p256dh
+ "Rejecting pushkey %s; device.data is not a dict", device.pushkey
  )
  return [device.pushkey]
 
@@ -122,8 +123,8 @@ async def _dispatch_notification_unlimited(self, n, device, context):
 
  if not p256dh or not endpoint or not auth:
  logger.warn(
- "subscription info incomplete "
- + "(p256dh: %s, endpoint: %s, auth: %s), rejecting pushkey",
+ "Rejecting pushkey; subscription info incomplete "
+ + "(p256dh: %s, endpoint: %s, auth: %s)",
  p256dh,
  endpoint,
  auth,
@@ -137,6 +138,11 @@ async def _dispatch_notification_unlimited(self, n, device, context):
  payload = WebpushPushkin._build_payload(n, device)
  data = json.dumps(payload)
 
+ # note that webpush modifies vapid_claims, so make sure it's only used once
+ vapid_claims = {
+ "sub": "mailto:{}".format(self.vapid_contact_email),
+ "exp": self._get_vapid_exp(),
+ }
  # we use the semaphore to actually limit the number of concurrent
  # requests, since the HTTPConnectionPool will actually just lead to more
  # requests being created but not pooled – it does not perform limiting.
@@ -151,19 +157,37 @@ async def _dispatch_notification_unlimited(self, n, device, context):
  subscription_info=subscription_info,
  data=data,
  vapid_private_key=self.vapid_private_key,
- vapid_claims=self.vapid_claims,
+ vapid_claims=vapid_claims,
  requests_session=self.http_agent_wrapper,
  )
  response = await response_wrapper.deferred
- await readBody(response)
+ response_text = await response_wrapper.read_body(response)
  finally:
  self.connection_semaphore.release()
 
  # assume 4xx is permanent and 5xx is temporary
  if 400 <= response.code < 500:
+ logger.warn(
+ "Rejecting pushkey %s; gateway %s failed with %d: %s",
+ device.pushkey,
+ urlparse(endpoint).netloc,
+ response.code,
+ response_text,
+ )
  return [device.pushkey]
  return []
 
+ def _get_vapid_exp(self):
+ """
+ Returns the expire value for the vapid claims.
+
+ Having this in a separate method allows to
+ provide a different time source in unit tests.
+ """
+
+ # current time + 12h
+ return int(time.time()) + (12 * 60 * 60)
+
  @staticmethod
  def _build_payload(n, device):
  """
@@ -272,3 +296,6 @@ class HttpResponseWrapper:
 
  def __init__(self, deferred):
  self.deferred = deferred
+
+ async def read_body(self, response):
+ return (await readBody(response)).decode()