Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801993002:
Abandon user sign in when policy is retrieved before session started (Closed)
|
Created:
3 years, 8 months ago by igorcov Modified:
3 years, 7 months ago Reviewers:
Daniel Erat, Andrew T Wilson (Slow), Thiemo Nagel, Alexei Svitkine (slow), emaxx, twalker CC:
chromium-reviews, hashimoto+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAbandon 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 #Messages
Total messages: 56 (26 generated)
Description was changed from ========== Force to crash 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. In this case we prefer to crash. BUG=689206 ========== to ========== 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 ==========
igorcov@chromium.org changed reviewers: + derat@chromium.org, emaxx@chromium.org
emaxx@chromium.org: Please review changes in user_cloud_policy_store_chromeos. derat@chromium.org: Please review changes in session_manager_client. Thank you,
igorcov@chromium.org changed reviewers: + atwilson@chromium.org
derat@chromium.org changed reviewers: + tnagel@chromium.org
+thiemo https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:46: "org.chromium.SessionManagerInterface.SessionDoesNotExist"; this is an implementation detail of the d-bus interface between chrome and session_manager and probably doesn't belong here. it'd likely be better to check for this in SessionManagerClient, and the string constant should be defined in system_api if both chrome and session_manager are using it. https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:255: LOG(ERROR) << "Error on policy retrieved " << error_name << ":" add space after ':' https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:258: // from Chrome OS. this is chrome-os-only code, so you should probably write "from session_manager" here instead https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:262: chrome::AttemptUserExit(); should chrome crash instead, or will that put us into a restart loop? signing out silently is going to be confusing for users, i think. https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right): https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:76: void OnPolicyRetrievedWithError(const std::string& error_name, please add a comment documenting what calls this and what it does. consider renaming these to e.g. OnPolicyRetrievedSuccess and OnPolicyRetrievedError https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:525: writer.AppendString(account_id); please avoid copy-and-pasting code (i.e. the existing method is almost exactly the same) https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.h:152: using ErrorCallback = base::Callback<void(const std::string& error_name, mentioned earlier, but this should pass an enum rather than a string https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.h:177: virtual void RetrievePolicyForUserWithErrorCallback( please use a single method and update callers instead of adding a new method to the interface that's only called in one place
https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:262: chrome::AttemptUserExit(); On 2017/04/10 18:59:26, Daniel Erat wrote: > should chrome crash instead, or will that put us into a restart loop? > > signing out silently is going to be confusing for users, i think. Signing out silently can indeed be confusing for users, but this is what we've done in other places where assumptions around loading policy have been violated. The idea is that the situation is rare enough that just signing out the user is sufficient (we don't need to build and maintain more elaborate UI). I would suggest maybe adding a UMA stat though for how often this routine is called and how often the value is kSessionDoesNotExist. https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.h:152: using ErrorCallback = base::Callback<void(const std::string& error_name, On 2017/04/10 18:59:26, Daniel Erat wrote: > mentioned earlier, but this should pass an enum rather than a string +1 as this lets us log UMA stats for it.
Changed to have the error type in RetrievePolicyCallback and both retrieve policy for user and retrieve device policy get the D-Bus errors in response. Those are logged in UMA stats. https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:46: "org.chromium.SessionManagerInterface.SessionDoesNotExist"; On 2017/04/10 18:59:26, Daniel Erat wrote: > this is an implementation detail of the d-bus interface between chrome and > session_manager and probably doesn't belong here. it'd likely be better to check > for this in SessionManagerClient, and the string constant should be defined in > system_api if both chrome and session_manager are using it. Done - https://chromium-review.googlesource.com/c/474806/ https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:255: LOG(ERROR) << "Error on policy retrieved " << error_name << ":" On 2017/04/10 18:59:26, Daniel Erat wrote: > add space after ':' Done. https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:258: // from Chrome OS. On 2017/04/10 18:59:26, Daniel Erat wrote: > this is chrome-os-only code, so you should probably write "from session_manager" > here instead Done. https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right): https://codereview.chromium.org/2801993002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:76: void OnPolicyRetrievedWithError(const std::string& error_name, On 2017/04/10 18:59:26, Daniel Erat wrote: > please add a comment documenting what calls this and what it does. > > consider renaming these to e.g. OnPolicyRetrievedSuccess and > OnPolicyRetrievedError Done. https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:525: writer.AppendString(account_id); On 2017/04/10 18:59:26, Daniel Erat wrote: > please avoid copy-and-pasting code (i.e. the existing method is almost exactly > the same) Done. https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.h:152: using ErrorCallback = base::Callback<void(const std::string& error_name, On 2017/04/11 13:35:04, Andrew T Wilson (Slow) wrote: > On 2017/04/10 18:59:26, Daniel Erat wrote: > > mentioned earlier, but this should pass an enum rather than a string > > +1 as this lets us log UMA stats for it. Done. https://codereview.chromium.org/2801993002/diff/40001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.h:177: virtual void RetrievePolicyForUserWithErrorCallback( On 2017/04/10 18:59:26, Daniel Erat wrote: > please use a single method and update callers instead of adding a new method to > the interface that's only called in one place Done.
https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_policy_store.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.cc:63: chromeos::SessionManagerClient::SUCCESS); It's a bit weird that the asynchronous method (RetrieveDeviceLocalAccountPolicy()) was changed to support passing the error, while the synchronous method (BlockingRetrieveDeviceLocalAccountPolicy()) wasn't. They should really behave in the same way. And also we shouldn't pass SUCCESS here without actually knowing whether the operation was successful. So I guess when choosing a solution (either pass the error in both cases or don't pass it at all), the right question is: should we protect the device-local accounts from reading policy before session start? If yes (which I guess is the case), then the error should be passed and actually handled in ValidateLoadedPolicyBlob() below. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_policy_store.h (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.h:65: const chromeos::SessionManagerClient::RetrievePolicyResponseType& nit: #include "chromeos/dbus/session_manager_client.h" https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.h:65: const chromeos::SessionManagerClient::RetrievePolicyResponseType& nit: Enum values are usually passed by value, not by the const reference. (This applies to other files in this CL as well.) https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:220: // Disallow the sign in when the error is dbus_error::kSessionDoesNotExist nit: Please rephrase the comment to be explanatory instead of writing down the condition in words. E.g. something like (not insisting on the exact text - just giving the idea): "Disallow the sign in when the Chrome OS user session has not started, which should always happen before the profile construction. An attempt to read the policy outside the session will always fail and return an empty policy blob." https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:222: // TODO(igorcov): crbug/689206. Find the root cause for the behavior that nit: Is this TODO really needed here? I thought the idea was to put this code here forever - regardless of whether http://crbug.com/68920 gets resolved. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/session_manager_operation.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.cc:143: SessionManagerClient::SUCCESS); Same comment as was above for DeviceLocalAccountPolicyStore: it's better to obtain the actual response type instead of injecting the possibly untrue "SUCCESS" flag. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.cc:158: response_type != SessionManagerClient::SUCCESS) { nit: Maybe move the response_type comparison into the beginning of the condition, so that the parsing is skipped completely in case of errors? https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/session_manager_operation.h (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.h:116: const chromeos::SessionManagerClient::RetrievePolicyResponseType& nit: #include "chromeos/dbus/session_manager_client.h" https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.h:116: const chromeos::SessionManagerClient::RetrievePolicyResponseType& nit: "chromeos::" is unnecessary here https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:622: // kSessionManagerRetrievePolicyForUser method is complete. nit: Maybe change "complete" to "successfully complete"? https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:634: void OnRetrievePolicyError(const RetrievePolicyCallback& callback, nit: Please add a comment for this method. https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:31: // policy. Please explain in the comment the restriction that are put on this enum due to it being used for UMA, e.g.: // This enum is used to define the buckets for an enumerated UMA histogram. // Hence, // (a) existing enumerated constants should never be deleted or reordered, and // (b) new constants should only be appended at the end of the enumeration. https://codereview.chromium.org/2801993002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801993002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15070: +<histogram name="Enterprise.RetrievePolicyResponse" Would it be useful to have different metrics depending on the policy type (like user/device/device-local)? They are stored quite differently, and the implications of errors with each of them are quite different.
Thank you Maksim for your suggestions, I've tried to address your comments. Could you please take a look again? https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_policy_store.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.cc:63: chromeos::SessionManagerClient::SUCCESS); On 2017/04/18 15:03:16, emaxx wrote: > It's a bit weird that the asynchronous method > (RetrieveDeviceLocalAccountPolicy()) was changed to support passing the error, > while the synchronous method (BlockingRetrieveDeviceLocalAccountPolicy()) > wasn't. > They should really behave in the same way. And also we shouldn't pass SUCCESS > here without actually knowing whether the operation was successful. > > So I guess when choosing a solution (either pass the error in both cases or > don't pass it at all), the right question is: should we protect the device-local > accounts from reading policy before session start? If yes (which I guess is the > case), then the error should be passed and actually handled in > ValidateLoadedPolicyBlob() below. Changed to have the same pattern for all. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_policy_store.h (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.h:65: const chromeos::SessionManagerClient::RetrievePolicyResponseType& On 2017/04/18 15:03:17, emaxx wrote: > nit: #include "chromeos/dbus/session_manager_client.h" Done. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.h:65: const chromeos::SessionManagerClient::RetrievePolicyResponseType& On 2017/04/18 15:03:16, emaxx wrote: > nit: Enum values are usually passed by value, not by the const reference. > (This applies to other files in this CL as well.) Done. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:220: // Disallow the sign in when the error is dbus_error::kSessionDoesNotExist On 2017/04/18 15:03:17, emaxx wrote: > nit: Please rephrase the comment to be explanatory instead of writing down the > condition in words. E.g. something like (not insisting on the exact text - just > giving the idea): > "Disallow the sign in when the Chrome OS user session has not started, which > should always happen before the profile construction. An attempt to read the > policy outside the session will always fail and return an empty policy blob." Done. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:222: // TODO(igorcov): crbug/689206. Find the root cause for the behavior that On 2017/04/18 15:03:17, emaxx wrote: > nit: Is this TODO really needed here? I thought the idea was to put this code > here forever - regardless of whether http://crbug.com/68920 gets resolved. Done. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/session_manager_operation.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.cc:143: SessionManagerClient::SUCCESS); On 2017/04/18 15:03:17, emaxx wrote: > Same comment as was above for DeviceLocalAccountPolicyStore: it's better to > obtain the actual response type instead of injecting the possibly untrue > "SUCCESS" flag. Done. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.cc:158: response_type != SessionManagerClient::SUCCESS) { On 2017/04/18 15:03:17, emaxx wrote: > nit: Maybe move the response_type comparison into the beginning of the > condition, so that the parsing is skipped completely in case of errors? Didn't notice that, thanks. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/session_manager_operation.h (right): https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.h:116: const chromeos::SessionManagerClient::RetrievePolicyResponseType& On 2017/04/18 15:03:17, emaxx wrote: > nit: #include "chromeos/dbus/session_manager_client.h" Done. https://codereview.chromium.org/2801993002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.h:116: const chromeos::SessionManagerClient::RetrievePolicyResponseType& On 2017/04/18 15:03:17, emaxx wrote: > nit: "chromeos::" is unnecessary here Done. https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:622: // kSessionManagerRetrievePolicyForUser method is complete. On 2017/04/18 15:03:17, emaxx wrote: > nit: Maybe change "complete" to "successfully complete"? Done. https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:634: void OnRetrievePolicyError(const RetrievePolicyCallback& callback, On 2017/04/18 15:03:17, emaxx wrote: > nit: Please add a comment for this method. Done. https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/100001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:31: // policy. On 2017/04/18 15:03:17, emaxx wrote: > Please explain in the comment the restriction that are put on this enum due to > it being used for UMA, e.g.: > // This enum is used to define the buckets for an enumerated UMA histogram. > // Hence, > // (a) existing enumerated constants should never be deleted or reordered, and > // (b) new constants should only be appended at the end of the enumeration. Done. https://codereview.chromium.org/2801993002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801993002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15070: +<histogram name="Enterprise.RetrievePolicyResponse" On 2017/04/18 15:03:17, emaxx wrote: > Would it be useful to have different metrics depending on the policy type (like > user/device/device-local)? They are stored quite differently, and the > implications of errors with each of them are quite different. Done.
https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:118: RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST) { if this is unexpected, can you at least do something like: LOG(ERROR) << "Session manager claims that session doesn't exist; signing out"; here? https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:236: RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST) { same comment here about logging an error https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/blocking... File chromeos/dbus/blocking_method_caller.h (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/blocking... chromeos/dbus/blocking_method_caller.h:39: dbus::ScopedDBusError* error); error_out https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:896: if (!callback.is_null()) not your fault, but please add curly brackets here https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:36: enum RetrievePolicyResponseType { use 'enum class' so this is properly namespaced https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:37: SUCCESS, // The policy retrieved successfully. since these can't be reordered, i'd recommend explicitly assigning values to them. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:37: SUCCESS, // The policy retrieved successfully. nit: "The policy was retrieved successfully." https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:41: // session manager. it looks like this comes from session_manager, but the name is weird and non-obvious. does "SIG" stand for signature? consider giving this a more-human-intelligible name. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:43: // policy data(e.g. D-Bus timeout). nit: add space before '(' https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15234: + <owner>igorcov@chromium.org</owner> please list additional owners for these if possible https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15236: + The response obtained to retrieve device local account policy request. make all of these start with "On Chrome OS, " to make it clear that they're chrome-os-only
https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:126: // The session manager doesn't have policy, or the call failed. nit: Looks like the "or the call failed" part is not applicable anymore? https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:136: status_ = STATUS_PARSE_ERROR; The STATUS_PARSE_ERROR error is probably not the best match to the errors from the session manager. I think STATUS_LOAD_ERROR would be more suitable (while keeping STATUS_PARSE_ERROR only for ParseFromString failures). (Same applies to OnPolicyRetrieved().) https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:117: } else if (error_name == login_manager::dbus_error::kSessionDoesNotExist) { nit: Sorry for not pointing this out before, but it's recommended to omit "else" after "return": https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:128: void LogUma(const std::string& method_name, nit: Give this function a more specific name, e.g. LogPolicyResponseUma. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:145: "Enterprise." + stat_name, response, I doubt that it will work as expected. The |name| parameter of the macro is expected to be a constant, see here: https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?l=23 // All of these macros must be called with |name| as a runtime constant - it // doesn't have to literally be a constant, but it must be the same string on // all calls from a particular call site. Therefore the typical pattern I believe is to have a separate invocation of UMA_HISTOGRAM_* macro for each name. (It's possible to bypass these issues by using factory methods of base::Histogram, but this sacrifices the performance.) It'll be also useful to test in the end that everything works as expected by looking at chrome://histograms. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:42: OTHER_ERROR, // Other type of error while retrieving This item should be better located not in the end of the list, as after future additions of the new values it will be weird to have it lost somewhere in the middle. So proposing to place it right after SUCCESS or even to the first position. This is actually recommended by the guideline: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15232: +<histogram name="Enterprise.RetrieveDeviceLocalAccountPolicyResponse" nit: Maybe group all three histograms under a common prefix? E.g.: Enterprise.RetrievePolicyResponse.Device, Enterprise.RetrievePolicyResponse.User, Enterprise.RetrievePolicyResponse.DeviceLocalAccount. https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:110407: +<enum name="RetrievePolicyResponseType" type="int"> nit: Please prepend the enum name with the word "Enteprise" (that's how it is for the existing enterprise-related histograms). https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:110407: +<enum name="RetrievePolicyResponseType" type="int"> nit: Many <enum>'s have a corresponding <summary>, which I think won't harm to do. E.g.: <summary> Status codes produced by SessionManagerClient for policy retrieval requests. Corresponds to RetrievePolicyResponseType in chromeos/dbus/session_manager_client.h. </summary>
Thank you Dan and Maksim for your comments. I've updated the code, please take a look again. https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:118: RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST) { On 2017/04/20 21:06:38, Daniel Erat wrote: > if this is unexpected, can you at least do something like: > > LOG(ERROR) << "Session manager claims that session doesn't exist; signing out"; > > here? Done. https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:126: // The session manager doesn't have policy, or the call failed. On 2017/04/21 00:01:52, emaxx wrote: > nit: Looks like the "or the call failed" part is not applicable anymore? Done. https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:136: status_ = STATUS_PARSE_ERROR; On 2017/04/21 00:01:52, emaxx wrote: > The STATUS_PARSE_ERROR error is probably not the best match to the errors from > the session manager. I think STATUS_LOAD_ERROR would be more suitable (while > keeping STATUS_PARSE_ERROR only for ParseFromString failures). > (Same applies to OnPolicyRetrieved().) Done. https://codereview.chromium.org/2801993002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:236: RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST) { On 2017/04/20 21:06:38, Daniel Erat wrote: > same comment here about logging an error Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/blocking... File chromeos/dbus/blocking_method_caller.h (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/blocking... chromeos/dbus/blocking_method_caller.h:39: dbus::ScopedDBusError* error); On 2017/04/20 21:06:38, Daniel Erat wrote: > error_out Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:117: } else if (error_name == login_manager::dbus_error::kSessionDoesNotExist) { On 2017/04/21 00:01:52, emaxx wrote: > nit: Sorry for not pointing this out before, but it's recommended to omit "else" > after "return": > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:128: void LogUma(const std::string& method_name, On 2017/04/21 00:01:52, emaxx wrote: > nit: Give this function a more specific name, e.g. LogPolicyResponseUma. Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:145: "Enterprise." + stat_name, response, On 2017/04/21 00:01:52, emaxx wrote: > I doubt that it will work as expected. The |name| parameter of the macro is > expected to be a constant, see here: > https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?l=23 > // All of these macros must be called with |name| as a runtime constant - it > // doesn't have to literally be a constant, but it must be the same string on > // all calls from a particular call site. > > Therefore the typical pattern I believe is to have a separate invocation of > UMA_HISTOGRAM_* macro for each name. (It's possible to bypass these issues by > using factory methods of base::Histogram, but this sacrifices the performance.) > > > It'll be also useful to test in the end that everything works as expected by > looking at chrome://histograms. Changed to have separate calls. Thanks. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:896: if (!callback.is_null()) On 2017/04/20 21:06:39, Daniel Erat wrote: > not your fault, but please add curly brackets here Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:36: enum RetrievePolicyResponseType { On 2017/04/20 21:06:39, Daniel Erat wrote: > use 'enum class' so this is properly namespaced Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:37: SUCCESS, // The policy retrieved successfully. On 2017/04/20 21:06:39, Daniel Erat wrote: > nit: "The policy was retrieved successfully." Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:37: SUCCESS, // The policy retrieved successfully. On 2017/04/20 21:06:39, Daniel Erat wrote: > since these can't be reordered, i'd recommend explicitly assigning values to > them. Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:41: // session manager. On 2017/04/20 21:06:39, Daniel Erat wrote: > it looks like this comes from session_manager, but the name is weird and > non-obvious. does "SIG" stand for signature? consider giving this a > more-human-intelligible name. Session manager responds with either session_does_not_exist or with sig_encode_fail. First happens when session is not started, while the second happens when it failed to encode the policy when serializing. I guess the "SIG" is here from old times. Makes sense to rename the corresponding dbus::error constant too. Changed to POLICY_ENCODE_FAIL. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:42: OTHER_ERROR, // Other type of error while retrieving On 2017/04/21 00:01:52, emaxx wrote: > This item should be better located not in the end of the list, as after future > additions of the new values it will be weird to have it lost somewhere in the > middle. So proposing to place it right after SUCCESS or even to the first > position. This is actually recommended by the guideline: > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Done. https://codereview.chromium.org/2801993002/diff/160001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:43: // policy data(e.g. D-Bus timeout). On 2017/04/20 21:06:39, Daniel Erat wrote: > nit: add space before '(' Done. https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15232: +<histogram name="Enterprise.RetrieveDeviceLocalAccountPolicyResponse" On 2017/04/21 00:01:53, emaxx wrote: > nit: Maybe group all three histograms under a common prefix? E.g.: > Enterprise.RetrievePolicyResponse.Device, > Enterprise.RetrievePolicyResponse.User, > Enterprise.RetrievePolicyResponse.DeviceLocalAccount. Done. https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15234: + <owner>igorcov@chromium.org</owner> On 2017/04/20 21:06:39, Daniel Erat wrote: > please list additional owners for these if possible Done. https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15236: + The response obtained to retrieve device local account policy request. On 2017/04/20 21:06:39, Daniel Erat wrote: > make all of these start with "On Chrome OS, " to make it clear that they're > chrome-os-only Done. https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:110407: +<enum name="RetrievePolicyResponseType" type="int"> On 2017/04/21 00:01:53, emaxx wrote: > nit: Please prepend the enum name with the word "Enteprise" (that's how it is > for the existing enterprise-related histograms). Done. https://codereview.chromium.org/2801993002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:110407: +<enum name="RetrievePolicyResponseType" type="int"> On 2017/04/21 00:01:53, emaxx wrote: > nit: Please prepend the enum name with the word "Enteprise" (that's how it is > for the existing enterprise-related histograms). Done.
Patchset #11 (id:200001) has been deleted
LGTM with nits. Thanks for the patience! https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:154: return; nit: Unnecessary return. https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:45: RETRIEVE_POLICY_RESPONSE_TYPE_COUNT // Has to be the last value of nit: s/RETRIEVE_POLICY_RESPONSE_TYPE_COUNT/COUNT/ (there's no need to duplicate the enum type name, as it's already scoped to it).
lgtm with comments too. i think you need a histograms reviewer https://codereview.chromium.org/2801993002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:134: chromeos::SessionManagerClient::RetrievePolicyResponseType::SUCCESS) { in this file and others, feel free to pull RetrievePolicyResponseType in via 'using' (if possible) so you don't have to use these really long identifiers.
igorcov@chromium.org changed reviewers: + haraken@chromium.org
I've changed the code to disallow login only when the specific error SESSION_DOES_NOT_EXIST is received. For other error cases I left functionality unchanged (meaning the user sign in is allowed), so that consumer owned devices don't experience problems due to D-Bus timeouts or some policy encoding problems. I might get back to this code later if UMA stats will show small number of errors. Please take a look again. haraken@chromium.org: Please review changes in histograms.xml https://codereview.chromium.org/2801993002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right): https://codereview.chromium.org/2801993002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:134: chromeos::SessionManagerClient::RetrievePolicyResponseType::SUCCESS) { On 2017/04/21 14:22:45, Daniel Erat wrote: > in this file and others, feel free to pull RetrievePolicyResponseType in via > 'using' (if possible) so you don't have to use these really long identifiers. Done. https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:154: return; On 2017/04/21 13:43:43, emaxx wrote: > nit: Unnecessary return. Done. https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/220001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:45: RETRIEVE_POLICY_RESPONSE_TYPE_COUNT // Has to be the last value of On 2017/04/21 13:43:44, emaxx wrote: > nit: s/RETRIEVE_POLICY_RESPONSE_TYPE_COUNT/COUNT/ (there's no need to duplicate > the enum type name, as it's already scoped to it). Done.
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
https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_policy_store.cc (right): https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.cc:59: chromeos::SessionManagerClient::RetrievePolicyResponseType response = add 'using' directive to this file too so you can shorten these https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right): https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:23: using RetrievePolicyResponseType = this one needs to go back to the way it was before; don't put 'using' in header files (since it pollutes the namespace of every file that includes the header) https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/session_manager_operation.h (right): https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.h:115: chromeos::SessionManagerClient::RetrievePolicyResponseType response_type); you don't need chromeos:: here; this is already in the chromeos namespace. https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:120: } if it doesn't wrap, how about saving some lines here by using "} else if (error_name == ...) {" instead? https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:35: // (b) new constants should only be appended at the end of the enumeration. nit: "new constants should be inserted immediately before COUNT." https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:182: std::string* policy) = 0; policy_out https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:200: std::string* policy) = 0; policy_out https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:217: std::string* policy) = 0; policy_out
Patchset #14 (id:280001) has been deleted
Thank you Dan, I've fixed your suggestions. 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. On 2017/04/24 16:37:50, emaxx wrote: > nit: Please restore the original comment, given that the condition is now > changed back. Done. 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 = On 2017/04/24 16:37:50, emaxx wrote: > 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 Done. https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_local_account_policy_store.cc (right): https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_local_account_policy_store.cc:59: chromeos::SessionManagerClient::RetrievePolicyResponseType response = On 2017/04/24 19:50:13, Daniel Erat wrote: > add 'using' directive to this file too so you can shorten these Done. https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right): https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:23: using RetrievePolicyResponseType = On 2017/04/24 19:50:13, Daniel Erat wrote: > this one needs to go back to the way it was before; don't put 'using' in header > files (since it pollutes the namespace of every file that includes the header) Done. https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/session_manager_operation.h (right): https://codereview.chromium.org/2801993002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/settings/session_manager_operation.h:115: chromeos::SessionManagerClient::RetrievePolicyResponseType response_type); On 2017/04/24 19:50:13, Daniel Erat wrote: > you don't need chromeos:: here; this is already in the chromeos namespace. Done. https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:120: } On 2017/04/24 19:50:13, Daniel Erat wrote: > if it doesn't wrap, how about saving some lines here by using "} else if > (error_name == ...) {" instead? Done. https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:35: // (b) new constants should only be appended at the end of the enumeration. On 2017/04/24 19:50:13, Daniel Erat wrote: > nit: "new constants should be inserted immediately before COUNT." Done. https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:182: std::string* policy) = 0; On 2017/04/24 19:50:13, Daniel Erat wrote: > policy_out Done. https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:200: std::string* policy) = 0; On 2017/04/24 19:50:13, Daniel Erat wrote: > policy_out Done. https://codereview.chromium.org/2801993002/diff/260001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:217: std::string* policy) = 0; On 2017/04/24 19:50:13, Daniel Erat wrote: > policy_out Done.
lgtm
igorcov@chromium.org changed reviewers: + asvitkine@chromium.org - haraken@chromium.org
asvitkine@ PTAL at histograms.xml. Thank you.
https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:130: void LogPolicyResponseUma(const std::string& method_name, Nit: Make this param a StringPiece to avoid conversion since at least in one place you pass a char[] constant to it. https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:38: // data (e.g. D-Bus timeout). Nit: For longer comments like these, just put them above the enums as line comments. https://codereview.chromium.org/2801993002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801993002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15310: +<histogram name="Enterprise.RetrievePolicyResponse.User" Please use the histogram_suffixes construct - since User/Device/DeviceLocalAccount are specializations of the metric. The comments at the top of this file give you an example how to use it - and you can specify separator="."
Thank you Alexei, please take a look again. https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.cc:130: void LogPolicyResponseUma(const std::string& method_name, On 2017/04/25 14:41:41, Alexei Svitkine (slow) wrote: > Nit: Make this param a StringPiece to avoid conversion since at least in one > place you pass a char[] constant to it. Done. https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... File chromeos/dbus/session_manager_client.h (right): https://codereview.chromium.org/2801993002/diff/300001/chromeos/dbus/session_... chromeos/dbus/session_manager_client.h:38: // data (e.g. D-Bus timeout). On 2017/04/25 14:41:41, Alexei Svitkine (slow) wrote: > Nit: For longer comments like these, just put them above the enums as line > comments. Done. https://codereview.chromium.org/2801993002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801993002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:15310: +<histogram name="Enterprise.RetrievePolicyResponse.User" On 2017/04/25 14:41:41, Alexei Svitkine (slow) wrote: > Please use the histogram_suffixes construct - since > User/Device/DeviceLocalAccount are specializations of the metric. The comments > at the top of this file give you an example how to use it - and you can specify > separator="." Done.
Patchset #15 (id:320001) has been deleted
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.
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org, derat@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2801993002/#ps360001 (title: "Fixed review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you, Alexei 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, On 2017/04/26 14:57:26, Alexei Svitkine (slow) wrote: > Pass StringPiece by value (see comments in its header file). Done. 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" On 2017/04/26 14:57:26, Alexei Svitkine (slow) wrote: > Add base="true" to indicate you never log the unsuffixed version. Done.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Patchset #17 (id:380001) has been deleted
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #17 (id:400001) has been deleted
LGTM with nits regarding the last patchset https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc (right): https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc:502: .WillOnce(DoAll(SetArgumentPointee<1>(policy_data), gmock's changelog says that SetArgumentPointee is deprecated: https://cs.chromium.org/chromium/src/testing/gmock/CHANGES?l=58&rcl=0421b6f35... I believe that SetArgPointee, which is recommended instead, accepts string literals, so you won't need to store them in separate variables. https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc:527: const std::string policy_data = ""; nit: s/ = ""//
Thank you, Maksim. https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc (right): https://codereview.chromium.org/2801993002/diff/420001/chrome/browser/chromeo... 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: > gmock's changelog says that SetArgumentPointee is deprecated: > https://cs.chromium.org/chromium/src/testing/gmock/CHANGES?l=58&rcl=0421b6f35... > I believe that SetArgPointee, which is recommended instead, accepts string > literals, so you won't need to store them in separate variables. Done.
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, derat@chromium.org, emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2801993002/#ps440001 (title: "Fixed review 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": 440001, "attempt_start_ts": 1493387354613640,
"parent_rev": "5aea160185999ed1bee581bf5d1a6b5757a5eb8d", "commit_rev":
"f305a94dd4908c4975db3444e6b80732f48dcb39"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/f305a94dd4908c4975db3444e6b8... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:440001) as https://chromium.googlesource.com/chromium/src/+/f305a94dd4908c4975db3444e6b8...
Message was sent while issue was closed.
*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?* On Monday, April 10, 2017 at 12:02:35 PM UTC-5, igo...@chromium.org wrote: > > Reviewers: emaxx, Daniel Erat > CL: https://codereview.chromium.org/2801993002/ > > Message: > em...@chromium.org <javascript:>: Please review changes in > user_cloud_policy_store_chromeos. > > de...@chromium.org <javascript:>: Please review changes in > session_manager_client. > > Thank you, > > > > 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 > > Affected files (+82, -1 lines): > M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > M chromeos/dbus/session_manager_client.h > M chromeos/dbus/session_manager_client.cc > > > Index: chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > diff --git > a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > index > f73ec0ceb7924b4cee34ba619a98db1525294b92..e299227f1895e3edc29e13b3293f1dcbf3772d0b > 100644 > --- a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > +++ b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > @@ -19,6 +19,7 @@ > #include "base/stl_util.h" > #include "base/strings/stringprintf.h" > #include "chrome/browser/chromeos/policy/user_policy_token_loader.h" > +#include "chrome/browser/lifetime/application_lifetime.h" > #include "chromeos/cryptohome/cryptohome_parameters.h" > #include "chromeos/dbus/cryptohome_client.h" > #include "chromeos/dbus/session_manager_client.h" > @@ -41,6 +42,9 @@ const base::FilePath::CharType kPolicyKeyFile[] = > // Maximum key size that will be loaded, in bytes. > const size_t kKeySizeLimit = 16 * 1024; > > +const char kSessionDoesNotExist[] = > + "org.chromium.SessionManagerInterface.SessionDoesNotExist"; > + > enum ValidationFailure { > VALIDATION_FAILURE_DBUS, > VALIDATION_FAILURE_LOAD_KEY, > @@ -94,9 +98,11 @@ void UserCloudPolicyStoreChromeOS::Store( > void UserCloudPolicyStoreChromeOS::Load() { > // Cancel all pending requests. > weak_factory_.InvalidateWeakPtrs(); > - session_manager_client_->RetrievePolicyForUser( > + session_manager_client_->RetrievePolicyForUserWithErrorCallback( > cryptohome::Identification(account_id_), > base::Bind(&UserCloudPolicyStoreChromeOS::OnPolicyRetrieved, > + weak_factory_.GetWeakPtr()), > + base::Bind(&UserCloudPolicyStoreChromeOS::OnPolicyRetrievedWithError, > weak_factory_.GetWeakPtr())); > } > > @@ -243,6 +249,24 @@ void UserCloudPolicyStoreChromeOS::OnPolicyRetrieved( > } > } > > +void UserCloudPolicyStoreChromeOS::OnPolicyRetrievedWithError( > + const std::string& error_name, > + const std::string& error_message) { > + LOG(ERROR) << "Error on policy retrieved " << error_name << ":" > + << error_message; > + // Disallow the sign in when the error is > dbus_error::kSessionDoesNotExist > + // from Chrome OS. > + // TODO(igorcov): crbug/689206. Find the root cause for the behavior that > + // makes Chrome request the user policy before the session is started. > + if (error_name == kSessionDoesNotExist) { > + chrome::AttemptUserExit(); > + return; > + } > + > + status_ = STATUS_PARSE_ERROR; > + NotifyStoreError(); > +} > + > void UserCloudPolicyStoreChromeOS::ValidateRetrievedPolicy( > std::unique_ptr<em::PolicyFetchResponse> policy) { > // Create and configure a validator for the loaded policy. > Index: chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > diff --git > a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > index > 4c4021890eae7af03bba8576cec527451d3511eb..683a0416e8166c635563bd9e2b84c64b7efec16d > 100644 > --- a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > +++ b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > @@ -73,6 +73,9 @@ class UserCloudPolicyStoreChromeOS : public > UserCloudPolicyStoreBase { > // Called back from SessionManagerClient for policy load operations. > void OnPolicyRetrieved(const std::string& policy_blob); > > + void OnPolicyRetrievedWithError(const std::string& error_name, > + const std::string& error_message); > + > // Starts validation of the loaded |policy| before installing it. > void ValidateRetrievedPolicy( > std::unique_ptr<enterprise_management::PolicyFetchResponse> policy); > Index: chromeos/dbus/session_manager_client.cc > diff --git a/chromeos/dbus/session_manager_client.cc > b/chromeos/dbus/session_manager_client.cc > index > 9e87be84e0a371296c29620fa4930b9e484d6049..5f1bce63c752f651d3d2d81b75973d507cfffe91 > 100644 > --- a/chromeos/dbus/session_manager_client.cc > +++ b/chromeos/dbus/session_manager_client.cc > @@ -229,6 +229,15 @@ class SessionManagerClientImpl : public > SessionManagerClient { > callback); > } > > + void RetrievePolicyForUserWithErrorCallback( > + const cryptohome::Identification& cryptohome_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) override { > + CallRetrievePolicyByUsernameWithErrorCallback( > + login_manager::kSessionManagerRetrievePolicyForUser, cryptohome_id.id(), > + callback, error_callback); > + } > + > std::string BlockingRetrievePolicyForUser( > const cryptohome::Identification& cryptohome_id) override { > dbus::MethodCall method_call( > @@ -505,6 +514,23 @@ class SessionManagerClientImpl : public > SessionManagerClient { > callback)); > } > > + void CallRetrievePolicyByUsernameWithErrorCallback( > + const std::string& method_name, > + const std::string& account_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) { > + dbus::MethodCall method_call(login_manager::kSessionManagerInterface, > + method_name); > + dbus::MessageWriter writer(&method_call); > + writer.AppendString(account_id); > + session_manager_proxy_->CallMethodWithErrorCallback( > + &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, > + base::Bind(&SessionManagerClientImpl::OnRetrievePolicy, > + weak_ptr_factory_.GetWeakPtr(), method_name, callback), > + base::Bind(&SessionManagerClientImpl::OnRetrievePolicyWithError, > + weak_ptr_factory_.GetWeakPtr(), error_callback)); > + } > + > void CallStorePolicyByUsername(const std::string& method_name, > const std::string& account_id, > const std::string& policy_blob, > @@ -623,6 +649,16 @@ class SessionManagerClientImpl : public > SessionManagerClient { > callback.Run(serialized_proto); > } > > + void OnRetrievePolicyWithError(const ErrorCallback& callback, > + dbus::ErrorResponse* response) { > + std::string error_name; > + std::string error_message; > + dbus::MessageReader reader(response); > + error_name = response->GetErrorName(); > + reader.PopString(&error_message); > + callback.Run(error_name, error_message); > + } > + > // Called when kSessionManagerStorePolicy or > kSessionManagerStorePolicyForUser > // method is complete. > void OnStorePolicy(const std::string& method_name, > @@ -894,6 +930,12 @@ class SessionManagerClientStubImpl : public > SessionManagerClient { > GetUserFilePath(cryptohome_id, kStubPolicyFile)), > callback); > } > + void RetrievePolicyForUserWithErrorCallback( > + const cryptohome::Identification& cryptohome_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) override { > + RetrievePolicyForUser(cryptohome_id, callback); > + } > std::string BlockingRetrievePolicyForUser( > const cryptohome::Identification& cryptohome_id) override { > return GetFileContent(GetUserFilePath(cryptohome_id, kStubPolicyFile)); > Index: chromeos/dbus/session_manager_client.h > diff --git a/chromeos/dbus/session_manager_client.h > b/chromeos/dbus/session_manager_client.h > index > b294b761c8b9c692d7414f78e8432d46d663ec21..6a161cee5ff675e8e1e5458a2e48b94d1aba1ef4 > 100644 > --- a/chromeos/dbus/session_manager_client.h > +++ b/chromeos/dbus/session_manager_client.h > @@ -147,6 +147,11 @@ class CHROMEOS_EXPORT SessionManagerClient : public > DBusClient { > using RetrievePolicyCallback = > base::Callback<void(const std::string& protobuf)>; > > + // Used to RetrievePolicyForUserWithErrorCallback. Triggered when the > + // D-Bus request fails. > + using ErrorCallback = base::Callback<void(const std::string& error_name, > + const std::string& error_message)>; > + > // Fetches the device policy blob stored by the session manager. Upon > // completion of the retrieve attempt, we will call the provided callback. > virtual void RetrieveDevicePolicy(const RetrievePolicyCallback& callback) > = 0; > @@ -167,6 +172,13 @@ class CHROMEOS_EXPORT SessionManagerClient : public > DBusClient { > const cryptohome::Identification& cryptohome_id, > const RetrievePolicyCallback& callback) = 0; > > + // Same as RetrievePolicyForUser() but the error_callback is invoked in > case > + // of an error. > + virtual void RetrievePolicyForUserWithErrorCallback( > + const cryptohome::Identification& cryptohome_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) = 0; > + > // Same as RetrievePolicyForUser() but blocks until a reply is received, > and > // returns the policy synchronously. Returns an empty string if the method > // call fails. > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
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? On Monday, April 10, 2017 at 12:02:35 PM UTC-5, igo...@chromium.org wrote: > > Reviewers: emaxx, Daniel Erat > CL: https://codereview.chromium.org/2801993002/ > > Message: > em...@chromium.org <javascript:>: Please review changes in > user_cloud_policy_store_chromeos. > > de...@chromium.org <javascript:>: Please review changes in > session_manager_client. > > Thank you, > > > > 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 > > Affected files (+82, -1 lines): > M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > M chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > M chromeos/dbus/session_manager_client.h > M chromeos/dbus/session_manager_client.cc > > > Index: chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > diff --git > a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > index > f73ec0ceb7924b4cee34ba619a98db1525294b92..e299227f1895e3edc29e13b3293f1dcbf3772d0b > 100644 > --- a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > +++ b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc > @@ -19,6 +19,7 @@ > #include "base/stl_util.h" > #include "base/strings/stringprintf.h" > #include "chrome/browser/chromeos/policy/user_policy_token_loader.h" > +#include "chrome/browser/lifetime/application_lifetime.h" > #include "chromeos/cryptohome/cryptohome_parameters.h" > #include "chromeos/dbus/cryptohome_client.h" > #include "chromeos/dbus/session_manager_client.h" > @@ -41,6 +42,9 @@ const base::FilePath::CharType kPolicyKeyFile[] = > // Maximum key size that will be loaded, in bytes. > const size_t kKeySizeLimit = 16 * 1024; > > +const char kSessionDoesNotExist[] = > + "org.chromium.SessionManagerInterface.SessionDoesNotExist"; > + > enum ValidationFailure { > VALIDATION_FAILURE_DBUS, > VALIDATION_FAILURE_LOAD_KEY, > @@ -94,9 +98,11 @@ void UserCloudPolicyStoreChromeOS::Store( > void UserCloudPolicyStoreChromeOS::Load() { > // Cancel all pending requests. > weak_factory_.InvalidateWeakPtrs(); > - session_manager_client_->RetrievePolicyForUser( > + session_manager_client_->RetrievePolicyForUserWithErrorCallback( > cryptohome::Identification(account_id_), > base::Bind(&UserCloudPolicyStoreChromeOS::OnPolicyRetrieved, > + weak_factory_.GetWeakPtr()), > + base::Bind(&UserCloudPolicyStoreChromeOS::OnPolicyRetrievedWithError, > weak_factory_.GetWeakPtr())); > } > > @@ -243,6 +249,24 @@ void UserCloudPolicyStoreChromeOS::OnPolicyRetrieved( > } > } > > +void UserCloudPolicyStoreChromeOS::OnPolicyRetrievedWithError( > + const std::string& error_name, > + const std::string& error_message) { > + LOG(ERROR) << "Error on policy retrieved " << error_name << ":" > + << error_message; > + // Disallow the sign in when the error is > dbus_error::kSessionDoesNotExist > + // from Chrome OS. > + // TODO(igorcov): crbug/689206. Find the root cause for the behavior that > + // makes Chrome request the user policy before the session is started. > + if (error_name == kSessionDoesNotExist) { > + chrome::AttemptUserExit(); > + return; > + } > + > + status_ = STATUS_PARSE_ERROR; > + NotifyStoreError(); > +} > + > void UserCloudPolicyStoreChromeOS::ValidateRetrievedPolicy( > std::unique_ptr<em::PolicyFetchResponse> policy) { > // Create and configure a validator for the loaded policy. > Index: chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > diff --git > a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > index > 4c4021890eae7af03bba8576cec527451d3511eb..683a0416e8166c635563bd9e2b84c64b7efec16d > 100644 > --- a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > +++ b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h > @@ -73,6 +73,9 @@ class UserCloudPolicyStoreChromeOS : public > UserCloudPolicyStoreBase { > // Called back from SessionManagerClient for policy load operations. > void OnPolicyRetrieved(const std::string& policy_blob); > > + void OnPolicyRetrievedWithError(const std::string& error_name, > + const std::string& error_message); > + > // Starts validation of the loaded |policy| before installing it. > void ValidateRetrievedPolicy( > std::unique_ptr<enterprise_management::PolicyFetchResponse> policy); > Index: chromeos/dbus/session_manager_client.cc > diff --git a/chromeos/dbus/session_manager_client.cc > b/chromeos/dbus/session_manager_client.cc > index > 9e87be84e0a371296c29620fa4930b9e484d6049..5f1bce63c752f651d3d2d81b75973d507cfffe91 > 100644 > --- a/chromeos/dbus/session_manager_client.cc > +++ b/chromeos/dbus/session_manager_client.cc > @@ -229,6 +229,15 @@ class SessionManagerClientImpl : public > SessionManagerClient { > callback); > } > > + void RetrievePolicyForUserWithErrorCallback( > + const cryptohome::Identification& cryptohome_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) override { > + CallRetrievePolicyByUsernameWithErrorCallback( > + login_manager::kSessionManagerRetrievePolicyForUser, cryptohome_id.id(), > + callback, error_callback); > + } > + > std::string BlockingRetrievePolicyForUser( > const cryptohome::Identification& cryptohome_id) override { > dbus::MethodCall method_call( > @@ -505,6 +514,23 @@ class SessionManagerClientImpl : public > SessionManagerClient { > callback)); > } > > + void CallRetrievePolicyByUsernameWithErrorCallback( > + const std::string& method_name, > + const std::string& account_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) { > + dbus::MethodCall method_call(login_manager::kSessionManagerInterface, > + method_name); > + dbus::MessageWriter writer(&method_call); > + writer.AppendString(account_id); > + session_manager_proxy_->CallMethodWithErrorCallback( > + &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, > + base::Bind(&SessionManagerClientImpl::OnRetrievePolicy, > + weak_ptr_factory_.GetWeakPtr(), method_name, callback), > + base::Bind(&SessionManagerClientImpl::OnRetrievePolicyWithError, > + weak_ptr_factory_.GetWeakPtr(), error_callback)); > + } > + > void CallStorePolicyByUsername(const std::string& method_name, > const std::string& account_id, > const std::string& policy_blob, > @@ -623,6 +649,16 @@ class SessionManagerClientImpl : public > SessionManagerClient { > callback.Run(serialized_proto); > } > > + void OnRetrievePolicyWithError(const ErrorCallback& callback, > + dbus::ErrorResponse* response) { > + std::string error_name; > + std::string error_message; > + dbus::MessageReader reader(response); > + error_name = response->GetErrorName(); > + reader.PopString(&error_message); > + callback.Run(error_name, error_message); > + } > + > // Called when kSessionManagerStorePolicy or > kSessionManagerStorePolicyForUser > // method is complete. > void OnStorePolicy(const std::string& method_name, > @@ -894,6 +930,12 @@ class SessionManagerClientStubImpl : public > SessionManagerClient { > GetUserFilePath(cryptohome_id, kStubPolicyFile)), > callback); > } > + void RetrievePolicyForUserWithErrorCallback( > + const cryptohome::Identification& cryptohome_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) override { > + RetrievePolicyForUser(cryptohome_id, callback); > + } > std::string BlockingRetrievePolicyForUser( > const cryptohome::Identification& cryptohome_id) override { > return GetFileContent(GetUserFilePath(cryptohome_id, kStubPolicyFile)); > Index: chromeos/dbus/session_manager_client.h > diff --git a/chromeos/dbus/session_manager_client.h > b/chromeos/dbus/session_manager_client.h > index > b294b761c8b9c692d7414f78e8432d46d663ec21..6a161cee5ff675e8e1e5458a2e48b94d1aba1ef4 > 100644 > --- a/chromeos/dbus/session_manager_client.h > +++ b/chromeos/dbus/session_manager_client.h > @@ -147,6 +147,11 @@ class CHROMEOS_EXPORT SessionManagerClient : public > DBusClient { > using RetrievePolicyCallback = > base::Callback<void(const std::string& protobuf)>; > > + // Used to RetrievePolicyForUserWithErrorCallback. Triggered when the > + // D-Bus request fails. > + using ErrorCallback = base::Callback<void(const std::string& error_name, > + const std::string& error_message)>; > + > // Fetches the device policy blob stored by the session manager. Upon > // completion of the retrieve attempt, we will call the provided callback. > virtual void RetrieveDevicePolicy(const RetrievePolicyCallback& callback) > = 0; > @@ -167,6 +172,13 @@ class CHROMEOS_EXPORT SessionManagerClient : public > DBusClient { > const cryptohome::Identification& cryptohome_id, > const RetrievePolicyCallback& callback) = 0; > > + // Same as RetrievePolicyForUser() but the error_callback is invoked in > case > + // of an error. > + virtual void RetrievePolicyForUserWithErrorCallback( > + const cryptohome::Identification& cryptohome_id, > + const RetrievePolicyCallback& callback, > + const ErrorCallback& error_callback) = 0; > + > // Same as RetrievePolicyForUser() but blocks until a reply is received, > and > // returns the policy synchronously. Returns an empty string if the method > // call fails. > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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. |
