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 a list of authentication schemes #262

Closed

Conversation

martinthomson
Copy link
Member

@martinthomson martinthomson commented Jun 16, 2017

We added the ability to discover the content encodings, but more or less forgot to add the supported authentication schemes. This corrects that oversight.

Copy link
Member

@beverloo beverloo left a comment

Thanks! I'm a bit in two minds about this. On one hand I can see the value, the implementation cost most likely is low and modulo a few more details it's simple enough.

On the other hand, the property describes capabilities of the push service, not the user agent. Since they can update independently, a UA is now expected to synchronise the available authentication schemes with the push service it has chosen. (Either though an occasional handshake or by aligning release schedules.) That's OK for us, but may not be equally feasible for everyone.

/cc @ShijunS FYI

@@ -688,6 +689,11 @@
when requesting the sending of a <a>push message</a> from the <a>push service</a>.
</p>
<p>
The <dfn data-dfn-for="PushManager">supportedAuthenticationSchemes</dfn> attribute exposes
the sequence of supported authentication schemes that can be used by <a>applications
Copy link
Member

@beverloo beverloo Jun 19, 2017

Choose a reason for hiding this comment

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

nit: s/applications/application/

The <dfn data-dfn-for="PushManager">supportedAuthenticationSchemes</dfn> attribute exposes
the sequence of supported authentication schemes that can be used by <a>applications
servers</a> to authenticate with the <a>push service</a>.
</p>
Copy link
Member

@beverloo beverloo Jun 19, 2017

Choose a reason for hiding this comment

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

Can we list at least "vapid" (and ideally require it -- is that feasible?), but ideally also explain what sort of other values one might expect in this array? Would Chrome include a x-google-gcm or similar if a proprietary mechanism were available?

Copy link
Member

@beverloo beverloo Jun 19, 2017

Choose a reason for hiding this comment

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

Also, does vapid refer to the latest version of the draft, or the previous one (still using Crypto-Key) that still has more implementations?

"PushSubscriptionOptions">applicationServerKey</a></code> needs to be valid according to
one of the supported authentication schemes (see <code><a data-link-for=
"PushManager">supportedAuthenticationSchemes</a></code>). For the "vapid" scheme defined
in [[!WEBPUSH-VAPID]], the value of <a data-link-for=
Copy link
Member

@beverloo beverloo Jun 19, 2017

Choose a reason for hiding this comment

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

This now implies that applicationServerKey can contain non-P-256 values too. That creates a conflict in the algorithm at 8.5.2, and makes the paragraph just before this one ambiguous: it mentions an elliptic curve, but what about other sort of values?

@martinthomson
Copy link
Member Author

@martinthomson martinthomson commented Jun 19, 2017

FWIW, I share your reservations here.

@martinthomson
Copy link
Member Author

@martinthomson martinthomson commented Aug 23, 2017

I think that we can abandon this. VAPID is looking to gain support for challenges, which in combination with the change to reject subscriptions if VAPID is required, mostly cover this off.

@martinthomson martinthomson deleted the authentication_schemes branch Aug 23, 2017
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.

None yet

2 participants