Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531063002:
Implement FakeAuthPolicyClient::RefreshDevicePolicy() (Closed)
|
Created:
4 years ago by Thiemo Nagel Modified:
3 years, 4 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement FakeAuthPolicyClient::RefreshDevicePolicy()
To allow testing of Active Directory device policy with a fake D-Bus
client, simulate the combined actions of authpolicyd and session manager
by writing a minimal PolicyFetchResponse to stub_device_policy.
BUG=667780
Committed: https://crrev.com/020311f0c2abd317c7df310cd7aa9829dabe781c
Cr-Commit-Position: refs/heads/master@{#435706}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Write file on task runner #Patch Set 3 : Inject a PostTaskAndReply function #Patch Set 4 : Fix compilation errors #Patch Set 5 : Rebase #Patch Set 6 : Use base::PostTaskAndReplyWithResult() #Patch Set 7 : Polish #
Total comments: 17
Patch Set 8 : Address Achuith's comments #
Total comments: 8
Patch Set 9 : More simplifications following Achuith's comments #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 54 (34 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tnagel@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...
tnagel@chromium.org changed reviewers: + achuith@chromium.org
Hi Achuith, could you please take a look? Thank you, Thiemo
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client.cc:54: base::ThreadRestrictions::ScopedAllowIO permit_io_within_current_scope; > ** Presubmit ERRORS ** > Banned functions were used. > chromeos/dbus/fake_auth_policy_client.cc:54: > New code should not use ScopedAllowIO. Post a task to the blocking > pool or the FILE thread instead. Do I really need to post to a different thread in a *fake* client or is there some way around this?
https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client.cc:51: base::FilePath device_policy_path = nit const https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client.cc:54: base::ThreadRestrictions::ScopedAllowIO permit_io_within_current_scope; On 2016/11/25 15:07:03, Thiemo Nagel (slow) wrote: > > ** Presubmit ERRORS ** > > Banned functions were used. > > chromeos/dbus/fake_auth_policy_client.cc:54: > > New code should not use ScopedAllowIO. Post a task to the blocking > > pool or the FILE thread instead. > > Do I really need to post to a different thread in a *fake* client or is there > some way around this? Yeah, pretty sure you can't use this. Isn't there a blocking thread pool you can use? https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client.cc:55: int bytes_written = nit const
The CQ bit was checked by tnagel@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...
> Yeah, pretty sure you can't use this. Isn't there a blocking > thread pool you can use? Thanks Achuith, how can I access the blocking pool? I can't use anything from browser or content. Using base::SequencedTaskRunnerHandle::Get() leads to an assertion: [9045:9045:1128/195615:FATAL:thread_restrictions.cc(38)] Check failed: false. Function marked as IO-only was called from a thread that disallows IO! If this thread really should be allowed to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() in this thread's startup.
On 2016/11/28 19:18:40, Thiemo Nagel (slow) wrote: > > Yeah, pretty sure you can't use this. Isn't there a blocking > > thread pool you can use? > > Thanks Achuith, how can I access the blocking pool? I can't use anything from > browser or content. Using base::SequencedTaskRunnerHandle::Get() leads to an > assertion: > > [9045:9045:1128/195615:FATAL:thread_restrictions.cc(38)] Check failed: false. > Function marked as IO-only was called from a thread that disallows IO! If this > thread really should be allowed to make IO calls, adjust the call to > base::ThreadRestrictions::SetIOAllowed() in this thread's startup. Can't we add content to DEPS? I think you want to use this: https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tnagel@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...
> Can't we add content to DEPS? I'm somewhat reluctant about that. > I think you want to use this: > https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?... I've uploaded a new patchset which injects a function pointer into FakeAuthPolicyClient. Do you think that could be a viable route? (I'd still need to add some comments before submitting.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 tnagel@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.
Hold the reviews - I think I have a better idea...
The CQ bit was checked by tnagel@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...
On 2016/11/30 17:01:28, Thiemo Nagel (slow) wrote: > Hold the reviews - I think I have a better idea... Ok :)
The CQ bit was checked by tnagel@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...
Hi Achuith, could you please take another look now? I've taken a different approach by following the example of SessionManagerClientStubImpl and using base::PostTaskAndReplyWithResult() together with base::WorkerPool::GetTaskRunner() -- it works like a charm, afaics. https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client.cc:51: base::FilePath device_policy_path = On 2016/11/28 09:05:51, achuithb wrote: > nit const Done. https://codereview.chromium.org/2531063002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client.cc:55: int bytes_written = On 2016/11/28 09:05:51, achuithb wrote: > nit const That variable has been removed.
https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:47: int FakeAuthPolicyClient::WriteDevicePolicyFile() { move to anonymous namespace. I think it's better for the return value to be a bool here. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:61: if (!PathService::Get(FILE_OWNER_KEY, &owner_key_path)) { Drop {} https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:62: return -1; I think you should return false here. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:70: return base::WriteFile(device_policy_path, serialized_response.c_str(), I think you should return base::WriteFile(...) == serialized_response.size(); https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:76: int bytes_written) { I'd change this to bool success or something similar. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.h (right): https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.h:34: static int WriteDevicePolicyFile(); We prefer functions in the anonymous namespace in the cc file to private static fns, as this reduces the linker burden. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.h:36: int bytes_written); Let's add a function comment. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.h:38: base::WeakPtrFactory<FakeAuthPolicyClient> weak_factory_{this}; Is this allowed by the c++11 guidelines? Have you seen this elsewhere in the codebase? I haven't seen it, and I'd rather not have this be the first place. I think it's kinda cool though.
The CQ bit was checked by tnagel@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...
Thank you! Could you please take another look? https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:47: int FakeAuthPolicyClient::WriteDevicePolicyFile() { On 2016/11/30 19:23:34, achuithb wrote: > move to anonymous namespace. > > I think it's better for the return value to be a bool here. Done. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:61: if (!PathService::Get(FILE_OWNER_KEY, &owner_key_path)) { On 2016/11/30 19:23:34, achuithb wrote: > Drop {} Done. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:62: return -1; On 2016/11/30 19:23:34, achuithb wrote: > I think you should return false here. Done. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:70: return base::WriteFile(device_policy_path, serialized_response.c_str(), On 2016/11/30 19:23:34, achuithb wrote: > I think you should return > base::WriteFile(...) == serialized_response.size(); Done. (Due to type mismatch some more code is needed, though.) https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:76: int bytes_written) { On 2016/11/30 19:23:34, achuithb wrote: > I'd change this to bool success or something similar. Done. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.h (right): https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.h:34: static int WriteDevicePolicyFile(); On 2016/11/30 19:23:34, achuithb wrote: > We prefer functions in the anonymous namespace in the cc file to private static > fns, as this reduces the linker burden. Done. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.h:36: int bytes_written); On 2016/11/30 19:23:34, achuithb wrote: > Let's add a function comment. Done. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.h:38: base::WeakPtrFactory<FakeAuthPolicyClient> weak_factory_{this}; On 2016/11/30 19:23:34, achuithb wrote: > Is this allowed by the c++11 guidelines? We allow Uniform Initialization Syntax and Non-Static Class Member Initializers. All I'm doing is combining the two, afaics. https://chromium-cpp.appspot.com/ > Have you seen this elsewhere in the > codebase? No. > I haven't seen it, and I'd rather not have this be the first place. I think it's > kinda cool though. Removed it. Generally speaking, some place needs to be the first. What about starting a thread on chromium-dev?
Sorry for another round of this. https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.h (right): https://codereview.chromium.org/2531063002/diff/140001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.h:38: base::WeakPtrFactory<FakeAuthPolicyClient> weak_factory_{this}; On 2016/11/30 20:06:36, Thiemo Nagel (slow) wrote: > On 2016/11/30 19:23:34, achuithb wrote: > > Is this allowed by the c++11 guidelines? > > We allow Uniform Initialization Syntax and Non-Static Class Member Initializers. > All I'm doing is combining the two, afaics. > > https://chromium-cpp.appspot.com/ > > > Have you seen this elsewhere in the > > codebase? > > No. > > > I haven't seen it, and I'd rather not have this be the first place. I think > it's > > kinda cool though. > > Removed it. Generally speaking, some place needs to be the first. What about > starting a thread on chromium-dev? Personally, I really like this, and would love to see it adopted more widely, especially for WeakPtrFactory, but I think a thread on chromium-dev should be a starting point so the persnickety can spend hours debating it. Since this is your innovation, I think you should propose this? https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:23: // Create minimal stub device policy file and drop it at the place where This comment may be better served as a function comment right before line 22 https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:50: static_assert(std::numeric_limits<int>::max() <= What's your concern here? At a minimum we need a comment. https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:79: base::Bind(&FakeAuthPolicyClient::OnDevicePolicyFileWritten, Curious: do we need OnDevicePolicyFileWritten? Can't this argument simply be callback? At a minimum, it looks like OnDevicePolicyFileWritten can also be an anonymous function, so we don't need WeakPtrFactory at all.
https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:79: base::Bind(&FakeAuthPolicyClient::OnDevicePolicyFileWritten, On 2016/11/30 20:22:50, achuithb wrote: > Curious: do we need OnDevicePolicyFileWritten? Can't this argument simply be > callback? > > At a minimum, it looks like OnDevicePolicyFileWritten can also be an anonymous > function, so we don't need WeakPtrFactory at all. Ok, I don't think just passing callback will work, but maybe you could use a lambda instead of an anonymous function? https://groups.google.com/a/chromium.org/forum/#!msg/cxx/QxjsPELDYdQ/IH8wNQln...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Sorry for another round of this. I guess I deserve it for not writing better code in the first place. ;) Thank you for the thorough review and could you please take another look? https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:23: // Create minimal stub device policy file and drop it at the place where On 2016/11/30 20:22:49, achuithb wrote: > This comment may be better served as a function comment right before line 22 Good point. Done. https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:50: static_assert(std::numeric_limits<int>::max() <= On 2016/11/30 20:22:49, achuithb wrote: > What's your concern here? I was afraid that a narrowing cast could lead to undefined behavior. Looking it up on the web [1], it seems that a narrowing cast is implementation-defined which sgtm since the numbers will always be small anyways. Thus removing the static_assert(). [1] http://stackoverflow.com/questions/19273658/integer-conversionsnarrowing-wide... https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:79: base::Bind(&FakeAuthPolicyClient::OnDevicePolicyFileWritten, Good point, and passing the callback actually does work. :)
The CQ bit was checked by tnagel@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.
lgtm! Thanks for bearing with all the rounds of feedback. https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/160001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:79: base::Bind(&FakeAuthPolicyClient::OnDevicePolicyFileWritten, On 2016/12/01 09:52:04, Thiemo Nagel (slow) wrote: > Good point, and passing the callback actually does work. :) I'll never understand the voodoo of functors. https://codereview.chromium.org/2531063002/diff/180001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/180001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:50: return bytes_written == static_cast<int>(serialized_response.size()); It's a matter of taste, but worth considering combining the two conditions: return bytes_written > 0 && bytes_written == static_cast<int>(...);
Thanks a lot for the review! https://codereview.chromium.org/2531063002/diff/180001/chromeos/dbus/fake_aut... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2531063002/diff/180001/chromeos/dbus/fake_aut... chromeos/dbus/fake_auth_policy_client.cc:50: return bytes_written == static_cast<int>(serialized_response.size()); On 2016/12/01 20:26:14, achuithb wrote: > It's a matter of taste, but worth considering combining the two conditions: > return bytes_written > 0 && bytes_written == static_cast<int>(...); > I find the way I wrote it much clearer which proves that it's a matter of personal preference... ;)
The CQ bit was checked by tnagel@chromium.org
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": 180001, "attempt_start_ts": 1480625255459980,
"parent_rev": "fe24ed2ce8585112acd089ae077bffd8134954b0", "commit_rev":
"86c00f5adf4502a955e0a3e1de3b3a01b3c6f06e"}
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implement FakeAuthPolicyClient::RefreshDevicePolicy() To allow testing of Active Directory device policy with a fake D-Bus client, simulate the combined actions of authpolicyd and session manager by writing a minimal PolicyFetchResponse to stub_device_policy. BUG=667780 ========== to ========== Implement FakeAuthPolicyClient::RefreshDevicePolicy() To allow testing of Active Directory device policy with a fake D-Bus client, simulate the combined actions of authpolicyd and session manager by writing a minimal PolicyFetchResponse to stub_device_policy. BUG=667780 Committed: https://crrev.com/020311f0c2abd317c7df310cd7aa9829dabe781c Cr-Commit-Position: refs/heads/master@{#435706} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/020311f0c2abd317c7df310cd7aa9829dabe781c Cr-Commit-Position: refs/heads/master@{#435706}
Message was sent while issue was closed.
brettw@chromium.org changed reviewers: + brettw@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2531063002/diff/180001/chromeos/BUILD.gn File chromeos/BUILD.gn (right): https://codereview.chromium.org/2531063002/diff/180001/chromeos/BUILD.gn#newc... chromeos/BUILD.gn:33: "//chrome/browser/chromeos:device_policy_proto", Hi, this is a layering violation as //chromeos is as a lower layer than //chrome. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=758767 to track this. I would like to get this fixed soon before it becomes even harder to fix. Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
