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

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, 4 days ago by igorcov
Modified:
13 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: 2

Patch Set 13 : Nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+321 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 3 chunks +10 lines, -6 lines 0 comments Download
M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 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 3 chunks +25 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/settings/session_manager_operation.h View 1 2 3 4 5 6 7 8 9 10 11 12 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 4 chunks +36 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 chunks +164 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 2 chunks +40 lines, -0 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 19 (6 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 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 week, 6 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 ...
1 week, 6 days 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 ...
6 days, 6 hours 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 ...
6 days, 2 hours 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 ...
4 days, 2 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 ...
3 days, 19 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 ...
3 days, 17 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 ...
3 days, 5 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: ...
3 days, 3 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): ...
3 days, 2 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. ...
1 hour, 59 minutes ago (2017-04-24 15:06:47 UTC) #18
emaxx
28 minutes ago (2017-04-24 16:37:50 UTC) #19
Still LGTM (with a couple of nits on the latest change)

https://codereview.chromium.org/2801993002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/2801993002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:125: // The
session manager doesn't have policy.
nit: Please restore the original comment, given that the condition is now
changed back.

https://codereview.chromium.org/2801993002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/session_manager_operation.h (right):

https://codereview.chromium.org/2801993002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/session_manager_operation.h:18: using
RetrievePolicyResponseType =
nit: Global type aliases that are there only for typing saving shouldn't be
placed in headers:
https://google.github.io/styleguide/cppguide.html#Aliases
Sign in to reply to this message.

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