Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17009016:
Wire up the identity API for enterprise Kiosk Apps. (Closed)
|
Created:
7 years, 6 months ago by Mattias Nissler (ping if slow) Modified:
7 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWire up the identity API for enterprise Kiosk Apps.
This allows enterprise-managed Kiosk Apps to mint OAuth2 access tokens
for the device-level robot account via the identity extension API.
BUG=chromium:224594
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207835
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address review comments. #Patch Set 3 : Rebase. #
Total comments: 2
Patch Set 4 : Stop mint queue request on completion. #
Messages
Total messages: 14 (0 generated)
Zel, here's a cleaned-up version of the identity API wiring. I've tested it against a hand-minted auth code against prod GAIA. Do we want to land this now? Do you know who would be a good reviewer?
https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/experimental_identity_api.cc (right): https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/experimental_identity_api.cc:94: g_browser_process->browser_policy_connector()->IsEnterpriseManaged()) { do you want to test if DeviceOAuth2TokenServiceFactory has refresh token?
https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/experimental_identity_api.cc (right): https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/experimental_identity_api.cc:94: g_browser_process->browser_policy_connector()->IsEnterpriseManaged()) { On 2013/06/19 03:09:35, zel wrote: > do you want to test if DeviceOAuth2TokenServiceFactory has refresh token? Since we don't have the intention to send the user through the auth flow, the only thing we'd do if there's no token would be to bail out early. But then again, there's no harm in having DeviceOAuth2TokenService handle that situation (i.e. reporting an error immediately).
LGTM https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/experimental_identity_api.cc (right): https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/experimental_identity_api.cc:94: g_browser_process->browser_policy_connector()->IsEnterpriseManaged()) { On 2013/06/19 03:44:40, Mattias Nissler wrote: > On 2013/06/19 03:09:35, zel wrote: > > do you want to test if DeviceOAuth2TokenServiceFactory has refresh token? > > Since we don't have the intention to send the user through the auth flow, the > only thing we'd do if there's no token would be to bail out early. But then > again, there's no harm in having DeviceOAuth2TokenService handle that situation > (i.e. reporting an error immediately). Actually, we can still let app show sign-in UI on this identity API call. That way you will still permit enrolled kiosk app that don't have robot accounts associated with them.
+mpcomplete for OWNERS review.
lgtm
+courage FYI (per Zel's suggestion)
Since the whole flow for this case is completely disjoint from the existing code, t would be a lot clearer to have a separate class that encapsulates the whole device-level flow, and delegate to that class at the very top of Identity(Get|RemoveCached)AuthTokenFunction::RunImpl. At some point I'd like to integrate the Identity API class more closely with OAuth2TokenService since there is a lot of common functionality between them. When that happens it may be possible to merge the flows back together in a sensible way.
https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/experimental_identity_api.h (right): https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/experimental_identity_api.h:49: public OAuth2TokenService::Consumer { chrome.experimental.identity is only around for a couple of apps that got permission to use the API in order to launch before chrome.identity. Unless there's some reason, we should be doing everything for Kiosk on chrome.identity, and leave the experimental API as is. https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:112: chromeos::DeviceOAuth2TokenServiceFactory::Get()->StartRequest( The OAuth2TokenService will request tokens using Chrome's client ID. Is that expected/desired for kiosk apps? It's not normal for Identity API callers. Note also that the check on line 93 and also the OAuth2ManifestHandler will demand a "client_id" key, that is going to be ignored. https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:383: IdentityAPI::GetFactoryInstance()->GetForProfile(profile())->SetCachedToken( The Kiosk flow in RunImpl diverges before reading from the cache, so the tokens stored here will never be read. Similarly, calls to chrome.identity.removeCachedAuthToken (see IdentityRemoveCachedAuthTokenFunction) will not be propagated to OAuth2TokenService::InvalidateToken, so there will be no way to flush a bad token other than restarting. https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:395: CompleteFunctionWithError(error.ToString()); There's no way to get all error messages in sync given the different token fetching infrastructure, but OnGaiaFlowFailure prefixes these error strings with identity_constants::kAuthFailure.
Comments addressed, PTAL. https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/experimental_identity_api.h (right): https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/experimental_identity_api.h:49: public OAuth2TokenService::Consumer { On 2013/06/20 00:19:42, Michael Courage wrote: > chrome.experimental.identity is only around for a couple of apps that got > permission to use the API in order to launch before chrome.identity. > > Unless there's some reason, we should be doing everything for Kiosk on > chrome.identity, and leave the experimental API as is. OK, reverted the experimental changes. https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:112: chromeos::DeviceOAuth2TokenServiceFactory::Get()->StartRequest( On 2013/06/20 00:19:42, Michael Courage wrote: > The OAuth2TokenService will request tokens using Chrome's client ID. Is that > expected/desired for kiosk apps? It's not normal for Identity API callers. I'm aware of that. The any-api refresh token we have actually only allows us to fetch tokens for Chrome's client ID. That's certainly suboptimal, but we don't have a better alternative to the best of my knowledge. I left a TODO in the code now to address this. > Note also that the check on line 93 and also the OAuth2ManifestHandler will > demand a "client_id" key, that is going to be ignored. I think that's fine, given that we'd rather use the correct client ID eventually. https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:383: IdentityAPI::GetFactoryInstance()->GetForProfile(profile())->SetCachedToken( On 2013/06/20 00:19:42, Michael Courage wrote: > The Kiosk flow in RunImpl diverges before reading from the cache, so the tokens > stored here will never be read. > > Similarly, calls to chrome.identity.removeCachedAuthToken (see > IdentityRemoveCachedAuthTokenFunction) will not be propagated to > OAuth2TokenService::InvalidateToken, so there will be no way to flush a bad > token other than restarting. Moved token minting to the right place. https://codereview.chromium.org/17009016/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:395: CompleteFunctionWithError(error.ToString()); On 2013/06/20 00:19:42, Michael Courage wrote: > There's no way to get all error messages in sync given the different token > fetching infrastructure, but > OnGaiaFlowFailure prefixes these error strings with > identity_constants::kAuthFailure. Done.
lgtm with the mint queue fix. https://codereview.chromium.org/17009016/diff/20001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/17009016/diff/20001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:398: You need to CompleteMintTokenFlow() here to release subsequent requests from the mint_queue. (Identical GAIA requests are serialized.)
https://codereview.chromium.org/17009016/diff/20001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/17009016/diff/20001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:398: On 2013/06/21 06:13:39, Michael Courage wrote: > You need to CompleteMintTokenFlow() here to release subsequent requests from the > mint_queue. (Identical GAIA requests are serialized.) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/17009016/36001
Message was sent while issue was closed.
Change committed as 207835 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
