810427 - Device storage - Add access fields to permission checks

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

Currently we only check for "device-storage:music" for example. We have to add access fields like "read", "write" or "create"

Comment 1

[Gregor Wagner [:gwagner]]

I had to add the access field to the nsIContentPermissionRequest for bug 811026. So part of the implementation is already there. We should talk how/if we should split this.

Comment 2

[Doug Turner (:dougt)]

feel free to steal back from me!  I assigned it to myself so we wouldn't lose it.

Comment 3

[Gregor Wagner [:gwagner]]

(In reply to Doug Turner (:dougt) from comment #2)
> feel free to steal back from me!  I assigned it to myself so we wouldn't
> lose it.

Working on it.

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

We don't have a good way to check for "create" a file right?

I have to extend nsIContentPermissionRequest in order to make prompting work for contacts. So all these changes are needed just to make the ipdl stuff work. If we decide not to check for access fields for device storage we just have to make DeviceStorageTypeChecker::GetAccessForRequest return "undefined".

Comment 5

[Gregor Wagner [:gwagner]]

Push to try: https://tbpl.mozilla.org/?tree=Try&rev=8ef0f77ab439

Comment 6

[Doug Turner (:dougt)]

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

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

r+ with the nits fixed, a new uuid, and a change to the "undefined" access.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2074,5 @@
>      if (NS_FAILED(rv)) {
>        return rv;
>      }
> +    nsCString access = NS_LITERAL_CSTRING("read");
> +    child->SendPContentPermissionRequestConstructor(r, type, access, IPC::Principal(mPrincipal));

do you the temporary nsCString?  Can you just 

child->SendPContentPermissionRequestConstructor(r, type, NS_LITERAL_CSTRING("read"), IPC::Principal(mPrincipal));

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +25,5 @@
>    /**
> +   *  The access of the permission request, such as
> +   *  "read".
> +   */
> +  readonly attribute ACString access;

Bump the uuid

::: dom/src/geolocation/nsGeolocation.cpp
@@ +355,5 @@
>  
>  NS_IMETHODIMP
> +nsGeolocationRequest::GetAccess(nsACString & aAccess)
> +{
> +  aAccess = "undefined";

shouldn't this be "read"?

@@ +1421,5 @@
>      request->AddRef();
>  
>      nsCString type = NS_LITERAL_CSTRING("geolocation");
> +    nsCString access = NS_LITERAL_CSTRING("undefined");
> +    child->SendPContentPermissionRequestConstructor(request, type, access, IPC::Principal(mPrincipal));

just ditch the local varibles if you can.

::: dom/src/notification/nsDesktopNotification.cpp
@@ +106,5 @@
>      // Corresponding release occurs in DeallocPContentPermissionRequest.
>      nsRefPtr<nsDesktopNotificationRequest> copy = request;
>  
>      nsCString type = NS_LITERAL_CSTRING("desktop-notification");
> +    nsCString access = NS_LITERAL_CSTRING("undefined");

shouldn't this be "write"?

(also ditch the local vars if you can)

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Doug Turner (:dougt) from comment #6)
> 
> ::: dom/src/notification/nsDesktopNotification.cpp
> @@ +106,5 @@
> >      // Corresponding release occurs in DeallocPContentPermissionRequest.
> >      nsRefPtr<nsDesktopNotificationRequest> copy = request;
> >  
> >      nsCString type = NS_LITERAL_CSTRING("desktop-notification");
> > +    nsCString access = NS_LITERAL_CSTRING("undefined");
> 
> shouldn't this be "write"?
> 

We don't have access fields defined for desktop-notification and geolocation so I set them to "undefined". When we check for the permission we need a way to specify if we should use the access field or not.

Comment 8

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ac91e2b924

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f0ac91e2b924

Comment 10

[Gregor Wagner [:gwagner]]

Please don't merge to aurora without bug 813357

Comment 11

[Gregor Wagner [:gwagner]]

Ok this got merged with the uplift. We need 813357 on aurora asap.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e9d7bccb9b58