arc: Implement safe access to ArcAuthService.

arc: Implement safe access to ArcAuthService.

TEST=Extended browser_tests
TEST=Manually on device with Arc enabled/disabled.
BUG=627137
Committed: https://crrev.com/4b937b462197e87d4ac0c44c0275b7a5350963c0
Cr-Commit-Position: refs/heads/master@{#406307}

[khmel, 2016-07-14 23:17:10]

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

[khmel, 2016-07-14 23:17:11]

Hi Xiyuan,

PTAL

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (left):

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:344: profile_ = profile;
Actually this incorrect. It was changed while ago (by myself :)). I am fixing
current logic now. There is only ArcHostSupport who uses profile() from
ArcAuthService. I updated ArcHostSupport for safe work.

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:639: if (!IsAllowed())
There are several calls to IsArcEnabled without proper call to IsArcAllowed. I
think usage is confusing and it probably makes sense to add extra check
IsAllowed here instead of patching every usage. Logically if arc is not allowed
it is not enabled as well.

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
(left):

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:163: if
(arc::ArcBridgeService::Get() &&
This logic was not 100% correct. There are many factors that can disable Arc.
Now IsArcEnabled is safe call.

[xiyuan, 2016-07-15 17:12:53]

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:639: if (!IsAllowed())
On 2016/07/14 23:17:11, khmel wrote:
> There are several calls to IsArcEnabled without proper call to IsArcAllowed. I
> think usage is confusing and it probably makes sense to add extra check
> IsAllowed here instead of patching every usage. Logically if arc is not
allowed
> it is not enabled as well.

Makes sense.

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
(left):

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:163: if
(arc::ArcBridgeService::Get() &&
On 2016/07/14 23:17:11, khmel wrote:
> This logic was not 100% correct. There are many factors that can disable Arc.
> Now IsArcEnabled is safe call.

Acknowledged.

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views_browsertest.cc (right):

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:231:
OpenAppInfoDialog(extension_misc::kChromeAppId);
Not sure how useful this is since we always set up ARC in
AppListControllerAppInfoDialogBrowserTest::SetUpOnMainThread.

Maybe we should add a bool param to AppListControllerAppInfoDialogBrowserTest to
run all cases with or without arc ? This can be done by adding
testing::WithParamInterface<bool> to test base, e.g. [1]

[1]:
https://cs.chromium.org/chromium/src/chrome/browser/ui/profile_error_browsert...

[khmel, 2016-07-18 20:23:42]

Thanks Xiyuan for you OOO review :)!

PTAL

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views_browsertest.cc (right):

https://codereview.chromium.org/2149853004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:231:
OpenAppInfoDialog(extension_misc::kChromeAppId);
On 2016/07/15 17:12:53, xiyuan (ooo 7-15) wrote:
> Not sure how useful this is since we always set up ARC in
> AppListControllerAppInfoDialogBrowserTest::SetUpOnMainThread.
> 
> Maybe we should add a bool param to AppListControllerAppInfoDialogBrowserTest
to
> run all cases with or without arc ? This can be done by adding
> testing::WithParamInterface<bool> to test base, e.g. [1]
> 
> [1]:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/profile_error_browsert...

It seems that Arc initialization in
AppListControllerAppInfoDialogBrowserTest::SetUpOnMainThread has no sense
because:
1. kArcEnabled switch is not set
2. OnPrimaryUserProfilePrepared is called as part of normal profile
initialization.
3. EnableArc is not called.

I implemented as you recommended 1 case - Arc disabled. 2 case - Arc enabled.

https://codereview.chromium.org/2149853004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2149853004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:61: bool
g_allow_cryptohome_data_ephemeral_user_for_testing = false;
In most browser tests user is logged as Ephemeral user, which is disabled by
Arc. As I investigated there is no easy ways to make user non-Ephemeral. There
are Login browser tests that may do this however they have different hierarchy,
incompatible with AppList browser tests.

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/login_mana...

There is yet another sample ArcAuthBrowser Tests. However it is support is very
massive and I am not sure that this makes sense to add to AppList browser tests.

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...


Easiest way to solve this is to allow Ephemeral users for Arc tests.

WDYT?

[xiyuan, 2016-07-18 20:59:12]

https://codereview.chromium.org/2149853004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2149853004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:61: bool
g_allow_cryptohome_data_ephemeral_user_for_testing = false;
On 2016/07/18 20:23:42, khmel wrote:
> In most browser tests user is logged as Ephemeral user, which is disabled by
> Arc. As I investigated there is no easy ways to make user non-Ephemeral. There
> are Login browser tests that may do this however they have different
hierarchy,
> incompatible with AppList browser tests.
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/login_mana...
> 
> There is yet another sample ArcAuthBrowser Tests. However it is support is
very
> massive and I am not sure that this makes sense to add to AppList browser
tests.
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...
> 
> 
> Easiest way to solve this is to allow Ephemeral users for Arc tests.
> 
> WDYT?

Sounds like UserManagerBase::IsUserCryptohomeDataEphemeral is not doing a
correct job here. We take a shortcut there by checking whether user is persisted
in the list to test ephemeral. This is not 100% correct. We probably should also
check AreEphemeralUsersEnabled() in [1].

[1]
https://cs.chromium.org/chromium/src/components/user_manager/user_manager_bas...

Can you try if that could work without adding
g_allow_cryptohome_data_ephemeral_user_for_testing ?

[khmel, 2016-07-18 22:15:11]

PTAL

https://codereview.chromium.org/2149853004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2149853004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:61: bool
g_allow_cryptohome_data_ephemeral_user_for_testing = false;
On 2016/07/18 20:59:12, xiyuan wrote:
> On 2016/07/18 20:23:42, khmel wrote:
> > In most browser tests user is logged as Ephemeral user, which is disabled by
> > Arc. As I investigated there is no easy ways to make user non-Ephemeral.
There
> > are Login browser tests that may do this however they have different
> hierarchy,
> > incompatible with AppList browser tests.
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/login_mana...
> > 
> > There is yet another sample ArcAuthBrowser Tests. However it is support is
> very
> > massive and I am not sure that this makes sense to add to AppList browser
> tests.
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...
> > 
> > 
> > Easiest way to solve this is to allow Ephemeral users for Arc tests.
> > 
> > WDYT?
> 
> Sounds like UserManagerBase::IsUserCryptohomeDataEphemeral is not doing a
> correct job here. We take a shortcut there by checking whether user is
persisted
> in the list to test ephemeral. This is not 100% correct. We probably should
also
> check AreEphemeralUsersEnabled() in [1].
> 
> [1]
>
https://cs.chromium.org/chromium/src/components/user_manager/user_manager_bas...
> 
> Can you try if that could work without adding
> g_allow_cryptohome_data_ephemeral_user_for_testing ?

Yes, this helps

[khmel, 2016-07-18 22:15:24]

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

[commit-bot: I haz the power, 2016-07-18 22:16:56]

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

[xiyuan, 2016-07-18 22:23:40]

lgtm

Awesome!

[khmel, 2016-07-18 22:29:16]

khmel@chromium.org changed reviewers:
+ afakhry@chromium.org

[khmel, 2016-07-18 22:29:16]

Thanks Xiyuan for ideas and review.

Hi Ahmed,

I need approval for 
c/b/extensions/api/feedback_private/feedback_private_api.cc

PTAL

[commit-bot: I haz the power, 2016-07-18 23:00:10]

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

[commit-bot: I haz the power, 2016-07-18 23:00:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[khmel, 2016-07-18 23:55:42]

Updated unit_tests expectations.

PTAL

[xiyuan, 2016-07-19 14:42:05]

still lgtm

[afakhry, 2016-07-19 16:49:21]

feedback_private_api.cc lgtm.

[khmel, 2016-07-19 16:49:44]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2016-07-19 16:50:12]

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-07-19 17:58:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-19 18:00:32]

Description was changed from

==========
arc: Implement safe access to ArcAuthService.

TEST=Extended browser_tests
TEST=Manually on device with Arc enabled/disabled.
BUG=627137
==========

to

==========
arc: Implement safe access to ArcAuthService.

TEST=Extended browser_tests
TEST=Manually on device with Arc enabled/disabled.
BUG=627137

Committed: https://crrev.com/4b937b462197e87d4ac0c44c0275b7a5350963c0
Cr-Commit-Position: refs/heads/master@{#406307}
==========

[commit-bot: I haz the power, 2016-07-19 18:00:33]

Patchset 5 (id:??) landed as
https://crrev.com/4b937b462197e87d4ac0c44c0275b7a5350963c0
Cr-Commit-Position: refs/heads/master@{#406307}