Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2653913002:
Check username validity in the FakeAuthPolicyClient (Closed)
|
Created:
3 years, 11 months ago by Roman Sorokin (ftl) Modified:
3 years, 10 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, alemate+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck username validity in the FakeAuthPolicyClient
BUG=683878
TEST=manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2653913002
Cr-Commit-Position: refs/heads/master@{#446650}
Committed: https://chromium.googlesource.com/chromium/src/+/9b69dd7f422f29571bfd72da3d452a3580f33d79
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add unittests #
Total comments: 4
Patch Set 3 : Machine name tests #
Total comments: 4
Patch Set 4 : Comments #
Messages
Total messages: 33 (20 generated)
Description was changed from ========== Check username validity in the FakeAuthPolicyClient BUG=683878 TEST=manual ========== to ========== Check username validity in the FakeAuthPolicyClient BUG=683878 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by rsorokin@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...
rsorokin@chromium.org changed reviewers: + achuith@chromium.org, tnagel@chromium.org
Hey Thiemo, PTAL at chromeos/dbus/fake_auth_policy_client.cc Hey Achuith, PTAL at chrome/browser/resources/chromeos/login/offline_ad_login.js
https://codereview.chromium.org/2653913002/diff/1/chromeos/dbus/fake_auth_pol... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2653913002/diff/1/chromeos/dbus/fake_auth_pol... chromeos/dbus/fake_auth_policy_client.cc:71: if (parts.size() != 2U) { It seems that you'll have to check the sizes of the parts as well. I still managed to crash Chrome by entering "a@ " as the user name. Nit: Imho "2U" is uncommon, "2" would be easier to read.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2653913002/diff/1/chromeos/dbus/fake_auth_pol... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2653913002/diff/1/chromeos/dbus/fake_auth_pol... chromeos/dbus/fake_auth_policy_client.cc:71: if (parts.size() != 2U) { On 2017/01/24 14:17:36, Thiemo Nagel (slow) wrote: > It seems that you'll have to check the sizes of the parts as well. I still > managed to crash Chrome by entering "a@ " as the user name. > > Nit: Imho "2U" is uncommon, "2" would be easier to read. Sounds like we need a unit test
The CQ bit was checked by rsorokin@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...
https://codereview.chromium.org/2653913002/diff/1/chromeos/dbus/fake_auth_pol... File chromeos/dbus/fake_auth_policy_client.cc (right): https://codereview.chromium.org/2653913002/diff/1/chromeos/dbus/fake_auth_pol... chromeos/dbus/fake_auth_policy_client.cc:71: if (parts.size() != 2U) { On 2017/01/24 14:17:36, Thiemo Nagel (slow) wrote: > It seems that you'll have to check the sizes of the parts as well. I still > managed to crash Chrome by entering "a@ " as the user name. > > Nit: Imho "2U" is uncommon, "2" would be easier to read. Done. https://codereview.chromium.org/2653913002/diff/1/chromeos/dbus/fake_auth_pol... chromeos/dbus/fake_auth_policy_client.cc:71: if (parts.size() != 2U) { On 2017/01/24 19:42:11, achuithb wrote: > On 2017/01/24 14:17:36, Thiemo Nagel (slow) wrote: > > It seems that you'll have to check the sizes of the parts as well. I still > > managed to crash Chrome by entering "a@ " as the user name. > > > > Nit: Imho "2U" is uncommon, "2" would be easier to read. > > Sounds like we need a unit test Done.
Lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2653913002/diff/20001/chromeos/dbus/fake_auth... File chromeos/dbus/fake_auth_policy_client_unittest.cc (right): https://codereview.chromium.org/2653913002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:13: const char kMachineName[] = "machine_name"; I think this just adds noise - why not pass in "" https://codereview.chromium.org/2653913002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:17: EXPECT_EQ(expected, actual); Hmm, I believe you won't be able to tell which case failed when this fails. Does it make sense to also pass in the user_principal_name into this function and print it? In general we avoid calling EXPECT in helper functions for this reason.
The CQ bit was checked by rsorokin@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...
https://codereview.chromium.org/2653913002/diff/20001/chromeos/dbus/fake_auth... File chromeos/dbus/fake_auth_policy_client_unittest.cc (right): https://codereview.chromium.org/2653913002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:13: const char kMachineName[] = "machine_name"; On 2017/01/25 20:02:46, achuithb wrote: > I think this just adds noise - why not pass in "" I decided to add machine_name checks, and tests for them. https://codereview.chromium.org/2653913002/diff/20001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:17: EXPECT_EQ(expected, actual); On 2017/01/25 20:02:46, achuithb wrote: > Hmm, I believe you won't be able to tell which case failed when this fails. Does > it make sense to also pass in the user_principal_name into this function and > print it? > > In general we avoid calling EXPECT in helper functions for this reason. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2653913002/diff/40001/chromeos/dbus/fake_auth... File chromeos/dbus/fake_auth_policy_client_unittest.cc (right): https://codereview.chromium.org/2653913002/diff/40001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:35: TEST(FakeAuthPolicyClientTest, JoinAdDomain_BadMachineName) { add a comment https://codereview.chromium.org/2653913002/diff/40001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:46: TEST(FakeAuthPolicyClientTest, JoinAdDomain_ParseUPN) { add a comment
The CQ bit was checked by rsorokin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/2653913002/#ps60001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2653913002/diff/40001/chromeos/dbus/fake_auth... File chromeos/dbus/fake_auth_policy_client_unittest.cc (right): https://codereview.chromium.org/2653913002/diff/40001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:35: TEST(FakeAuthPolicyClientTest, JoinAdDomain_BadMachineName) { On 2017/01/27 01:02:15, achuithb wrote: > add a comment Done. https://codereview.chromium.org/2653913002/diff/40001/chromeos/dbus/fake_auth... chromeos/dbus/fake_auth_policy_client_unittest.cc:46: TEST(FakeAuthPolicyClientTest, JoinAdDomain_ParseUPN) { On 2017/01/27 01:02:15, achuithb wrote: > add a comment Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rsorokin@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": 60001, "attempt_start_ts": 1485517745077320,
"parent_rev": "2529431f34dfdfef0d698b5ab35a5c9cb9ff566a", "commit_rev":
"9b69dd7f422f29571bfd72da3d452a3580f33d79"}
Message was sent while issue was closed.
Description was changed from ========== Check username validity in the FakeAuthPolicyClient BUG=683878 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Check username validity in the FakeAuthPolicyClient BUG=683878 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2653913002 Cr-Commit-Position: refs/heads/master@{#446650} Committed: https://chromium.googlesource.com/chromium/src/+/9b69dd7f422f29571bfd72da3d45... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/9b69dd7f422f29571bfd72da3d45... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
