Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(631)

Issue 434493002: OAuth2 support for Webstore downloads. (Closed)

Created:
6 years, 4 months ago by Ken Rockot(use gerrit already)
Modified:
6 years, 4 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Project:
chromium
Visibility:
Public.

Description

OAuth2 support for Webstore downloads. This patch does three related things: 1. Plumbs an optional IdentityProvider through ExtensionUpdater / ExtensionDownloader. 2. Adds support in ExtensionDownloader for OAuth2 token- based authentication to protected Webstore download URLs, using the optional IdentityProvider as its account ID and token source. Bonus points for general cleanup of the credential iteration code in ExtensionDownloader. 3. Wires up Chrome's ExtensionDownloader (via ExtensionService) with a ProfileIdentityProvider so that it may authenticate as the Chrome-signed-in user via OAuth2. BUG=376553, 122763 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288261

Patch Set 1 #

Total comments: 9

Patch Set 2 #

Total comments: 7

Patch Set 3 : Remove WebstoreOAuth2TokenProvider #

Total comments: 6

Patch Set 4 : address asargent@ comments #

Patch Set 5 : axe //extensions changes for now #

Patch Set 6 #

Patch Set 7 : fix ExternalCache on chromeos #

Unified diffs Side-by-side diffs Delta from patch set Stats (+490 lines, -172 lines) Patch
M chrome/browser/chromeos/extensions/external_cache.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/extension_service.cc View 1 2 3 4 5 4 chunks +16 lines, -1 line 0 comments Download
M chrome/browser/extensions/updater/extension_downloader.h View 1 2 3 4 5 5 chunks +49 lines, -5 lines 0 comments Download
M chrome/browser/extensions/updater/extension_downloader.cc View 1 2 3 4 5 12 chunks +163 lines, -68 lines 0 comments Download
M chrome/browser/extensions/updater/extension_updater.h View 3 chunks +6 lines, -1 line 0 comments Download
M chrome/browser/extensions/updater/extension_updater.cc View 2 chunks +19 lines, -11 lines 0 comments Download
M chrome/browser/extensions/updater/extension_updater_unittest.cc View 1 21 chunks +236 lines, -85 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
Ken Rockot(use gerrit already)
Antony, could you please take a look?
6 years, 4 months ago (2014-08-05 19:38:00 UTC) #1
asargent_no_longer_on_chrome
https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensions/updater/extension_downloader.cc File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensions/updater/extension_downloader.cc#newcode201 chrome/browser/extensions/updater/extension_downloader.cc:201: } I think this function pushes ExtensionFetch over the ...
6 years, 4 months ago (2014-08-06 00:26:29 UTC) #2
Ken Rockot(use gerrit already)
+rogerta for google_apis/gaia dependency; please see inline discussion as well https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensions/updater/extension_downloader.cc File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensions/updater/extension_downloader.cc#newcode201 ...
6 years, 4 months ago (2014-08-06 16:49:56 UTC) #3
asargent_no_longer_on_chrome
https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS File extensions/browser/DEPS (right): https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS#newcode5 extensions/browser/DEPS:5: "+google_apis/gaia", On 2014/08/06 16:49:55, Ken Rockot wrote: > On ...
6 years, 4 months ago (2014-08-06 19:38:48 UTC) #4
Ken Rockot(use gerrit already)
> BTW I'm not strongly opposed to the way you have it or anything - ...
6 years, 4 months ago (2014-08-06 20:06:54 UTC) #5
Ken Rockot(use gerrit already)
On 2014/08/06 20:06:54, Ken Rockot wrote: > > BTW I'm not strongly opposed to the ...
6 years, 4 months ago (2014-08-06 20:19:43 UTC) #6
Roger Tawa OOO till Jul 10th
Hi guys, I don't see a problem introducing a dep on google_apis/gaia. See other comments ...
6 years, 4 months ago (2014-08-07 01:35:16 UTC) #7
Ken Rockot(use gerrit already)
Thanks! https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensions/updater/extension_downloader.cc File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensions/updater/extension_downloader.cc#newcode870 chrome/browser/extensions/updater/extension_downloader.cc:870: base::StringPrintf("%s:Bearer %s", On 2014/08/07 01:35:15, Roger Tawa (build ...
6 years, 4 months ago (2014-08-07 16:49:09 UTC) #8
asargent_no_longer_on_chrome
lgtm w/ a few small suggestions https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensions/updater/extension_downloader.cc File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensions/updater/extension_downloader.cc#newcode753 chrome/browser/extensions/updater/extension_downloader.cc:753: this); It might ...
6 years, 4 months ago (2014-08-07 17:09:12 UTC) #9
Ken Rockot(use gerrit already)
Many thanks. Roger, could you please take another look? https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensions/updater/extension_downloader.cc File chrome/browser/extensions/updater/extension_downloader.cc (right): https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensions/updater/extension_downloader.cc#newcode753 chrome/browser/extensions/updater/extension_downloader.cc:753: ...
6 years, 4 months ago (2014-08-07 19:50:48 UTC) #10
Roger Tawa OOO till Jul 10th
lgtm Thanks Ken.
6 years, 4 months ago (2014-08-07 20:09:32 UTC) #11
Ken Rockot(use gerrit already)
James, could you please take a look for //e/b/DEPS and adding myself as an OWNER ...
6 years, 4 months ago (2014-08-07 20:15:55 UTC) #12
Ken Rockot(use gerrit already)
Oops. Nevermind James. No changes to //e/b/DEPS in this CL now. :D
6 years, 4 months ago (2014-08-07 20:16:48 UTC) #13
Ken Rockot(use gerrit already)
The CQ bit was checked by rockot@chromium.org
6 years, 4 months ago (2014-08-07 21:03:12 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/434493002/300001
6 years, 4 months ago (2014-08-07 21:22:23 UTC) #15
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux ...
6 years, 4 months ago (2014-08-08 02:30:47 UTC) #16
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-08 02:42:33 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_swarming/builds/2117)
6 years, 4 months ago (2014-08-08 02:42:34 UTC) #18
Ken Rockot(use gerrit already)
The CQ bit was checked by rockot@chromium.org
6 years, 4 months ago (2014-08-08 02:55:03 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rockot@chromium.org/434493002/320001
6 years, 4 months ago (2014-08-08 02:56:22 UTC) #20
commit-bot: I haz the power
6 years, 4 months ago (2014-08-08 08:35:15 UTC) #21
Message was sent while issue was closed.
Change committed as 288261

Powered by Google App Engine
This is Rietveld 408576698