Device storage - Add access fields to permission checks

RESOLVED FIXED in Firefox 18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

unspecified
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

6 years ago
blocking-basecamp: --- → ?
(Assignee)

Updated

6 years ago
Blocks: 809944
Assignee: nobody → doug.turner
(Assignee)

Comment 1

6 years ago
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.
feel free to steal back from me!  I assigned it to myself so we wouldn't lose it.
(Assignee)

Comment 3

6 years ago
(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.
Assignee: doug.turner → anygregor
Flags: needinfo?(jonas)
(Assignee)

Comment 4

6 years ago
Created attachment 681136 [details] [diff] [review]
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".
(Assignee)

Updated

6 years ago
Attachment #681136 - Flags: review?(doug.turner)
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)
Attachment #681136 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 7

6 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/f0ac91e2b924
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

6 years ago
blocking-basecamp: ? → +
(Assignee)

Comment 10

6 years ago
Please don't merge to aurora without bug 813357
Depends on: 813357
(Assignee)

Comment 11

6 years ago
Ok this got merged with the uplift. We need 813357 on aurora asap.
https://hg.mozilla.org/releases/mozilla-beta/rev/e9d7bccb9b58
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.