-
Notifications
You must be signed in to change notification settings - Fork 301
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
fix: [BREAKING] remove legacy subscription; change subscription interface #394
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<?php | ||
|
||
namespace Minishlink\WebPush; | ||
|
||
enum ContentEncoding: string | ||
{ | ||
/** Outdated historic encoding. Was used by some browsers before rfc standard. Not recommended. */ | ||
case aesgcm = "aesgcm"; | ||
/** Defined in rfc8291. */ | ||
case aes128gcm = "aes128gcm"; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,22 +15,33 @@ | |
|
||
class Subscription implements SubscriptionInterface | ||
{ | ||
protected ContentEncoding $contentEncoding; | ||
/** | ||
* @param string|null $contentEncoding (Optional) Must be "aesgcm" | ||
* This is a data class. No key validation is done. | ||
* @param string|\Minishlink\WebPush\ContentEncoding $contentEncoding (Optional) defaults to "aes128gcm" as defined to rfc8291. | ||
* @throws \ErrorException | ||
*/ | ||
public function __construct( | ||
private string $endpoint, | ||
private ?string $publicKey = null, | ||
private ?string $authToken = null, | ||
private ?string $contentEncoding = null | ||
protected readonly string $endpoint, | ||
protected readonly string $publicKey, | ||
protected readonly string $authToken, | ||
ContentEncoding|string $contentEncoding = ContentEncoding::aes128gcm, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extraneous space? |
||
) { | ||
if($publicKey || $authToken || $contentEncoding) { | ||
$supportedContentEncodings = ['aesgcm', 'aes128gcm']; | ||
if ($contentEncoding && !in_array($contentEncoding, $supportedContentEncodings, true)) { | ||
throw new \ErrorException('This content encoding ('.$contentEncoding.') is not supported.'); | ||
if(is_string($contentEncoding)) { | ||
try { | ||
if(empty($contentEncoding)) { | ||
$this->contentEncoding = ContentEncoding::aesgcm; // default | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the default be |
||
} else { | ||
$this->contentEncoding = ContentEncoding::from($contentEncoding); | ||
} | ||
} catch(\ValueError) { | ||
throw new \ValueError('This content encoding ('.$contentEncoding.') is not supported.'); | ||
} | ||
$this->contentEncoding = $contentEncoding ?: "aesgcm"; | ||
} else { | ||
$this->contentEncoding = $contentEncoding; | ||
} | ||
if(empty($publicKey) || empty($authToken)) { | ||
throw new \ValueError('Missing values.'); | ||
} | ||
} | ||
|
||
|
@@ -42,55 +53,37 @@ public static function create(array $associativeArray): self | |
{ | ||
if (array_key_exists('keys', $associativeArray) && is_array($associativeArray['keys'])) { | ||
return new self( | ||
$associativeArray['endpoint'], | ||
$associativeArray['keys']['p256dh'] ?? null, | ||
$associativeArray['keys']['auth'] ?? null, | ||
$associativeArray['contentEncoding'] ?? "aesgcm" | ||
); | ||
} | ||
|
||
if (array_key_exists('publicKey', $associativeArray) || array_key_exists('authToken', $associativeArray) || array_key_exists('contentEncoding', $associativeArray)) { | ||
return new self( | ||
$associativeArray['endpoint'], | ||
$associativeArray['publicKey'] ?? null, | ||
$associativeArray['authToken'] ?? null, | ||
$associativeArray['contentEncoding'] ?? "aesgcm" | ||
$associativeArray['endpoint'] ?? "", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. endpoint shouldn't be empty, it should throw before, shouldn't it? |
||
$associativeArray['keys']['p256dh'] ?? "", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is an improvement, having an empty string instead of null ? |
||
$associativeArray['keys']['auth'] ?? "", | ||
$associativeArray['contentEncoding'] ?? ContentEncoding::aes128gcm, | ||
); | ||
} | ||
|
||
return new self( | ||
$associativeArray['endpoint'] | ||
$associativeArray['endpoint'] ?? "", | ||
$associativeArray['publicKey'] ?? "", | ||
$associativeArray['authToken'] ?? "", | ||
$associativeArray['contentEncoding'] ?? ContentEncoding::aes128gcm, | ||
); | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function getEndpoint(): string | ||
{ | ||
return $this->endpoint; | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function getPublicKey(): ?string | ||
public function getPublicKey(): string | ||
{ | ||
return $this->publicKey; | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function getAuthToken(): ?string | ||
public function getAuthToken(): string | ||
{ | ||
return $this->authToken; | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function getContentEncoding(): ?string | ||
public function getContentEncoding(): ContentEncoding | ||
{ | ||
return $this->contentEncoding; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,15 +14,16 @@ | |
namespace Minishlink\WebPush; | ||
|
||
/** | ||
* Subscription details from user agent. | ||
* @author Sergii Bondarenko <[email protected]> | ||
*/ | ||
interface SubscriptionInterface | ||
{ | ||
public function getEndpoint(): string; | ||
|
||
public function getPublicKey(): ?string; | ||
public function getPublicKey(): string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A subscription can have no public key, auth token or content encoding (sent without any payload) |
||
|
||
public function getAuthToken(): ?string; | ||
public function getAuthToken(): string; | ||
|
||
public function getContentEncoding(): ?string; | ||
public function getContentEncoding(): ContentEncoding; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add
@throws \ValueError
here?