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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 6 days ago by igorcov
Modified:
14 hours, 25 minutes 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

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: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+317 lines, -123 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/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 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/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 12 chunks +163 lines, -75 lines 1 comment Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +33 lines, -0 lines 1 comment Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 29 (9 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,
2 weeks, 2 days 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 ...
2 weeks, 2 days 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 ...
2 weeks, 1 day 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 week, 1 day 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 week, 1 day 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 ...
6 days, 14 hours 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 ...
6 days, 8 hours 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 ...
6 days, 5 hours 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 ...
5 days, 17 hours 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: ...
5 days, 15 hours 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): ...
5 days, 15 hours 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. ...
2 days, 14 hours 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): ...
2 days, 12 hours 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 ...
2 days, 9 hours 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 day, 20 hours ago (2017-04-25 09:18:42 UTC) #22
Daniel Erat
lgtm
1 day, 15 hours ago (2017-04-25 13:40:55 UTC) #23
igorcov
asvitkine@ PTAL at histograms.xml. Thank you.
1 day, 15 hours 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 day, 14 hours 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 ...
17 hours, 48 minutes ago (2017-04-26 11:34:32 UTC) #27
Alexei Svitkine (slow)
14 hours, 25 minutes ago (2017-04-26 14:57:26 UTC) #29
lgtm % remaining comments

https://codereview.chromium.org/2801993002/diff/340001/chromeos/dbus/session_...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/2801993002/diff/340001/chromeos/dbus/session_...
chromeos/dbus/session_manager_client.cc:130: void LogPolicyResponseUma(const
base::StringPiece& method_name,
Pass StringPiece by value (see comments in its header file).

https://codereview.chromium.org/2801993002/diff/340001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2801993002/diff/340001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:15291: +<histogram
name="Enterprise.RetrievePolicyResponse"
Add base="true" to indicate you never log the unsuffixed version.
Sign in to reply to this message.

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