New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify extension of the Permissions API for concrete sensor implementations #22

Closed
tobie opened this Issue May 28, 2015 · 27 comments

Comments

Projects
None yet
6 participants
@tobie
Member

tobie commented May 28, 2015

Accessing sensor data has multiple privacy implications and is therefore subject to permissioning. The Permission API allows developers to check whether the permission to access a given API was already granted (or denied) by the user on a per origin basis.

Currently, exposing that status through the Permissions API requires a modification of the Permission API spec itself. Ideally, we'd want sped editors authoring concrete sensor specs to be able to extend the Permission API within the same document.

This needs concertation with the editors of the Permissions API.

Actions:

  • Liaise with @mounirlamouri and @marcoscaceres
  • Investigate relationship to Permissions API.
  • Try to find a solution that doesn't imply a Permissions API update for every concrete Sensor spec.
  • Email @public-script-coord about partial enum proposal
  • Wait for @public-script-coord feedback
  • Document current issue as a note in the Extensibility section of the spec.
  • Rewrite the permission seeking abstract operations once w3c/permissions#66 lands..

@tobie tobie added this to the FPWD milestone May 28, 2015

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie May 28, 2015

Member

@marcoscaceres the goal of the Generic Sensor API is to provide abstract classes from which to inherit along with a description on how to author specs so as to provide consistant APIs across sensors. I think it would make sense to include instructions on how to extend the Permissions API as part of this spec, rather than expect the Permissions API to be updated every time a new Sensor spec is written. Question for you is how we should go about doing that.

Member

tobie commented May 28, 2015

@marcoscaceres the goal of the Generic Sensor API is to provide abstract classes from which to inherit along with a description on how to author specs so as to provide consistant APIs across sensors. I think it would make sense to include instructions on how to extend the Permissions API as part of this spec, rather than expect the Permissions API to be updated every time a new Sensor spec is written. Question for you is how we should go about doing that.

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Jun 8, 2015

Member

Chatted with @marcoscaceres over irc. Seems the solution would involve the adjunction of partial enums to WebIDL so that extending the Permissions API to accommodate a new sensor would be limited to doing:

partial enum PermissionName {
    "proximity"
};

And when needed:

dictionary ProximitySensorPermissionDescriptor : PermissionDescriptor {
    // ...
};
Member

tobie commented Jun 8, 2015

Chatted with @marcoscaceres over irc. Seems the solution would involve the adjunction of partial enums to WebIDL so that extending the Permissions API to accommodate a new sensor would be limited to doing:

partial enum PermissionName {
    "proximity"
};

And when needed:

dictionary ProximitySensorPermissionDescriptor : PermissionDescriptor {
    // ...
};
@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Jun 10, 2015

Member

Emailed @public-script-coord about this.

Member

tobie commented Jun 10, 2015

Emailed @public-script-coord about this.

@marcoscaceres

This comment has been minimized.

Show comment
Hide comment
@marcoscaceres

marcoscaceres Jun 10, 2015

Member

FYI, I share the fragmentation concerns that Boris mentioned. Having partials all over the place can be hard to track.

Member

marcoscaceres commented Jun 10, 2015

FYI, I share the fragmentation concerns that Boris mentioned. Having partials all over the place can be hard to track.

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Jun 10, 2015

Member

@marcoscaceres so your suggestion is what? To use a DOMString instead of an enum here? Or to require spec editors to get a change in the Permission API for every new spec they write?

Member

tobie commented Jun 10, 2015

@marcoscaceres so your suggestion is what? To use a DOMString instead of an enum here? Or to require spec editors to get a change in the Permission API for every new spec they write?

@marcoscaceres

This comment has been minimized.

Show comment
Hide comment
@marcoscaceres

marcoscaceres Jun 11, 2015

Member

require spec editors to get a change in the Permission API for every new spec they write?

I hate to say it, but this one :( It's the least worst.

I don't want a DOMString because in Gecko we generate all the bindings from the WebIDL - so not having them in an enum is a pain for us (or we will make a fake enum... but then we might get funky behavior)... the spec will have to treat the DOMString like it's an enum, which would be redundant.

Worst case, we have the permission enum in its own spec/wiki/register - where people have to register their keys (including the Permissions API).

Member

marcoscaceres commented Jun 11, 2015

require spec editors to get a change in the Permission API for every new spec they write?

I hate to say it, but this one :( It's the least worst.

I don't want a DOMString because in Gecko we generate all the bindings from the WebIDL - so not having them in an enum is a pain for us (or we will make a fake enum... but then we might get funky behavior)... the spec will have to treat the DOMString like it's an enum, which would be redundant.

Worst case, we have the permission enum in its own spec/wiki/register - where people have to register their keys (including the Permissions API).

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Jun 11, 2015

Member

Well, there's always the third option which is what people will pick: not bother about exposing permission status.

Member

tobie commented Jun 11, 2015

Well, there's always the third option which is what people will pick: not bother about exposing permission status.

@marcoscaceres

This comment has been minimized.

Show comment
Hide comment
@marcoscaceres

marcoscaceres Jun 11, 2015

Member

That's not an option. The current situation around permissioning is a nightmare on the Web.

Member

marcoscaceres commented Jun 11, 2015

That's not an option. The current situation around permissioning is a nightmare on the Web.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 11, 2015

Editing the spec seems fine. PRs are cheap.

domenic commented Jun 11, 2015

Editing the spec seems fine. PRs are cheap.

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Jun 11, 2015

Member

What if a vendor implements the Permissions API but not, say, Temperature sensors. You'll still have "temperature-sensor" in PermissionName, or will the vendor be expected to manually edit said enum?

Member

tobie commented Jun 11, 2015

What if a vendor implements the Permissions API but not, say, Temperature sensors. You'll still have "temperature-sensor" in PermissionName, or will the vendor be expected to manually edit said enum?

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Jun 11, 2015

Member

There's a bug against the WebIDL spec to add support for partial enums. I'm still rather convinced this is the right thing to do for cross-cutting concerns such as permissions.

Member

tobie commented Jun 11, 2015

There's a bug against the WebIDL spec to add support for partial enums. I'm still rather convinced this is the right thing to do for cross-cutting concerns such as permissions.

@tobie tobie modified the milestones: Level 1, FPWD Oct 16, 2015

anssiko added a commit that referenced this issue Aug 23, 2016

Fix Bikeshed fatal errors
- Add 'Level' metadata
- Remove inline GH issues (#22, #101)
- Fix some <dfn>s and <a>s
- Remove WebIDL example syntax highlighting
@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Sep 19, 2016

Contributor

F2F agreement to use partial enums. Pending on the WebIDL spec adding the feature.

In addition, explore:

  • The use of only two high-level sensor permission names e.g. movement and (environment) ambient.
  • Exposing permissionName string on the sensor object itself e.g. sensor.permissionName or have a method that returns a possibly opaque permission string, e.g. sensor.getPermissionName().
Contributor

anssiko commented Sep 19, 2016

F2F agreement to use partial enums. Pending on the WebIDL spec adding the feature.

In addition, explore:

  • The use of only two high-level sensor permission names e.g. movement and (environment) ambient.
  • Exposing permissionName string on the sensor object itself e.g. sensor.permissionName or have a method that returns a possibly opaque permission string, e.g. sensor.getPermissionName().
@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Mar 10, 2017

Contributor

It seems we diverged from the F2F agreement documented in #22 (comment) when we landed w3c/permissions@4f95684

@tobie can you remind me, did we unearth new information between TPAC and now that justifies overriding the consensus position of the group? This is an honest question -- there's been so much going on that I can't tell for sure.

@pozdnyakov @alexshalamov

Contributor

anssiko commented Mar 10, 2017

It seems we diverged from the F2F agreement documented in #22 (comment) when we landed w3c/permissions@4f95684

@tobie can you remind me, did we unearth new information between TPAC and now that justifies overriding the consensus position of the group? This is an honest question -- there's been so much going on that I can't tell for sure.

@pozdnyakov @alexshalamov

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Mar 10, 2017

Member

Partial enums aren't a thing yet. There's also a clearly expressed desire to have a centralized registry for permission names from a number of people. Current consensus seems to be to keep it in the spec. it would require a lot of work to change that consensus and I don't think it's the right place to invest energy at the moment.

Member

tobie commented Mar 10, 2017

Partial enums aren't a thing yet. There's also a clearly expressed desire to have a centralized registry for permission names from a number of people. Current consensus seems to be to keep it in the spec. it would require a lot of work to change that consensus and I don't think it's the right place to invest energy at the moment.

@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Mar 10, 2017

Contributor

Thanks, how about the exploration item:

The use of only two high-level sensor permission names e.g. movement and (environment) ambient.

Was out exploration outcome to go with low-level sensor permission names such as ambient-light-sensor? With that as a precedence, I'm expecting we are now converging on:

  • proximity-sensor
  • accelerometer
  • gyroscope
  • magnetometer
  • orientation-sensor

Rather than high-level sensor permission names:

  • environment-sensors
  • motion-sensors

FTR, I don't have a personal opinion on which way to go, but I think we should settle on something rather sooner than later, and stick with it.

As discussed in this issue, these strings are probably never a good idea to be surfaced to the UI level, rather mapped to something more sensible to the user. And that's great, and by design.

Edit: to satisfy use cases described in #174 (comment) would need to go with low-level sensor permission names.

Contributor

anssiko commented Mar 10, 2017

Thanks, how about the exploration item:

The use of only two high-level sensor permission names e.g. movement and (environment) ambient.

Was out exploration outcome to go with low-level sensor permission names such as ambient-light-sensor? With that as a precedence, I'm expecting we are now converging on:

  • proximity-sensor
  • accelerometer
  • gyroscope
  • magnetometer
  • orientation-sensor

Rather than high-level sensor permission names:

  • environment-sensors
  • motion-sensors

FTR, I don't have a personal opinion on which way to go, but I think we should settle on something rather sooner than later, and stick with it.

As discussed in this issue, these strings are probably never a good idea to be surfaced to the UI level, rather mapped to something more sensible to the user. And that's great, and by design.

Edit: to satisfy use cases described in #174 (comment) would need to go with low-level sensor permission names.

@owencm

This comment has been minimized.

Show comment
Hide comment
@owencm

owencm Mar 10, 2017

Just to chime in since I did on the other thread - I am explicitly OK with either approach. I would simply prioritize consensus at this point 👍

owencm commented Mar 10, 2017

Just to chime in since I did on the other thread - I am explicitly OK with either approach. I would simply prioritize consensus at this point 👍

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Mar 10, 2017

Member

I'll write up my thinking on this first thing next week.

Member

tobie commented Mar 10, 2017

I'll write up my thinking on this first thing next week.

@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Mar 14, 2017

Contributor

Landed w3c/ambient-light#21

Based on the resolution of this issue, should update the other specs accordingly.

Contributor

anssiko commented Mar 14, 2017

Landed w3c/ambient-light#21

Based on the resolution of this issue, should update the other specs accordingly.

@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Mar 14, 2017

Contributor

@tobie can you prioritize resolving this issue?

Related, in w3c/permissions#138 (comment) @raymeskhoury "think having a separate [permission name] per sensor type is probably a better approach given the different security profile of each."

(@raymeskhoury is the editor of https://noncombatant.github.io/permission-delegation-api/ among other Security UX things.)

Contributor

anssiko commented Mar 14, 2017

@tobie can you prioritize resolving this issue?

Related, in w3c/permissions#138 (comment) @raymeskhoury "think having a separate [permission name] per sensor type is probably a better approach given the different security profile of each."

(@raymeskhoury is the editor of https://noncombatant.github.io/permission-delegation-api/ among other Security UX things.)

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Mar 14, 2017

Member

I have a draft about the permission's topic for sensors. It needs a bit more work, my week is busy with WebIDL work though, so I probably won't get to it until early next week.

Member

tobie commented Mar 14, 2017

I have a draft about the permission's topic for sensors. It needs a bit more work, my week is busy with WebIDL work though, so I probably won't get to it until early next week.

@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Mar 27, 2017

Contributor

@tobie I'd like to see progress on this issue. It is blocking our related implementation.

Contributor

anssiko commented Mar 27, 2017

@tobie I'd like to see progress on this issue. It is blocking our related implementation.

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Mar 27, 2017

Member

I know. I'm a week behind on all the things due to having been ill.

Member

tobie commented Mar 27, 2017

I know. I'm a week behind on all the things due to having been ill.

@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Apr 11, 2017

Contributor

@tobie, any updates on this issue?

Contributor

anssiko commented Apr 11, 2017

@tobie, any updates on this issue?

@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Apr 11, 2017

Contributor

@pozdnyakov, @alexshalamov et al. have documented some security and privacy considerations in the Chromium implementation design doc with input and feedback from Google's Security UX team.

@tobie, feel free to use this doc as input in your research if deemed helpful.

Contributor

anssiko commented Apr 11, 2017

@pozdnyakov, @alexshalamov et al. have documented some security and privacy considerations in the Chromium implementation design doc with input and feedback from Google's Security UX team.

@tobie, feel free to use this doc as input in your research if deemed helpful.

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Apr 11, 2017

Member

@anssiko made a few comments on that implementation design doc directly. There's a lot of considerations to be taken in account for this permission-related work which is proving more thorny by the minute.

I think one of the key issue here is that the permission spec wasn't really designed with such use cases in mind. That combined with the fact that no one is considering working on the permission spec a priority slows progress down significantly.

The input from the afore mentioned design doc, related implementer feedback, and post TPAC considerations given some of the security work happening around features (notably revoking permissions, allowing certain features in nested iframes, etc.) has also forced me to reconsider a number of things.

It's complicated! :)

Member

tobie commented Apr 11, 2017

@anssiko made a few comments on that implementation design doc directly. There's a lot of considerations to be taken in account for this permission-related work which is proving more thorny by the minute.

I think one of the key issue here is that the permission spec wasn't really designed with such use cases in mind. That combined with the fact that no one is considering working on the permission spec a priority slows progress down significantly.

The input from the afore mentioned design doc, related implementer feedback, and post TPAC considerations given some of the security work happening around features (notably revoking permissions, allowing certain features in nested iframes, etc.) has also forced me to reconsider a number of things.

It's complicated! :)

@anssiko

This comment has been minimized.

Show comment
Hide comment
@anssiko

anssiko Apr 19, 2017

Contributor

w3c/permissions#142 landed and added the following values to the PermissionName enum defined in the Permissions spec:

accelerometer
gyroscope
magnetometer
(ambient-light-sensor already landed earlier in w3c/permissions#138)

Also added was the following note:

The enumeration values accelerometer, gyroscope and magnetometer are considered provisional and are subject to change based on feedback from early implementations. See w3c/sensors#22 issue for more information.

The Chromium implementation is in process of being updated accordingly for the first Origin Trial. The mapping between the low-level sensors and the AbsoluteOrientationSensor fusion sensor is defined in the Model section of the Orientation Sensor spec.

@tobie, would you like to keep this issue open, or close this for now and reopen if/when new information emerges from Origin Trial(s) and related research?

Contributor

anssiko commented Apr 19, 2017

w3c/permissions#142 landed and added the following values to the PermissionName enum defined in the Permissions spec:

accelerometer
gyroscope
magnetometer
(ambient-light-sensor already landed earlier in w3c/permissions#138)

Also added was the following note:

The enumeration values accelerometer, gyroscope and magnetometer are considered provisional and are subject to change based on feedback from early implementations. See w3c/sensors#22 issue for more information.

The Chromium implementation is in process of being updated accordingly for the first Origin Trial. The mapping between the low-level sensors and the AbsoluteOrientationSensor fusion sensor is defined in the Model section of the Orientation Sensor spec.

@tobie, would you like to keep this issue open, or close this for now and reopen if/when new information emerges from Origin Trial(s) and related research?

@tobie

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie Apr 20, 2017

Member

Let's keep it open. There's a bunch of issues with the proposed solution that need to be addressed.

Member

tobie commented Apr 20, 2017

Let's keep it open. There's a bunch of issues with the proposed solution that need to be addressed.

@alexshalamov alexshalamov moved this from Tasks to Security & Privacy in Level 1 Jun 29, 2017

MXEBot pushed a commit to mirror/chromium that referenced this issue Aug 3, 2017

[sensors][permission] Add new permission types in permission module.
This is the first part of adding permission guard for sensors based on Generic Sensor Framework.
There was consensus to add granular permissions for sensors here
w3c/sensors#22

This CL adds permission name for the different sensors in the permission module.

Spec discussion : w3c/permissions#142

BUG=606766

Review-Url: https://codereview.chromium.org/2791623004
Cr-Commit-Position: refs/heads/master@{#491303}

@alexshalamov alexshalamov self-assigned this Sep 7, 2017

@alexshalamov alexshalamov moved this from Security & Privacy tasks to In Progress tasks in Level 1 Sep 7, 2017

alexshalamov added a commit to alexshalamov/sensors that referenced this issue Sep 7, 2017

alexshalamov added a commit to alexshalamov/sensors that referenced this issue Sep 20, 2017

@alexshalamov alexshalamov closed this in #265 Sep 20, 2017

alexshalamov added a commit that referenced this issue Sep 20, 2017

Add information to "Extending the Permission API" section (#265)
* Add information to "Extending the Permission API" section

Fixes: #132
Fixes: #22

@alexshalamov alexshalamov removed this from In Progress tasks in Level 1 Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment