New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 794780 link

Starred by 3 users
Status: Verified
Owner:
x...@chromium.org
Closed: Jan 2018
Cc:
█ jingwee@chromium.org
wzang@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Device wallpaper policy regression: device controlled policy may leak into user's session

Project Member Reported by x...@chromium.org, Dec 14 2017 
Chrome Version: 65.0.3293.0

Repro steps:
1) Enroll a device into a domain and login a user without specifying device wallpaper policy
2) specify the policy in DMTest server (do not manually pull the policy down to the device)
3) Now sign out the user. It can be seen that the user's wallpaper was shown in the login screen and then later was replaced by the device policy controlled wallpaper
4) Login the user again

Expected behavior:
After login, the user's wallpaper should show

Actual behavior:
After login, the device wallpaper still remains

Comment 1 by x...@chromium.org, Dec 14 2017 

The reason is that the user's own wallpaper was shown at first and thus was saved to current_user_wallpaper_info_, but when the device wallpaper was later shown, current_user_wallpaper_info_ was not updated, thus in WallpaperManager::EnsureLoggedInUserWallpaperLoaded(), (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc?rcl=407c3fcf555e8df54e8236531847720a0c3cbbed&l=969), it early returned and didn't update its wallpaper

Comment 2 by wzang@chromium.org, Dec 14 2017 

I see. Can we remove the early return?

Comment 3 by x...@chromium.org, Dec 14 2017 

Remove the early return seems defeat the purpose of the current_user_wallpaper_info_? I was wondering why we didn't update current_user_wallpaper_info_ inside of SetWallpaperImage()? Seems it should be the common place to do so.

Comment 4 by wzang@chromium.org, Dec 14 2017 

Sure. (I just prefer to deprecate EnsureLoggedInUserWallpaperLoaded and replace them with ShowUserWallpaper.)
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 20 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e748d831f61d8eb7930ec594a4a937dbbcd8f3d3

commit e748d831f61d8eb7930ec594a4a937dbbcd8f3d3
Author: Xiaoqian Dai <xdai@chromium.org>
Date: Wed Dec 20 01:02:28 2017

Revert "wallpaper refactoring: update |current_user_wallpaper_info_| when setting wallpaper."

This reverts commit 0242859110e56377d3a47f734c802c39aef56478.

Reason for revert: WallPaperPrivateApiTest.WallpaperPrivateApiTest in browser_tests failing on chromium.memory/Linux ChromiumOS MSan Tests. 

Original change's description:
> wallpaper refactoring: update |current_user_wallpaper_info_| when setting wallpaper.
> 
> Bug:  794780 
> Change-Id: I1696b84bb4987ab5ccf5c91525a195b110befa70
> Reviewed-on: https://chromium-review.googlesource.com/829881
> Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
> Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525098}

TBR=xdai@chromium.org,wzang@chromium.org

Change-Id: I10afbe5d66f5adf0c2568dd6adacf9e662c6dffc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  794780 ,  796375 
Reviewed-on: https://chromium-review.googlesource.com/835474
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525206}
[modify] https://crrev.com/e748d831f61d8eb7930ec594a4a937dbbcd8f3d3/ash/wallpaper/wallpaper_controller.cc
[modify] https://crrev.com/e748d831f61d8eb7930ec594a4a937dbbcd8f3d3/ash/wallpaper/wallpaper_controller.h
[modify] https://crrev.com/e748d831f61d8eb7930ec594a4a937dbbcd8f3d3/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/e748d831f61d8eb7930ec594a4a937dbbcd8f3d3/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_policy_browsertest.cc
[modify] https://crrev.com/e748d831f61d8eb7930ec594a4a937dbbcd8f3d3/chrome/test/data/extensions/api_test/wallpaper_manager/test.js
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a12772b8a6897713831105b1e54b6c8e81989973

commit a12772b8a6897713831105b1e54b6c8e81989973
Author: xdai <xdai@chromium.org>
Date: Thu Dec 21 03:49:35 2017

Reland: wallpaper refactoring: update |current_user_wallpaper_info_| when setting wallpaper.

Bug:  794780 
Change-Id: Id481b2f3bdb2fba9557ac9a1fab75b57434bcba5
Reviewed-on: https://chromium-review.googlesource.com/829881
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525098}

TBR=wzang@chromium.org

Change-Id: Id481b2f3bdb2fba9557ac9a1fab75b57434bcba5
Reviewed-on: https://chromium-review.googlesource.com/838364
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525588}
[modify] https://crrev.com/a12772b8a6897713831105b1e54b6c8e81989973/ash/wallpaper/wallpaper_controller.cc
[modify] https://crrev.com/a12772b8a6897713831105b1e54b6c8e81989973/ash/wallpaper/wallpaper_controller.h
[modify] https://crrev.com/a12772b8a6897713831105b1e54b6c8e81989973/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/a12772b8a6897713831105b1e54b6c8e81989973/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_policy_browsertest.cc
[modify] https://crrev.com/a12772b8a6897713831105b1e54b6c8e81989973/chrome/test/data/extensions/api_test/wallpaper_manager/test.js

Comment 8 by wzang@chromium.org, Jan 5 2018 

On the ToT the device policy wallpaper for a public account still remains during the active session. Is it intended? (For a regular users it changes to the user wallpaper after logs in.)

Comment 9 by x...@chromium.org, Jan 5 2018 

Not intended.... I'll take a look.
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 17 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/312aafc0b3110e28da58db512517c056cadaa9ee

commit 312aafc0b3110e28da58db512517c056cadaa9ee
Author: Wenzhao Zang <wzang@chromium.org>
Date: Wed Jan 17 07:06:08 2018

cros: Change device policy wallpaper of public sessions after sign-in

1) Deprecate |EnsureLoggedInUserWallpaperLoaded|.

2) Deprecate |current_user_wallpaper_info_|. Create a ephemeral user
   map to replace it.

Bug:  794780 
Change-Id: I9b1aba63e492a3be5f27570365be4bfbba5c7045
Reviewed-on: https://chromium-review.googlesource.com/858218
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529659}
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/ash/public/interfaces/user_info.mojom
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/ash/session/test_session_controller_client.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/ash/wallpaper/wallpaper_controller.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/ash/wallpaper/wallpaper_controller.h
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/ash/wallpaper/wallpaper_controller_unittest.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/ash/wallpaper/wallpaper_view.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/chrome/browser/chromeos/extensions/wallpaper_manager_util.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_policy_browsertest.cc
[modify] https://crrev.com/312aafc0b3110e28da58db512517c056cadaa9ee/chrome/browser/ui/ash/session_controller_client.cc

Comment 12 by x...@chromium.org, Jan 17 2018

Status: Fixed (was: Untriaged)
Should have been fixed with #11 in place.

Comment 13 by krishna...@chromium.org, Jan 23 2018

Cc: jingwee@chromium.org

Comment 14 by jingwee@chromium.org, Jan 24 2018

Status: Verified (was: Fixed)
As verified in M65.0.3325.9 10323.1.0 dev candy, the issue was not reproducible.
►

Sign in to add a comment