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

Provide support for subscription association #182

Closed
wants to merge 0 commits into from

Conversation

martinthomson
Copy link
Member

I think that this is better than #180 on balance. It's a little simpler.

ensure that it describes a valid point on the P-256 curve). If the
<code><a>applicationServerKey</a></code> value is invalid, reject <var>promise</var> with
an <code><a href=
"http://heycam.github.io/webidl/#invalidaccesserror">InvalidAccessError</a></code> and
Copy link
Member

Choose a reason for hiding this comment

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

nit: https

@beverloo
Copy link
Member

LGTM

I prefer using a BufferSource because (a) the UA generally won't have to do anything with the key, and (b) there is no ubiquitous support for uncompressed EC points across Web Crypto implementations.

I'm guessing that the common case will become something like:

const APPLICATION_SERVER_KEY = new Uint8Array([
    0x04, 0x00, 0x00, 0x00, ...
]);

Which is much more developer friendly than loading a CryptoKey object (which, in Chrome, requires converting to a JWK object prior to importing :-/).

Thank you! :)

@martinthomson martinthomson changed the title A lower level version of #180 Provide support for subscription association Jan 20, 2016
@beverloo
Copy link
Member

One question that came to mind - developers tend to call subscribe() multiple times, also as a means to get an existing subscription, since the API only supports once.

What happens when calling subscribe() with an applicationServerKey that differs from the subscription that's currently stored by the UA? Should we throw an error? InvalidAccessError sounds most appropriate, but NoModificationAllowedError (less common) might also work. Happy to address this in a follow-up.

@martinthomson
Copy link
Member Author

Ahh, good point. I'll add a commit for that. Another question that occurs, similar to the above, do we want the value to be available for inspection? That seems relatively harmless, but it means that it will be serialized, which I'm not sure about.

@beverloo
Copy link
Member

How would we confirm that the applicationServerKeys match without storing it? (Or a hash of the value.) It feels like a bit of a footgun if we don't throw an error for this situation.

Regarding exposing it again - it could be added to PushSubscription, but then it might be best to just expose the entire PushSubscriptionOptions object. This won't be very straightforward to implement this in Chrome and has some overhead, but we can do it.

I could live with not exposing it for a v1 as well, but I suspect that we'll get use-cases for it pretty quickly (e.g. "do I have to cycle my subscription").

@martinthomson
Copy link
Member Author

There's an alternative design that this suggests: subscribe with different options creates a new subscription.

I think that exposing the options would be good. We can probably exclude them from the serializer. I'll open an issue to track both the change in options (throw an error) and the exposing of the current options. Might as well incrementally improve, and allow you time to work out how to expose stuff.

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.

2 participants