Closed Bug 810427 Opened 8 years ago Closed 8 years ago

Device storage - Add access fields to permission checks

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

Attachments

(1 file)

Currently we only check for "device-storage:music" for example. We have to add access fields like "read", "write" or "create"
blocking-basecamp: --- → ?
Blocks: 809944
Assignee: nobody → doug.turner
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.
(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)
Attached patch patchSplinter Review
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".
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+
(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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
blocking-basecamp: ? → +
Please don't merge to aurora without bug 813357
Depends on: 813357
Ok this got merged with the uplift. We need 813357 on aurora asap.
You need to log in before you can comment on or make changes to this bug.