Closed Bug 1058319 Opened 10 years ago Closed 10 years ago

Add a geolocation-noprompt permission

Categories

(Firefox OS Graveyard :: Runtime, defect)

Product:
Firefox OS Graveyard
Component:
Runtime
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Status:
RESOLVED FIXED
Milestone:
2.1 S3 (29aug)
Project Flags:
blocking-b2g 2.0+
Tracking Flags:
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(2 files, 2 obsolete files)

patch v2
2.09 KB, patch
gwagner
: review+
Details | Diff | Splinter Review
1058319-xpcshell.patch
1.76 KB, patch
fabrice
: review+
Details | Diff | Splinter Review 
That will let certified apps use the geolocation api without prompting the user. This is needed for FMD.
Attached patch geolocation-noprompt.patch (obsolete) — — Splinter Review 

        Attachment #8478694 -
        Flags: review?(anygregor)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        geolocation-noprompt.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Tested by setting geolocation-noprompt in the camera app and checking that the geolocation was included in the picture's meta-data.

        Attachment #8478694 -
        Attachment is obsolete: true

        Attachment #8478694 -
        Flags: review?(anygregor)

        Attachment #8478721 -
        Flags: review?(anygregor)

    
    

    
  
Comment on attachment 8478721 [details] [diff] [review]
geolocation-noprompt.patch

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

Thanks!

        Attachment #8478721 -
        Flags: review?(anygregor) → review+

    
    

    
  
Adding Jonas and Andrew so they are aware that we made this change.
Flags: needinfo?(overholt)
Flags: needinfo?(jonas)

    
    

    
  
[Blocking Requested - why for this release]:
This is a very low risk change that improves the security of the fmd app (see https://bugzilla.mozilla.org/show_bug.cgi?id=1058330#c1).
blocking-b2g: --- → 2.0?

    
    

    
  
Comment on attachment 8478721 [details] [diff] [review]
geolocation-noprompt.patch

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

::: dom/apps/src/PermissionsTable.jsm
@@ +38,5 @@
>                               certified: PROMPT_ACTION
>                             },
> +                           "geolocation-noprompt": {
> +                             app: PROMPT_ACTION,
> +                             privileged: PROMPT_ACTION,

Shouldn't these two be set to DENY? Not that important, but seems like the right thing to do.

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #7)
> Comment on attachment 8478721 [details] [diff] [review]
> geolocation-noprompt.patch
> 
> Review of attachment 8478721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +38,5 @@
> >                               certified: PROMPT_ACTION
> >                             },
> > +                           "geolocation-noprompt": {
> > +                             app: PROMPT_ACTION,
> > +                             privileged: PROMPT_ACTION,
> 
> Shouldn't these two be set to DENY? Not that important, but seems like the
> right thing to do.

We can do that if you really care, but I didn't think of any reason do make it different from the default geolocation one.

    
    

    
  
silently treating geolocation-noprompt as geolocation seems bad. So yeah, I think we should DENY.

(Separately, I wish that we rejected installing any apps that ask for a permission which has DENY for the apps app-type, but that's of course separate)
Flags: needinfo?(jonas)

    
    

    
  
sorry had to backout this change since i guess this caused https://tbpl.mozilla.org/php/getParsedLog.php?id=46751241&tree=B2g-Inbound
Flags: needinfo?(overholt)
blocking-b2g: 2.0? → 2.0+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2
        — 
          — Splinter Review
      

    


  
I changed the permissions to deny for non-certified apps, and fixed an issue we had with substitute permissions in the reverse table. We were overriding the non-substituted version and end up with a bad value in isExplicitInPermissionsTable().

        Attachment #8478721 -
        Attachment is obsolete: true

        Attachment #8480877 -
        Flags: review?(anygregor)

        Attachment #8480877 -
        Flags: review?(anygregor) → review+

    
    

    
  


  
Requires Fabrice's patch

    
  

        Attachment #8480888 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c108c1bcce79
https://hg.mozilla.org/mozilla-central/rev/e7529ddd2e28
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: