Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2911583003:
ChromeCryptAuthService: only perform device sync once enrollment is complete. (Closed)
|
Created:
3 years, 7 months ago by Ryan Hansberry Modified:
3 years, 6 months ago CC:
chromium-reviews, Jeremy Klein, lesliewatkins, Kyle Horimoto, James Hawkins, sacomoto Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeCryptAuthService: only perform device sync once enrollment is complete.
BUG=726553
Review-Url: https://codereview.chromium.org/2911583003
Cr-Commit-Position: refs/heads/master@{#476440}
Committed: https://chromium.googlesource.com/chromium/src/+/bdcf8951a275f7f572ea15bdfe921ed222598159
Patch Set 1 #Patch Set 2 : Fix comment. #
Total comments: 11
Patch Set 3 : khorimoto@ comments. #
Total comments: 2
Patch Set 4 : Always remove service as EnrollmentManager observer on enrollment finished. #
Total comments: 2
Patch Set 5 : khorimoto@ comments. #Patch Set 6 : khorimoto@ comments. #
Messages
Total messages: 18 (7 generated)
Description was changed from ========== ChromeCryptAuthService: only perform device sync once enrollment is complete. BUG=726553 ========== to ========== ChromeCryptAuthService: only perform device sync once enrollment is complete. BUG=726553 ==========
hansberry@chromium.org changed reviewers: + tengs@chromium.org
khorimoto@google.com changed reviewers: + khorimoto@google.com
https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:271: void ChromeCryptAuthService::OnEnrollmentFinished(bool success) { If !success, the device manager won't actually work, right? Should we be retrying the enrollment attempt here? https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:278: PA_LOG(ERROR) << "ChromeCAS: OnRefreshTokenAvailable"; Remove these logs. https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:287: // If enrollment is not valid, wait for the new enrollment attempt to finish nit: This comment should probably be moved to the else block. https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:294: enrollment_manager_->Start(); Do we still need to call Start() here if the enrollment is already valid above?
https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:271: void ChromeCryptAuthService::OnEnrollmentFinished(bool success) { On 2017/05/30 21:38:18, khorimoto wrote: > If !success, the device manager won't actually work, right? Should we be > retrying the enrollment attempt here? The enrollment manager should have its own retry logic, so we don't need to retry the enrollment. However, I agree that we should check that success == true.
https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:271: void ChromeCryptAuthService::OnEnrollmentFinished(bool success) { On 2017/05/31 00:04:21, Tim Song wrote: > On 2017/05/30 21:38:18, khorimoto wrote: > > If !success, the device manager won't actually work, right? Should we be > > retrying the enrollment attempt here? > > The enrollment manager should have its own retry logic, so we don't need to > retry the enrollment. > > However, I agree that we should check that success == true. Done. https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:278: PA_LOG(ERROR) << "ChromeCAS: OnRefreshTokenAvailable"; On 2017/05/30 21:38:18, khorimoto wrote: > Remove these logs. Oops, removed. https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:287: // If enrollment is not valid, wait for the new enrollment attempt to finish On 2017/05/30 21:38:18, khorimoto wrote: > nit: This comment should probably be moved to the else block. Done. https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:294: enrollment_manager_->Start(); On 2017/05/30 21:38:18, khorimoto wrote: > Do we still need to call Start() here if the enrollment is already valid above? Yes, in order for EnrollmentManager to schedule its next enrollment.
https://codereview.chromium.org/2911583003/diff/40001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/40001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:273: return; What do we expect to happen in this case? Should we be emitting any events or otherwise notifying other classes about this? At minimum, I think we should log something.
https://codereview.chromium.org/2911583003/diff/40001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/40001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:273: return; On 2017/05/31 17:30:06, khorimoto wrote: > What do we expect to happen in this case? Should we be emitting any events or > otherwise notifying other classes about this? > > At minimum, I think we should log something. Looking at EnrollmentManager, it looks like it gives up after failing (it does not schedule another enrollment attempt). There's really not much we can do about a failure (it's likely a bad idea to simply force another enrollment attempt) so I've added a log for the event, and also made the RemoveObserver method always be called.
lgtm https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:294: enrollment_manager_->Start(); On 2017/05/31 17:08:09, Ryan Hansberry wrote: > On 2017/05/30 21:38:18, khorimoto wrote: > > Do we still need to call Start() here if the enrollment is already valid > above? > > Yes, in order for EnrollmentManager to schedule its next enrollment. nit: Thanks for the explanation - please add a comment explaining why we need to Start() regardless. https://codereview.chromium.org/2911583003/diff/60001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/60001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:275: PA_LOG(WARNING) << "CryptAuth enrollment failed."; WARNING -> ERROR Also, nit: Log that the device manager was not started.
https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/20001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:294: enrollment_manager_->Start(); On 2017/05/31 17:53:38, khorimoto wrote: > On 2017/05/31 17:08:09, Ryan Hansberry wrote: > > On 2017/05/30 21:38:18, khorimoto wrote: > > > Do we still need to call Start() here if the enrollment is already valid > > above? > > > > Yes, in order for EnrollmentManager to schedule its next enrollment. > > nit: Thanks for the explanation - please add a comment explaining why we need to > Start() regardless. Done. https://codereview.chromium.org/2911583003/diff/60001/chrome/browser/cryptaut... File chrome/browser/cryptauth/chrome_cryptauth_service.cc (right): https://codereview.chromium.org/2911583003/diff/60001/chrome/browser/cryptaut... chrome/browser/cryptauth/chrome_cryptauth_service.cc:275: PA_LOG(WARNING) << "CryptAuth enrollment failed."; On 2017/05/31 17:53:38, khorimoto wrote: > WARNING -> ERROR > > Also, nit: Log that the device manager was not started. Done.
lgtm
The CQ bit was checked by hansberry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@google.com Link to the patchset: https://codereview.chromium.org/2911583003/#ps100001 (title: "khorimoto@ comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496347954054720,
"parent_rev": "551719914e284b2a6e3121951c95ba99e66b5e86", "commit_rev":
"bdcf8951a275f7f572ea15bdfe921ed222598159"}
Message was sent while issue was closed.
Description was changed from ========== ChromeCryptAuthService: only perform device sync once enrollment is complete. BUG=726553 ========== to ========== ChromeCryptAuthService: only perform device sync once enrollment is complete. BUG=726553 Review-Url: https://codereview.chromium.org/2911583003 Cr-Commit-Position: refs/heads/master@{#476440} Committed: https://chromium.googlesource.com/chromium/src/+/bdcf8951a275f7f572ea15bdfe92... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/bdcf8951a275f7f572ea15bdfe92... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
