Permissions for device info API

Permissions for device info APIs.
BUG=170375
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221893

R=mpcomplete@chromium.org

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

[not at google - send to devlin, 2013-09-03 20:20:28]

https://codereview.chromium.org/23856005/diff/3001/chrome/common/extensions/d...
File
chrome/common/extensions/docs/templates/public/extensions/signed_in_devices.html
(right):

https://codereview.chromium.org/23856005/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/public/extensions/signed_in_devices.html:1:
{{+partials.standard_extensions_api api:apis.signed_in_devices}}
this file should be signedInDevices.html not signed_in_devices.html.

[lipalani1, 2013-09-04 21:45:42]

Please review.

[Matt Perry, 2013-09-04 22:30:03]

https://codereview.chromium.org/23856005/diff/26001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23856005/diff/26001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4127: +        Access signed in devices
Did we discuss this string? "Access the list of signed in devices" would be more
accurate, since you can't really do anything with them. Also, does this really
need a warning message? It's a fairly benign API. Have you talked with security
team about this?

[lipalani1, 2013-09-05 00:12:56]

On 2013/09/04 22:30:03, Matt Perry wrote:
>
https://codereview.chromium.org/23856005/diff/26001/chrome/app/generated_reso...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/23856005/diff/26001/chrome/app/generated_reso...
> chrome/app/generated_resources.grd:4127: +        Access signed in devices
> Did we discuss this string? "Access the list of signed in devices" would be
more
> accurate, since you can't really do anything with them. Also, does this really
> need a warning message? It's a fairly benign API. Have you talked with
security
> team about this?

I have removed the permission for now. I haven't spoken to the security team. I
agree with you leaving it is probably better(unless there is a security concern,
in which case we can add it back. Also do I need to go through a security review
for this?)

[Matt Perry, 2013-09-05 00:15:00]

On 2013/09/05 00:12:56, lipalani1 wrote:
> On 2013/09/04 22:30:03, Matt Perry wrote:
> >
>
https://codereview.chromium.org/23856005/diff/26001/chrome/app/generated_reso...
> > File chrome/app/generated_resources.grd (right):
> > 
> >
>
https://codereview.chromium.org/23856005/diff/26001/chrome/app/generated_reso...
> > chrome/app/generated_resources.grd:4127: +        Access signed in devices
> > Did we discuss this string? "Access the list of signed in devices" would be
> more
> > accurate, since you can't really do anything with them. Also, does this
really
> > need a warning message? It's a fairly benign API. Have you talked with
> security
> > team about this?
> 
> I have removed the permission for now. I haven't spoken to the security team.
I
> agree with you leaving it is probably better(unless there is a security
concern,
> in which case we can add it back. Also do I need to go through a security
review
> for this?)

I don't know about security review - I'd at least send them an email to get
their thoughts on a permission string.

[Matt Perry, 2013-09-05 00:15:05]

lgtm

[not at google - send to devlin, 2013-09-05 19:32:14]

https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:550: "extension_types":
["extension"]
This should include "legacy_packaged_app" because the sessions API depends on
it. And it might as well be.

Also: why not platform apps? Is that a conscious decision?

[lipalani1, 2013-09-06 17:51:35]

On 2013/09/05 19:32:14, kalman wrote:
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> File chrome/common/extensions/api/_permission_features.json (right):
> 
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> chrome/common/extensions/api/_permission_features.json:550: "extension_types":
> ["extension"]
> This should include "legacy_packaged_app" because the sessions API depends on
> it. And it might as well be.
> 
> Also: why not platform apps? Is that a conscious decision?

Sorry was oof yesterday. Added the legacy packaged app. (let me know if this is
still lgtm. Also there was a unit test failure and I fixed it. The file is
permission_set_unittest.cc)

For platform apps: This API needs a browser process running(with profile sync
service initialized) to work correctly. I asked around if this initialization
can be relied upon for platform apps but never got around to investigating it.
Do you have any idea? In any case if there is confusion I can get to this
investigation after I finish coding up an example implementation for this API.

[not at google - send to devlin, 2013-09-06 17:55:22]

+miket for that question.

https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:550: "extension_types":
["extension"]
On 2013/09/05 19:32:15, kalman wrote:
> This should include "legacy_packaged_app" because the sessions API depends on
> it. And it might as well be.
> 
> Also: why not platform apps? Is that a conscious decision?

Yes. In fact the storage API relies on this.

I'm moderately sure that this should include packaged_apps.

[not at google - send to devlin, 2013-09-06 17:55:45]

On 2013/09/06 17:51:35, lipalani1 wrote:
> On 2013/09/05 19:32:14, kalman wrote:
> >
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> > File chrome/common/extensions/api/_permission_features.json (right):
> > 
> >
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> > chrome/common/extensions/api/_permission_features.json:550:
"extension_types":
> > ["extension"]
> > This should include "legacy_packaged_app" because the sessions API depends
on
> > it. And it might as well be.
> > 
> > Also: why not platform apps? Is that a conscious decision?
> 
> Sorry was oof yesterday. Added the legacy packaged app. (let me know if this
is
> still lgtm. Also there was a unit test failure and I fixed it. The file is
> permission_set_unittest.cc)
> 
> For platform apps: This API needs a browser process running(with profile sync
> service initialized) to work correctly. I asked around if this initialization
> can be relied upon for platform apps but never got around to investigating it.
> Do you have any idea? In any case if there is confusion I can get to this
> investigation after I finish coding up an example implementation for this API.

^ previous comment was in response to this, not in response to myself.

[lipalani1, 2013-09-06 20:57:59]

On 2013/09/06 17:55:45, kalman wrote:
> On 2013/09/06 17:51:35, lipalani1 wrote:
> > On 2013/09/05 19:32:14, kalman wrote:
> > >
> >
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> > > File chrome/common/extensions/api/_permission_features.json (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> > > chrome/common/extensions/api/_permission_features.json:550:
> "extension_types":
> > > ["extension"]
> > > This should include "legacy_packaged_app" because the sessions API depends
> on
> > > it. And it might as well be.
> > > 
> > > Also: why not platform apps? Is that a conscious decision?
> > 
> > Sorry was oof yesterday. Added the legacy packaged app. (let me know if this
> is
> > still lgtm. Also there was a unit test failure and I fixed it. The file is
> > permission_set_unittest.cc)
> > 
> > For platform apps: This API needs a browser process running(with profile
sync
> > service initialized) to work correctly. I asked around if this
initialization
> > can be relied upon for platform apps but never got around to investigating
it.
> > Do you have any idea? In any case if there is confusion I can get to this
> > investigation after I finish coding up an example implementation for this
API.
> 
> ^ previous comment was in response to this, not in response to myself.

Cool after looking at storage API, I have added plaform_app as well. If there is
an issue I should be able to find it when I code up the example apps.

[lipalani1, 2013-09-06 22:12:26]

On 2013/09/06 20:57:59, lipalani1 wrote:
> On 2013/09/06 17:55:45, kalman wrote:
> > On 2013/09/06 17:51:35, lipalani1 wrote:
> > > On 2013/09/05 19:32:14, kalman wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> > > > File chrome/common/extensions/api/_permission_features.json (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/23856005/diff/54001/chrome/common/extensions/...
> > > > chrome/common/extensions/api/_permission_features.json:550:
> > "extension_types":
> > > > ["extension"]
> > > > This should include "legacy_packaged_app" because the sessions API
depends
> > on
> > > > it. And it might as well be.
> > > > 
> > > > Also: why not platform apps? Is that a conscious decision?
> > > 
> > > Sorry was oof yesterday. Added the legacy packaged app. (let me know if
this
> > is
> > > still lgtm. Also there was a unit test failure and I fixed it. The file is
> > > permission_set_unittest.cc)
> > > 
> > > For platform apps: This API needs a browser process running(with profile
> sync
> > > service initialized) to work correctly. I asked around if this
> initialization
> > > can be relied upon for platform apps but never got around to investigating
> it.
> > > Do you have any idea? In any case if there is confusion I can get to this
> > > investigation after I finish coding up an example implementation for this
> API.
> > 
> > ^ previous comment was in response to this, not in response to myself.
> 
> Cool after looking at storage API, I have added plaform_app as well. If there
is
> an issue I should be able to find it when I code up the example apps.

Looks like the try job has passed. Would either of you mind doing a quick review
of the pacaged_app change in _permission_features.json?

Thanks!

[Matt Perry, 2013-09-06 22:20:51]

lgtm

[commit-bot: I haz the power, 2013-09-06 22:24:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/23856005/27008

[commit-bot: I haz the power, 2013-09-07 04:40:45]

Change committed as 221893

[lipalani1, 2013-09-09 20:24:00]

Committed patchset #8 manually as r222079 (presubmit successful).