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

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

Created:
7 months, 2 weeks ago by igorcov
Modified:
5 months, 4 weeks 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

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,
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months 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 ...
7 months 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 ...
7 months 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 ...
7 months 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 ...
7 months 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 ...
7 months 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: ...
7 months 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): ...
7 months 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. ...
6 months, 3 weeks 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): ...
6 months, 3 weeks 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 ...
6 months, 3 weeks 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 ...
6 months, 3 weeks ago (2017-04-25 09:18:42 UTC) #22
Daniel Erat
lgtm
6 months, 3 weeks ago (2017-04-25 13:40:55 UTC) #23
igorcov
asvitkine@ PTAL at histograms.xml. Thank you.
6 months, 3 weeks 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 ...
6 months, 3 weeks 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 ...
6 months, 3 weeks 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 ...
6 months, 3 weeks 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
6 months, 3 weeks 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 ...
6 months, 3 weeks 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)
6 months, 3 weeks 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 ...
6 months, 3 weeks 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: ...
6 months, 3 weeks 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
6 months, 3 weeks 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
6 months, 3 weeks 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 months, 4 weeks 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 months, 4 weeks ago (2017-05-23 18:25:08 UTC) #55
Daniel Erat
5 months, 4 weeks 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.

Powered by Google App Engine
This is Rietveld efc10ee0f