Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
129873019:
Support extension update dwnloads which require auth (Closed)
|
Created:
6 years, 11 months ago by Ken Rockot(use gerrit already) Modified:
6 years, 10 months ago Reviewers:
asargent_no_longer_on_chrome CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport extension update dwnloads which require auth
BUG=336945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248387
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add unit test #
Total comments: 6
Patch Set 3 : new tests, nits #
Messages
Total messages: 40 (0 generated)
Work in progress (still more code to come, tentatively). Looking to validate my strategy here. After going down the surprisingly deep rabbit-hole of adding data to sync to track whether or not an extension requires auth to update, I settled on this monumentally simpler approach of simply trying the download and falling back on a less-privatized fetch request (i.e. one with cookies enabled) in the case of a 401 response. https://codereview.chromium.org/129873019/diff/1/chrome/browser/extensions/up... File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/129873019/diff/1/chrome/browser/extensions/up... chrome/browser/extensions/updater/extension_downloader.cc:80: if ((url).DomainIs("google.com")) { \ Just cleaning up an uninteresting presubmit warning here.
A few thoughts: -I think we should come to some definitive conclusion about whether to use the sync login identity or not, and whether we want to use regular cookie jar cookies. It probably makes sense to setup a meeting with our PM for this feature and the PMs working on sync/login. (I can provide pointers if you don't know who those folks are.) -Given the privacy concerns, I think we are still going to need to restrict sending cookies to those extensions where the webstore told us at install time that the item would require authentication. https://codereview.chromium.org/129873019/diff/1/chrome/browser/extensions/up... File chrome/browser/extensions/updater/extension_downloader.h (right): https://codereview.chromium.org/129873019/diff/1/chrome/browser/extensions/up... chrome/browser/extensions/updater/extension_downloader.h:131: bool is_protected; nit: a comment here would be helpful to readers
We're going to attempt to move forward with this approach for M34 barring any
objections from privacy review.
Added a unit test to verify the 40{1,3} response flow. PTAL
https://codereview.chromium.org/129873019/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_downloader.h (right):
https://codereview.chromium.org/129873019/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_downloader.h:131: bool is_protected;
On 2014/01/23 19:37:52, Antony Sargent wrote:
> nit: a comment here would be helpful to readers
Done.
lgtm w/ some suggestions https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... chrome/browser/extensions/updater/extension_downloader.cc:106: ExtensionDownloader::ExtensionFetch::ExtensionFetch() : url() {} nit: make sure to initialize is_protected in this constructor too, or we could get uninitialized memory reads (maybe you would have gotten a clang warning about this, I'm not sure) https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... chrome/browser/extensions/updater/extension_downloader.cc:666: } Perhaps we should also only send cookies on https urls? This would help avoid exposing user credentials, eg on open wireless networks. (You should double check with the webstore folks about this - if they need to do the eventually download over plain http, maybe we could arrange to have some kind of redirection where the valuable cookies are sent over https which then generates a redirect to a non-high-value origin over plain http url with some token parameter which only provides access to download that crx file and nothing else, perhaps for a limited time too). https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... File chrome/browser/extensions/updater/extension_updater_unittest.cc (right): https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... chrome/browser/extensions/updater/extension_updater_unittest.cc:1119: Can you also add a test that proves that going into protected mode but then getting a second auth failure doesn't result in continually polling the server? You have a good protection for that in the implementation where you only re-queue the request if the first attempt had not used auth, but I'm worried future refactorings might break that, etc.
https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... chrome/browser/extensions/updater/extension_downloader.cc:106: ExtensionDownloader::ExtensionFetch::ExtensionFetch() : url() {} On 2014/01/31 18:22:26, Antony Sargent wrote: > nit: make sure to initialize is_protected in this constructor too, or we could > get uninitialized memory reads (maybe you would have gotten a clang warning > about this, I'm not sure) Done. https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... chrome/browser/extensions/updater/extension_downloader.cc:666: } On 2014/01/31 18:22:26, Antony Sargent wrote: > Perhaps we should also only send cookies on https urls? This would help avoid > exposing user credentials, eg on open wireless networks. (You should double > check with the webstore folks about this - if they need to do the eventually > download over plain http, maybe we could arrange to have some kind of > redirection where the valuable cookies are sent over https which then generates > a redirect to a non-high-value origin over plain http url with some token > parameter which only provides access to download that crx file and nothing else, > perhaps for a limited time too). Good catch. Done. https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... File chrome/browser/extensions/updater/extension_updater_unittest.cc (right): https://codereview.chromium.org/129873019/diff/70001/chrome/browser/extension... chrome/browser/extensions/updater/extension_updater_unittest.cc:1119: On 2014/01/31 18:22:26, Antony Sargent wrote: > Can you also add a test that proves that going into protected mode but then > getting a second auth failure doesn't result in continually polling the server? > You have a good protection for that in the implementation where you only > re-queue the request if the first attempt had not used auth, but I'm worried > future refactorings might break that, etc. Done. Also added a test to verify that non-https download URLs will never go into protected mode.
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/129873019/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/129873019/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/129873019/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/129873019/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/129873019/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/129873019/110001
Message was sent while issue was closed.
Change committed as 248387
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
