Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1767443002:
Add enterprise enrollment support for fake users. (Closed)
|
Created:
4 years, 9 months ago by jdufault Modified:
4 years, 8 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd enterprise enrollment support for fake users.
This is one CL in a set of three:
- https://codereview.chromium.org/1767443002 (this one)
- https://codereview.chromium.org/1765483004
- https://chromium-review.googlesource.com/#/c/330272
BUG=586195
Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6
Cr-Commit-Position: refs/heads/master@{#387375}
Committed: https://crrev.com/56f9c7717b86f7a67b70257f3d61dd6c7187a7fc
Cr-Commit-Position: refs/heads/master@{#388259}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Split PolicyOAuth2TokenFetcher into impl/fake classes; fix SAML browsertests #
Total comments: 32
Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : Rebase #Patch Set 5 : Nits #
Total comments: 3
Patch Set 6 : Rebase #Patch Set 7 : Fix compile #Patch Set 8 : Rebase #Patch Set 9 : Rebase #Patch Set 10 : Check instance before derefing it #Messages
Total messages: 60 (27 generated)
Description was changed from ========== Add enterprise enrollment support for fake users. BUG=586195 ========== to ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 ==========
jdufault@chromium.org changed reviewers: + achuith@chromium.org
jdufault@chromium.org changed reviewers: + dzhioev@chromium.org
Achuith/Pavel, PTAL at the three CL's I've posted. This is the primary one. I will pull in more reviewers later. Another approach is to implement a fake EnterpriseEnrollmentHelper to avoid the calls to PolicyOAuth2TokenFetcher. The problem though is that UserCloudPolicyManagerChromeOS will still call into PolicyOAuth2TokenFetcher.
https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:34: bool PolicyOAuth2TokenFetcher::enable_fake_for_testing_ = false; do we use comment //static here? I think we do? https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:52: if (enable_fake_for_testing_) { Rather than pollute this class, is it possible to create a PolicyOAuth2TokenFetcherTest class or something, and create instances of that class instead of this one? https://codereview.chromium.org/1767443002/diff/1/chrome/browser/policy/test/... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/policy/test/... chrome/browser/policy/test/policy_testserver.py:600: response.device_attribute_update_permission_response.result =\ Can we use parens instead of \ for line break? https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/login_common.js:304: setTimeout(runner, 100); No better way than polling? Are there no events we can listen to instead?
https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:52: if (enable_fake_for_testing_) { On 2016/03/03 22:45:00, achuithb wrote: > Rather than pollute this class, is it possible to create a > PolicyOAuth2TokenFetcherTest class or something, and create instances of that > class instead of this one? Sure; I will have to make PolicyOAuth2TokenFetcher an interface though, and move all of the code from this file into PolicyOAuth2TokenFetcherImpl. The fake will then be PolicyOAuth2TokenFetcherFake. https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/login_common.js:304: setTimeout(runner, 100); On 2016/03/03 22:45:00, achuithb wrote: > No better way than polling? Are there no events we can listen to instead? There is OobeScreenWaiter on the C++ side but nothing I could find on the JS side. What do you think of firing an event here[1] so that it can be listened to with something with something like Oobe.addEventListener('onscreenchanged', function(screen) { ... }); Not 100% sure that will work since I haven't tested it, but I think it should. 1: https://code.google.com/p/chromium/codesearch#chromium/src/ui/login/display_m...
https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:52: if (enable_fake_for_testing_) { On 2016/03/03 23:11:04, jdufault wrote: > On 2016/03/03 22:45:00, achuithb wrote: > > Rather than pollute this class, is it possible to create a > > PolicyOAuth2TokenFetcherTest class or something, and create instances of that > > class instead of this one? > > Sure; I will have to make PolicyOAuth2TokenFetcher an interface though, and move > all of the code from this file into PolicyOAuth2TokenFetcherImpl. The fake will > then be PolicyOAuth2TokenFetcherFake. I think that would be cleaner? https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/login_common.js:304: setTimeout(runner, 100); On 2016/03/03 23:11:04, jdufault wrote: > On 2016/03/03 22:45:00, achuithb wrote: > > No better way than polling? Are there no events we can listen to instead? > > There is OobeScreenWaiter on the C++ side but nothing I could find on the JS > side. > > What do you think of firing an event here[1] so that it can be listened to with > something with something like > > Oobe.addEventListener('onscreenchanged', function(screen) { ... }); > > Not 100% sure that will work since I haven't tested it, but I think it should. > > 1: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/login/display_m... Let's do it. Maybe Pavel has a better idea?
https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:52: if (enable_fake_for_testing_) { On 2016/03/03 23:23:43, achuithb wrote: > On 2016/03/03 23:11:04, jdufault wrote: > > On 2016/03/03 22:45:00, achuithb wrote: > > > Rather than pollute this class, is it possible to create a > > > PolicyOAuth2TokenFetcherTest class or something, and create instances of > that > > > class instead of this one? > > > > Sure; I will have to make PolicyOAuth2TokenFetcher an interface though, and > move > > all of the code from this file into PolicyOAuth2TokenFetcherImpl. The fake > will > > then be PolicyOAuth2TokenFetcherFake. > > I think that would be cleaner? Or maybe this is too much work for not much gain. Maybe create a helper like ReturnFakeTokenForTesting and call that instead. Also, enable_fake_for_testing_ sounds incomplete. Maybe fake_authcode_for_testing_? Or a better name if you can think of one
I fixed the SAML tests by making them handle the attribute prompt screen. This required adding some additional API support to policy_testserver.py. Alternatively, I could have fixed the SAML tests by having policy_testserver.py disallow attribute prompt updates. https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:34: bool PolicyOAuth2TokenFetcher::enable_fake_for_testing_ = false; On 2016/03/03 22:45:00, achuithb wrote: > do we use comment //static here? I think we do? Done. https://codereview.chromium.org/1767443002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:52: if (enable_fake_for_testing_) { On 2016/03/04 19:12:37, achuithb wrote: > On 2016/03/03 23:23:43, achuithb wrote: > > On 2016/03/03 23:11:04, jdufault wrote: > > > On 2016/03/03 22:45:00, achuithb wrote: > > > > Rather than pollute this class, is it possible to create a > > > > PolicyOAuth2TokenFetcherTest class or something, and create instances of > > that > > > > class instead of this one? > > > > > > Sure; I will have to make PolicyOAuth2TokenFetcher an interface though, and > > move > > > all of the code from this file into PolicyOAuth2TokenFetcherImpl. The fake > > will > > > then be PolicyOAuth2TokenFetcherFake. > > > > I think that would be cleaner? > > Or maybe this is too much work for not much gain. > > Maybe create a helper like ReturnFakeTokenForTesting and call that instead. > > Also, enable_fake_for_testing_ sounds incomplete. Maybe > fake_authcode_for_testing_? Or a better name if you can think of one Done. Also moved the impl header file to the cc file so the h file is much cleaner now. https://codereview.chromium.org/1767443002/diff/1/chrome/browser/policy/test/... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/policy/test/... chrome/browser/policy/test/policy_testserver.py:600: response.device_attribute_update_permission_response.result =\ On 2016/03/03 22:45:00, achuithb wrote: > Can we use parens instead of \ for line break? Done. https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/login_common.js:304: setTimeout(runner, 100); On 2016/03/03 23:23:43, achuithb wrote: > On 2016/03/03 23:11:04, jdufault wrote: > > On 2016/03/03 22:45:00, achuithb wrote: > > > No better way than polling? Are there no events we can listen to instead? > > > > There is OobeScreenWaiter on the C++ side but nothing I could find on the JS > > side. > > > > What do you think of firing an event here[1] so that it can be listened to > with > > something with something like > > > > Oobe.addEventListener('onscreenchanged', function(screen) { ... }); > > > > Not 100% sure that will work since I haven't tested it, but I think it should. > > > > 1: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/login/display_m... > > Let's do it. Maybe Pavel has a better idea? Done.
https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:34: public base::SupportsWeakPtr<PolicyOAuth2TokenFetcherImpl>, switch to using WeakPtrFactory instead. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:39: ~PolicyOAuth2TokenFetcherImpl() override; protected ctor + dtor? https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:41: // Fetches the device management service's oauth2 token. This may be fetched don't need this comment, but you need // PolicyOauth2TokenFetcher overrides. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:43: void StartWithSigninContext( make the overriden methods private? https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:258: ~PolicyOAuth2TokenFetcherFake() override {} make ctor/dtor protected https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:260: void StartWithSigninContext( Make all the rest private https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:292: void ForwardPolicyToken(const TokenCallback& callback) { methods should precede data members, so this should move up in the private section https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:296: }; DISABLE_COPY_AND_ASSIGN https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:301: bool PolicyOAuth2TokenFetcher::use_fake_tokens_for_testing_ = false; move this to anon namespace? https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:41: typedef base::Callback<void(const std::string&, switch this to 'using' since you're here. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:64: virtual bool failed() const = 0; Shouldn't this be Failed()? Same for methods below? They are no longer inline https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:71: static bool use_fake_tokens_for_testing_; move this to anonymous namespace in cc? https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_common.js:299: enterpriseEnroll) { should we use a default? I'm not confident this won't break something somewhere. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_common.js:308: $('oobe').removeEventListener('screenchanged', handler); should the remove come before fn()?
https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:34: public base::SupportsWeakPtr<PolicyOAuth2TokenFetcherImpl>, On 2016/03/08 08:11:44, achuithb wrote: > switch to using WeakPtrFactory instead. Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:39: ~PolicyOAuth2TokenFetcherImpl() override; On 2016/03/08 08:11:44, achuithb wrote: > protected ctor + dtor? ctor needs to be public so the base class can instantiate the instance (alternatively, we can make PolicyOAuth2TokenFetcher a friend class). The dtor can be private, but I kept it public for consistency. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:41: // Fetches the device management service's oauth2 token. This may be fetched On 2016/03/08 08:11:44, achuithb wrote: > don't need this comment, but you need > // PolicyOauth2TokenFetcher overrides. Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:43: void StartWithSigninContext( On 2016/03/08 08:11:44, achuithb wrote: > make the overriden methods private? Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:258: ~PolicyOAuth2TokenFetcherFake() override {} On 2016/03/08 08:11:44, achuithb wrote: > make ctor/dtor protected See previous comment. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:260: void StartWithSigninContext( On 2016/03/08 08:11:44, achuithb wrote: > Make all the rest private Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:292: void ForwardPolicyToken(const TokenCallback& callback) { On 2016/03/08 08:11:44, achuithb wrote: > methods should precede data members, so this should move up in the private > section Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:296: }; On 2016/03/08 08:11:44, achuithb wrote: > DISABLE_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:301: bool PolicyOAuth2TokenFetcher::use_fake_tokens_for_testing_ = false; On 2016/03/08 08:11:44, achuithb wrote: > move this to anon namespace? Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:41: typedef base::Callback<void(const std::string&, On 2016/03/08 08:11:45, achuithb wrote: > switch this to 'using' since you're here. Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:64: virtual bool failed() const = 0; On 2016/03/08 08:11:45, achuithb wrote: > Shouldn't this be Failed()? Same for methods below? They are no longer inline Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:71: static bool use_fake_tokens_for_testing_; On 2016/03/08 08:11:44, achuithb wrote: > move this to anonymous namespace in cc? Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_common.js:299: enterpriseEnroll) { On 2016/03/08 08:11:45, achuithb wrote: > should we use a default? I'm not confident this won't break something somewhere. Done. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_common.js:308: $('oobe').removeEventListener('screenchanged', handler); On 2016/03/08 08:11:45, achuithb wrote: > should the remove come before fn()? Done, makes sense because fn() could invoke another screen change.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767443002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767443002/40001
lgtm. Please ask Pavel if he also wants to review this before you land this. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:39: ~PolicyOAuth2TokenFetcherImpl() override; On 2016/03/08 21:37:08, jdufault wrote: > On 2016/03/08 08:11:44, achuithb wrote: > > protected ctor + dtor? > > ctor needs to be public so the base class can instantiate the instance > (alternatively, we can make PolicyOAuth2TokenFetcher a friend class). The dtor > can be private, but I kept it public for consistency. Acknowledged. https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:258: ~PolicyOAuth2TokenFetcherFake() override {} On 2016/03/08 21:37:08, jdufault wrote: > On 2016/03/08 08:11:44, achuithb wrote: > > make ctor/dtor protected > > See previous comment. Acknowledged. https://codereview.chromium.org/1767443002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:298: std::string access_token_ = "fake_access_token"; make both const
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM with nit https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_common.js:333: chrome.send('completeLogin', ['12345', username, password, false]); Is '12345' supposed to be |gaia_id| here?
Patchset #4 (id:60001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/login_common.js (right): https://codereview.chromium.org/1767443002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_common.js:333: chrome.send('completeLogin', ['12345', username, password, false]); On 2016/03/09 00:19:28, dzhioev wrote: > Is '12345' supposed to be |gaia_id| here? Good catch. https://codereview.chromium.org/1767443002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right): https://codereview.chromium.org/1767443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:298: std::string access_token_ = "fake_access_token"; On 2016/03/08 22:13:26, achuithb wrote: > make both const Done.
jdufault@chromium.org changed reviewers: + atwilson@chromium.org, bartfab@chromium.org
Either atwilson@ or bartfab@, PTAL at policy_testserver.py changes. Thanks!
On 2016/03/18 17:41:35, jdufault wrote: > Either atwilson@ or bartfab@, PTAL at policy_testserver.py changes. Thanks! Ping
Changes mostly LG, but I have some concerns about how we're mocking out the PolicyOAuthFetcher. https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc (right): https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:95: oauth_fetcher_.reset(policy::PolicyOAuth2TokenFetcher::CreateInstance()); Why are we going with a static factory method + a static "put into test mode" API rather than just injecting a mock OAuth2TokenFetcher? That seems like a cleaner solution than having global state. https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h (right): https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:31: static PolicyOAuth2TokenFetcher* CreateInstance(); See my previous comment around using injection rather than static factory methods.
Changes mostly LG, but I have some concerns about how we're mocking out the PolicyOAuthFetcher. https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc (right): https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:95: oauth_fetcher_.reset(policy::PolicyOAuth2TokenFetcher::CreateInstance()); Why are we going with a static factory method + a static "put into test mode" API rather than just injecting a mock OAuth2TokenFetcher? That seems like a cleaner solution than having global state. https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h (right): https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:31: static PolicyOAuth2TokenFetcher* CreateInstance(); See my previous comment around using injection rather than static factory methods.
https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc (right): https://codereview.chromium.org/1767443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:95: oauth_fetcher_.reset(policy::PolicyOAuth2TokenFetcher::CreateInstance()); On 2016/03/23 12:37:13, Andrew T Wilson (Slow) wrote: > Why are we going with a static factory method + a static "put into test mode" > API rather than just injecting a mock OAuth2TokenFetcher? That seems like a > cleaner solution than having global state. I can see two ways to do injection: 1: policy::PolicyOAuth2TokenFetcher::SetAllocatorFn(/* fn that allocates a token fetcher */) 2: Call into EnterpriseEnrollmentHelperImpl, UserCloudPolicyManagerChromeos, and WildcardLoginChecker from EnrollmentScreenHandler and set the PolicyOAuth2TokenFetcher to a fake instance. With 1, we still have the global state. Given the way that this code is tested (autotest), the fake implementation has to live in a non-test folder. With 2, there are a myriad of lifetime issues to deal with and the code now has to support setting the PolicyOAuth2TokenFetcher multiple ways. For example, in EnterpriseEnrollmentHelperImpl we allocate a fresh PolicyOAuth2TokenFetcher instance after receiving a callback from it [1]. I think either the existing solution or solution #1 is fine. If you feel strongly #2 is the right way to go, we can discuss it more. 1: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
ping atwilson@
policy_testserver.py LGTM
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dzhioev@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/1767443002/#ps120001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767443002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767443002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, dzhioev@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/1767443002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767443002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767443002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, dzhioev@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/1767443002/#ps160001 (title: "Fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767443002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767443002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/resources/chromeos/login/login_common.js:
While running git apply --index -3 -p1;
error: chrome/browser/resources/chromeos/login/login_common.js: does not exist
in index
Patch: chrome/browser/resources/chromeos/login/login_common.js
Index: chrome/browser/resources/chromeos/login/login_common.js
diff --git a/chrome/browser/resources/chromeos/login/login_common.js
b/chrome/browser/resources/chromeos/login/login_common.js
index
ac76cfeb98099e54ad7600e2dc8f6aa9a34b6f47..ed1df926e8349210fa0b827e1f7f67d8a10b45c2
100644
--- a/chrome/browser/resources/chromeos/login/login_common.js
+++ b/chrome/browser/resources/chromeos/login/login_common.js
@@ -293,11 +293,40 @@ cr.define('cr.ui', function() {
* Login for telemetry.
* @param {string} username Login username.
* @param {string} password Login password.
- */
- Oobe.loginForTesting = function(username, password, gaia_id) {
+ * @param {boolean} enterpriseEnroll Login as an enterprise enrollment?
+ */
+ Oobe.loginForTesting = function(username, password, gaia_id,
+ enterpriseEnroll = false) {
+ // Helper method that runs |fn| after |screenName| is visible.
+ function waitForOobeScreen(screenName, fn) {
+ if (Oobe.getInstance().currentScreen.id === screenName) {
+ fn();
+ } else {
+ $('oobe').addEventListener('screenchanged', function handler(e) {
+ if (e.detail == screenName) {
+ $('oobe').removeEventListener('screenchanged', handler);
+ fn();
+ }
+ });
+ }
+ }
+
Oobe.disableSigninUI();
chrome.send('skipToLoginForTesting', [username]);
- chrome.send('completeLogin', [gaia_id, username, password, false]);
+
+ if (!enterpriseEnroll) {
+ chrome.send('completeLogin', [gaia_id, username, password, false]);
+ } else {
+ waitForOobeScreen('gaia-signin', function() {
+ chrome.send('toggleEnrollmentScreen');
+ chrome.send('toggleFakeEnrollment');
+ });
+
+ waitForOobeScreen('oauth-enrollment', function() {
+ chrome.send('oauthEnrollCompleteLogin', [username, 'authcode']);
+ chrome.send('completeLogin', [gaia_id, username, password, false]);
+ });
+ }
};
/**
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, dzhioev@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/1767443002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767443002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767443002/180001
Message was sent while issue was closed.
Description was changed from ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 ========== to ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 ========== to ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:180001) has been created in https://codereview.chromium.org/1889213002/ by jhorwich@chromium.org. The reason for reverting is: Chrome PFQ for ChromeOS broke. BUG=603697.
Message was sent while issue was closed.
Description was changed from ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375} ========== to ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375} ==========
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, dzhioev@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/1767443002/#ps220001 (title: "Check instance before derefing it")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767443002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767443002/220001
Message was sent while issue was closed.
Description was changed from ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375} ========== to ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375} ========== to ========== Add enterprise enrollment support for fake users. This is one CL in a set of three: - https://codereview.chromium.org/1767443002 (this one) - https://codereview.chromium.org/1765483004 - https://chromium-review.googlesource.com/#/c/330272 BUG=586195 Committed: https://crrev.com/562979ca9fc04c61c521123f9978c174dfbcbfc6 Cr-Commit-Position: refs/heads/master@{#387375} Committed: https://crrev.com/56f9c7717b86f7a67b70257f3d61dd6c7187a7fc Cr-Commit-Position: refs/heads/master@{#388259} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/56f9c7717b86f7a67b70257f3d61dd6c7187a7fc Cr-Commit-Position: refs/heads/master@{#388259} |
