Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2436393002:
Disallow repeated PushManager.subscribes with different sender ids (Closed)
|
Created:
4 years, 2 months ago by awdf Modified:
4 years, 1 month ago CC:
chromium-reviews, awdf+watch_chromium.org, Peter Beverloo, johnme+watch_chromium.org, jam, darin-cc_chromium.org, blink-reviews, harkness+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisallow repeated PushManager.subscribes with different sender ids
- Now we throw an InvalidStateError if you try to subscribe again with
a different sender id without unsubscribing first (instead of
confusingly returning the end point associated with the previous
sender id).
- This still allows resubscribing with no key if the first subscription
used a numeric gcm sender id and the second subscription is from a
service worker (see crbug.com/659230)
BUG=638924
Committed: https://crrev.com/b9dabcc3e279cb499c74e7d7e57765af5c21d91e
Cr-Commit-Position: refs/heads/master@{#430692}
Patch Set 1 #Patch Set 2 : Get registration id and sender id from storage at once #Patch Set 3 : Add value to histograms enum, tidy blink test #Patch Set 4 : Remove leftover method declaration from header file #Patch Set 5 : Fail with an InvalidStateError, instead of an AbortError #Patch Set 6 : Rename registration_id -> push_registration_id #
Total comments: 13
Patch Set 7 : make things const; fix up html test file #Patch Set 8 : Make resubscribing for push with a different key trigger an InvalidStateError #Patch Set 9 : rebase on b659230 fix; add browser test #
Total comments: 2
Patch Set 10 : Remove unnecessary ternary #Patch Set 11 : Re-rebase on base patch #
Total comments: 6
Patch Set 12 : rebase and comments #
Total comments: 3
Patch Set 13 : oops - forgot js changes required for new browser test to pass #Patch Set 14 : Make error message more informative #Depends on Patchset: Messages
Total messages: 52 (25 generated)
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Minimal code to pass the test - IN PROG, could be simplified Added a failing layout test for re-subscribing with a different sender id BUG=638924 ========== to ========== Reject PushManager.subscribe() calls that have a different sender id - Also added a layout test to check an error is given when attepmting to re-subscribe with a different sender id BUG=638924 ==========
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
awdf@chromium.org changed reviewers: + harkness@chromium.org
+harkness for review
https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:300: auto& sender_id = push_registration_id_and_sender_id[1]; These can be const. https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html (right): https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:2: <html> FYI: In the future we're going to move to an indented HTML code style, but currently this is consistent with many of the files in the directory, so it can be caught with the mass consistency pass later. https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:18: // This is equal to the gcm_sender_id in push_manifest.json Since you're setting the applicationServerKey, you shouldn't need to provide the manifest. Having it there when you're also providing applicationServerKey can hide real errors. https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:34: 'push-messaging', 'granted', location.origin, location.origin); We are trying to convert to 80 column width, if you can wrap here and other long lines.
Drive-by review comment https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:301: if (data.options.sender_info != sender_id) { data.options.sender_info might be empty here, and if it is empty we interpret that as "keep using the stored sender ID" (see below where we lookup kPushSenderIdServiceWorkerKey). You hit this case if you register from a document with a manifest containing gcm_sender_id, then later register from a Service Worker, since SWs can't have an associated manifest so we cache the document manifest's gcm_sender_id in that case. tl;dr: I think this check should be: if (!data.options.sender_info.empty() && data.options.sender_info != sender_id) Hopefully this was already causing a test to fail (either in PushMessagingBrowserTest or layout tests); if not we might want to add one. (it might be nice to distinguish between omitting the applicationServerKey and explicitly setting the applicationServerKey to the empty string, and still throw in the latter case, but that's a bit of an edge case, and awkward to implement since std::string doesn't have a null state, so probably fine to just leave a comment documenting our quirky behavior in that edge case)
On 2016/10/25 10:11:22, harkness wrote: > https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... > File content/browser/push_messaging/push_messaging_message_filter.cc (right): > > https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... > content/browser/push_messaging/push_messaging_message_filter.cc:300: auto& > sender_id = push_registration_id_and_sender_id[1]; > These can be const. > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html > (right): > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:2: > <html> > FYI: In the future we're going to move to an indented HTML code style, but > currently this is consistent with many of the files in the directory, so it can > be caught with the mass consistency pass later. > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:18: > // This is equal to the gcm_sender_id in push_manifest.json > Since you're setting the applicationServerKey, you shouldn't need to provide the > manifest. Having it there when you're also providing applicationServerKey can > hide real errors. > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:34: > 'push-messaging', 'granted', location.origin, location.origin); > We are trying to convert to 80 column width, if you can wrap here and other long > lines. Done. It's a pity 'git cl format' doesn't work on html files.
gah, replied in the wrong place, sorry. **runs off cursing rietveld** On Tue, 25 Oct 2016 at 14:03 <awdf@chromium.org> wrote: > On 2016/10/25 10:11:22, harkness wrote: > > > > https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... > > File content/browser/push_messaging/push_messaging_message_filter.cc > (right): > > > > > > https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... > > content/browser/push_messaging/push_messaging_message_filter.cc:300: > auto& > > sender_id = push_registration_id_and_sender_id[1]; > > These can be const. > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > File > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html > > (right): > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:2: > > <html> > > FYI: In the future we're going to move to an indented HTML code style, > but > > currently this is consistent with many of the files in the directory, so > it > can > > be caught with the mass consistency pass later. > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:18: > > // This is equal to the gcm_sender_id in push_manifest.json > > Since you're setting the applicationServerKey, you shouldn't need to > provide > the > > manifest. Having it there when you're also providing > applicationServerKey can > > hide real errors. > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:34: > > 'push-messaging', 'granted', location.origin, location.origin); > > We are trying to convert to 80 column width, if you can wrap here and > other > long > > lines. > > Done. It's a pity 'git cl format' doesn't work on html files. > > https://codereview.chromium.org/2436393002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
gah, replied in the wrong place, sorry. **runs off cursing rietveld** On Tue, 25 Oct 2016 at 14:03 <awdf@chromium.org> wrote: > On 2016/10/25 10:11:22, harkness wrote: > > > > https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... > > File content/browser/push_messaging/push_messaging_message_filter.cc > (right): > > > > > > https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... > > content/browser/push_messaging/push_messaging_message_filter.cc:300: > auto& > > sender_id = push_registration_id_and_sender_id[1]; > > These can be const. > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > File > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html > > (right): > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:2: > > <html> > > FYI: In the future we're going to move to an indented HTML code style, > but > > currently this is consistent with many of the files in the directory, so > it > can > > be caught with the mass consistency pass later. > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:18: > > // This is equal to the gcm_sender_id in push_manifest.json > > Since you're setting the applicationServerKey, you shouldn't need to > provide > the > > manifest. Having it there when you're also providing > applicationServerKey can > > hide real errors. > > > > > > https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... > > > > third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:34: > > 'push-messaging', 'granted', location.origin, location.origin); > > We are trying to convert to 80 column width, if you can wrap here and > other > long > > lines. > > Done. It's a pity 'git cl format' doesn't work on html files. > > https://codereview.chromium.org/2436393002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:300: auto& sender_id = push_registration_id_and_sender_id[1]; On 2016/10/25 10:11:22, harkness wrote: > These can be const. Done. https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:301: if (data.options.sender_info != sender_id) { On 2016/10/25 12:33:31, johnme wrote: > data.options.sender_info might be empty here, and if it is empty we interpret > that as "keep using the stored sender ID" (see below where we lookup > kPushSenderIdServiceWorkerKey). You hit this case if you register from a > document with a manifest containing gcm_sender_id, then later register from a > Service Worker, since SWs can't have an associated manifest so we cache the > document manifest's gcm_sender_id in that case. > > tl;dr: I think this check should be: > if (!data.options.sender_info.empty() && > data.options.sender_info != sender_id) > > Hopefully this was already causing a test to fail (either in > PushMessagingBrowserTest or layout tests); if not we might want to add one. > > (it might be nice to distinguish between omitting the applicationServerKey and > explicitly setting the applicationServerKey to the empty string, and still throw > in the latter case, but that's a bit of an edge case, and awkward to implement > since std::string doesn't have a null state, so probably fine to just leave a > comment documenting our quirky behavior in that edge case) If data.options.sender_info is empty here though, won't it necessarily be different from the retrieved sender_id, given that one was retrieved? (We only get to this point if both kPushSenderIdServiceWorkerKey and kPushRegistrationIdServiceWorkerKey are successfully retrieved). I can see that we might want to be explicit about checking it's not empty but I want to understand why you think it should cause test failures right now. Might the retrieved stored sender id be empty too? In which case where would gcm_sender_id get retrieved to use instead? No push_messaging/ layout tests or tests in PushMessagingBrowserTest are failing right now. https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html (right): https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:2: <html> On 2016/10/25 10:11:22, harkness wrote: > FYI: In the future we're going to move to an indented HTML code style, but > currently this is consistent with many of the files in the directory, so it can > be caught with the mass consistency pass later. Acknowledged. https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:18: // This is equal to the gcm_sender_id in push_manifest.json On 2016/10/25 10:11:22, harkness wrote: > Since you're setting the applicationServerKey, you shouldn't need to provide the > manifest. Having it there when you're also providing applicationServerKey can > hide real errors. Good spot. (I copied another test and didn't notice that by including push_manifest.json I was setting the gcm_sender-id too, I just thought this comment was saying that applicationServerKey was equivalent to gcm_sender_id if one exists.) Have stopped including the manifest and removed this comment. https://codereview.chromium.org/2436393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/push_messaging/subscribe-failure-mismatched-sender-id.html:34: 'push-messaging', 'granted', location.origin, location.origin); On 2016/10/25 10:11:22, harkness wrote: > We are trying to convert to 80 column width, if you can wrap here and other long > lines. Done.
https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:301: if (data.options.sender_info != sender_id) { On 2016/10/25 13:18:18, awdf wrote: > On 2016/10/25 12:33:31, johnme wrote: > > data.options.sender_info might be empty here, and if it is empty we interpret > > that as "keep using the stored sender ID" (see below where we lookup > > kPushSenderIdServiceWorkerKey). You hit this case if you register from a > > document with a manifest containing gcm_sender_id, then later register from a > > Service Worker, since SWs can't have an associated manifest so we cache the > > document manifest's gcm_sender_id in that case. > > > > tl;dr: I think this check should be: > > if (!data.options.sender_info.empty() && > > data.options.sender_info != sender_id) > > > > Hopefully this was already causing a test to fail (either in > > PushMessagingBrowserTest or layout tests); if not we might want to add one. > > > > (it might be nice to distinguish between omitting the applicationServerKey and > > explicitly setting the applicationServerKey to the empty string, and still > throw > > in the latter case, but that's a bit of an edge case, and awkward to implement > > since std::string doesn't have a null state, so probably fine to just leave a > > comment documenting our quirky behavior in that edge case) > > If data.options.sender_info is empty here though, won't it necessarily be > different from the retrieved sender_id, given that one was retrieved? (We only > get to this point if both kPushSenderIdServiceWorkerKey and > kPushRegistrationIdServiceWorkerKey are successfully retrieved). I can see that > we might want to be explicit about checking it's not empty but I want to > understand why you think it should cause test failures right now. Might the > retrieved stored sender id be empty too? In which case where would gcm_sender_id > get retrieved to use instead? > > No push_messaging/ layout tests or tests in PushMessagingBrowserTest are failing > right now. PushMessagingBrowserTest.SubscribeWorker and PushMessagingBrowserTest.SubscribeWorkerUsingManifest cover the case I'm talking about, where a subscribe call from a SW with neither manifest nor applicationServerKey should succeed even though data.options.sender_info is empty because we cached the last sender ID they successfully subscribed with. Unfortunately those tests are disabled on Linux because of unexplained flakiness ;) The caching is a bit of a hack, but back before applicationServerKey when we only supported manifests, it was the only way to enable subscribing from SWs, and we shouldn't break it unnecessarily.
https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:301: if (data.options.sender_info != sender_id) { On 2016/10/25 13:28:25, johnme wrote: > On 2016/10/25 13:18:18, awdf wrote: > > On 2016/10/25 12:33:31, johnme wrote: > > > data.options.sender_info might be empty here, and if it is empty we > interpret > > > that as "keep using the stored sender ID" (see below where we lookup > > > kPushSenderIdServiceWorkerKey). You hit this case if you register from a > > > document with a manifest containing gcm_sender_id, then later register from > a > > > Service Worker, since SWs can't have an associated manifest so we cache the > > > document manifest's gcm_sender_id in that case. > > > > > > tl;dr: I think this check should be: > > > if (!data.options.sender_info.empty() && > > > data.options.sender_info != sender_id) > > > > > > Hopefully this was already causing a test to fail (either in > > > PushMessagingBrowserTest or layout tests); if not we might want to add one. > > > > > > (it might be nice to distinguish between omitting the applicationServerKey > and > > > explicitly setting the applicationServerKey to the empty string, and still > > throw > > > in the latter case, but that's a bit of an edge case, and awkward to > implement > > > since std::string doesn't have a null state, so probably fine to just leave > a > > > comment documenting our quirky behavior in that edge case) > > > > If data.options.sender_info is empty here though, won't it necessarily be > > different from the retrieved sender_id, given that one was retrieved? (We only > > get to this point if both kPushSenderIdServiceWorkerKey and > > kPushRegistrationIdServiceWorkerKey are successfully retrieved). I can see > that > > we might want to be explicit about checking it's not empty but I want to > > understand why you think it should cause test failures right now. Might the > > retrieved stored sender id be empty too? In which case where would > gcm_sender_id > > get retrieved to use instead? > > > > No push_messaging/ layout tests or tests in PushMessagingBrowserTest are > failing > > right now. > > PushMessagingBrowserTest.SubscribeWorker and > PushMessagingBrowserTest.SubscribeWorkerUsingManifest cover the case I'm talking > about, where a subscribe call from a SW with neither manifest nor > applicationServerKey should succeed even though data.options.sender_info is > empty because we cached the last sender ID they successfully subscribed with. > Unfortunately those tests are disabled on Linux because of unexplained flakiness > ;) > > The caching is a bit of a hack, but back before applicationServerKey when we > only supported manifests, it was the only way to enable subscribing from SWs, > and we shouldn't break it unnecessarily. Oh, sorry, got confused, I now understand you're saying the sender id is *allowed* to be different if it's now empty but we had a valid one before. (Think I was ignoring the '!' when reading your tl;dr before!) I'll fix it and see if I can add a layout test for that scenario.
https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/100001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:301: if (data.options.sender_info != sender_id) { On 2016/10/25 13:50:36, awdf wrote: > On 2016/10/25 13:28:25, johnme wrote: > > On 2016/10/25 13:18:18, awdf wrote: > > > On 2016/10/25 12:33:31, johnme wrote: > > > > data.options.sender_info might be empty here, and if it is empty we > > interpret > > > > that as "keep using the stored sender ID" (see below where we lookup > > > > kPushSenderIdServiceWorkerKey). You hit this case if you register from a > > > > document with a manifest containing gcm_sender_id, then later register > from > > a > > > > Service Worker, since SWs can't have an associated manifest so we cache > the > > > > document manifest's gcm_sender_id in that case. > > > > > > > > tl;dr: I think this check should be: > > > > if (!data.options.sender_info.empty() && > > > > data.options.sender_info != sender_id) > > > > > > > > Hopefully this was already causing a test to fail (either in > > > > PushMessagingBrowserTest or layout tests); if not we might want to add > one. > > > > > > > > (it might be nice to distinguish between omitting the applicationServerKey > > and > > > > explicitly setting the applicationServerKey to the empty string, and still > > > throw > > > > in the latter case, but that's a bit of an edge case, and awkward to > > implement > > > > since std::string doesn't have a null state, so probably fine to just > leave > > a > > > > comment documenting our quirky behavior in that edge case) > > > > > > If data.options.sender_info is empty here though, won't it necessarily be > > > different from the retrieved sender_id, given that one was retrieved? (We > only > > > get to this point if both kPushSenderIdServiceWorkerKey and > > > kPushRegistrationIdServiceWorkerKey are successfully retrieved). I can see > > that > > > we might want to be explicit about checking it's not empty but I want to > > > understand why you think it should cause test failures right now. Might the > > > retrieved stored sender id be empty too? In which case where would > > gcm_sender_id > > > get retrieved to use instead? > > > > > > No push_messaging/ layout tests or tests in PushMessagingBrowserTest are > > failing > > > right now. > > > > PushMessagingBrowserTest.SubscribeWorker and > > PushMessagingBrowserTest.SubscribeWorkerUsingManifest cover the case I'm > talking > > about, where a subscribe call from a SW with neither manifest nor > > applicationServerKey should succeed even though data.options.sender_info is > > empty because we cached the last sender ID they successfully subscribed with. > > Unfortunately those tests are disabled on Linux because of unexplained > flakiness > > ;) > > > > The caching is a bit of a hack, but back before applicationServerKey when we > > only supported manifests, it was the only way to enable subscribing from SWs, > > and we shouldn't break it unnecessarily. > > Oh, sorry, got confused, I now understand you're saying the sender id is > *allowed* to be different if it's now empty but we had a valid one before. > (Think I was ignoring the '!' when reading your tl;dr before!) > > I'll fix it and see if I can add a layout test for that scenario. There is already a browser test for that, but it's one of the ones that is currently disabled for being flaky. :(
Have rebased on the other review for no-key-resubscribes ( https://codereview.chromium.org/2469293002/ ) so now the change to content/browser/push_messaging/push_messaging_message_filter.cc is much simpler. Also added a browser test for my own reassurance, since I found in the other review that layout tests proved to be insufficient for catching that dcheck.
https://codereview.chromium.org/2436393002/diff/160001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/160001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:382: data.options.sender_info.empty() ? sender_id[0] Here I can now always use the stored sender id since it is guaranteed to be the same as the passed-in one thanks to the new check above. Will fix.
https://codereview.chromium.org/2436393002/diff/160001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/160001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:382: data.options.sender_info.empty() ? sender_id[0] On 2016/11/04 11:45:41, awdf wrote: > Here I can now always use the stored sender id since it is guaranteed to be the > same as the passed-in one thanks to the new check above. > > Will fix. Done.
Description was changed from ========== Reject PushManager.subscribe() calls that have a different sender id - Also added a layout test to check an error is given when attepmting to re-subscribe with a different sender id BUG=638924 ========== to ========== Disallow repeated PushManager.subscribes with different sender ids - Also added a layout test to check an error is given when attepmting to re-subscribe with a different sender id BUG=638924 ==========
lgtm % nits https://codereview.chromium.org/2436393002/diff/200001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2436393002/diff/200001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:844: EndpointToToken(script_result, false /* standard_protocol */)); If you're going to parse the endpoint, you should check that it's different from the original. https://codereview.chromium.org/2436393002/diff/200001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/200001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:321: std::string sender_id = nit: Could you make this validated_sender_id? Just to signify that it's the real one we should use as opposed to all the other sender_ids. https://codereview.chromium.org/2436393002/diff/200001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:331: // TODO(crbug.com/638924): Check that stored sender ID equals Remove the TODO now.
https://codereview.chromium.org/2436393002/diff/200001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2436393002/diff/200001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:844: EndpointToToken(script_result, false /* standard_protocol */)); On 2016/11/08 11:56:59, harkness wrote: > If you're going to parse the endpoint, you should check that it's different from > the original. Done. https://codereview.chromium.org/2436393002/diff/200001/content/browser/push_m... File content/browser/push_messaging/push_messaging_message_filter.cc (right): https://codereview.chromium.org/2436393002/diff/200001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:321: std::string sender_id = On 2016/11/08 11:56:59, harkness wrote: > nit: Could you make this validated_sender_id? Just to signify that it's the real > one we should use as opposed to all the other sender_ids. Renamed to 'fixed_sender_id' in https://codereview.chromium.org/2469293002 (as 'validated' perhaps implies a higher level of validation than it has gone through, whereas 'fixed' is helpfully vague) https://codereview.chromium.org/2436393002/diff/200001/content/browser/push_m... content/browser/push_messaging/push_messaging_message_filter.cc:331: // TODO(crbug.com/638924): Check that stored sender ID equals On 2016/11/08 11:56:59, harkness wrote: > Remove the TODO now. oops, this crept back in, good spot. Done.
awdf@chromium.org changed reviewers: + johnme@chromium.org
+johnme@chromium.org for OWNERS review (you're an owner for all of it right?)
Description was changed from ========== Disallow repeated PushManager.subscribes with different sender ids - Also added a layout test to check an error is given when attepmting to re-subscribe with a different sender id BUG=638924 ========== to ========== Disallow repeated PushManager.subscribes with different sender ids - Now we throw an InvalidStateError if you try to subscribe again with a different sender id without unsubscribing first (instead of confusingly returning the end point associated with the previous sender id). - This still allows resubscribing with no key if the first subscription used a numeric gcm sender id and the second subscription is from a service worker (see crbug.com/659230) BUG=638924 ==========
Description was changed from ========== Disallow repeated PushManager.subscribes with different sender ids - Now we throw an InvalidStateError if you try to subscribe again with a different sender id without unsubscribing first (instead of confusingly returning the end point associated with the previous sender id). - This still allows resubscribing with no key if the first subscription used a numeric gcm sender id and the second subscription is from a service worker (see crbug.com/659230) BUG=638924 ========== to ========== Disallow repeated PushManager.subscribes with different sender ids - Now we throw an InvalidStateError if you try to subscribe again with a different sender id without unsubscribing first (instead of confusingly returning the end point associated with the previous sender id). - This still allows resubscribing with no key if the first subscription used a numeric gcm sender id and the second subscription is from a service worker (see crbug.com/659230) BUG=638924 ==========
lgtm with nit - thanks! > +johnme@chromium.org for OWNERS review (you're an owner for all of it right?) You'll need separate approvals for content/public/ and histograms.xml. Those reviewers are usually pretty quick though :) https://codereview.chromium.org/2436393002/diff/220001/content/public/common/... File content/public/common/push_messaging_status.cc (right): https://codereview.chromium.org/2436393002/diff/220001/content/public/common/... content/public/common/push_messaging_status.cc:57: return "Registration failed - A subscription with a different sender ID " Let's be explicit. How about "A subscription with a different applicationServerKey (or gcm_sender_id) already exists; to change the applicationServerKey, unsubscribe then resubscribe"
https://codereview.chromium.org/2436393002/diff/220001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/2436393002/diff/220001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:833: RunScript("workerSubscribePushWithNumericKey('11111')", &script_result)); I actually forgot to reapply the javascript changes to support this so this test didn't pass in this patch - now done in patch set 13 (push_test.js and service_worker.js, please take a quick look) https://codereview.chromium.org/2436393002/diff/220001/content/public/common/... File content/public/common/push_messaging_status.cc (right): https://codereview.chromium.org/2436393002/diff/220001/content/public/common/... content/public/common/push_messaging_status.cc:57: return "Registration failed - A subscription with a different sender ID " On 2016/11/08 13:39:55, johnme wrote: > Let's be explicit. How about "A subscription with a different > applicationServerKey (or gcm_sender_id) already exists; to change the > applicationServerKey, unsubscribe then resubscribe" ok - how about "Registration failed - A subscription with a different applicationServerKey (or gcm_sender_id) already exists; to change the applicationServerKey, unsubscribe before calling subscribe() again."?
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
lgtm still
The CQ bit was checked by awdf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org Link to the patchset: https://codereview.chromium.org/2436393002/#ps260001 (title: "Make error message more informative")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
awdf@chromium.org changed reviewers: + avi@chromium.org, jwd@chromium.org
avi@chromium.org: Please review changes in content/public jwd@chromium.org: Please review changes in tools/metrics/histograms/histograms.xml Thanks!
lgtm 👍
lgtm
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disallow repeated PushManager.subscribes with different sender ids - Now we throw an InvalidStateError if you try to subscribe again with a different sender id without unsubscribing first (instead of confusingly returning the end point associated with the previous sender id). - This still allows resubscribing with no key if the first subscription used a numeric gcm sender id and the second subscription is from a service worker (see crbug.com/659230) BUG=638924 ========== to ========== Disallow repeated PushManager.subscribes with different sender ids - Now we throw an InvalidStateError if you try to subscribe again with a different sender id without unsubscribing first (instead of confusingly returning the end point associated with the previous sender id). - This still allows resubscribing with no key if the first subscription used a numeric gcm sender id and the second subscription is from a service worker (see crbug.com/659230) BUG=638924 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Disallow repeated PushManager.subscribes with different sender ids - Now we throw an InvalidStateError if you try to subscribe again with a different sender id without unsubscribing first (instead of confusingly returning the end point associated with the previous sender id). - This still allows resubscribing with no key if the first subscription used a numeric gcm sender id and the second subscription is from a service worker (see crbug.com/659230) BUG=638924 ========== to ========== Disallow repeated PushManager.subscribes with different sender ids - Now we throw an InvalidStateError if you try to subscribe again with a different sender id without unsubscribing first (instead of confusingly returning the end point associated with the previous sender id). - This still allows resubscribing with no key if the first subscription used a numeric gcm sender id and the second subscription is from a service worker (see crbug.com/659230) BUG=638924 Committed: https://crrev.com/b9dabcc3e279cb499c74e7d7e57765af5c21d91e Cr-Commit-Position: refs/heads/master@{#430692} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b9dabcc3e279cb499c74e7d7e57765af5c21d91e Cr-Commit-Position: refs/heads/master@{#430692} |
