Stop running arc container on supervised user creation flow

Stop running arc container on supervised user creation flow

BUG=657934
TEST=Manually tested (enter supervised user creation flow, and confirm arc is not running)

Committed: https://crrev.com/c172fcc9dd0a684ebddb3e288d063a5174fcee88
Cr-Commit-Position: refs/heads/master@{#436332}

[yoshiki, 2016-11-18 17:13:28]

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

[commit-bot: I haz the power, 2016-11-18 17:14:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2016-11-18 17:14:52]

Description was changed from

==========
.

BUG=
==========

to

==========
Stop running arc container on supervised user creation flow

BUG=657934
TEST=Manually tested (enter supervised user creation flow, and confirm arc is
not running)
==========

[yoshiki, 2016-11-18 17:14:52]

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

[commit-bot: I haz the power, 2016-11-18 19:34:07]

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

[commit-bot: I haz the power, 2016-11-18 19:34:08]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-11-21 03:38:25]

Hidehiko-san, could you take a first look? Thanks.

[hidehiko, 2016-11-22 03:34:48]

+cc: Victor.

I'm ok with this direction for now, but we may want to revisit here later, as a
part of work related to crrev.com/2498033004. AFAIK, Victor will be ooo for a
while, so let's discuss when he's back.

https://codereview.chromium.org/2513473006/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2513473006/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:174:
chromeos::ChromeUserManager::Get()->GetUserFlow(user->GetAccountId());
Is there a case where user_flow is nullptr even in the case where we want to run
ARC?
If not, DCHECK will be better? Or, at least

if (!user_flow || !user_flow->CanStartArc())
  ...

is the condition?

[Luis Héctor Chávez, 2016-11-22 19:12:13]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2016-11-22 19:12:14]

drive-by nit.

https://codereview.chromium.org/2513473006/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2513473006/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:176: VLOG(1) << "ARC is
prohibited in the currenet user flow.";
nit: s/currenet/current/. also, consider changing "prohibited" by "not allowed".

[yoshiki, 2016-11-29 11:29:52]

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

[commit-bot: I haz the power, 2016-11-29 11:30:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2016-11-29 11:30:50]

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

[commit-bot: I haz the power, 2016-11-29 11:31:04]

Dry run: 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, 2016-11-29 12:00:15]

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

[commit-bot: I haz the power, 2016-11-29 12:00:15]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-11-29 18:07:39]

Patchset #3 (id:40001) has been deleted

[yoshiki, 2016-11-29 18:08:44]

Hidehiko, Luis, PTAL

https://codereview.chromium.org/2513473006/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2513473006/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:174:
chromeos::ChromeUserManager::Get()->GetUserFlow(user->GetAccountId());
On 2016/11/22 03:34:48, hidehiko wrote:
> Is there a case where user_flow is nullptr even in the case where we want to
run
> ARC?
> If not, DCHECK will be better? Or, at least
> 
> if (!user_flow || !user_flow->CanStartArc())
>   ...
> 
> is the condition?

Done.

https://codereview.chromium.org/2513473006/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:176: VLOG(1) << "ARC is
prohibited in the currenet user flow.";
On 2016/11/22 19:12:14, Luis Héctor Chávez wrote:
> nit: s/currenet/current/. also, consider changing "prohibited" by "not
allowed".

Done.

[hidehiko, 2016-11-30 00:52:14]

lgtm

[yoshiki, 2016-11-30 17:27:16]

achuith, could take a look at the changes under C/B/chromeos/login/? Thanks.

[yoshiki, 2016-11-30 17:27:40]

yoshiki@chromium.org changed reviewers:
+ achuith@chromium.org

[yoshiki, 2016-11-30 17:27:40]

achuith, could take a look at the changes under C/B/chromeos/login/? Thanks.

[achuithb, 2016-11-30 19:03:12]

https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc
(right):

https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc:50:
return false;
Is this correct?

[yoshiki, 2016-12-02 16:07:56]

https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc
(right):

https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc:50:
return false;
On 2016/11/30 19:03:12, achuithb wrote:
> Is this correct?

We don't allow ARC for supervised user. 

Actually, it is also blocked in ArcSessionManager and we don't need to set
"false" here.
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_...

[achuithb, 2016-12-02 20:38:55]

lgtm

https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc
(right):

https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc:50:
return false;
On 2016/12/02 16:07:56, yoshiki wrote:
> On 2016/11/30 19:03:12, achuithb wrote:
> > Is this correct?
> 
> We don't allow ARC for supervised user. 
> 
> Actually, it is also blocked in ArcSessionManager and we don't need to set
> "false" here.
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_...

Interesting. I know there are some use cases where supervised users are used in
schools, etc (even though this is not what we recommend). Ok.

[yoshiki, 2016-12-05 05:48:11]

On 2016/12/02 20:38:55, achuithb wrote:
> lgtm
> 
>
https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc
> (right):
> 
>
https://codereview.chromium.org/2513473006/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc:50:
> return false;
> On 2016/12/02 16:07:56, yoshiki wrote:
> > On 2016/11/30 19:03:12, achuithb wrote:
> > > Is this correct?
> > 
> > We don't allow ARC for supervised user. 
> > 
> > Actually, it is also blocked in ArcSessionManager and we don't need to set
> > "false" here.
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_...
> 
> Interesting. I know there are some use cases where supervised users are used
in
> schools, etc (even though this is not what we recommend). Ok.

This CL disallows ARC++ not for NON-LEGACY supervised user (aka. child account)
but for LEGACY supervised user. Non-legacy supervised user should keep using
ARC++ even with this CL.

[yoshiki, 2016-12-05 05:48:17]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-12-05 05:48:26]

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, 2016-12-05 06:25:15]

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

[commit-bot: I haz the power, 2016-12-05 06:25:18]

Failed to apply patch for chrome/browser/chromeos/arc/arc_session_manager.cc:
While running git apply --index -p1;
  error: patch failed: chrome/browser/chromeos/arc/arc_session_manager.cc:22
  error: chrome/browser/chromeos/arc/arc_session_manager.cc: patch does not
apply

Patch:       chrome/browser/chromeos/arc/arc_session_manager.cc
Index: chrome/browser/chromeos/arc/arc_session_manager.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager.cc
b/chrome/browser/chromeos/arc/arc_session_manager.cc
index
21effca88e1741d7f087ae3c547c954e774f4945..8778639fd005d8399e716dab11422e2ba69909ab
100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -22,6 +22,8 @@
 #include "chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h"
 #include "chrome/browser/chromeos/arc/policy/arc_android_management_checker.h"
 #include "chrome/browser/chromeos/arc/policy/arc_policy_util.h"
+#include "chrome/browser/chromeos/login/user_flow.h"
+#include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
@@ -168,6 +170,13 @@ bool ArcSessionManager::IsAllowedForProfile(const Profile*
profile) {
     return false;
   }
 
+  chromeos::UserFlow* user_flow =
+      chromeos::ChromeUserManager::Get()->GetUserFlow(user->GetAccountId());
+  if (!user_flow || !user_flow->CanStartArc()) {
+    VLOG(1) << "ARC is not allowed in the current user flow.";
+    return false;
+  }
+
   if (user_manager::UserManager::Get()
           ->IsCurrentUserCryptohomeDataEphemeral()) {
     VLOG(2) << "Users with ephemeral data are not supported in Arc.";

[yoshiki, 2016-12-05 11:09:30]

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

[commit-bot: I haz the power, 2016-12-05 11:09:51]

Dry run: 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, 2016-12-05 11:37:54]

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

[commit-bot: I haz the power, 2016-12-05 11:37:55]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-12-05 17:19:47]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-12-05 17:19:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2513473006/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2016-12-05 17:20:07]

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, 2016-12-05 17:38:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480958387778720,
"parent_rev": "ff756be8694b7294e821b1fa85194f069c43caa6", "commit_rev":
"48fdb41741134fed5349851558cd0297ae3dc0c6"}

[commit-bot: I haz the power, 2016-12-05 17:38:59]

Description was changed from

==========
Stop running arc container on supervised user creation flow

BUG=657934
TEST=Manually tested (enter supervised user creation flow, and confirm arc is
not running)
==========

to

==========
Stop running arc container on supervised user creation flow

BUG=657934
TEST=Manually tested (enter supervised user creation flow, and confirm arc is
not running)
==========

[commit-bot: I haz the power, 2016-12-05 17:39:01]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-05 17:40:26]

Description was changed from

==========
Stop running arc container on supervised user creation flow

BUG=657934
TEST=Manually tested (enter supervised user creation flow, and confirm arc is
not running)
==========

to

==========
Stop running arc container on supervised user creation flow

BUG=657934
TEST=Manually tested (enter supervised user creation flow, and confirm arc is
not running)

Committed: https://crrev.com/c172fcc9dd0a684ebddb3e288d063a5174fcee88
Cr-Commit-Position: refs/heads/master@{#436332}
==========

[commit-bot: I haz the power, 2016-12-05 17:40:27]

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