Add "kiosk_only" manifest attribute for platform apps.

Add "kiosk_only" manifest attribute for platform apps.

This top-level manifest attribute enforces that the app can only be installed and run in ChromeOS kiosk mode.

BUG=284964
TEST=added new tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225400

[Tim Song, 2013-09-24 01:34:58]

Please take a look.

[not at google - send to devlin, 2013-09-24 18:35:16]

Main comment is in kiosk_mode_info; I think it could be a lot simpler.

Also, adding benwells to review the apps changes. You will  need somebody for
the chromeos changes too.

https://codereview.chromium.org/23604068/diff/4001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_basic_info.cc (right):

https://codereview.chromium.org/23604068/diff/4001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_basic_info.cc:22: const char
kKioskOnlyKey[] = "kioskRequired";
kioskOnly?

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handlers/kiosk_mode_info.h (right):

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handlers/kiosk_mode_info.h:26: 
consider replacing these with an enum:

// something like this:
struct KioskModeInfo {
  enum Value {
    NONE,
    ENABLED,
    ONLY,
  }

  ...
};

and enforcing that safety at parse time.

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:520: if
(KioskOnlyInfo::IsKioskOnly(installer_.extension())) {
installation sure; but what about extension loading? I remember the email about
this but being confused...

[Tim Song, 2013-09-24 18:50:00]

+xiyuan for chromeos changes.

https://codereview.chromium.org/23604068/diff/4001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_basic_info.cc (right):

https://codereview.chromium.org/23604068/diff/4001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_basic_info.cc:22: const char
kKioskOnlyKey[] = "kioskRequired";
On 2013/09/24 18:35:17, kalman wrote:
> kioskOnly?

Done.

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handlers/kiosk_mode_info.h (right):

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handlers/kiosk_mode_info.h:26: 
On 2013/09/24 18:35:17, kalman wrote:
> consider replacing these with an enum:
> 
> // something like this:
> struct KioskModeInfo {
>   enum Value {
>     NONE,
>     ENABLED,
>     ONLY,
>   }
> 
>   ...
> };
> 
> and enforcing that safety at parse time.

I considered this, but an issue arises when calling Extension::SetManifestData.
If we replace both of these fields with one struct, then we lose the symmetry of
fields between the manifest json and the internal data representation.

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:520: if
(KioskOnlyInfo::IsKioskOnly(installer_.extension())) {
On 2013/09/24 18:35:17, kalman wrote:
> installation sure; but what about extension loading? I remember the email
about
> this but being confused...

Can an extension be loaded without being first installed?

[not at google - send to devlin, 2013-09-24 18:55:17]

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handlers/kiosk_mode_info.h (right):

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handlers/kiosk_mode_info.h:26: 
On 2013/09/24 18:50:01, Tim Song wrote:
> On 2013/09/24 18:35:17, kalman wrote:
> > consider replacing these with an enum:
> > 
> > // something like this:
> > struct KioskModeInfo {
> >   enum Value {
> >     NONE,
> >     ENABLED,
> >     ONLY,
> >   }
> > 
> >   ...
> > };
> > 
> > and enforcing that safety at parse time.
> 
> I considered this, but an issue arises when calling
Extension::SetManifestData.
> If we replace both of these fields with one struct, then we lose the symmetry
of
> fields between the manifest json and the internal data representation.

I think this is fine; I CC'ed Yoyo who is the manifest handler person. Safe
internal model is better than making call sites figure it out.

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:520: if
(KioskOnlyInfo::IsKioskOnly(installer_.extension())) {
On 2013/09/24 18:50:01, Tim Song wrote:
> On 2013/09/24 18:35:17, kalman wrote:
> > installation sure; but what about extension loading? I remember the email
> about
> > this but being confused...
> 
> Can an extension be loaded without being first installed?

I mean like, if somebody installs in kiosk mode then switches into non-kiosk
mode.

Also there is the unpacked installation case.

Lastly: what happens if an extension is installed in kiosk mode then synced to a
profile that isn't in kiosk mode?

[Tim Song, 2013-09-24 19:09:06]

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handlers/kiosk_mode_info.h (right):

https://codereview.chromium.org/23604068/diff/4001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handlers/kiosk_mode_info.h:26: 
On 2013/09/24 18:55:17, kalman wrote:
> On 2013/09/24 18:50:01, Tim Song wrote:
> > On 2013/09/24 18:35:17, kalman wrote:
> > > consider replacing these with an enum:
> > > 
> > > // something like this:
> > > struct KioskModeInfo {
> > >   enum Value {
> > >     NONE,
> > >     ENABLED,
> > >     ONLY,
> > >   }
> > > 
> > >   ...
> > > };
> > > 
> > > and enforcing that safety at parse time.
> > 
> > I considered this, but an issue arises when calling
> Extension::SetManifestData.
> > If we replace both of these fields with one struct, then we lose the
symmetry
> of
> > fields between the manifest json and the internal data representation.
> 
> I think this is fine; I CC'ed Yoyo who is the manifest handler person. Safe
> internal model is better than making call sites figure it out.

Alright, then we'll just have one ManifestData for the two fields keyed by
keys::kKioskMode.

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:520: if
(KioskOnlyInfo::IsKioskOnly(installer_.extension())) {
On 2013/09/24 18:55:18, kalman wrote:
> On 2013/09/24 18:50:01, Tim Song wrote:
> > On 2013/09/24 18:35:17, kalman wrote:
> > > installation sure; but what about extension loading? I remember the email
> > about
> > > this but being confused...
> > 
> > Can an extension be loaded without being first installed?
> 
> I mean like, if somebody installs in kiosk mode then switches into non-kiosk
> mode.
> 
> Also there is the unpacked installation case.
> 
> Lastly: what happens if an extension is installed in kiosk mode then synced to
a
> profile that isn't in kiosk mode?

For kiosk mode, ChromeOS generates a special local account and cryptohome for
each app that is registered to be run in kiosk mode. You can't login to this
account for a regular session nor can you sync the profile.

[not at google - send to devlin, 2013-09-24 19:22:25]

thanks for the enum thing, ping me when you want me to take another look.

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:520: if
(KioskOnlyInfo::IsKioskOnly(installer_.extension())) {
On 2013/09/24 19:09:06, Tim Song wrote:
> On 2013/09/24 18:55:18, kalman wrote:
> > On 2013/09/24 18:50:01, Tim Song wrote:
> > > On 2013/09/24 18:35:17, kalman wrote:
> > > > installation sure; but what about extension loading? I remember the
email
> > > about
> > > > this but being confused...
> > > 
> > > Can an extension be loaded without being first installed?
> > 
> > I mean like, if somebody installs in kiosk mode then switches into non-kiosk
> > mode.
> > 
> > Also there is the unpacked installation case.
> > 
> > Lastly: what happens if an extension is installed in kiosk mode then synced
to
> a
> > profile that isn't in kiosk mode?
> 
> For kiosk mode, ChromeOS generates a special local account and cryptohome for
> each app that is registered to be run in kiosk mode. You can't login to this
> account for a regular session nor can you sync the profile.

Ok cool.

You might want to still be cautious and add this check in UnpackedInstaller,
since on desktop somebody could grab the CRX, unpack it, then install it that
way. Maybe you don't want that.

One last question here though: what about policy installations?

[Tim Song, 2013-09-24 20:15:17]

On 2013/09/24 19:22:25, kalman wrote:
> thanks for the enum thing, ping me when you want me to take another look.
> 
>
https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
> File chrome/browser/extensions/crx_installer.cc (right):
> 
>
https://codereview.chromium.org/23604068/diff/8001/chrome/browser/extensions/...
> chrome/browser/extensions/crx_installer.cc:520: if
> (KioskOnlyInfo::IsKioskOnly(installer_.extension())) {
> On 2013/09/24 19:09:06, Tim Song wrote:
> > On 2013/09/24 18:55:18, kalman wrote:
> > > On 2013/09/24 18:50:01, Tim Song wrote:
> > > > On 2013/09/24 18:35:17, kalman wrote:
> > > > > installation sure; but what about extension loading? I remember the
> email
> > > > about
> > > > > this but being confused...
> > > > 
> > > > Can an extension be loaded without being first installed?
> > > 
> > > I mean like, if somebody installs in kiosk mode then switches into
non-kiosk
> > > mode.
> > > 
> > > Also there is the unpacked installation case.
> > > 
> > > Lastly: what happens if an extension is installed in kiosk mode then
synced
> to
> > a
> > > profile that isn't in kiosk mode?
> > 
> > For kiosk mode, ChromeOS generates a special local account and cryptohome
for
> > each app that is registered to be run in kiosk mode. You can't login to this
> > account for a regular session nor can you sync the profile.
> 
> Ok cool.
> 
> You might want to still be cautious and add this check in UnpackedInstaller,
> since on desktop somebody could grab the CRX, unpack it, then install it that
> way. Maybe you don't want that.
> 
> One last question here though: what about policy installations?

Kiosk apps set via policy basically works the same as through consumer mode. A
device local account is pushed for each kiosk app, and a cryptohome and profile
is created when that app is first run in kiosk mode.

Good catch with the UnpackedInstaller. But if the user is able to unpack the
crx, it would be pretty trivial for them to edit the manifest to remove the
"kiosk_only" key, so I'm not sure what we would really gain.

[not at google - send to devlin, 2013-09-24 20:16:30]

> 
> Kiosk apps set via policy basically works the same as through consumer mode. A
> device local account is pushed for each kiosk app, and a cryptohome and
profile
> is created when that app is first run in kiosk mode.

Great.

> 
> Good catch with the UnpackedInstaller. But if the user is able to unpack the
> crx, it would be pretty trivial for them to edit the manifest to remove the
> "kiosk_only" key, so I'm not sure what we would really gain.

Ah, of course, good point.

[xiyuan, 2013-09-24 20:20:39]

chrome/browser/chromeos/* LGTM

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/fake_user_manager.h (right):

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/fake_user_manager.h:31: void UserLoggedIn(const
std::string& email);
I have added a LoginUser method that does exactly the same thing.

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/crx_installer_browsertest.cc (right):

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/crx_installer_browsertest.cc:29: #include
"chrome/browser/chromeos/login/user_manager.h"
nit: only fake_user_manager.h is needed.

[Tim Song, 2013-09-24 21:07:25]

I changed kiosk_mode_info.* to use a single struct and enum as suggested. Please
take another look.

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/fake_user_manager.h (right):

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/fake_user_manager.h:31: void UserLoggedIn(const
std::string& email);
On 2013/09/24 20:20:40, xiyuan wrote:
> I have added a LoginUser method that does exactly the same thing.

Done.

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/crx_installer_browsertest.cc (right):

https://codereview.chromium.org/23604068/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/crx_installer_browsertest.cc:29: #include
"chrome/browser/chromeos/login/user_manager.h"
On 2013/09/24 20:20:40, xiyuan wrote:
> nit: only fake_user_manager.h is needed.

We should include user_manager.h for explicit definition of
ScopedUserManagerEnabler.

[not at google - send to devlin, 2013-09-24 21:57:38]

lgtm

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/articles/manifest/kiosk_only.html
(right):

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/articles/manifest/kiosk_only.html:5:
kiosk mode. This field is only valid if the "kiosk_enabled" field is true.
Thanks for adding docs! Very much appreciated.

Small change - I would combine this with the kiosk_enabled docs and link to it
with an anchor.

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:82: "documentation":
"manifest/kiosk_only.html",
I.e. "manifest/kiosk_enabled.html#kiosk_only"

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handlers/kiosk_mode_info.cc (right):

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
chrome/common/extensions/manifest_handlers/kiosk_mode_info.cc:67: *error =
ASCIIToUTF16(manifest_errors::kInvalidKioskOnlyButNotEnabled);
could you add some quick unit tests for this class, in particular for this error
case?

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
chrome/common/extensions/manifest_handlers/kiosk_mode_info.cc:73:
DCHECK(extension->is_platform_app());
IMO these checks aren't that useful but it's fine to leave it in.

[Tim Song, 2013-09-24 22:38:25]

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/articles/manifest/kiosk_only.html
(right):

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/articles/manifest/kiosk_only.html:5:
kiosk mode. This field is only valid if the "kiosk_enabled" field is true.
On 2013/09/24 21:57:39, kalman wrote:
> Thanks for adding docs! Very much appreciated.
> 
> Small change - I would combine this with the kiosk_enabled docs and link to it
> with an anchor.

Done.

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:82: "documentation":
"manifest/kiosk_only.html",
On 2013/09/24 21:57:39, kalman wrote:
> I.e. "manifest/kiosk_enabled.html#kiosk_only"

Done.

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handlers/kiosk_mode_info.cc (right):

https://codereview.chromium.org/23604068/diff/32001/chrome/common/extensions/...
chrome/common/extensions/manifest_handlers/kiosk_mode_info.cc:67: *error =
ASCIIToUTF16(manifest_errors::kInvalidKioskOnlyButNotEnabled);
On 2013/09/24 21:57:39, kalman wrote:
> could you add some quick unit tests for this class, in particular for this
error
> case?

Done. I added a few tests to extension_manifests_kiosk_unittest.cc, including
this test case.

[not at google - send to devlin, 2013-09-24 23:03:07]

lgtm

https://codereview.chromium.org/23604068/diff/40001/chrome/common/extensions/...
File
chrome/common/extensions/docs/templates/articles/manifest/kiosk_enabled.html
(right):

https://codereview.chromium.org/23604068/diff/40001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/articles/manifest/kiosk_enabled.html:14:
kiosk mode. This field is only valid if the "kiosk_enabled" field is true.
nit: <code>"kiosk_enabled"</code> and <code>true</code>.

[benwells, 2013-09-25 01:55:42]

apps/launcher.cc and apps/DEPS lgtm

[commit-bot: I haz the power, 2013-09-25 22:29:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/23604068/56001

[commit-bot: I haz the power, 2013-09-26 06:02:00]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-26 07:13:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/23604068/56001

[commit-bot: I haz the power, 2013-09-26 08:42:36]

Change committed as 225400