Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2756443003:
arc: Update device owner check implementation (Closed)
|
Created:
3 years, 9 months ago by abhishekbh Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, cbentzel+watch_chromium.org, yusukes+watch_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, hidehiko+watch_chromium.org, sdefresne+watchlist_chromium.org, net-reviews_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, abhishekbh_chromium.org, Kevin Cernekee, Sameer Nanda Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Update device owner check implementation
Update the device owner check to compare the account id of the active
user with the account id of the owner as provided by the user manager
API. The old implementation uses the logged in user state API which for
unbeknownst reasons returns the owner as a regular user. It can also
change asynchronously at boot resulting in racy behavior as its used
right now.
BUG=b:34661187
BUG=b:33488433
TEST=run cts -c android.net.wifi.cts.WifiEnterpriseConfigTest -m
testAddEapNetwork
TEST=run cts -c android.net.wifi.cts.WifiManagerTest -m
testWifiManagerNetWork
Review-Url: https://codereview.chromium.org/2756443003
Cr-Commit-Position: refs/heads/master@{#457563}
Committed: https://chromium.googlesource.com/chromium/src/+/eb260ae77f464ec4b53627278b9e842cafb3490e
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update deps order, commit message and add comments #
Total comments: 3
Patch Set 3 : Remove chrome dependency #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== arc: Update device owner check implementation Update the device owner check to compare the account id of the active user with the account id of the owner as provided by the user manager API. The old implementation uses the logged in user state API which for unbeknownst reasons returns the owner as a regular user. It can also change asynchronously at boot resulting in racy behavior as its used right now. BUG=b:34661187 BUG=b:33488433 TEST=run cts -c android.net.wifi.cts.WifiEnterpriseConfigTest -m testAddEapNetwork TEST=run cts -c android.net.wifi.cts.WifiManagerTest -m testWifiManagerNetWork ========== to ========== arc: Update device owner check implementation Update the device owner check to compare the account id of the active user with the account id of the owner as provided by the user manager API. The old implementation uses the logged in user state API which for unbeknownst reasons returns the owner as a regular user. It can also change asynchronously at boot resulting in racy behavior as its used right now. BUG=b:34661187 BUG=b:33488433 TEST=run cts -c android.net.wifi.cts.WifiEnterpriseConfigTest -m testAddEapNetwork TEST=run cts -c android.net.wifi.cts.WifiManagerTest -m testWifiManagerNetWork ==========
abhishekbh@google.com changed reviewers: + cernekee@chromium.org, lhchavez@chromium.org, piman@chromium.org, rkc@chromium.org, xiyuan@chromium.org
https://codereview.chromium.org/2756443003/diff/1/components/arc/net/DEPS File components/arc/net/DEPS (right): https://codereview.chromium.org/2756443003/diff/1/components/arc/net/DEPS#new... components/arc/net/DEPS:5: "+chrome/browser/chromeos/login/users", Do these need to be alphabetized?
https://codereview.chromium.org/2756443003/diff/1/components/arc/net/DEPS File components/arc/net/DEPS (right): https://codereview.chromium.org/2756443003/diff/1/components/arc/net/DEPS#new... components/arc/net/DEPS:5: "+chrome/browser/chromeos/login/users", On 2017/03/16 04:15:27, Kevin Cernekee wrote: > Do these need to be alphabetized? Yes, I think we should alpha sort the list. My impression is that it is like how we sort #include, i.e. the list can be broken down into blocks and each block should be alpha sorted. https://codereview.chromium.org/2756443003/diff/1/components/arc/net/arc_net_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/2756443003/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:50: chromeos::ChromeUserManager::Get()->GetActiveUser()->GetAccountId(); I don't think this helps. GetOwnerAccountId() also changes based on ownership check, in the same as the logged-in user type, See [1] and [2]. We should get the same racy behavior. There is an OwnerSettingsService::IsOwnerAsync that we can use. The ownership check takes a few seconds after login (after loading the owner private key). Unfortunately, there is no synchronous way to check this. [1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro... [2] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro...
lgtm https://codereview.chromium.org/2756443003/diff/1/components/arc/net/arc_net_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/2756443003/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:50: chromeos::ChromeUserManager::Get()->GetActiveUser()->GetAccountId(); On 2017/03/16 05:13:21, xiyuan wrote: > I don't think this helps. GetOwnerAccountId() also changes based on ownership > check, in the same as the logged-in user type, See [1] and [2]. We should get > the same racy behavior. > > There is an OwnerSettingsService::IsOwnerAsync that we can use. The ownership > check takes a few seconds after login (after loading the owner private key). > Unfortunately, there is no synchronous way to check this. > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro... > [2] > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro... Per discussion with abhishekbh@, the LoginState code is a 100% failure. The owner state of that code seems broken. He will filed a bug and I will take a look. Meanwhile, I am okay switching to use GetOwnerAccountId().
On 2017/03/16 17:14:51, xiyuan wrote: > lgtm > > https://codereview.chromium.org/2756443003/diff/1/components/arc/net/arc_net_... > File components/arc/net/arc_net_host_impl.cc (right): > > https://codereview.chromium.org/2756443003/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.cc:50: > chromeos::ChromeUserManager::Get()->GetActiveUser()->GetAccountId(); > On 2017/03/16 05:13:21, xiyuan wrote: > > I don't think this helps. GetOwnerAccountId() also changes based on ownership > > check, in the same as the logged-in user type, See [1] and [2]. We should get > > the same racy behavior. > > > > There is an OwnerSettingsService::IsOwnerAsync that we can use. The ownership > > check takes a few seconds after login (after loading the owner private key). > > Unfortunately, there is no synchronous way to check this. > > > > [1] > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro... > > [2] > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro... > > Per discussion with abhishekbh@, the LoginState code is a 100% failure. The > owner state of that code seems broken. He will filed a bug and I will take a > look. > > Meanwhile, I am okay switching to use GetOwnerAccountId(). Oh, please alpha sort the DEPS file.
Yes. Will do. On Mar 16, 2017 10:15 AM, <xiyuan@chromium.org> wrote: > On 2017/03/16 17:14:51, xiyuan wrote: > > lgtm > > > > > https://codereview.chromium.org/2756443003/diff/1/ > components/arc/net/arc_net_host_impl.cc > > File components/arc/net/arc_net_host_impl.cc (right): > > > > > https://codereview.chromium.org/2756443003/diff/1/ > components/arc/net/arc_net_host_impl.cc#newcode50 > > components/arc/net/arc_net_host_impl.cc:50: > > chromeos::ChromeUserManager::Get()->GetActiveUser()->GetAccountId(); > > On 2017/03/16 05:13:21, xiyuan wrote: > > > I don't think this helps. GetOwnerAccountId() also changes based on > ownership > > > check, in the same as the logged-in user type, See [1] and [2]. We > should > get > > > the same racy behavior. > > > > > > There is an OwnerSettingsService::IsOwnerAsync that we can use. The > ownership > > > check takes a few seconds after login (after loading the owner private > key). > > > Unfortunately, there is no synchronous way to check this. > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ > chromeos/login/users/chrome_user_manager_impl.cc?rcl= > 0ea824c6614b1cc7f3e8b725fc138ba23518fb4b&l=460 > > > [2] > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ > chromeos/login/users/chrome_user_manager_impl.cc?rcl= > 0ea824c6614b1cc7f3e8b725fc138ba23518fb4b&l=683,700 > > > > Per discussion with abhishekbh@, the LoginState code is a 100% failure. > The > > owner state of that code seems broken. He will filed a bug and I will > take a > > look. > > > > Meanwhile, I am okay switching to use GetOwnerAccountId(). > > Oh, please alpha sort the DEPS file. > > https://codereview.chromium.org/2756443003/ > -- 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.
The CQ bit was checked by abhishekbh@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2756443003/#ps20001 (title: "Update deps order, commit message and add comments")
https://codereview.chromium.org/2756443003/diff/1/components/arc/net/DEPS File components/arc/net/DEPS (right): https://codereview.chromium.org/2756443003/diff/1/components/arc/net/DEPS#new... components/arc/net/DEPS:5: "+chrome/browser/chromeos/login/users", On 2017/03/16 05:13:21, xiyuan wrote: > On 2017/03/16 04:15:27, Kevin Cernekee wrote: > > Do these need to be alphabetized? > > Yes, I think we should alpha sort the list. My impression is that it is like how > we sort #include, i.e. the list can be broken down into blocks and each block > should be alpha sorted. Done.
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
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...) linux_chromium_chromeos_ozone_rel_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_...)
https://codereview.chromium.org/2756443003/diff/20001/components/arc/net/DEPS File components/arc/net/DEPS (right): https://codereview.chromium.org/2756443003/diff/20001/components/arc/net/DEPS... components/arc/net/DEPS:2: "+chrome/browser/chromeos/login/users", eek, this is not allowed D: (as indicated by the preupload failures) you'll have to create a Delegate that does the IsDeviceOwner() check and pass that from chrome/browser/chromeos/arc.
https://codereview.chromium.org/2756443003/diff/20001/components/arc/net/DEPS File components/arc/net/DEPS (right): https://codereview.chromium.org/2756443003/diff/20001/components/arc/net/DEPS... components/arc/net/DEPS:2: "+chrome/browser/chromeos/login/users", On 2017/03/16 19:23:38, Luis Héctor Chávez wrote: > eek, this is not allowed D: (as indicated by the preupload failures) > > you'll have to create a Delegate that does the IsDeviceOwner() check and pass > that from chrome/browser/chromeos/arc. Right, components could not depend on chrome. Missed that... Instead of using GetOwnerAccountId from ChromeUserManager, you should use the UserManager interface from components/user_manager.
https://codereview.chromium.org/2756443003/diff/20001/components/arc/net/DEPS File components/arc/net/DEPS (right): https://codereview.chromium.org/2756443003/diff/20001/components/arc/net/DEPS... components/arc/net/DEPS:2: "+chrome/browser/chromeos/login/users", On 2017/03/16 19:23:38, Luis Héctor Chávez wrote: > eek, this is not allowed D: (as indicated by the preupload failures) > > you'll have to create a Delegate that does the IsDeviceOwner() check and pass > that from chrome/browser/chromeos/arc. Done.
The CQ bit was checked by abhishekbh@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2756443003/#ps40001 (title: "Remove chrome dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489697866497820,
"parent_rev": "bc59a7a79efc1149ecb849cb6427d61045383298", "commit_rev":
"eb260ae77f464ec4b53627278b9e842cafb3490e"}
Message was sent while issue was closed.
Description was changed from ========== arc: Update device owner check implementation Update the device owner check to compare the account id of the active user with the account id of the owner as provided by the user manager API. The old implementation uses the logged in user state API which for unbeknownst reasons returns the owner as a regular user. It can also change asynchronously at boot resulting in racy behavior as its used right now. BUG=b:34661187 BUG=b:33488433 TEST=run cts -c android.net.wifi.cts.WifiEnterpriseConfigTest -m testAddEapNetwork TEST=run cts -c android.net.wifi.cts.WifiManagerTest -m testWifiManagerNetWork ========== to ========== arc: Update device owner check implementation Update the device owner check to compare the account id of the active user with the account id of the owner as provided by the user manager API. The old implementation uses the logged in user state API which for unbeknownst reasons returns the owner as a regular user. It can also change asynchronously at boot resulting in racy behavior as its used right now. BUG=b:34661187 BUG=b:33488433 TEST=run cts -c android.net.wifi.cts.WifiEnterpriseConfigTest -m testAddEapNetwork TEST=run cts -c android.net.wifi.cts.WifiManagerTest -m testWifiManagerNetWork Review-Url: https://codereview.chromium.org/2756443003 Cr-Commit-Position: refs/heads/master@{#457563} Committed: https://chromium.googlesource.com/chromium/src/+/eb260ae77f464ec4b53627278b9e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/eb260ae77f464ec4b53627278b9e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
