776668 - Split permission for Settings/Contacts access into ReadOnly and ReadWrite variants

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Comment 1

[Jason Smith [:jsmith]]

Chris - Can you give more context on what this bug is and help us find an assignee?

Comment 2

[Andrew Overholt [:overholt]]

This may seem artificial to remove it at this point but Gregor told me that he and Jonas decided this and bug 776671 aren't features so I'm removing the feature keyword.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Can we morph this to cover read vs. write privileges for the settings API?

Comment 4

[Gregor Wagner [:gwagner]]

(In reply to ben turner [:bent] from comment #3)
> Can we morph this to cover read vs. write privileges for the settings API?

Yes we can!

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

For settings and contacts

Comment 7

[David Dahl :ddahl]

Comment on attachment 675411 [details] [diff] [review]
patch

Review of attachment 675411 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good,  only a few nits.

::: dom/contacts/ContactManager.js
@@ +446,5 @@
> +        break
> +      case "update":
> +      case "remove":
> +        access = "write";
> +        break

nit: break;

::: dom/contacts/fallback/ContactService.jsm
@@ +68,5 @@
>    },
>  
> +  assertPermission: function(aMessage, aPerm) {
> +    if (!aMessage.target.assertPermission(aPerm)) {
> +      dump("Contacts message " + msg.name +

debug() instead of dump()?

::: dom/settings/SettingsChangeNotifier.jsm
@@ +80,5 @@
>      let mm = aMessage.target;
>      switch (aMessage.name) {
>        case "Settings:Changed":
> +        if (!aMessage.target.assertPermission("settings-write")) {
> +          dump("Settings message " + msg.name +

debug() instead of dump() - or should this be Cu.reportError()? - same with above?

::: dom/settings/SettingsManager.js
@@ +339,5 @@
> +    this.hasReadPrivileges = readPerm == Ci.nsIPermissionManager.ALLOW_ACTION;
> +    this.hasWritePrivileges = writePerm == Ci.nsIPermissionManager.ALLOW_ACTION;
> +
> +    if (!this.hasReadPrivileges && !this.hasWritePrivileges) {
> +      dump("NO SETTINGS PERMISSION FOR: " + aWindow.document.nodePrincipal.origin + "\n");

I think if we want these messages to bubble up when developers are wokring on apps, etc, we want to use Cu.reportError instead of dump

Comment 8

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/858e49a77dca

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/858e49a77dca

Comment 10

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/501dd2c7deea