Store local auth credentials after successful login or re-auth.

Store local auth credentials after successful login or re-auth.

BUG=405550
Committed: https://crrev.com/d6d4cbd2f7d9cd1e5748ef25daa31f4366301eb2
Cr-Commit-Position: refs/heads/master@{#292122}

[Marc Treib, 2014-08-26 12:23:51]

treib@chromium.org changed reviewers:
+ rogerta@chromium.org

[Marc Treib, 2014-08-26 12:23:51]

PTAL!

https://codereview.chromium.org/508433004/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/508433004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:129: if
(is_primary_account)
This assumes we want to store the credentials on every successful sign-in or
re-auth. (Right now we only store them after unlock!)
If we really only want to store on re-auth (as written on the bug), then the
test should be "AreEmailsSame(..) && source == SOURCE_REAUTH".

[Roger Tawa OOO till Jul 10th, 2014-08-26 15:29:57]

Thanks Marc for taking my bug and fixing it!  Looks good, see comments below.

https://codereview.chromium.org/508433004/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/508433004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:129: if
(is_primary_account)
On 2014/08/26 12:23:51, Marc Treib wrote:
> This assumes we want to store the credentials on every successful sign-in or
> re-auth. (Right now we only store them after unlock!)
> If we really only want to store on re-auth (as written on the bug), then the
> test should be "AreEmailsSame(..) && source == SOURCE_REAUTH".

We already store a hash on first time sign-in, see:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...

Should change the if at line chrome_signin_client.cc:211 to be
IsNewProfileManagement() though.  Can you do that please as part of this CL?

For the code here, yes I think you should add "&& source == SOURCE_REAUTH".  I
think we should add a check for IsNewProfileManagement() as well.

For the primary check, it might be easier to change this to:

   bool is_primary_account = email ==
       SigninManagerFactory::GetForProfile(profile_)->
           GetAuthenticatedAccountId();

but I'm fine if you leave it as is.  I'll be changing it soon anyway as part of
the migration away from email and to gaia ids as account ids.

[Marc Treib, 2014-08-26 16:54:07]

On 2014/08/26 15:29:57, Roger Tawa wrote:
> Thanks Marc for taking my bug and fixing it!  Looks good, see comments below.
> 
>
https://codereview.chromium.org/508433004/diff/1/chrome/browser/ui/webui/sign...
> File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/508433004/diff/1/chrome/browser/ui/webui/sign...
> chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:129: if
> (is_primary_account)
> On 2014/08/26 12:23:51, Marc Treib wrote:
> > This assumes we want to store the credentials on every successful sign-in or
> > re-auth. (Right now we only store them after unlock!)
> > If we really only want to store on re-auth (as written on the bug), then the
> > test should be "AreEmailsSame(..) && source == SOURCE_REAUTH".
> 
> We already store a hash on first time sign-in, see:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...
> 
> Should change the if at line chrome_signin_client.cc:211 to be
> IsNewProfileManagement() though.  Can you do that please as part of this CL?

Turns out CodeSearch is (once again) out of date: That check has already been
changed to IsNewProfileManagement in https://codereview.chromium.org/475533006.
However, I've copied the platform ifdef from there.
(CodeSearch being out of date is also the reason I didn't find this.. have to
learn not to trust CS :-/ )

> For the code here, yes I think you should add "&& source == SOURCE_REAUTH".  I
> think we should add a check for IsNewProfileManagement() as well.

Done. No point in storing the credentials twice during sign-in. (Though it's a
bit weird that re-auth does it in a completely different place than sign-in, but
oh well.)

> For the primary check, it might be easier to change this to:
> 
>    bool is_primary_account = email ==
>        SigninManagerFactory::GetForProfile(profile_)->
>            GetAuthenticatedAccountId();
> 
> but I'm fine if you leave it as is.  I'll be changing it soon anyway as part
of
> the migration away from email and to gaia ids as account ids.

As long as we have to use email, we do still need the normalization from
gaia::AreEmailsSame, don't we?!

[Roger Tawa OOO till Jul 10th, 2014-08-26 17:01:12]

lgtm

Ah codesearch... thanks for updating the comment!

Yup, as long as we still use email here, we still need the AreEmailsSame() call.

https://codereview.chromium.org/508433004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/508433004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:132: #endif
Don't really need the #ifdef here because this file only applies to desktop
platforms.

[Marc Treib, 2014-08-27 07:34:52]

https://codereview.chromium.org/508433004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/508433004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:132: #endif
On 2014/08/26 17:01:12, Roger Tawa wrote:
> Don't really need the #ifdef here because this file only applies to desktop
> platforms.

Ah thanks, I didn't know that. I removed it again.

[Marc Treib, 2014-08-27 07:34:55]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-08-27 07:35:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/508433004/40001

[commit-bot: I haz the power, 2014-08-27 08:28:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-27 09:04:12]

Committed patchset #3 (40001) as 3469edbffec2ef71c2ae8774083e362717f62e7e

[commit-bot: I haz the power, 2014-09-10 02:50:36]

Patchset 3 (id:??) landed as
https://crrev.com/d6d4cbd2f7d9cd1e5748ef25daa31f4366301eb2
Cr-Commit-Position: refs/heads/master@{#292122}