Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902293002:
Introduce lock screen app manager (Closed)
|
Created:
3 years, 7 months ago by tbarzic Modified:
3 years, 5 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, alemate+watch_chromium.org, tfarina, achuith+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce lock screen app manager
Adds default lock screen app manager implementation.
The app manager, when started, checks if a lock screen enabled
note taking app is set, and if so, loads it to the profile used
to run apps on the lock screen. When stopped, app manager will
remove (and uninstall) the app from the profile.
The app is installed and loaded to the lock screen profile as follows:
* for unpacked apps - the app is loaded from the (original) unpacked
app location
* for others - the app resources are copied from the original, user
profile to the lock screen app profile's extension install
directory and loaded from the copied location
The app manager observes the note taking app state in the user
profile and updates lock screen app's profile accordingly.
BUG=715781
Review-Url: https://codereview.chromium.org/2902293002
Cr-Commit-Position: refs/heads/master@{#484815}
Committed: https://chromium.googlesource.com/chromium/src/+/c577bdc5b338dd58d27c8aeec32351263a47ad61
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 8
Patch Set 6 : . #Patch Set 7 : . #
Total comments: 20
Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : . #Patch Set 12 : . #Patch Set 13 : rebase #Patch Set 14 : . #
Total comments: 21
Patch Set 15 : . #Patch Set 16 : . #Patch Set 17 : . #Patch Set 18 : unpacked apps only #Patch Set 19 : rebase #Patch Set 20 : Copying non-unpacked apps #Patch Set 21 : . #Patch Set 22 : . #Patch Set 23 : . #Patch Set 24 : rebase #
Total comments: 14
Patch Set 25 : . #Patch Set 26 : . #
Total comments: 2
Patch Set 27 : fix comments #Patch Set 28 : rebase #
Messages
Total messages: 72 (29 generated)
tbarzic@chromium.org changed reviewers: + jdufault@chromium.org, rdevlin.cronin@chromium.org, xiyuan@chromium.org
jdufault - primarily for note_taking_helper xiyuan - chrome/browser/chromeos rdevlin.cronin - app_manager_impl.cc (as it adds a usage for RUNS_ON_LOCK_SCREEN flag I wanted to introduce in https://codereview.chromium.org/2892403002/)
https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:81: UnloadLockApp(lock_screen_app_id_); what if state_ == State::kNotInitialized? https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h (right): https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h:55: kIdle, kInitializing? Idle implies that this class actively does work over a (long) period of time. https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:158: if (!ash::palette_utils::HasStylusInput()) Why do we disable preferred app if there is no stylus? https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:162: if (LooksLikeAndroidPackageName(preferred_app_id)) Add a comment, // Lock screen does not currently support android.
https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:81: UnloadLockApp(lock_screen_app_id_); On 2017/05/26 19:15:52, jdufault wrote: > what if state_ == State::kNotInitialized? Done. https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h (right): https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h:55: kIdle, On 2017/05/26 19:15:53, jdufault wrote: > kInitializing? > > Idle implies that this class actively does work over a (long) period of time. Changed name to kInactive https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:158: if (!ash::palette_utils::HasStylusInput()) On 2017/05/26 19:15:53, jdufault wrote: > Why do we disable preferred app if there is no stylus? Yeah, I guess we don;t have to. https://codereview.chromium.org/2902293002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_helper.cc:162: if (LooksLikeAndroidPackageName(preferred_app_id)) On 2017/05/26 19:15:53, jdufault wrote: > Add a comment, > > // Lock screen does not currently support android. Added a comment, but at the place where this method is used. (returning null here matches the method name nicely, and I don't think it needs extra clarification)
https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager.h (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager.h:44: virtual void Start(Observer* observer) = 0; Prefer to use a Callback than using an interface pointer unless we plan to add more stuff to the interface. The benefit of using a Callback is that we can decouple the lifetime of an observer with AppManager by using WeakPtr when using Bind to create the Callback etc. If we want to use Observer interface, we probably should have a way for an Observer to remove itself from AppManager (e.g. provide an ObserverDetroyed() in the interface, or passing WeakPtr instead of raw pointer) or ensure it stays alive (e.g. scoped_refptr). Or at least, document the lifetime expectation. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h:24: namespace lock_screen_apps { nit: insert an empty line https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc:257: true); nit: The above lines are quite similar in the test cases before this one. Create a helper to reduce dup code? https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:68: launch_count_++; nit: ++launch_count_; https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:107: Profile* expected_lock_screen_profile_; nit: Profile* const for the two above https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:158: std::string preferred_app_id = nit: const std::string https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:162: const extensions::Extension* preferred_app = nit: insert an empty line before https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:167: if (!IsWhitelistedChromeApp(preferred_app) && nit: insert an empty line before https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:172: NoteTakingLockScreenSupport lock_screen_support = nit: insert an empty line before https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:182: std::unique_ptr<NoteTakingAppInfo> info = nit: insert an empty line before
https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager.h (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager.h:44: virtual void Start(Observer* observer) = 0; On 2017/05/26 22:44:52, xiyuan wrote: > Prefer to use a Callback than using an interface pointer unless we plan to add > more stuff to the interface. The benefit of using a Callback is that we can > decouple the lifetime of an observer with AppManager by using WeakPtr when using > Bind to create the Callback etc. > > If we want to use Observer interface, we probably should have a way for an > Observer to remove itself from AppManager (e.g. provide an ObserverDetroyed() in > the interface, or passing WeakPtr instead of raw pointer) or ensure it stays > alive (e.g. scoped_refptr). > > Or at least, document the lifetime expectation. Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h:24: namespace lock_screen_apps { On 2017/05/26 22:44:52, xiyuan wrote: > nit: insert an empty line Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc:257: true); On 2017/05/26 22:44:52, xiyuan wrote: > nit: The above lines are quite similar in the test cases before this one. Create > a helper to reduce dup code? Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:68: launch_count_++; On 2017/05/26 22:44:52, xiyuan wrote: > nit: ++launch_count_; Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc:107: Profile* expected_lock_screen_profile_; On 2017/05/26 22:44:52, xiyuan wrote: > nit: Profile* const for the two above Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:158: std::string preferred_app_id = On 2017/05/26 22:44:52, xiyuan wrote: > nit: const std::string Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:162: const extensions::Extension* preferred_app = On 2017/05/26 22:44:52, xiyuan wrote: > nit: insert an empty line before Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:167: if (!IsWhitelistedChromeApp(preferred_app) && On 2017/05/26 22:44:52, xiyuan wrote: > nit: insert an empty line before Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:172: NoteTakingLockScreenSupport lock_screen_support = On 2017/05/26 22:44:52, xiyuan wrote: > nit: insert an empty line before Done. https://codereview.chromium.org/2902293002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:182: std::unique_ptr<NoteTakingAppInfo> info = On 2017/05/26 22:44:53, xiyuan wrote: > nit: insert an empty line before Done.
(focusing on https://codereview.chromium.org/2892403002 before this one)
lgtm
The CQ bit was checked by tbarzic@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tbarzic@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.
https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:39: DCHECK_NE(primary_profile, lock_screen_profile); Is there any defined relation between primary and lock screen profile? e.g., would calling lockscreen_profile->GetOriginalProfile() return primary profile? https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:81: extensions_observer_.RemoveAll(); Is the ExtensionRegistry guaranteed to outlive this object? https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:105: Profile* target_profile = lock_screen_profile_->GetOriginalProfile(); can we somewhere explain target, lockscreen, and primary profile distinctions? https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:188: ExtensionService::UninstallExtensionHelper( nit: prefer just using ExtensionService::UninstallExtension(). https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying and installing the app to the target profile. Yeah, this strikes me as very strange. What happens when one of these profiles tries to write to the extension directory? Does the lock screen profile read access to all the files under the user profile? https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:211: void AppManagerImpl::UnloadLockApp(const std::string& app_id) { s/Lock/LockScreen? https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:222: ExtensionService::UninstallExtensionHelper( Do we unload it, or uninstall it? The two are not synonymous in the extension system. https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:222: ExtensionService::UninstallExtensionHelper( This is pretty strange in combination with using the different path above. We delete the extension files when we uninstall the extension. By virtue of implementation details, I think this happens to work, but we really shouldn't be relying on that. https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:66: app_manager_ = std::move(app_manager); nit: DCHECK(!app_manager_)?
https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:39: DCHECK_NE(primary_profile, lock_screen_profile); On 2017/06/05 16:09:35, Devlin wrote: > Is there any defined relation between primary and lock screen profile? e.g., > would calling lockscreen_profile->GetOriginalProfile() return primary profile? Not really. GetOriginalProfile would return the sign-in profile that is OTR (which is the case for sign-in profile). Though, we don't really need OTR sign-in profile here - all usages will get redirected to the original profile (either using ExtensionSystem/ExtensionRegistry service getters, or explicitly in LaunchNoteTaking) - I think I can just pass in the orignial profile as lock_screen_profile in |Initialize|. https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:81: extensions_observer_.RemoveAll(); On 2017/06/05 16:09:35, Devlin wrote: > Is the ExtensionRegistry guaranteed to outlive this object? yes, this is owned by StateController, which is destroyed before profiles (in ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun before "shared" PostMainMessageLoopRun, which starts process tear down, part of which is destroying profiles). https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:105: Profile* target_profile = lock_screen_profile_->GetOriginalProfile(); On 2017/06/05 16:09:35, Devlin wrote: > can we somewhere explain target, lockscreen, and primary profile distinctions? Documentation for AppManager::Initialize has some explanations for lock screen and primary profile. I removed target profile (which was just non-OTR lock_screen_profile) https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:188: ExtensionService::UninstallExtensionHelper( On 2017/06/05 16:09:35, Devlin wrote: > nit: prefer just using ExtensionService::UninstallExtension(). Done. https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying and installing the app to the target profile. On 2017/06/05 16:09:35, Devlin wrote: > Yeah, this strikes me as very strange. What happens when one of these profiles > tries to write to the extension directory? Does the lock screen profile read > access to all the files under the user profile? Yeah, I have not yet completely figured out the best approach here :/ Lock screen profile should be able to load resources from user profile location (provided that the user cryptohome remains decrypted; this might change in future, but I don't see it happening soon) - after all the resources reading is done by the same process. As writing to the extension directory goes, that should generally happen on extension installation/update, right? I'd expect that an extension directory remains constant while the extension is loaded, and that the resource changes become visible to the app after reload - the similar thing would happen in lock screen profile (when user profile updates an extension) - loading/unloading of the user profile app should cause the app load/unload in the lock screen profile. And lock screen profile should not really write to paths under the user profile (we probably should also ensure that the extension updater is not started for lock screen profile, so there are no unexpected updates - though, looking at code, these would seem to end up in the lock screen profile). https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:211: void AppManagerImpl::UnloadLockApp(const std::string& app_id) { On 2017/06/05 16:09:35, Devlin wrote: > s/Lock/LockScreen? Done. https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:222: ExtensionService::UninstallExtensionHelper( On 2017/06/05 16:09:34, Devlin wrote: > Do we unload it, or uninstall it? The two are not synonymous in the extension > system. Closer to uninstall (though, the initial version of the cl had unloading - I changed that so the app data is cleared between session locks). Yeah, this happens to work due to implementation in chrome_assets_manager - I do plan to make this check more explicit there when I introduce designated lock screen app profile (provided the way the app is added to lock screen profile does not change - otherwise this would become a moot point). Or maybe I can just add an option to ExtensionService::UninstallExtension to skip deleting extension files. https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/state_controller.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/state_controller.cc:66: app_manager_ = std::move(app_manager); On 2017/06/05 16:09:35, Devlin wrote: > nit: DCHECK(!app_manager_)? Done.
ping. Can I please get some traction on this?
This looks fine except for the weirdness in installation/uninstallation. https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying and installing the app to the target profile. On 2017/06/06 01:54:55, tbarzic wrote: > On 2017/06/05 16:09:35, Devlin wrote: > > Yeah, this strikes me as very strange. What happens when one of these > profiles > > tries to write to the extension directory? Does the lock screen profile read > > access to all the files under the user profile? > > Yeah, I have not yet completely figured out the best approach here :/ > > Lock screen profile should be able to load resources from user profile location > (provided that the user cryptohome remains decrypted; this might change in > future, but I don't see it happening soon) - after all the resources reading is > done by the same process. > > As writing to the extension directory goes, that should generally happen on > extension installation/update, right? > I'd expect that an extension directory remains constant while the extension is > loaded, and that the resource changes become visible to the app after reload - > the similar thing would happen in lock screen profile (when user profile updates > an extension) - loading/unloading of the user profile app should cause the app > load/unload in the lock screen profile. > > And lock screen profile should not really write to paths under the user profile > (we probably should also ensure that the extension updater is not started for > lock screen profile, so there are no unexpected updates - though, looking at > code, these would seem to end up in the lock screen profile). Out of curiosity, is there a reason to not write the extension to the lockscreen profile, thus avoiding this issue and the uninstall/delete file problem?
https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying and installing the app to the target profile. On 2017/06/09 18:18:40, Devlin (sheriff Jun 9 - 12) wrote: > On 2017/06/06 01:54:55, tbarzic wrote: > > On 2017/06/05 16:09:35, Devlin wrote: > > > Yeah, this strikes me as very strange. What happens when one of these > > profiles > > > tries to write to the extension directory? Does the lock screen profile > read > > > access to all the files under the user profile? > > > > Yeah, I have not yet completely figured out the best approach here :/ > > > > Lock screen profile should be able to load resources from user profile > location > > (provided that the user cryptohome remains decrypted; this might change in > > future, but I don't see it happening soon) - after all the resources reading > is > > done by the same process. > > > > As writing to the extension directory goes, that should generally happen on > > extension installation/update, right? > > I'd expect that an extension directory remains constant while the extension is > > loaded, and that the resource changes become visible to the app after reload - > > the similar thing would happen in lock screen profile (when user profile > updates > > an extension) - loading/unloading of the user profile app should cause the app > > load/unload in the lock screen profile. > > > > And lock screen profile should not really write to paths under the user > profile > > (we probably should also ensure that the extension updater is not started for > > lock screen profile, so there are no unexpected updates - though, looking at > > code, these would seem to end up in the lock screen profile). > > Out of curiosity, is there a reason to not write the extension to the lockscreen > profile, thus avoiding this issue and the uninstall/delete file problem? One reason is time - I do plan to look more closely into possibility of copying extension to the lock screen profile, but I think getting the lock screen APIs working and possibly unbounding lock screen profile from sign-in profile have somewhat higher priority at this moment. One potential concern is that we'd end up copying data from (encrypted) user profile to (unencrypted) lock screen profile, though I expect this being relevant mainly for unpacked extensions; then there's the question of if the memory cost of duplicating the extension dir and time cost of copying data is justified; handling app updates (instead of just reloading the app in lock screen profile, we'd have to reload and copy app directory).. I don't think there's anything huge, but it's not that clear-cut to me that introducing a new installer (from a location within another profile) would be beneficial.
rdevlin.cronin@chromium.org changed reviewers: + kerrnel@chromium.org
https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying and installing the app to the target profile. On 2017/06/09 19:01:45, tbarzic wrote: > On 2017/06/09 18:18:40, Devlin (sheriff Jun 9 - 12) wrote: > > On 2017/06/06 01:54:55, tbarzic wrote: > > > On 2017/06/05 16:09:35, Devlin wrote: > > > > Yeah, this strikes me as very strange. What happens when one of these > > > profiles > > > > tries to write to the extension directory? Does the lock screen profile > > read > > > > access to all the files under the user profile? > > > > > > Yeah, I have not yet completely figured out the best approach here :/ > > > > > > Lock screen profile should be able to load resources from user profile > > location > > > (provided that the user cryptohome remains decrypted; this might change in > > > future, but I don't see it happening soon) - after all the resources reading > > is > > > done by the same process. > > > > > > As writing to the extension directory goes, that should generally happen on > > > extension installation/update, right? > > > I'd expect that an extension directory remains constant while the extension > is > > > loaded, and that the resource changes become visible to the app after reload > - > > > the similar thing would happen in lock screen profile (when user profile > > updates > > > an extension) - loading/unloading of the user profile app should cause the > app > > > load/unload in the lock screen profile. > > > > > > And lock screen profile should not really write to paths under the user > > profile > > > (we probably should also ensure that the extension updater is not started > for > > > lock screen profile, so there are no unexpected updates - though, looking at > > > code, these would seem to end up in the lock screen profile). > > > > Out of curiosity, is there a reason to not write the extension to the > lockscreen > > profile, thus avoiding this issue and the uninstall/delete file problem? > > One reason is time - I do plan to look more closely into possibility of copying > extension to the lock screen profile, but I think getting the lock screen APIs > working and possibly unbounding lock screen profile from sign-in profile have > somewhat higher priority at this moment. > > One potential concern is that we'd end up copying data from (encrypted) user > profile to (unencrypted) lock screen profile, though I expect this being > relevant mainly for unpacked extensions; then there's the question of if the > memory cost of duplicating the extension dir and time cost of copying data is > justified; handling app updates (instead of just reloading the app in lock > screen profile, we'd have to reload and copy app directory).. > I don't think there's anything huge, but it's not that clear-cut to me that > introducing a new installer (from a location within another profile) would be > beneficial. I talked with kerrnel@ about this, and we both agreed that it makes us a little uncomfortable from a security standpoint to have the lockscreen profile require read access to the user's profile. I think we really should strive to have the lockscreen profile version separate from the other, which also alleviates any concerns around the lockscreen profile interfering with the user's profile.
tbarzic@chromium.org changed reviewers: + rkc@chromium.org
This doesn't really give lock screen profile access to the user profile - lock screen profile can already read user profile dir (user profile is not encrypted while the user is logged in, so the user profile directory could be read from any other profile). TBH, I don't think this is conceptually that much different from loading an extension from a shared location (which in this case may be inside a user profile directory). I will look into changing extension loading logic to create a separate extension dir from which to load the app, but, honestly, I'd prefer to focus on getting required APIs working first, so the Keep team can start iterating on their end (which is blocked on loading the app to the lock screen profile).
On 2017/06/12 22:11:49, tbarzic wrote: > This doesn't really give lock screen profile access to the user profile - lock > screen profile can already read user profile dir (user profile is not encrypted > while the user is logged in, so the user profile directory could be read from > any other profile). TBH, I don't think this is conceptually that much different > from loading an extension from a shared location (which in this case may be > inside a user profile directory). > > I will look into changing extension loading logic to create a separate extension > dir from which to load the app, but, honestly, I'd prefer to focus on getting > required APIs working first, so the Keep team can start iterating on their end > (which is blocked on loading the app to the lock screen profile). I'll defer to kerrnel@ here whether this should be strict requirement from a security stand point. For predictability and consistency, it's my strong preference (otherwise we may have to audit uses of Extension::path() to see whether any are going to use that path to modify the extension contents). I realize that this is more work in the short term, but it's a significantly better state to be in IMO.
https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying and installing the app to the target profile. On 2017/06/12 18:31:45, Devlin wrote: > On 2017/06/09 19:01:45, tbarzic wrote: > > On 2017/06/09 18:18:40, Devlin (sheriff Jun 9 - 12) wrote: > > > On 2017/06/06 01:54:55, tbarzic wrote: > > > > On 2017/06/05 16:09:35, Devlin wrote: > > > > > Yeah, this strikes me as very strange. What happens when one of these > > > > profiles > > > > > tries to write to the extension directory? Does the lock screen profile > > > read > > > > > access to all the files under the user profile? > > > > > > > > Yeah, I have not yet completely figured out the best approach here :/ > > > > > > > > Lock screen profile should be able to load resources from user profile > > > location > > > > (provided that the user cryptohome remains decrypted; this might change in > > > > future, but I don't see it happening soon) - after all the resources > reading > > > is > > > > done by the same process. > > > > > > > > As writing to the extension directory goes, that should generally happen > on > > > > extension installation/update, right? > > > > I'd expect that an extension directory remains constant while the > extension > > is > > > > loaded, and that the resource changes become visible to the app after > reload > > - > > > > the similar thing would happen in lock screen profile (when user profile > > > updates > > > > an extension) - loading/unloading of the user profile app should cause the > > app > > > > load/unload in the lock screen profile. > > > > > > > > And lock screen profile should not really write to paths under the user > > > profile > > > > (we probably should also ensure that the extension updater is not started > > for > > > > lock screen profile, so there are no unexpected updates - though, looking > at > > > > code, these would seem to end up in the lock screen profile). > > > > > > Out of curiosity, is there a reason to not write the extension to the > > lockscreen > > > profile, thus avoiding this issue and the uninstall/delete file problem? > > > > One reason is time - I do plan to look more closely into possibility of > copying > > extension to the lock screen profile, but I think getting the lock screen APIs > > working and possibly unbounding lock screen profile from sign-in profile have > > somewhat higher priority at this moment. > > > > One potential concern is that we'd end up copying data from (encrypted) user > > profile to (unencrypted) lock screen profile, though I expect this being > > relevant mainly for unpacked extensions; then there's the question of if the > > memory cost of duplicating the extension dir and time cost of copying data is > > justified; handling app updates (instead of just reloading the app in lock > > screen profile, we'd have to reload and copy app directory).. > > I don't think there's anything huge, but it's not that clear-cut to me that > > introducing a new installer (from a location within another profile) would be > > beneficial. > > I talked with kerrnel@ about this, and we both agreed that it makes us a little > uncomfortable from a security standpoint to have the lockscreen profile require > read access to the user's profile. I think we really should strive to have the > lockscreen profile version separate from the other, which also alleviates any > concerns around the lockscreen profile interfering with the user's profile. We've had this discussion in many different forms now. This code runs in a process that *has* access to the user profile. We are depending on the context keying of code to prevent access to the logged in user's profile from the lockscreen handling code. That being said, making one explicit call to read data from the user's profile, does not equate to "giving the lock screen access to the user profile". Again, the lockscreen code *already* has that access. I do want us to take any and all measures we can to ensure that we mitigate any possible security issues - I just don't see removing this call does that. We need to load the app. The app is in the user profile. Whether we have code that copies the app out first, or code that directly loads the app - we *will* be reading this one particular piece of data from the user profile. I would argue that given that this call is a well defined call in very mature code, it would be safer to use it than to introduce new code to do the copy.
Description was changed from ========== Introduce lock screen app manager The manager is responsible for loading/unloading the lock screen note taking app (if one is selected) to/from the profile used for lock screen apps. StateController starts it when the user session is locked and stops it when user session is unlocked (at which moment the action handler app is unloaded from sign-in profile). StateController uses LockAppManager to determine whether a lock screen enabled note taking app is set up, and to dispatch launch event to the app in question. BUG=715781 ========== to ========== Introduce lock screen app manager The manager is responsible for loading/unloading the lock screen note taking app (if one is selected) to/from the profile used for lock screen apps. StateController starts it when the user session is locked and stops it when user session is unlocked (at which moment the action handler app is unloaded from sign-in profile). StateController uses LockAppManager to determine whether a lock screen enabled note taking app is set up, and to dispatch launch event to the app in question. BUG=715781 ==========
kerrnel@chromium.org changed reviewers: + jorgelo@chromium.org
On 2017/06/13 21:18:17, rkc wrote: > https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... > File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): > > https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... > chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying and > installing the app to the target profile. > On 2017/06/12 18:31:45, Devlin wrote: > > On 2017/06/09 19:01:45, tbarzic wrote: > > > On 2017/06/09 18:18:40, Devlin (sheriff Jun 9 - 12) wrote: > > > > On 2017/06/06 01:54:55, tbarzic wrote: > > > > > On 2017/06/05 16:09:35, Devlin wrote: > > > > > > Yeah, this strikes me as very strange. What happens when one of these > > > > > profiles > > > > > > tries to write to the extension directory? Does the lock screen > profile > > > > read > > > > > > access to all the files under the user profile? > > > > > > > > > > Yeah, I have not yet completely figured out the best approach here :/ > > > > > > > > > > Lock screen profile should be able to load resources from user profile > > > > location > > > > > (provided that the user cryptohome remains decrypted; this might change > in > > > > > future, but I don't see it happening soon) - after all the resources > > reading > > > > is > > > > > done by the same process. > > > > > > > > > > As writing to the extension directory goes, that should generally happen > > on > > > > > extension installation/update, right? > > > > > I'd expect that an extension directory remains constant while the > > extension > > > is > > > > > loaded, and that the resource changes become visible to the app after > > reload > > > - > > > > > the similar thing would happen in lock screen profile (when user profile > > > > updates > > > > > an extension) - loading/unloading of the user profile app should cause > the > > > app > > > > > load/unload in the lock screen profile. > > > > > > > > > > And lock screen profile should not really write to paths under the user > > > > profile > > > > > (we probably should also ensure that the extension updater is not > started > > > for > > > > > lock screen profile, so there are no unexpected updates - though, > looking > > at > > > > > code, these would seem to end up in the lock screen profile). > > > > > > > > Out of curiosity, is there a reason to not write the extension to the > > > lockscreen > > > > profile, thus avoiding this issue and the uninstall/delete file problem? > > > > > > One reason is time - I do plan to look more closely into possibility of > > copying > > > extension to the lock screen profile, but I think getting the lock screen > APIs > > > working and possibly unbounding lock screen profile from sign-in profile > have > > > somewhat higher priority at this moment. > > > > > > One potential concern is that we'd end up copying data from (encrypted) user > > > profile to (unencrypted) lock screen profile, though I expect this being > > > relevant mainly for unpacked extensions; then there's the question of if the > > > memory cost of duplicating the extension dir and time cost of copying data > is > > > justified; handling app updates (instead of just reloading the app in lock > > > screen profile, we'd have to reload and copy app directory).. > > > I don't think there's anything huge, but it's not that clear-cut to me that > > > introducing a new installer (from a location within another profile) would > be > > > beneficial. > > > > I talked with kerrnel@ about this, and we both agreed that it makes us a > little > > uncomfortable from a security standpoint to have the lockscreen profile > require > > read access to the user's profile. I think we really should strive to have > the > > lockscreen profile version separate from the other, which also alleviates any > > concerns around the lockscreen profile interfering with the user's profile. > > We've had this discussion in many different forms now. This code runs in a > process that *has* access to the user profile. We are depending on the context > keying of code to prevent access to the logged in user's profile from the > lockscreen handling code. > > That being said, making one explicit call to read data from the user's profile, > does not equate to "giving the lock screen access to the user profile". Again, > the lockscreen code *already* has that access. > > I do want us to take any and all measures we can to ensure that we mitigate any > possible security issues - I just don't see removing this call does that. > > We need to load the app. The app is in the user profile. Whether we have code > that copies the app out first, or code that directly loads the app - we *will* > be reading this one particular piece of data from the user profile. > > I would argue that given that this call is a well defined call in very mature > code, it would be safer to use it than to introduce new code to do the copy. I'm a bit distracted by other issues right now, so I'll defer to Jorge on this. - Greg
On 2017/06/13 22:25:57, Greg K wrote: > On 2017/06/13 21:18:17, rkc wrote: > > > https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... > > File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): > > > > > https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... > > chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying > and > > installing the app to the target profile. > > On 2017/06/12 18:31:45, Devlin wrote: > > > On 2017/06/09 19:01:45, tbarzic wrote: > > > > On 2017/06/09 18:18:40, Devlin (sheriff Jun 9 - 12) wrote: > > > > > On 2017/06/06 01:54:55, tbarzic wrote: > > > > > > On 2017/06/05 16:09:35, Devlin wrote: > > > > > > > Yeah, this strikes me as very strange. What happens when one of > these > > > > > > profiles > > > > > > > tries to write to the extension directory? Does the lock screen > > profile > > > > > read > > > > > > > access to all the files under the user profile? > > > > > > > > > > > > Yeah, I have not yet completely figured out the best approach here :/ > > > > > > > > > > > > Lock screen profile should be able to load resources from user profile > > > > > location > > > > > > (provided that the user cryptohome remains decrypted; this might > change > > in > > > > > > future, but I don't see it happening soon) - after all the resources > > > reading > > > > > is > > > > > > done by the same process. > > > > > > > > > > > > As writing to the extension directory goes, that should generally > happen > > > on > > > > > > extension installation/update, right? > > > > > > I'd expect that an extension directory remains constant while the > > > extension > > > > is > > > > > > loaded, and that the resource changes become visible to the app after > > > reload > > > > - > > > > > > the similar thing would happen in lock screen profile (when user > profile > > > > > updates > > > > > > an extension) - loading/unloading of the user profile app should cause > > the > > > > app > > > > > > load/unload in the lock screen profile. > > > > > > > > > > > > And lock screen profile should not really write to paths under the > user > > > > > profile > > > > > > (we probably should also ensure that the extension updater is not > > started > > > > for > > > > > > lock screen profile, so there are no unexpected updates - though, > > looking > > > at > > > > > > code, these would seem to end up in the lock screen profile). > > > > > > > > > > Out of curiosity, is there a reason to not write the extension to the > > > > lockscreen > > > > > profile, thus avoiding this issue and the uninstall/delete file problem? > > > > > > > > One reason is time - I do plan to look more closely into possibility of > > > copying > > > > extension to the lock screen profile, but I think getting the lock screen > > APIs > > > > working and possibly unbounding lock screen profile from sign-in profile > > have > > > > somewhat higher priority at this moment. > > > > > > > > One potential concern is that we'd end up copying data from (encrypted) > user > > > > profile to (unencrypted) lock screen profile, though I expect this being > > > > relevant mainly for unpacked extensions; then there's the question of if > the > > > > memory cost of duplicating the extension dir and time cost of copying data > > is > > > > justified; handling app updates (instead of just reloading the app in lock > > > > screen profile, we'd have to reload and copy app directory).. > > > > I don't think there's anything huge, but it's not that clear-cut to me > that > > > > introducing a new installer (from a location within another profile) would > > be > > > > beneficial. > > > > > > I talked with kerrnel@ about this, and we both agreed that it makes us a > > little > > > uncomfortable from a security standpoint to have the lockscreen profile > > require > > > read access to the user's profile. I think we really should strive to have > > the > > > lockscreen profile version separate from the other, which also alleviates > any > > > concerns around the lockscreen profile interfering with the user's profile. > > > > We've had this discussion in many different forms now. This code runs in a > > process that *has* access to the user profile. We are depending on the context > > keying of code to prevent access to the logged in user's profile from the > > lockscreen handling code. > > > > That being said, making one explicit call to read data from the user's > profile, > > does not equate to "giving the lock screen access to the user profile". Again, > > the lockscreen code *already* has that access. > > > > I do want us to take any and all measures we can to ensure that we mitigate > any > > possible security issues - I just don't see removing this call does that. > > > > We need to load the app. The app is in the user profile. Whether we have code > > that copies the app out first, or code that directly loads the app - we *will* > > be reading this one particular piece of data from the user profile. > > > > I would argue that given that this call is a well defined call in very mature > > code, it would be safer to use it than to introduce new code to do the copy. > > I'm a bit distracted by other issues right now, so I'll defer to Jorge on this. > > - Greg By the way, is there a reason not to just install the CRX into the lock screen profile? - Greg
On 2017/06/14 20:18:02, Greg K wrote: > On 2017/06/13 22:25:57, Greg K wrote: > > On 2017/06/13 21:18:17, rkc wrote: > > > > > > https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... > > > File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2902293002/diff/260001/chrome/browser/chromeo... > > > chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:194: // copying > > and > > > installing the app to the target profile. > > > On 2017/06/12 18:31:45, Devlin wrote: > > > > On 2017/06/09 19:01:45, tbarzic wrote: > > > > > On 2017/06/09 18:18:40, Devlin (sheriff Jun 9 - 12) wrote: > > > > > > On 2017/06/06 01:54:55, tbarzic wrote: > > > > > > > On 2017/06/05 16:09:35, Devlin wrote: > > > > > > > > Yeah, this strikes me as very strange. What happens when one of > > these > > > > > > > profiles > > > > > > > > tries to write to the extension directory? Does the lock screen > > > profile > > > > > > read > > > > > > > > access to all the files under the user profile? > > > > > > > > > > > > > > Yeah, I have not yet completely figured out the best approach here > :/ > > > > > > > > > > > > > > Lock screen profile should be able to load resources from user > profile > > > > > > location > > > > > > > (provided that the user cryptohome remains decrypted; this might > > change > > > in > > > > > > > future, but I don't see it happening soon) - after all the resources > > > > reading > > > > > > is > > > > > > > done by the same process. > > > > > > > > > > > > > > As writing to the extension directory goes, that should generally > > happen > > > > on > > > > > > > extension installation/update, right? > > > > > > > I'd expect that an extension directory remains constant while the > > > > extension > > > > > is > > > > > > > loaded, and that the resource changes become visible to the app > after > > > > reload > > > > > - > > > > > > > the similar thing would happen in lock screen profile (when user > > profile > > > > > > updates > > > > > > > an extension) - loading/unloading of the user profile app should > cause > > > the > > > > > app > > > > > > > load/unload in the lock screen profile. > > > > > > > > > > > > > > And lock screen profile should not really write to paths under the > > user > > > > > > profile > > > > > > > (we probably should also ensure that the extension updater is not > > > started > > > > > for > > > > > > > lock screen profile, so there are no unexpected updates - though, > > > looking > > > > at > > > > > > > code, these would seem to end up in the lock screen profile). > > > > > > > > > > > > Out of curiosity, is there a reason to not write the extension to the > > > > > lockscreen > > > > > > profile, thus avoiding this issue and the uninstall/delete file > problem? > > > > > > > > > > One reason is time - I do plan to look more closely into possibility of > > > > copying > > > > > extension to the lock screen profile, but I think getting the lock > screen > > > APIs > > > > > working and possibly unbounding lock screen profile from sign-in profile > > > have > > > > > somewhat higher priority at this moment. > > > > > > > > > > One potential concern is that we'd end up copying data from (encrypted) > > user > > > > > profile to (unencrypted) lock screen profile, though I expect this being > > > > > relevant mainly for unpacked extensions; then there's the question of if > > the > > > > > memory cost of duplicating the extension dir and time cost of copying > data > > > is > > > > > justified; handling app updates (instead of just reloading the app in > lock > > > > > screen profile, we'd have to reload and copy app directory).. > > > > > I don't think there's anything huge, but it's not that clear-cut to me > > that > > > > > introducing a new installer (from a location within another profile) > would > > > be > > > > > beneficial. > > > > > > > > I talked with kerrnel@ about this, and we both agreed that it makes us a > > > little > > > > uncomfortable from a security standpoint to have the lockscreen profile > > > require > > > > read access to the user's profile. I think we really should strive to > have > > > the > > > > lockscreen profile version separate from the other, which also alleviates > > any > > > > concerns around the lockscreen profile interfering with the user's > profile. > > > > > > We've had this discussion in many different forms now. This code runs in a > > > process that *has* access to the user profile. We are depending on the > context > > > keying of code to prevent access to the logged in user's profile from the > > > lockscreen handling code. > > > > > > That being said, making one explicit call to read data from the user's > > profile, > > > does not equate to "giving the lock screen access to the user profile". > Again, > > > the lockscreen code *already* has that access. > > > > > > I do want us to take any and all measures we can to ensure that we mitigate > > any > > > possible security issues - I just don't see removing this call does that. > > > > > > We need to load the app. The app is in the user profile. Whether we have > code > > > that copies the app out first, or code that directly loads the app - we > *will* > > > be reading this one particular piece of data from the user profile. > > > > > > I would argue that given that this call is a well defined call in very > mature > > > code, it would be safer to use it than to introduce new code to do the copy. > > > > I'm a bit distracted by other issues right now, so I'll defer to Jorge on > this. > > > > - Greg > > By the way, is there a reason not to just install the CRX into the lock screen > profile? > > - Greg There might not be a crx present (as it often gets disposed after the app is installed; or might not even exist - e.g. for unpacked apps). Though, that's actually, a possibility I plan to look into: - for unpacked apps - load the app from the extension path - i.e. what's done here - for apps installed from CRX - when a lock screen handler app is installed, cache the source CRX somewhere, and later, when require, install that CRX to the lock screen profile For now, I updated this CL to only handle unpacked apps, so I can unblock work on browser tests while I figure out the best way to install the rest. WDYT?
On 2017/06/14 20:25:48, tbarzic wrote: > There might not be a crx present (as it often gets disposed after the app is > installed; or might not even exist - e.g. for unpacked apps). Though, that's > actually, a possibility I plan to look into: > - for unpacked apps - load the app from the extension path - i.e. what's done > here > - for apps installed from CRX - when a lock screen handler app is installed, > cache the source CRX somewhere, and later, when require, install that CRX to the > lock screen profile > > For now, I updated this CL to only handle unpacked apps, so I can unblock work > on browser tests while I figure out the best way to install the rest. > WDYT? In practice, on CrOS, won't an unpacked app still always point to a directory in the user's profile? So won't this still have all the same concerns, including access to user profile and possibility of a lockscreen profile changing state (since it will still point to the same directory from which the app is loaded in the user's profile)?
On 2017/06/15 17:49:50, Devlin wrote: > On 2017/06/14 20:25:48, tbarzic wrote: > > There might not be a crx present (as it often gets disposed after the app is > > installed; or might not even exist - e.g. for unpacked apps). Though, that's > > actually, a possibility I plan to look into: > > - for unpacked apps - load the app from the extension path - i.e. what's done > > here > > - for apps installed from CRX - when a lock screen handler app is installed, > > cache the source CRX somewhere, and later, when require, install that CRX to > the > > lock screen profile > > > > For now, I updated this CL to only handle unpacked apps, so I can unblock work > > on browser tests while I figure out the best way to install the rest. > > WDYT? > > In practice, on CrOS, won't an unpacked app still always point to a directory in > the user's profile? So won't this still have all the same concerns, including > access to user profile and possibility of a lockscreen profile changing state > (since it will still point to the same directory from which the app is loaded in > the user's profile)? One huge difference is that loading an unpacked app from anywhere else than the user's directory is not acceptable, as it would require copying user data to a location outside the user's cryptohome. And when it comes to unpacked extensions, I think browser process changing the app dir state on behalf of user profile is just as bad as doing it on behalf of any other profile - it should not really happen. (though, I'd argue something similar for all apps - we should already be ensuring that browser process does not change extension's path as long as the extension is installed, regardless of profile for which that's done - am I missing a situation where that is allowed?)
On 2017/06/15 19:52:41, tbarzic wrote: > On 2017/06/15 17:49:50, Devlin wrote: > > On 2017/06/14 20:25:48, tbarzic wrote: > > > There might not be a crx present (as it often gets disposed after the app is > > > installed; or might not even exist - e.g. for unpacked apps). Though, that's > > > actually, a possibility I plan to look into: > > > - for unpacked apps - load the app from the extension path - i.e. what's > done > > > here > > > - for apps installed from CRX - when a lock screen handler app is installed, > > > cache the source CRX somewhere, and later, when require, install that CRX to > > the > > > lock screen profile > > > > > > For now, I updated this CL to only handle unpacked apps, so I can unblock > work > > > on browser tests while I figure out the best way to install the rest. > > > WDYT? > > > > In practice, on CrOS, won't an unpacked app still always point to a directory > in > > the user's profile? So won't this still have all the same concerns, including > > access to user profile and possibility of a lockscreen profile changing state > > (since it will still point to the same directory from which the app is loaded > in > > the user's profile)? > > One huge difference is that loading an unpacked app from anywhere else than the > user's directory is not acceptable, as it would require copying user data to a > location outside the user's cryptohome. I'm a bit confused by this - I read it as saying "unpacked extensions *must* be loaded from the original directory, which is in the user's (and not the lockscreen) profile". Is that right? If so, that sounds like it has all the same concerns would apply? > And when it comes to unpacked extensions, I think browser process changing the > app dir state on behalf of user profile is just as bad as doing it on behalf of > any other profile - it should not really happen. > (though, I'd argue something similar for all apps - we should already be > ensuring that browser process does not change extension's path as long as the > extension is installed, regardless of profile for which that's done - am I > missing a situation where that is allowed?) Why? The extension is installed in a given profile, and the profile data dir is designed to be modifiable by the profile... why would we assume that the extension install directory is frozen in state while the rest is not? As to when this happens, it's relatively rare that we modify the contents. The main case I can think of off the top of my head (excepting updates) is content verification, but that wouldn't apply for unpacked extensions. ------ Overall, I am less worried about unpacked extensions than I am about packed extensions from *a functionality standpoint*, since they don't unconditionally reside in the user's profile (though perhaps they do in practice on CrOS). But I'd still like Jorge's opinion on the security aspect, since this still seems a little odd.
On 2017/06/16 15:21:56, Devlin wrote: > On 2017/06/15 19:52:41, tbarzic wrote: > > On 2017/06/15 17:49:50, Devlin wrote: > > > On 2017/06/14 20:25:48, tbarzic wrote: > > > > There might not be a crx present (as it often gets disposed after the app > is > > > > installed; or might not even exist - e.g. for unpacked apps). Though, > that's > > > > actually, a possibility I plan to look into: > > > > - for unpacked apps - load the app from the extension path - i.e. what's > > done > > > > here > > > > - for apps installed from CRX - when a lock screen handler app is > installed, > > > > cache the source CRX somewhere, and later, when require, install that CRX > to > > > the > > > > lock screen profile > > > > > > > > For now, I updated this CL to only handle unpacked apps, so I can unblock > > work > > > > on browser tests while I figure out the best way to install the rest. > > > > WDYT? > > > > > > In practice, on CrOS, won't an unpacked app still always point to a > directory > > in > > > the user's profile? So won't this still have all the same concerns, > including > > > access to user profile and possibility of a lockscreen profile changing > state > > > (since it will still point to the same directory from which the app is > loaded > > in > > > the user's profile)? > > > > One huge difference is that loading an unpacked app from anywhere else than > the > > user's directory is not acceptable, as it would require copying user data to a > > location outside the user's cryptohome. > > I'm a bit confused by this - I read it as saying "unpacked extensions *must* be > loaded from the original directory, which is in the user's (and not the > lockscreen) profile". Is that right? If so, that sounds like it has all the > same concerns would apply? > Yes, they're generally be loaded from the user's cryptohome (though, that's not neccesarilly the case, afaik we support loading an app from a removable storage). The difference is that they're not loaded from a path "owned" by the profile; they're load from user data dir, and as such I would expect then not to be modifyable by Chrome extensions system, so concerns are little different. > > And when it comes to unpacked extensions, I think browser process changing the > > app dir state on behalf of user profile is just as bad as doing it on behalf > of > > any other profile - it should not really happen. > > (though, I'd argue something similar for all apps - we should already be > > ensuring that browser process does not change extension's path as long as the > > extension is installed, regardless of profile for which that's done - am I > > missing a situation where that is allowed?) > > Why? The extension is installed in a given profile, and the profile data dir is > designed to be modifiable by the profile... why would we assume that the > extension install directory is frozen in state while the rest is not? As to > when this happens, it's relatively rare that we modify the contents. The main > case I can think of off the top of my head (excepting updates) is content > verification, but that wouldn't apply for unpacked extensions. > Yes, that's a fair point. I don't thing having extension->path() remain constant (and equivalent to unpacked CRX contents) would be unreasonable proposition, given that the extension has read access to the path (via chrome.runtime.getPackageDirectoryEntry), and the possibility of the extension being installed to a shared path rather than in the user profile dir (as it can be the case on Chrome OS); but I wasn't trying to say I'd consider that requirement either. > ------ > > Overall, I am less worried about unpacked extensions than I am about packed > extensions from *a functionality standpoint*, since they don't unconditionally > reside in the user's profile (though perhaps they do in practice on CrOS). But > I'd still like Jorge's opinion on the security aspect, since this still seems a > little odd. OK, that sounds good to me.
Description was changed from ========== Introduce lock screen app manager The manager is responsible for loading/unloading the lock screen note taking app (if one is selected) to/from the profile used for lock screen apps. StateController starts it when the user session is locked and stops it when user session is unlocked (at which moment the action handler app is unloaded from sign-in profile). StateController uses LockAppManager to determine whether a lock screen enabled note taking app is set up, and to dispatch launch event to the app in question. BUG=715781 ========== to ========== Introduce lock screen app manager Adds default lock screen app manager implementation. The app manager, when started, checks if a lock screen enabled note taking app is set, and if so, loads it to the profile used to run apps on the lock screen. When stopped, app manager will remove (and uninstall) the app from the profile. The app manager observes the note taking app state in the user profile and updates lock screen app's profile accordingly. BUG=715781 ==========
jorgelo, kerrnel; ping
On 2017/06/22 19:00:11, tbarzic wrote: > jorgelo, kerrnel; ping Let me see if I understand what we're doing in this CL: when a lock screen app is set, as soon as the screen is locked we'll load an unpacked app into a separate, lock-screen profile, and then synchronize the data between the version on the lock screen and the version in the user profile?
On 2017/06/23 18:57:59, Jorge Lucangeli Obes wrote: > On 2017/06/22 19:00:11, tbarzic wrote: > > jorgelo, kerrnel; ping > > Let me see if I understand what we're doing in this CL: when a lock screen app > is set, as soon as the screen is locked we'll load an unpacked app into a > separate, lock-screen profile, and then synchronize the data between the version > on the lock screen and the version in the user profile? We'll load the app into a lock-screen profile, but we won't really synchronize data between the profiles (i.e. the app prefs/local storage will be separate for the app in the lock screen profile and the app in the user profile). Though, the app in the lock screen profile would be loaded from the same location as the app in the user profile. (i.e. the app resources would be shared between app instances).
The CQ bit was checked by tbarzic@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.
During offline discussion last Friday, we ended up on the proposal to create a read-only symbolic link to the app path in the user profile, and install the app to the lock screen profile from that path. Unfortunately, it doesn't seem that would be easily achievable: * chmod on a symbolic link writes through to the target link path (so, changing symbolic link access permissions would change the target path, too, w) * mount --bind, which can create a read only view of a directory subtree, requires root permission, so it can't be used by Chrome I'm not aware of another way to achieve that goal (I'm open for suggestions), so I abandoned that approach and switched to the following: * for unpacked apps - load from the original app path * for the rest - copy app path to the lock screen profile install directory, and install the app from the copied location Let me know what you think.
friendly ping
https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:49: // Loads extension with the provided extension ID. install source location and nit: add punctuation ... the provided |extension_id|, |location|, and |creation_flags| https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:87: std::unique_ptr<base::ScopedTempDir> extension_temp_dir = optional nit: auto extension_temp_dir = base::MakeUnique<...>() MakeUnique<T> is known to only ever return a std::unique_ptr<T>. https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:91: callback.Run(scoped_refptr<const extensions::Extension>()); does Run(nullptr) work? https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:281: // If the app is already installed, remove it. Can you describe when this would happen? https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:289: is_unpacked ? app->path() : base::FilePath(), app->location(), add a comment explaining this https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:293: if (!lock_profile_app) When could this happen? https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:324: if (install_id != install_count_ || state_ != State::kActivating) When do we clean up the resources we copied?
https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:49: // Loads extension with the provided extension ID. install source location and On 2017/07/05 16:12:05, Devlin wrote: > nit: add punctuation > ... the provided |extension_id|, |location|, and |creation_flags| Done. https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:87: std::unique_ptr<base::ScopedTempDir> extension_temp_dir = On 2017/07/05 16:12:05, Devlin wrote: > optional nit: auto extension_temp_dir = base::MakeUnique<...>() > MakeUnique<T> is known to only ever return a std::unique_ptr<T>. Done. https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:91: callback.Run(scoped_refptr<const extensions::Extension>()); On 2017/07/05 16:12:05, Devlin wrote: > does Run(nullptr) work? it does. https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:281: // If the app is already installed, remove it. On 2017/07/05 16:12:05, Devlin wrote: > Can you describe when this would happen? I can't really think of a scenario as long as the lock screen profile is ephemeral (it the profile were not ephemeral, an app might be left lingering from the previous user session (potentially from a different user)). I'll remove this (at least for now). https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:289: is_unpacked ? app->path() : base::FilePath(), app->location(), On 2017/07/05 16:12:05, Devlin wrote: > add a comment explaining this Done. https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:293: if (!lock_profile_app) On 2017/07/05 16:12:05, Devlin wrote: > When could this happen? In theory, if we attempt to create an extension with invalid properties; though, in that case the original app would not be installed in the user profile - I changed this to a dcheck, and added a comment. https://codereview.chromium.org/2902293002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:324: if (install_id != install_count_ || state_ != State::kActivating) On 2017/07/05 16:12:05, Devlin wrote: > When do we clean up the resources we copied? It will be cleaned up with the profile on Chrome restart (or if the app is later loaded to the lock screen profile, when the app gets uninstalled). (given that the chance of this happening - i.e. lock screen app status changing while the app is installed - are very slim, I don't think we need something more complex).
interfacing with extension system lgtm. Thanks for your patience and iteration here! https://codereview.chromium.org/2902293002/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:77: // |callback| - called with the app loaded from the final installation path, nit: s/,/.
https://codereview.chromium.org/2902293002/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc (right): https://codereview.chromium.org/2902293002/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc:77: // |callback| - called with the app loaded from the final installation path, On 2017/07/06 15:14:14, Devlin wrote: > nit: s/,/. Done.
The CQ bit was checked by tbarzic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2902293002/#ps540001 (title: "rebase")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tbarzic@chromium.org
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by tbarzic@chromium.org
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/06/27 23:59:33, tbarzic wrote: > During offline discussion last Friday, we ended up on the proposal to create a > read-only symbolic link to the app path in the user profile, and install the app > to the lock screen profile from that path. > > Unfortunately, it doesn't seem that would be easily achievable: > * chmod on a symbolic link writes through to the target link path (so, changing > symbolic link access permissions would change the target path, too, w) > * mount --bind, which can create a read only view of a directory subtree, > requires root permission, so it can't be used by Chrome > > I'm not aware of another way to achieve that goal (I'm open for suggestions), so > I abandoned that approach and switched to the following: > * for unpacked apps - load from the original app path > * for the rest - copy app path to the lock screen profile install directory, and > install the app from > the copied location > > Let me know what you think. Apologies for the delay. What does "copy app path to the lock screen" mean? Is it copying actual files, or just the path?
On 2017/07/06 22:57:01, Jorge Lucangeli Obes wrote: > On 2017/06/27 23:59:33, tbarzic wrote: > > During offline discussion last Friday, we ended up on the proposal to create > a > > read-only symbolic link to the app path in the user profile, and install the > app > > to the lock screen profile from that path. > > > > Unfortunately, it doesn't seem that would be easily achievable: > > * chmod on a symbolic link writes through to the target link path (so, > changing > > symbolic link access permissions would change the target path, too, w) > > * mount --bind, which can create a read only view of a directory subtree, > > requires root permission, so it can't be used by Chrome > > > > I'm not aware of another way to achieve that goal (I'm open for suggestions), > so > > I abandoned that approach and switched to the following: > > * for unpacked apps - load from the original app path > > * for the rest - copy app path to the lock screen profile install directory, > and > > install the app from > > the copied location > > > > Let me know what you think. > > Apologies for the delay. What does "copy app path to the lock screen" mean? Is > it copying actual files, or just the path? it's copying actual files
On 2017/07/06 22:58:04, tbarzic wrote: > On 2017/07/06 22:57:01, Jorge Lucangeli Obes wrote: > > On 2017/06/27 23:59:33, tbarzic wrote: > > > During offline discussion last Friday, we ended up on the proposal to > create > > a > > > read-only symbolic link to the app path in the user profile, and install the > > app > > > to the lock screen profile from that path. > > > > > > Unfortunately, it doesn't seem that would be easily achievable: > > > * chmod on a symbolic link writes through to the target link path (so, > > changing > > > symbolic link access permissions would change the target path, too, w) > > > * mount --bind, which can create a read only view of a directory subtree, > > > requires root permission, so it can't be used by Chrome > > > > > > I'm not aware of another way to achieve that goal (I'm open for > suggestions), > > so > > > I abandoned that approach and switched to the following: > > > * for unpacked apps - load from the original app path > > > * for the rest - copy app path to the lock screen profile install directory, > > and > > > install the app from > > > the copied location > > > > > > Let me know what you think. > > > > Apologies for the delay. What does "copy app path to the lock screen" mean? Is > > it copying actual files, or just the path? > > it's copying actual files Alright, that is one of the options we discussed. lgtm.
Description was changed from
==========
Introduce lock screen app manager
Adds default lock screen app manager implementation.
The app manager, when started, checks if a lock screen enabled
note taking app is set, and if so, loads it to the profile used
to run apps on the lock screen. When stopped, app manager will
remove (and uninstall) the app from the profile.
The app manager observes the note taking app state in the user
profile and updates lock screen app's profile accordingly.
BUG=715781
==========
to
==========
Introduce lock screen app manager
Adds default lock screen app manager implementation.
The app manager, when started, checks if a lock screen enabled
note taking app is set, and if so, loads it to the profile used
to run apps on the lock screen. When stopped, app manager will
remove (and uninstall) the app from the profile.
The app is installed and loaded to the lock screen profile as follows:
* for unpacked apps - the app is loaded from the (original) unpacked
app location
* for others - the app resources are copied from the original, user
profile to the lock screen app profile's extension install
directory and loaded from the copied location
The app manager observes the note taking app state in the user
profile and updates lock screen app's profile accordingly.
BUG=715781
==========
The CQ bit was checked by tbarzic@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": 540001, "attempt_start_ts": 1499392444240850,
"parent_rev": "b18578bd35569b971144d4547ee048b68adf795a", "commit_rev":
"c577bdc5b338dd58d27c8aeec32351263a47ad61"}
Message was sent while issue was closed.
Description was changed from
==========
Introduce lock screen app manager
Adds default lock screen app manager implementation.
The app manager, when started, checks if a lock screen enabled
note taking app is set, and if so, loads it to the profile used
to run apps on the lock screen. When stopped, app manager will
remove (and uninstall) the app from the profile.
The app is installed and loaded to the lock screen profile as follows:
* for unpacked apps - the app is loaded from the (original) unpacked
app location
* for others - the app resources are copied from the original, user
profile to the lock screen app profile's extension install
directory and loaded from the copied location
The app manager observes the note taking app state in the user
profile and updates lock screen app's profile accordingly.
BUG=715781
==========
to
==========
Introduce lock screen app manager
Adds default lock screen app manager implementation.
The app manager, when started, checks if a lock screen enabled
note taking app is set, and if so, loads it to the profile used
to run apps on the lock screen. When stopped, app manager will
remove (and uninstall) the app from the profile.
The app is installed and loaded to the lock screen profile as follows:
* for unpacked apps - the app is loaded from the (original) unpacked
app location
* for others - the app resources are copied from the original, user
profile to the lock screen app profile's extension install
directory and loaded from the copied location
The app manager observes the note taking app state in the user
profile and updates lock screen app's profile accordingly.
BUG=715781
Review-Url: https://codereview.chromium.org/2902293002
Cr-Commit-Position: refs/heads/master@{#484815}
Committed:
https://chromium.googlesource.com/chromium/src/+/c577bdc5b338dd58d27c8aeec323...
==========
Message was sent while issue was closed.
Committed patchset #28 (id:540001) as https://chromium.googlesource.com/chromium/src/+/c577bdc5b338dd58d27c8aeec323... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
