Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446563002:
Refactor ArcAndroidManagementChecker. (Closed)
|
Created:
4 years, 1 month ago by hidehiko Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel, Polina Bondarenko Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ArcAndroidManagementChecker.
This is the preparation to remove management check from re-auth
flow. Also, this is preparation to introduce ArcSessionManager.
- Move more management check logic from CheckAndroidManagement(),
to ArcAndroidManagementChecker, so that CheckAndroidManagement()
is removed. Management check is not a part of authorization.
- Get rid of Delegate, instead use Callback to easily switch the
following operations.
- Remove "background" concept from ArcAndroidManagementChecker.
- IsAccountManaged() is moved to arc_policy_util, so that it can be
shared with current ArcAuthService and ArcAndroidManagementChecker.
- Use base::ResetAndReturn in AndroidManagementClient, so that
callback can destruct the instance.
BUG=657687
BUG=b/31079732
TEST=Ran on test device. Tried to inject RESULT_MANAGED and it works.
Ran trybots.
Committed: https://crrev.com/b1fb427de5a8fc9d3c1a5c5abaddd023a0e2ca52
Cr-Commit-Position: refs/heads/master@{#427672}
Patch Set 1 #
Total comments: 32
Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : Address comments #Patch Set 5 : Address comments. #
Total comments: 5
Patch Set 6 : rebase #
Messages
Total messages: 28 (13 generated)
The CQ bit was checked by hidehiko@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...
hidehiko@chromium.org changed reviewers: + bartfab@chromium.org, lhchavez@chromium.org
PTAL: - lhchavez@: all - bartfab@: c/b/c/policy FYI: kheml@, pbond@. Luis: I know this conflicts with your file moving CL, but I think the conflict is small enough, and rebasing (in either direction) will not be so difficult. So, let me send this now. Thank you for review in advance.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
first round https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_android_management_checker.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.cc:100: void ArcAndroidManagementChecker::StartCheckInternal() { DCHECK(!callback_.is_null()); ? https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.cc:115: policy::AndroidManagementClient::Result result) { DCHECK(!callback_.is_null()); ? https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_android_management_checker.h (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.h:32: // |result|. This cannot be called if there is inflight check. nit: s/cannot/must not/ https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:639: StartArc(); IIUC you are not calling FetchAuthCode(); in this flow, whereas it was called before (unless IsOptInVerificationDisabled()). https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:649: android_management_checker_->StartCheck( Seems like you need some state to track whether there is one pending StartCheck() call in between disabling/reenabling of ARC (or document clearly how that scenario is impossible). https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:918: // Do nothing. The ARC should be started already. nit: s/The ARC/ARC/. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:921: DisableArc(); Is there a way to intentionally delay the receipt of this message (e.g. by selectively blocking certain network connections) such that after your change it is possible to interact with ARC in a managed environment whereas it was not possible before? I think there might be. If there aren't, please clearly document the reason and/or enforce it explicitly in some way. Otherwise, we might have to return to the previous structure. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:925: // retry_on_error sould be set. nit: s/sould/should/ https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/android_management_client.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/android_management_client.cc:57: DCHECK_EQ(token_request_.get(), request); DCHECK(!callback_.is_null()); https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/android_management_client.cc:92: const em::DeviceManagementResponse& response) { DCHECK(!callback_.is_null());
https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_android_management_checker.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.cc:23: constexpr int kRetryTimeMinMs = 10 * 1000; // 10 sec. nit: constexpr base::TimeDelta kRetryTimeMin = base::TimeDelta::FromSeconds(10); constexpr base::TimeDelta kRetryTimeMax = base::TimeDelta::FromHours(1); https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_android_management_checker.h (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.h:61: int retry_time_ms_; This looks like it can be a base::TimeDelta. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:74: const char kStateNotInitialized[] = "NOT_INITIALIZED"; can you make these constexpr while you're at it? https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:79: bool IsArcDisabledForEnterprise() { Can we also move this to arc_policy_util.h?
The CQ bit was checked by hidehiko@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...
Thank you for review. PTAL. Note: Luis, for minor clean up, please let me work on it in separate CLs. I've already sent them to review. Thank you for review in advance for those CLs, too! https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_android_management_checker.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.cc:23: constexpr int kRetryTimeMinMs = 10 * 1000; // 10 sec. On 2016/10/24 22:39:56, Luis Héctor Chávez wrote: > nit: > > constexpr base::TimeDelta kRetryTimeMin = base::TimeDelta::FromSeconds(10); > constexpr base::TimeDelta kRetryTimeMax = base::TimeDelta::FromHours(1); Addressed in https://codereview.chromium.org/2450683002/ https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.cc:100: void ArcAndroidManagementChecker::StartCheckInternal() { On 2016/10/24 22:36:02, Luis Héctor Chávez wrote: > DCHECK(!callback_.is_null()); ? Done. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.cc:115: policy::AndroidManagementClient::Result result) { On 2016/10/24 22:36:02, Luis Héctor Chávez wrote: > DCHECK(!callback_.is_null()); ? Done. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_android_management_checker.h (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.h:32: // |result|. This cannot be called if there is inflight check. On 2016/10/24 22:36:02, Luis Héctor Chávez wrote: > nit: s/cannot/must not/ Done. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_android_management_checker.h:61: int retry_time_ms_; On 2016/10/24 22:39:56, Luis Héctor Chávez wrote: > This looks like it can be a base::TimeDelta. Addressed in https://codereview.chromium.org/2450683002/ https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:74: const char kStateNotInitialized[] = "NOT_INITIALIZED"; On 2016/10/24 22:39:57, Luis Héctor Chávez wrote: > can you make these constexpr while you're at it? Addressed in https://codereview.chromium.org/2445233003/ https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:79: bool IsArcDisabledForEnterprise() { On 2016/10/24 22:39:57, Luis Héctor Chávez wrote: > Can we also move this to arc_policy_util.h? Addressed in https://codereview.chromium.org/2448013002/ https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:639: StartArc(); On 2016/10/24 22:36:02, Luis Héctor Chávez wrote: > IIUC you are not calling FetchAuthCode(); in this flow, whereas it was called > before (unless IsOptInVerificationDisabled()). FetchAuthCode was not called even in the original code. Here, kArcSignedIn pref is *true*, so if-stmt in OnAndroidManagementPassed() runs its then-body always in this case, which is StartArc(). https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:649: android_management_checker_->StartCheck( On 2016/10/24 22:36:02, Luis Héctor Chávez wrote: > Seems like you need some state to track whether there is one pending > StartCheck() call in between disabling/reenabling of ARC (or document clearly > how that scenario is impossible). Inflight check is canceled by reset above and/or (probably) ShutdownBridge(). Added the explicit comment about the cancel in arc_android_management_checker.h. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:918: // Do nothing. The ARC should be started already. On 2016/10/24 22:36:02, Luis Héctor Chávez wrote: > nit: s/The ARC/ARC/. Done. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:921: DisableArc(); On 2016/10/24 22:36:03, Luis Héctor Chávez wrote: > Is there a way to intentionally delay the receipt of this message (e.g. by > selectively blocking certain network connections) such that after your change it > is possible to interact with ARC in a managed environment whereas it was not > possible before? I think there might be. If there aren't, please clearly > document the reason and/or enforce it explicitly in some way. Otherwise, we > might have to return to the previous structure. Maybe, I'm not understanding the goal to delay the message. Could you tell me more details about your concern here? As for network error, it is handled in ArcAndroidManagementChecker as is (= retry on network error). Is it what you're looking for? https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:925: // retry_on_error sould be set. On 2016/10/24 22:36:03, Luis Héctor Chávez wrote: > nit: s/sould/should/ Done. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/android_management_client.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/android_management_client.cc:57: DCHECK_EQ(token_request_.get(), request); On 2016/10/24 22:36:03, Luis Héctor Chávez wrote: > DCHECK(!callback_.is_null()); Done. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/android_management_client.cc:92: const em::DeviceManagementResponse& response) { On 2016/10/24 22:36:03, Luis Héctor Chávez wrote: > DCHECK(!callback_.is_null()); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
khmel@chromium.org changed reviewers: + khmel@chromium.org
https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc (right): https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc:60: DCHECK(callback_.is_null()); Now your logic is incorrect, before all checks were started in CTOR and this guarantied that you cannot reactivate the check. With this StartCheck this is no longer true. Based on this structure you may call StartCheck many times. Potentially you may have android_management_client_.StartCheckAndroidManagement already started before next call of StartCheck. Assume you are existing earlier on line 67. In this case you have to stop all previous check. Otherwise check may be triggered in wrong time for another check. I understand that in reality this should never happen however based on code it is not clear.
https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc (right): https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc:60: DCHECK(callback_.is_null()); On 2016/10/25 14:55:20, khmel wrote: > Now your logic is incorrect, before all checks were started in CTOR and this > guarantied that you cannot reactivate the check. With this StartCheck this is no > longer true. Based on this structure you may call StartCheck many times. > Potentially you may have android_management_client_.StartCheckAndroidManagement > already started before next call of StartCheck. > This is common pattern used in Chrome, AFAIK. Actually, starting the check looks not a part of initialization of the checker. Considering that even fail-able initialization is discouraged to do in the ctor, I think we should not start it in the ctor. cf) https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors "Ensuring not calling StartCheck while there is inflight operation" is the caller's responsibility. Theoretically we can have more complicated mechanism to support such cases, but I cannot find any benefit on it for this specific case, because it is not used. > Assume you are existing earlier on line 67. > In this case you have to stop all previous check. Otherwise check may be > triggered in wrong time for another check. > Sorry, I couldn't get your point. Are you worried about the case that a client wants to start another StartCheck while there is inflight operation? Then, it is supported. Remove the instance, create another instance and call StartCheck(). Removing the instance cancels inflight operations. > I understand that in reality this should never happen however based on code it > is not clear. > Both the responsibility of the caller (guarantee at most one inflight operation), and the cancel behavior are explicitly documented in StartCheck().
https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc (right): https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc:60: DCHECK(callback_.is_null()); On 2016/10/25 16:15:20, hidehiko wrote: > On 2016/10/25 14:55:20, khmel wrote: > > Now your logic is incorrect, before all checks were started in CTOR and this > > guarantied that you cannot reactivate the check. With this StartCheck this is > no > > longer true. Based on this structure you may call StartCheck many times. > > Potentially you may have > android_management_client_.StartCheckAndroidManagement > > already started before next call of StartCheck. > > > > This is common pattern used in Chrome, AFAIK. > > Actually, starting the check looks not a part of initialization of the checker. > Considering that even fail-able initialization is discouraged to do in the ctor, > I think we should not start it in the ctor. > > cf) https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors > > "Ensuring not calling StartCheck while there is inflight operation" is the > caller's responsibility. Theoretically we can have more complicated mechanism to > support such cases, but I cannot find any benefit on it for this specific case, > because it is not used. > > > Assume you are existing earlier on line 67. > > In this case you have to stop all previous check. Otherwise check may be > > triggered in wrong time for another check. > > > > Sorry, I couldn't get your point. Are you worried about the case that a client > wants to start another StartCheck while there is inflight operation? > Then, it is supported. Remove the instance, create another instance and call > StartCheck(). > Removing the instance cancels inflight operations. > > > I understand that in reality this should never happen however based on code it > > is not clear. > > > > Both the responsibility of the caller (guarantee at most one inflight > operation), and the cancel behavior are explicitly documented in StartCheck(). Acknowledged.
policy lgtm
https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:639: StartArc(); On 2016/10/25 07:22:42, hidehiko wrote: > On 2016/10/24 22:36:02, Luis Héctor Chávez wrote: > > IIUC you are not calling FetchAuthCode(); in this flow, whereas it was called > > before (unless IsOptInVerificationDisabled()). > > FetchAuthCode was not called even in the original code. > > Here, kArcSignedIn pref is *true*, so if-stmt in OnAndroidManagementPassed() > runs its then-body always in this case, which is StartArc(). Oh, right (good thing we're cleaning up this code). https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:921: DisableArc(); On 2016/10/25 07:22:42, hidehiko wrote: > On 2016/10/24 22:36:03, Luis Héctor Chávez wrote: > > Is there a way to intentionally delay the receipt of this message (e.g. by > > selectively blocking certain network connections) such that after your change > it > > is possible to interact with ARC in a managed environment whereas it was not > > possible before? I think there might be. If there aren't, please clearly > > document the reason and/or enforce it explicitly in some way. Otherwise, we > > might have to return to the previous structure. > > Maybe, I'm not understanding the goal to delay the message. > Could you tell me more details about your concern here? > > As for network error, it is handled in ArcAndroidManagementChecker as is (= > retry on network error). Is it what you're looking for? The concern is about a malicious actor that wants to work around management restrictions. It seems that constantly "causing" network errors will result in a semi-permanent state where ARC is running and might* be interacted with. * Users _shouldn't_ be able to interact with ARC at that point, but there might be other bugs we are not aware of and should be a bit more defensive here.
lgtm (unless khmel has any more comments). https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:921: DisableArc(); On 2016/10/25 17:11:39, Luis Héctor Chávez wrote: > On 2016/10/25 07:22:42, hidehiko wrote: > > On 2016/10/24 22:36:03, Luis Héctor Chávez wrote: > > > Is there a way to intentionally delay the receipt of this message (e.g. by > > > selectively blocking certain network connections) such that after your > change > > it > > > is possible to interact with ARC in a managed environment whereas it was not > > > possible before? I think there might be. If there aren't, please clearly > > > document the reason and/or enforce it explicitly in some way. Otherwise, we > > > might have to return to the previous structure. > > > > Maybe, I'm not understanding the goal to delay the message. > > Could you tell me more details about your concern here? > > > > As for network error, it is handled in ArcAndroidManagementChecker as is (= > > retry on network error). Is it what you're looking for? > > The concern is about a malicious actor that wants to work around management > restrictions. It seems that constantly "causing" network errors will result in a > semi-permanent state where ARC is running and might* be interacted with. > > * Users _shouldn't_ be able to interact with ARC at that point, but there might > be other bugs we are not aware of and should be a bit more defensive here. As discussed offline, since this is *not* a regression and to keep making progress on this much-needed cleanup, this will be addressed separately. https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:913: NOTREACHED(); I wish policy::AndroidManagementClient::Result were an enum class to avoid this :(
lgtm
Thank you for review. Rebased, and submitting. https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:921: DisableArc(); On 2016/10/25 17:47:58, Luis Héctor Chávez wrote: > On 2016/10/25 17:11:39, Luis Héctor Chávez wrote: > > On 2016/10/25 07:22:42, hidehiko wrote: > > > On 2016/10/24 22:36:03, Luis Héctor Chávez wrote: > > > > Is there a way to intentionally delay the receipt of this message (e.g. by > > > > selectively blocking certain network connections) such that after your > > change > > > it > > > > is possible to interact with ARC in a managed environment whereas it was > not > > > > possible before? I think there might be. If there aren't, please clearly > > > > document the reason and/or enforce it explicitly in some way. Otherwise, > we > > > > might have to return to the previous structure. > > > > > > Maybe, I'm not understanding the goal to delay the message. > > > Could you tell me more details about your concern here? > > > > > > As for network error, it is handled in ArcAndroidManagementChecker as is (= > > > retry on network error). Is it what you're looking for? > > > > The concern is about a malicious actor that wants to work around management > > restrictions. It seems that constantly "causing" network errors will result in > a > > semi-permanent state where ARC is running and might* be interacted with. > > > > * Users _shouldn't_ be able to interact with ARC at that point, but there > might > > be other bugs we are not aware of and should be a bit more defensive here. > > As discussed offline, since this is *not* a regression and to keep making > progress on this much-needed cleanup, this will be addressed separately. Acknowledged. Let's discuss separately. https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2446563002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:913: NOTREACHED(); On 2016/10/25 17:47:58, Luis Héctor Chávez wrote: > I wish policy::AndroidManagementClient::Result were an enum class to avoid this > :( I can give it a try to do it separately. Let me work on it separately, to avoid conflict during the review.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, lhchavez@chromium.org, khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2446563002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Refactor ArcAndroidManagementChecker.
This is the preparation to remove management check from re-auth
flow. Also, this is preparation to introduce ArcSessionManager.
- Move more management check logic from CheckAndroidManagement(),
to ArcAndroidManagementChecker, so that CheckAndroidManagement()
is removed. Management check is not a part of authorization.
- Get rid of Delegate, instead use Callback to easily switch the
following operations.
- Remove "background" concept from ArcAndroidManagementChecker.
- IsAccountManaged() is moved to arc_policy_util, so that it can be
shared with current ArcAuthService and ArcAndroidManagementChecker.
- Use base::ResetAndReturn in AndroidManagementClient, so that
callback can destruct the instance.
BUG=657687
BUG=b/31079732
TEST=Ran on test device. Tried to inject RESULT_MANAGED and it works.
Ran trybots.
==========
to
==========
Refactor ArcAndroidManagementChecker.
This is the preparation to remove management check from re-auth
flow. Also, this is preparation to introduce ArcSessionManager.
- Move more management check logic from CheckAndroidManagement(),
to ArcAndroidManagementChecker, so that CheckAndroidManagement()
is removed. Management check is not a part of authorization.
- Get rid of Delegate, instead use Callback to easily switch the
following operations.
- Remove "background" concept from ArcAndroidManagementChecker.
- IsAccountManaged() is moved to arc_policy_util, so that it can be
shared with current ArcAuthService and ArcAndroidManagementChecker.
- Use base::ResetAndReturn in AndroidManagementClient, so that
callback can destruct the instance.
BUG=657687
BUG=b/31079732
TEST=Ran on test device. Tried to inject RESULT_MANAGED and it works.
Ran trybots.
Committed: https://crrev.com/b1fb427de5a8fc9d3c1a5c5abaddd023a0e2ca52
Cr-Commit-Position: refs/heads/master@{#427672}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b1fb427de5a8fc9d3c1a5c5abaddd023a0e2ca52 Cr-Commit-Position: refs/heads/master@{#427672} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
