Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(10)

Issue 2801993002: Abandon user sign in when policy is retrieved before session started (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 3 weeks ago by igorcov
Modified:
5 days, 4 hours ago
CC:
chromium-reviews, hashimoto+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Abandon user sign in when policy is retrieved before session started. If it happens that user policy is retrieved before session is started, then there's no policy to apply to the user which could be used as policy enforcement escape. This is a case when some flow went wrong, and we abandon the user sign in. BUG=689206 Review-Url: https://codereview.chromium.org/2801993002 Cr-Commit-Position: refs/heads/master@{#468000} Committed: https://chromium.googlesource.com/chromium/src/+/f305a94dd4908c4975db3444e6b80732f48dcb39

Patch Set 1 #

Patch Set 2 : Implemented to attempt user exit on error #

Patch Set 3 : Added comments to new functions #

Total comments: 17

Patch Set 4 : Fixed review comments #

Patch Set 5 : Nit #

Patch Set 6 : Nit #

Total comments: 26

Patch Set 7 : Fixed review comments #

Patch Set 8 : Nits #

Patch Set 9 : Nits #

Total comments: 40

Patch Set 10 : Fixed review comments #

Patch Set 11 : Nit #

Total comments: 6

Patch Set 12 : Fixed to ignore the generic errors #

Total comments: 4

Patch Set 13 : Nits #

Total comments: 16

Patch Set 14 : Fixed review comments #

Total comments: 6

Patch Set 15 : Fixed review comments #

Total comments: 4

Patch Set 16 : Fixed review comments #

Patch Set 17 : Fixed the tests #

Total comments: 3

Patch Set 18 : Fixed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+407 lines, -170 lines) Patch
M chrome/browser/chromeos/policy/device_local_account_policy_store.h View 1 2 3 4 5 6 3 chunks +5 lines, -6 lines 0 comments Download
M chrome/browser/chromeos/policy/device_local_account_policy_store.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +12 lines, -6 lines 0 comments Download
M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h View 1 2 3 4 5 6 12 13 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +28 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 9 chunks +22 lines, -12 lines 0 comments Download
M chrome/browser/chromeos/settings/device_settings_test_helper.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +5 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/settings/device_settings_test_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +17 lines, -7 lines 0 comments Download
M chrome/browser/chromeos/settings/session_manager_operation.h View 1 2 3 4 5 6 12 13 3 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/settings/session_manager_operation.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +9 lines, -5 lines 0 comments Download
M chromeos/dbus/blocking_method_caller.h View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -0 lines 0 comments Download
M chromeos/dbus/blocking_method_caller.cc View 1 2 3 4 5 6 7 8 9 4 chunks +15 lines, -8 lines 0 comments Download
M chromeos/dbus/blocking_method_caller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +5 lines, -3 lines 0 comments Download
M chromeos/dbus/fake_session_manager_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +8 lines, -5 lines 0 comments Download
M chromeos/dbus/fake_session_manager_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +26 lines, -12 lines 0 comments Download
M chromeos/dbus/mock_session_manager_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +7 lines, -5 lines 0 comments Download
M chromeos/dbus/session_manager_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +37 lines, -14 lines 0 comments Download
M chromeos/dbus/session_manager_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 12 chunks +163 lines, -75 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +33 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 56 (26 generated)
igorcov
emaxx@chromium.org: Please review changes in user_cloud_policy_store_chromeos. derat@chromium.org: Please review changes in session_manager_client. Thank you,
1 month, 2 weeks ago (2017-04-10 17:02:32 UTC) #3
Daniel Erat
+thiemo https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc#newcode46 chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:46: "org.chromium.SessionManagerInterface.SessionDoesNotExist"; this is an implementation detail of the ...
1 month, 2 weeks ago (2017-04-10 18:59:26 UTC) #6
Andrew T Wilson (Slow)
https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc#newcode262 chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:262: chrome::AttemptUserExit(); On 2017/04/10 18:59:26, Daniel Erat wrote: > should ...
1 month, 2 weeks ago (2017-04-11 13:35:04 UTC) #7
igorcov
Changed to have the error type in RetrievePolicyCallback and both retrieve policy for user and ...
1 month, 1 week ago (2017-04-18 10:23:18 UTC) #8
emaxx
https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeos/policy/device_local_account_policy_store.cc File chrome/browser/chromeos/policy/device_local_account_policy_store.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeos/policy/device_local_account_policy_store.cc#newcode63 chrome/browser/chromeos/policy/device_local_account_policy_store.cc:63: chromeos::SessionManagerClient::SUCCESS); It's a bit weird that the asynchronous method ...
1 month, 1 week ago (2017-04-18 15:03:17 UTC) #9
igorcov
Thank you Maksim for your suggestions, I've tried to address your comments. Could you please ...
1 month, 1 week ago (2017-04-20 14:52:29 UTC) #10
Daniel Erat
https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc#newcode118 chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:118: RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST) { if this is unexpected, can you at ...
1 month, 1 week ago (2017-04-20 21:06:39 UTC) #11
emaxx
https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc#newcode126 chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:126: // The session manager doesn't have policy, or the ...
1 month, 1 week ago (2017-04-21 00:01:53 UTC) #12
igorcov
Thank you Dan and Maksim for your comments. I've updated the code, please take a ...
1 month, 1 week ago (2017-04-21 11:36:22 UTC) #13
emaxx
LGTM with nits. Thanks for the patience! https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_manager_client.cc File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_manager_client.cc#newcode154 chromeos/dbus/session_manager_client.cc:154: return; nit: ...
1 month, 1 week ago (2017-04-21 13:43:44 UTC) #15
Daniel Erat
lgtm with comments too. i think you need a histograms reviewer https://codereview.chromium.org/2801993002/diff/220001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): ...
1 month, 1 week ago (2017-04-21 14:22:46 UTC) #16
igorcov
I've changed the code to disallow login only when the specific error SESSION_DOES_NOT_EXIST is received. ...
1 month ago (2017-04-24 15:06:47 UTC) #18
emaxx
Still LGTM (with a couple of nits on the latest change) https://codereview.chromium.org/2801993002/diff/240001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): ...
1 month ago (2017-04-24 16:37:50 UTC) #19
Daniel Erat
https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeos/policy/device_local_account_policy_store.cc File chrome/browser/chromeos/policy/device_local_account_policy_store.cc (right): https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeos/policy/device_local_account_policy_store.cc#newcode59 chrome/browser/chromeos/policy/device_local_account_policy_store.cc:59: chromeos::SessionManagerClient::RetrievePolicyResponseType response = add 'using' directive to this file ...
1 month ago (2017-04-24 19:50:13 UTC) #20
igorcov
Thank you Dan, I've fixed your suggestions. https://codereview.chromium.org/2801993002/diff/240001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/240001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc#newcode125 chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:125: // The ...
1 month ago (2017-04-25 09:18:42 UTC) #22
Daniel Erat
lgtm
1 month ago (2017-04-25 13:40:55 UTC) #23
igorcov
asvitkine@ PTAL at histograms.xml. Thank you.
1 month ago (2017-04-25 14:00:08 UTC) #25
Alexei Svitkine (slow)
https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_manager_client.cc File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_manager_client.cc#newcode130 chromeos/dbus/session_manager_client.cc:130: void LogPolicyResponseUma(const std::string& method_name, Nit: Make this param a ...
1 month ago (2017-04-25 14:41:42 UTC) #26
igorcov
Thank you Alexei, please take a look again. https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_manager_client.cc File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_manager_client.cc#newcode130 chromeos/dbus/session_manager_client.cc:130: void ...
1 month ago (2017-04-26 11:34:32 UTC) #27
Alexei Svitkine (slow)
lgtm % remaining comments https://codereview.chromium.org/2801993002/diff/340001/chromeos/dbus/session_manager_client.cc File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/340001/chromeos/dbus/session_manager_client.cc#newcode130 chromeos/dbus/session_manager_client.cc:130: void LogPolicyResponseUma(const base::StringPiece& method_name, Pass ...
1 month ago (2017-04-26 14:57:26 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801993002/360001
1 month ago (2017-04-27 09:44:59 UTC) #32
igorcov
Thank you, Alexei https://codereview.chromium.org/2801993002/diff/340001/chromeos/dbus/session_manager_client.cc File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/340001/chromeos/dbus/session_manager_client.cc#newcode130 chromeos/dbus/session_manager_client.cc:130: void LogPolicyResponseUma(const base::StringPiece& method_name, On 2017/04/26 ...
1 month ago (2017-04-27 09:51:19 UTC) #33
commit-bot: I haz the power
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_chromeos_ozone_rel_ng/builds/371577)
1 month ago (2017-04-27 10:10:45 UTC) #35
emaxx
LGTM with nits regarding the last patchset https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc (right): https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc#newcode502 chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc:502: .WillOnce(DoAll(SetArgumentPointee<1>(policy_data), gmock's ...
1 month ago (2017-04-28 12:48:23 UTC) #46
igorcov
Thank you, Maksim. https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc (right): https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc#newcode502 chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc:502: .WillOnce(DoAll(SetArgumentPointee<1>(policy_data), On 2017/04/28 12:48:22, emaxx wrote: ...
1 month ago (2017-04-28 13:49:05 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801993002/440001
1 month ago (2017-04-28 13:49:42 UTC) #50
commit-bot: I haz the power
Committed patchset #18 (id:440001) as https://chromium.googlesource.com/chromium/src/+/f305a94dd4908c4975db3444e6b80732f48dcb39
1 month ago (2017-04-28 15:16:40 UTC) #53
twalker_hazelwoodschools.org
*Unfortunately, we have Dell Chromebook 11 (3120) and the update does not apply to us. ...
5 days, 5 hours ago (2017-05-23 18:19:11 UTC) #54
twalker_hazelwoodschools.org
Unfortunately, we have Dell Chromebook 11 (3120) and the update does not apply to us. ...
5 days, 5 hours ago (2017-05-23 18:25:08 UTC) #55
Daniel Erat
5 days, 4 hours ago (2017-05-23 19:44:34 UTC) #56
Message was sent while issue was closed.
On 2017/05/23 18:25:08, twalker_hazelwoodschools.org wrote:
> 
> Unfortunately, we have Dell Chromebook 11 (3120) and the update does not 
> apply to us.  Will there be an update coming out for that model?

Please comment on the bug (http://crbug.com/689206) rather than replying to the
code review.

The Dell Chromebook 11 (3120) is supported until Feb 2020 per
https://support.google.com/chrome/a/answer/6220366?hl=en, so your devices should
receive this change.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06