cros: No migration banner if user not allowed for ARC

cros: No migration banner if user not allowed for ARC

Only show migration banner for the following user types:
- Users with Gaia account (regular and child users);
- AD users when ARC is allowed for AD users;

BUG=722602
Review-Url: https://codereview.chromium.org/2879393003
Cr-Commit-Position: refs/heads/master@{#472128}
Committed: https://chromium.googlesource.com/chromium/src/+/468a6511775fca06ef0ae9d483cd2dc5668fdff8

[xiyuan, 2017-05-16 05:07:09]

xiyuan@chromium.org changed reviewers:
+ kinaba@chromium.org

[xiyuan, 2017-05-16 05:07:09]

PTAL. Thanks.

[xiyuan, 2017-05-16 05:07:16]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 05:07:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinaba, 2017-05-16 05:12:38]

kinaba@chromium.org changed reviewers:
+ hidehiko@chromium.org

[kinaba, 2017-05-16 05:12:39]

+hidehiko to double-check

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:162: return
user->HasGaiaAccount() || (user->IsActiveDirectoryUser() &&
Can this check filter out the
profile->IsLegacySupervised()
case in IsArcAllowedInAppListForProfile? (I believe it can because supervised
users don't seem to have Gaia account), but just to confirm.

[commit-bot: I haz the power, 2017-05-16 05:43:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 05:43:51]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-16 06:21:58]

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:162: return
user->HasGaiaAccount() || (user->IsActiveDirectoryUser() &&
On 2017/05/16 05:12:39, kinaba wrote:
> Can this check filter out the
> profile->IsLegacySupervised()
> case in IsArcAllowedInAppListForProfile? (I believe it can because supervised
> users don't seem to have Gaia account), but just to confirm.

Yes, legacy supervised user is managed locally (i.e. they do not have Gaia
accounts). So should be filtered out. I think it is the same as
profile->IsLegacySupervised().

[hidehiko, 2017-05-16 06:31:54]

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:162: return
user->HasGaiaAccount() || (user->IsActiveDirectoryUser() &&
On 2017/05/16 06:21:58, xiyuan wrote:
> On 2017/05/16 05:12:39, kinaba wrote:
> > Can this check filter out the
> > profile->IsLegacySupervised()
> > case in IsArcAllowedInAppListForProfile? (I believe it can because
supervised
> > users don't seem to have Gaia account), but just to confirm.
> 
> Yes, legacy supervised user is managed locally (i.e. they do not have Gaia
> accounts). So should be filtered out. I think it is the same as
> profile->IsLegacySupervised().

What about Ephemeral case?

Also, can the logic across dirs be unified in some place (like
components/arc/arc_util, etc.?) in later CL (not necessary to be done in this
CL, but I think it is better for maintenance for consistency of the conditions)?

[kinaba, 2017-05-16 07:20:11]

lgtm

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:162: return
user->HasGaiaAccount() || (user->IsActiveDirectoryUser() &&
On 2017/05/16 06:31:54, hidehiko wrote:
> On 2017/05/16 06:21:58, xiyuan wrote:
> > On 2017/05/16 05:12:39, kinaba wrote:
> > > Can this check filter out the
> > > profile->IsLegacySupervised()
> > > case in IsArcAllowedInAppListForProfile? (I believe it can because
> supervised
> > > users don't seem to have Gaia account), but just to confirm.
> > 
> > Yes, legacy supervised user is managed locally (i.e. they do not have Gaia
> > accounts). So should be filtered out. I think it is the same as
> > profile->IsLegacySupervised().
> 
> What about Ephemeral case?
> 
> Also, can the logic across dirs be unified in some place (like
> components/arc/arc_util, etc.?) in later CL (not necessary to be done in this
> CL, but I think it is better for maintenance for consistency of the
conditions)?

I believe ephemeral users don't have homedir to migrate so
it should be ok to ignore for this current purpose.
(if there's no existing ecryptfs homedir the banner won't be shown.)


That said I agree we want to have a unified source of this kind of check.
Filing a crbug and landing this CL is fine for me though.

[xiyuan, 2017-05-16 16:02:55]

Thanks for the review.

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2879393003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:162: return
user->HasGaiaAccount() || (user->IsActiveDirectoryUser() &&
On 2017/05/16 07:20:11, kinaba wrote:
> On 2017/05/16 06:31:54, hidehiko wrote:
> > On 2017/05/16 06:21:58, xiyuan wrote:
> > > On 2017/05/16 05:12:39, kinaba wrote:
> > > > Can this check filter out the
> > > > profile->IsLegacySupervised()
> > > > case in IsArcAllowedInAppListForProfile? (I believe it can because
> > supervised
> > > > users don't seem to have Gaia account), but just to confirm.
> > > 
> > > Yes, legacy supervised user is managed locally (i.e. they do not have Gaia
> > > accounts). So should be filtered out. I think it is the same as
> > > profile->IsLegacySupervised().
> > 
> > What about Ephemeral case?
> > 
> > Also, can the logic across dirs be unified in some place (like
> > components/arc/arc_util, etc.?) in later CL (not necessary to be done in
this
> > CL, but I think it is better for maintenance for consistency of the
> conditions)?
> 
> I believe ephemeral users don't have homedir to migrate so
> it should be ok to ignore for this current purpose.
> (if there's no existing ecryptfs homedir the banner won't be shown.)
> 
> 
> That said I agree we want to have a unified source of this kind of check.
> Filing a crbug and landing this CL is fine for me though.

Correct. Ephemeral users would not have their cryptohome persisted and would not
even have user pods on the login screen. The banner check logic would not run
for them.

Filed http://crbug.com/722876 for the follow up work.

[xiyuan, 2017-05-16 16:28:43]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2017-05-16 16:29:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 16:34:41]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1494952123700300, "parent_rev":
"51010b09aee5cfb455f39cee84cbdbaf58169cde", "commit_rev":
"468a6511775fca06ef0ae9d483cd2dc5668fdff8"}

[commit-bot: I haz the power, 2017-05-16 16:34:53]

Description was changed from

==========
cros: No migration banner if user not allowed for ARC

Only show migration banner for the following user types:
- Users with Gaia account (regular and child users);
- AD users when ARC is allowed for AD users;

BUG=722602
==========

to

==========
cros: No migration banner if user not allowed for ARC

Only show migration banner for the following user types:
- Users with Gaia account (regular and child users);
- AD users when ARC is allowed for AD users;

BUG=722602

Review-Url: https://codereview.chromium.org/2879393003
Cr-Commit-Position: refs/heads/master@{#472128}
Committed:
https://chromium.googlesource.com/chromium/src/+/468a6511775fca06ef0ae9d483cd...
==========

[commit-bot: I haz the power, 2017-05-16 16:34:54]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/468a6511775fca06ef0ae9d483cd...