Support for auth code based authentication flow for both app and web UI driven flow.

Support for auth code based authentication flow for both app and web UI driven flow.


BUG=181008
TEST=experiments that are in progress
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190338

[xiyuan, 2013-03-20 20:08:29]

LGTM

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:438:
command_line->HasSwitch(::switches::kAppId);
nit: use chrome::IsRunningInForcedAppMode() from app_model_util?

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/oauth_login_manager.h:74: //
RESTORE_FROM_COMMAND_LINE_OAUTH2_REFRESH_TOKEN or
nit: RESTORE_FROM_COMMAND_LINE_OAUTH2_REFRESH_TOKEN ->
RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN and 
RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN ->
RESTORE_FROM_AUTH_CODE?

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:318:
chrome.send('completeAuthentication', [msg.email, msg.authCode]);
nit: 2-space indent from previous line.

https://codereview.chromium.org/12704002/diff/5001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:97: const char kAppModeAuthCode[]            =
"app-mode-auth-code";
nit: align '=' with other lines.

[zel, 2013-03-20 21:03:38]

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:438:
command_line->HasSwitch(::switches::kAppId);
On 2013/03/20 20:08:29, xiyuan wrote:
> nit: use chrome::IsRunningInForcedAppMode() from app_model_util?

Done.

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/oauth_login_manager.h:74: //
RESTORE_FROM_COMMAND_LINE_OAUTH2_REFRESH_TOKEN or
On 2013/03/20 20:08:29, xiyuan wrote:
> nit: RESTORE_FROM_COMMAND_LINE_OAUTH2_REFRESH_TOKEN ->
> RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN and 
> RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN ->
> RESTORE_FROM_AUTH_CODE?

Done.

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:318:
chrome.send('completeAuthentication', [msg.email, msg.authCode]);
On 2013/03/20 20:08:29, xiyuan wrote:
> nit: 2-space indent from previous line.

Done.

https://codereview.chromium.org/12704002/diff/5001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12704002/diff/5001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:97: const char kAppModeAuthCode[]            =
"app-mode-auth-code";
On 2013/03/20 20:08:29, xiyuan wrote:
> nit: align '=' with other lines.

Done.

[Mattias Nissler (ping if slow), 2013-03-21 07:02:03]

Maybe it's too late, but still asking just in case: Why do we need the login
flow to support auth codes? At least for device-level enterprise robots, we're
planning to handle all this without any user interaction behind the scenes at
enrollment time.

[zel, 2013-03-21 16:53:50]

On 2013/03/21 07:02:03, Mattias Nissler wrote:
> Maybe it's too late, but still asking just in case: Why do we need the login
> flow to support auth codes? At least for device-level enterprise robots, we're
> planning to handle all this without any user interaction behind the scenes at
> enrollment time.

I've answered this question in a separate email thread.

[Jay Civelli, 2013-03-21 18:23:21]

lgtm

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:91:
delegate_->OnOAuth2TokensAvailable(oauth_tokens_);
Should we clear auth_code_ here?

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/online_attempt_host.h (right):

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/online_attempt_host.h:39: const UserCredentials&
credentials);
NIt: can this fit on the previous line?

[Nikita (slow), 2013-03-21 22:17:32]

lgtm

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/existing_user_controller.cc:423:
credentials.auth_code.empty())
Is auth code really required for a regular user login?

[zel, 2013-03-22 05:03:15]

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/12704002/diff/26001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/existing_user_controller.cc:423:
credentials.auth_code.empty())
On 2013/03/21 22:17:32, Nikita Kostylev wrote:
> Is auth code really required for a regular user login?

nope, you either need to have username+pwd or auth_code

[Nikita (slow), 2013-03-22 09:28:46]

Zel, when do you plan to land this change?

I have a change with a similar plumbing:
https://codereview.chromium.org/12902019/

In that case I would like to merge with your CL and pass username_hash back as a
part of UserCredentials.
In that case maybe it makes sense to rename this structure?

[Nikita (slow), 2013-03-22 15:36:33]

On 2013/03/22 09:28:46, Nikita Kostylev wrote:
> Zel, when do you plan to land this change?
> 
> I have a change with a similar plumbing:
> https://codereview.chromium.org/12902019/
> 
> In that case I would like to merge with your CL and pass username_hash back as
a
> part of UserCredentials.
> In that case maybe it makes sense to rename this structure?

UserCredentials could be renamed to UserContext

[commit-bot: I haz the power, 2013-03-23 12:50:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/12704002/38001

[commit-bot: I haz the power, 2013-03-23 15:19:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/12704002/38001

[commit-bot: I haz the power, 2013-03-23 16:17:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[zel, 2013-03-23 17:08:34]

On 2013/03/22 09:28:46, Nikita Kostylev wrote:
> Zel, when do you plan to land this change?
> 
> I have a change with a similar plumbing:
> https://codereview.chromium.org/12902019/
> 
> In that case I would like to merge with your CL and pass username_hash back as
a
> part of UserCredentials.
> In that case maybe it makes sense to rename this structure?

Yes, I am just about to land it.

[commit-bot: I haz the power, 2013-03-24 17:12:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/12704002/52001

[commit-bot: I haz the power, 2013-03-24 18:11:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-24 18:14:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/12704002/52001

[commit-bot: I haz the power, 2013-03-24 18:35:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-03-24 20:12:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/12704002/68001

[commit-bot: I haz the power, 2013-03-24 20:18:33]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-03-25 02:59:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/12704002/36004

[commit-bot: I haz the power, 2013-03-25 05:11:42]

Change committed as 190338