Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2926893002:
arc: Start ARC for Public Session users.
|
Created:
3 years, 6 months ago by Sergey Poromov Modified:
3 years, 2 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Start ARC for Public Session users.
If 'ArcEnabled' policy is set to 'true' for public session account,
then ARC container will be started. 'ArcBackupRestoreEnabled' and
'ArcLocationServiceEnabled' policies should also be defined to start
ARC silently without any user interaction (skipping ToS).
Android authentication flow is the same as for ARC Kiosk accounts, robot
account is reused.
As side effect of this change, ephemeral mode block is removed and ARC
could be started in ephemeral mode for regular user session.
TEST=Set policies, start public session.
BUG=731097
Patch Set 1 #Patch Set 2 : Update unit tests. #
Total comments: 11
Patch Set 3 : Fix missed IsPublicSessionMode() checks. #Patch Set 4 : Fix missed IsPublicSessionMode() checks. #
Total comments: 4
Patch Set 5 : Combine IsArcKioskMode() and IsPublicSessionMode() into IsRobotAccountMode(). #
Total comments: 4
Patch Set 6 : Remove IsPublicSessionMode() #Patch Set 7 : Hide Play Store #Patch Set 8 : Hide Play Store #
Total comments: 1
Messages
Total messages: 42 (24 generated)
poromov@google.com changed reviewers: + hidehiko@chromium.org, poromov@google.com
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you add concrete BUG= line? Also, could you share a (even shorter) design doc, so that I can make sure what should it be for public session? As for ephemeral user; Is allowing ARC for them "intentional" as issues in b/26402681 are resolved? Or is just "side effect" but issues are still remaining? https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:263: if (IsArcKioskMode()) { Need to check here, too? https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:417: !IsArcKioskMode()) { Clarification: Do we want to show error for Public user? https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:661: DCHECK(!IsArcKioskMode()); DCHECK(!IsPublicSession()); too. https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:825: if (IsArcOptInVerificationDisabled() || IsArcKioskMode() || Is it ok to run the check for the Public session? https://codereview.chromium.org/2926893002/diff/20001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2926893002/diff/20001/components/arc/arc_util... components/arc/arc_util.h:72: bool IsPublicSessionMode(); (Optional) Looks not ARC specific? I'm ok to keep it here, but do you have any other better place to put this?
Description was changed from ========== arc: Start ARC for Public Session users. If 'ArcEnabled' policy is set to 'true' for public session account, then ARC container will be started. 'ArcBackupRestoreEnabled' and 'ArcLocationServiceEnabled' policies should also be defined to start ARC silently without any user interaction (skipping ToS). Android authentication flow is the same as for ARC Kiosk accounts, robot account is reused. As side effect of this change, ephemeral mode block is removed and ARC could be started in ephemeral mode for regular user session. TEST=Set policies, start public session. Tested on MNC. BUG= ========== to ========== arc: Start ARC for Public Session users. If 'ArcEnabled' policy is set to 'true' for public session account, then ARC container will be started. 'ArcBackupRestoreEnabled' and 'ArcLocationServiceEnabled' policies should also be defined to start ARC silently without any user interaction (skipping ToS). Android authentication flow is the same as for ARC Kiosk accounts, robot account is reused. As side effect of this change, ephemeral mode block is removed and ARC could be started in ephemeral mode for regular user session. TEST=Set policies, start public session. Tested on MNC. BUG=731097 ==========
On 2017/06/08 12:02:27, hidehiko wrote: > Could you add concrete BUG= line? > Also, could you share a (even shorter) design doc, so that I can make sure what > should it be for public session? > > As for ephemeral user; > Is allowing ARC for them "intentional" as issues in b/26402681 are resolved? Or > is just "side effect" but issues are still remaining? > > https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_session_manager.cc (right): > > https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_session_manager.cc:263: if (IsArcKioskMode()) { > Need to check here, too? > > https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_session_manager.cc:417: !IsArcKioskMode()) { > Clarification: Do we want to show error for Public user? > > https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_session_manager.cc:661: > DCHECK(!IsArcKioskMode()); > DCHECK(!IsPublicSession()); too. > > https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_session_manager.cc:825: if > (IsArcOptInVerificationDisabled() || IsArcKioskMode() || > Is it ok to run the check for the Public session? > > https://codereview.chromium.org/2926893002/diff/20001/components/arc/arc_util.h > File components/arc/arc_util.h (right): > > https://codereview.chromium.org/2926893002/diff/20001/components/arc/arc_util... > components/arc/arc_util.h:72: bool IsPublicSessionMode(); > (Optional) Looks not ARC specific? > I'm ok to keep it here, but do you have any other better place to put this? Bug created and added. As per enabling ephemeral ARC for all users - we have an ongoing thread where discussing this. According to http://b/38255237 it's not that bad to enable it.
https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:263: if (IsArcKioskMode()) { On 2017/06/08 12:02:27, hidehiko wrote: > Need to check here, too? Done. https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:310: AreArcAllOptInPreferencesManagedForProfile(profile_)); While OptIn preferences should be managed for Public Sessions, why shouldn't rely on that and never start Play Store app for Public Sessions as well. https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:417: !IsArcKioskMode()) { On 2017/06/08 12:02:26, hidehiko wrote: > Clarification: Do we want to show error for Public user? I think not. In case of an error there is nothing the user can do. Most likely we should just not start ARC then. What for is the ArcSupportHost used, btw? https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:661: DCHECK(!IsArcKioskMode()); On 2017/06/08 12:02:26, hidehiko wrote: > DCHECK(!IsPublicSession()); too. Done. https://codereview.chromium.org/2926893002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:825: if (IsArcOptInVerificationDisabled() || IsArcKioskMode() || On 2017/06/08 12:02:27, hidehiko wrote: > Is it ok to run the check for the Public session? Public Session is always Chrome OS managed user, so the check is useless in that case. https://codereview.chromium.org/2926893002/diff/20001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2926893002/diff/20001/components/arc/arc_util... components/arc/arc_util.h:72: bool IsPublicSessionMode(); On 2017/06/08 12:02:27, hidehiko wrote: > (Optional) Looks not ARC specific? > I'm ok to keep it here, but do you have any other better place to put this? The check is not ARC specific, but is designed to be used only for ARC checks (with IsArcKioskMode()), so not sure where is the best place for it. Even looks like it's currently not differentiated with IsArcKioskMode() check usage, but more to come.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
poromov@chromium.org changed reviewers: + khmel@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== arc: Start ARC for Public Session users. If 'ArcEnabled' policy is set to 'true' for public session account, then ARC container will be started. 'ArcBackupRestoreEnabled' and 'ArcLocationServiceEnabled' policies should also be defined to start ARC silently without any user interaction (skipping ToS). Android authentication flow is the same as for ARC Kiosk accounts, robot account is reused. As side effect of this change, ephemeral mode block is removed and ARC could be started in ephemeral mode for regular user session. TEST=Set policies, start public session. Tested on MNC. BUG=731097 ========== to ========== arc: Start ARC for Public Session users. If 'ArcEnabled' policy is set to 'true' for public session account, then ARC container will be started. 'ArcBackupRestoreEnabled' and 'ArcLocationServiceEnabled' policies should also be defined to start ARC silently without any user interaction (skipping ToS). Android authentication flow is the same as for ARC Kiosk accounts, robot account is reused. As side effect of this change, ephemeral mode block is removed and ARC could be started in ephemeral mode for regular user session. TEST=Set policies, start public session. BUG=731097 ==========
Hey all, please review this CL. It's recalled again as required ChromeOS ephemeral filesystem CL is also in review and this one is the next step for the feature.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/2926893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2926893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:746: // In Kiosk and PublicSession-mode, Terms of Service negotiation should be nit: Public Session (for consistency) https://codereview.chromium.org/2926893002/diff/60001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2926893002/diff/60001/components/arc/arc_util... components/arc/arc_util.h:84: bool IsPublicSessionMode(); So far all the code is doing are variations of (IsArcKioskMode() || IsPublicSessionMode()) (probably negated). Can you combine both into a single concept to avoid accidentally only allowing one? If there are places where you just want one, can you explicitly call them out?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2926893002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2926893002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:746: // In Kiosk and PublicSession-mode, Terms of Service negotiation should be On 2017/08/28 18:33:28, Luis Héctor Chávez wrote: > nit: Public Session (for consistency) Done. https://codereview.chromium.org/2926893002/diff/60001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2926893002/diff/60001/components/arc/arc_util... components/arc/arc_util.h:84: bool IsPublicSessionMode(); On 2017/08/28 18:33:28, Luis Héctor Chávez wrote: > So far all the code is doing are variations of (IsArcKioskMode() || > IsPublicSessionMode()) (probably negated). Can you combine both into a single > concept to avoid accidentally only allowing one? If there are places where you > just want one, can you explicitly call them out? Done.
https://codereview.chromium.org/2926893002/diff/80001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2926893002/diff/80001/components/arc/arc_util... components/arc/arc_util.h:79: bool IsArcKioskMode(); This _should_ now be unused. Can you remove it? There are also mentions of kiosk mode in https://cs.chromium.org/chromium/src/ash/metrics/user_metrics_recorder.cc?q=I... , should those also be updated? https://codereview.chromium.org/2926893002/diff/80001/components/arc/arc_util... components/arc/arc_util.h:84: bool IsPublicSessionMode(); IIUC this is unused. Can you remove it?
https://codereview.chromium.org/2926893002/diff/80001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2926893002/diff/80001/components/arc/arc_util... components/arc/arc_util.h:79: bool IsArcKioskMode(); On 2017/08/29 16:24:09, Luis Héctor Chávez wrote: > This _should_ now be unused. Can you remove it? > > There are also mentions of kiosk mode in > https://cs.chromium.org/chromium/src/ash/metrics/user_metrics_recorder.cc?q=I... > , should those also be updated? IsArcKioskMode() is used in https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_... that should be done for Public Sessions. What about UserMetricsRecorder - this code is in another component, so probably the check should be left there. https://codereview.chromium.org/2926893002/diff/80001/components/arc/arc_util... components/arc/arc_util.h:84: bool IsPublicSessionMode(); On 2017/08/29 16:24:09, Luis Héctor Chávez wrote: > IIUC this is unused. Can you remove it? Done.
Friendly ping.
Sorry for delay. This CL looks incomplete. From bug desc: Access to the Play Store should be blocked. In this case we should not promote Google Play Store icons, default apps and so on. Also we have to update settings page, not to show not-usable options (Play Store). You may refer to IsPlayStoreAvailable handling for example.
Sorry for delay. This CL looks incomplete. From bug desc: Access to the Play Store should be blocked. In this case we should not promote Google Play Store icons, default apps and so on. Also we have to update settings page, not to show not-usable options (Play Store). You may refer to IsPlayStoreAvailable handling for example.
On 2017/08/30 16:22:21, khmel wrote: > Sorry for delay. > > This CL looks incomplete. > > From bug desc: > Access to the Play Store should be blocked. > > In this case we should not promote Google Play Store icons, default apps and so > on. Also we have to update settings page, not to show not-usable options (Play > Store). > You may refer to IsPlayStoreAvailable handling for example. Hiding Play Store was going to be a separate CL. Or just returning 'false' for IsPlayStoreAvailable() will be enough? Not sure what behavior will be in that case for Public Sessions...
On 2017/08/30 16:48:44, Sergey Poromov wrote: > On 2017/08/30 16:22:21, khmel wrote: > > Sorry for delay. > > > > This CL looks incomplete. > > > > From bug desc: > > Access to the Play Store should be blocked. > > > > In this case we should not promote Google Play Store icons, default apps and > so > > on. Also we have to update settings page, not to show not-usable options (Play > > Store). > > You may refer to IsPlayStoreAvailable handling for example. > > Hiding Play Store was going to be a separate CL. > Or just returning 'false' for IsPlayStoreAvailable() will be enough? Not sure > what behavior will be in that case for Public Sessions... I would prefer to handle this case in current CL to prevent appearing invalid behavior. Currently IsPlayStoreAvailable does not accept profile and analyzes only Chrome flags. You may want to extend this by providing profile there and additionally analyzing your case.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/08/30 16:56:36, khmel wrote: > On 2017/08/30 16:48:44, Sergey Poromov wrote: > > On 2017/08/30 16:22:21, khmel wrote: > > > Sorry for delay. > > > > > > This CL looks incomplete. > > > > > > From bug desc: > > > Access to the Play Store should be blocked. > > > > > > In this case we should not promote Google Play Store icons, default apps and > > so > > > on. Also we have to update settings page, not to show not-usable options > (Play > > > Store). > > > You may refer to IsPlayStoreAvailable handling for example. > > > > Hiding Play Store was going to be a separate CL. > > Or just returning 'false' for IsPlayStoreAvailable() will be enough? Not sure > > what behavior will be in that case for Public Sessions... > > I would prefer to handle this case in current CL to prevent appearing invalid > behavior. > Currently IsPlayStoreAvailable does not accept profile and analyzes only Chrome > flags. > You may want to extend this by providing profile there and additionally > analyzing your case. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2926893002/diff/140001/components/arc/arc_uti... File components/arc/arc_util.cc (right): https://codereview.chromium.org/2926893002/diff/140001/components/arc/arc_uti... components/arc/arc_util.cc:88: !IsRobotAccountMode()) { nit: I would prefer if (IsRobotAccountMode()) return false; ...
hidehiko@google.com changed reviewers: + hidehiko@google.com
LGTM with Yury's comment. Clarification: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_util.cc?... There is no "ARC is allowed only for public-session devices", unlike ARC Kiosk. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
