Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838423003:
Make sure challenging user keys doesn't work in signin profile. (Closed)
|
Created:
3 years, 8 months ago by The one and only Dr. Crash Modified:
3 years, 7 months ago Reviewers:
emaxx CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, dkrahn+watch_chromium.org, dkalin1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure that challenging user keys is unavailable on signin.
Returns a specific error saying the keys are not available in the
signin profile.
BUG=715121
TEST=unit tests
Review-Url: https://codereview.chromium.org/2838423003
Cr-Commit-Position: refs/heads/master@{#468019}
Committed: https://chromium.googlesource.com/chromium/src/+/2691676080f805e1b532f40ef3ef70dea8df8ac0
Patch Set 1 #Patch Set 2 : Rename machine key tests in 2841553002 not here #Patch Set 3 : Undo #Patch Set 4 : Rebased. #
Total comments: 2
Patch Set 5 : Addressed review feedback. #Patch Set 6 : Rebased #Patch Set 7 : Avoid crashing. #Patch Set 8 : Rebased #
Depends on Patchset: Messages
Total messages: 34 (26 generated)
Description was changed from ========== Ensure that challenging user keys is unavailable on signin. Returns a specific error saying the keys are not available in the signin profile. BUG=715121 TEST=unit tests ========== to ========== Ensure that challenging user keys is unavailable on signin. Returns a specific error saying the keys are not available in the signin profile. BUG=715121 TEST=unit tests ==========
drcrash@chromium.org changed reviewers: + emaxx@chromium.org
The CQ bit was checked by drcrash@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 checked by drcrash@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.
Left a comment, please see below. And I guess this change needs to wait until http://crrev.com/2841553002 is finished, and then rebased on top of that? I'm advocating for the tests simplification in that CL, and that will affect this CL a little bit too. https://codereview.chromium.org/2838423003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc (right): https://codereview.chromium.org/2838423003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:482: if (chromeos::ProfileHelper::Get()->IsSigninProfile(profile_)) { Use the ProfileHelper::GetUserByProfile() method for consistency with what you're doing in crrev.com/2846543002 for platform_keys::GetTokens()?
On 2017/04/27 18:10:28, emaxx wrote: > Left a comment, please see below. > And I guess this change needs to wait until http://crrev.com/2841553002 is > finished, and then rebased on top of that? I'm advocating for the tests > simplification in that CL, and that will affect this CL a little bit too. > > https://codereview.chromium.org/2838423003/diff/60001/chrome/browser/extensio... > File > chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc > (right): > > https://codereview.chromium.org/2838423003/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:482: > if (chromeos::ProfileHelper::Get()->IsSigninProfile(profile_)) { > Use the ProfileHelper::GetUserByProfile() method for consistency with what > you're doing in crrev.com/2846543002 for platform_keys::GetTokens()? Yes, this depend on http://crrev.com/2841553002. I could make it not to, but I don't know that it's worth the trouble.
https://codereview.chromium.org/2838423003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc (right): https://codereview.chromium.org/2838423003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:482: if (chromeos::ProfileHelper::Get()->IsSigninProfile(profile_)) { On 2017/04/27 18:10:28, emaxx wrote: > Use the ProfileHelper::GetUserByProfile() method for consistency with what > you're doing in crrev.com/2846543002 for platform_keys::GetTokens()? I can. I initially really wanted to limit this to the signin profile but "no user -> no user key challenge" seems reasonable.
The CQ bit was checked by drcrash@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by drcrash@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_...)
The CQ bit was checked by drcrash@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. And please add into the CL description that it affects the challengeUserKey method in the chrome.enterprise.platformKeys API.
The CQ bit was checked by drcrash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2838423003/#ps140001 (title: "Rebased")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2841553002 Patch 360001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by drcrash@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": 140001, "attempt_start_ts": 1493389142997310,
"parent_rev": "638b48e2f217dd8fe276e91906d7c3a6f6506eac", "commit_rev":
"2691676080f805e1b532f40ef3ef70dea8df8ac0"}
Message was sent while issue was closed.
Description was changed from ========== Ensure that challenging user keys is unavailable on signin. Returns a specific error saying the keys are not available in the signin profile. BUG=715121 TEST=unit tests ========== to ========== Ensure that challenging user keys is unavailable on signin. Returns a specific error saying the keys are not available in the signin profile. BUG=715121 TEST=unit tests Review-Url: https://codereview.chromium.org/2838423003 Cr-Commit-Position: refs/heads/master@{#468019} Committed: https://chromium.googlesource.com/chromium/src/+/2691676080f805e1b532f40ef3ef... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2691676080f805e1b532f40ef3ef... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
