[Lint: MissingPermission] Missing check for Permissions

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: vivek, Assigned: maurya1985)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Handle permission checks graceful considering that the user may have revoked some permissions in Android 6.0

GPSScanner.java [1], [2], [3]--> Permission checks are handled in StumberService [4], so add annotation for MissingPermission to the respective method block


GekoAppShell.java [5], [6] -> similar to the above, add annotations to method block. The permission are checked for at [7]



[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java?from=GPSScanner.java#85
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java?from=GPSScanner.java#136
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java?from=GPSScanner.java#141
[4] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java?from=StumblerService.java#183
[5] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#484
[6] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#546
[6] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#509
Ah, it's great that we get this from lint.
(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> Ah, it's great that we get this from lint.

Yeah, this is hot.  This and making sure we get the version checking correct is my motivation for improving lint support :)
(Assignee)

Comment 3

3 years ago
Could you please assign this bug to me? :)
We assign bugs once there is a patch attached. The build instructions for Android are at https://developer.mozilla.org/en-US/docs/Simple_Firefox_for_Android_build and if you need help irc://irc.mozilla.org/mobile is a good resource.
(Assignee)

Comment 5

3 years ago
Vivek,I don't get these warnings in the report generated when I run lint (./mach gradle lint). Am I missing something while running lint?
Flags: needinfo?(vivekb.balakrishnan)
(In reply to Maurya Talisetti from comment #5)
> Vivek,I don't get these warnings in the report generated when I run lint
> (./mach gradle lint). Am I missing something while running lint?

Post Bug 1233882, these may no longer be present.  I'll check this and close if necessary.  Thanks for digging into it!
Flags: needinfo?(vivekb.balakrishnan)
(Reporter)

Comment 7

3 years ago
@Nick: I remember filing the bug after applying your tentative patch for bug 1233882. And I can reproduce the same warnings with latest fx-team.

@Maurya: Can you check the following link after running ./mach gradle lint
path to objdir-droid/gradle/build/mobile/android/app/outputs/lint-results.html#MissingPermission
(Assignee)

Comment 8

3 years ago
Vivek, can you help me understand what fx-team is? I don't see any branch with that name in http://hg.mozilla.org/mozilla-central/branches.

I got the latest code from http://hg.mozilla.org/mozilla-central/ and ran `./mach gradle lint` on the default branch.
There's no item classified under MissingPermission. The report is [here](https://pastebin.mozilla.org/8860501) and the command line output is [here](https://pastebin.mozilla.org/8860502).
Flags: needinfo?(vivekb.balakrishnan)
(In reply to Maurya Talisetti from comment #8)
> Vivek, can you help me understand what fx-team is? I don't see any branch
> with that name in http://hg.mozilla.org/mozilla-central/branches.

Maurya - fx-team is an integration branch.  See https://wiki.mozilla.org/Tree_Rules/Integration.

> I got the latest code from http://hg.mozilla.org/mozilla-central/ and ran
> `./mach gradle lint` on the default branch.

I think this is sufficient because the relevant code has merged to mozilla-central, but would you mind reporting what commit your m-c is at?  Then we can know for sure.  You can find out what your m-c is at by running |hg parent|.

> There's no item classified under MissingPermission. The report is
> [here](https://pastebin.mozilla.org/8860501) and the command line output is
> [here](https://pastebin.mozilla.org/8860502).

That would be great!  I think this will be correct, due to the new projects "seeing" the full set of permissions now.
(Assignee)

Comment 10

3 years ago
Nick, thanks for those suggestions. My parent wasn't at the tip. I've updated it now.

However, I'm now not able to run `./mach gradle lint` anymore. Not able to run `./mach gradle <target>:lint` either. The the error I see is here (https://pastebin.mozilla.org/8860609) . Any idea if the command options changed?
(Assignee)

Comment 11

3 years ago
Never mind my question about not being able to run `./mach gradle lint`. I figured I had to rebuild after pulling the latest changes.

I'm able to see GPSScanner and GeckoAppShell listed in the lint report. I'll add the necessary annotations and regenerate the report to ensure they don't appear again.
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 12

3 years ago
Created attachment 8721948 [details] [diff] [review]
suppress-lint-permission-check
Attachment #8721948 - Flags: review?(vivekb.balakrishnan)
(Reporter)

Comment 13

3 years ago
Comment on attachment 8721948 [details] [diff] [review]
suppress-lint-permission-check

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

LGTM
Attachment #8721948 - Flags: review?(vivekb.balakrishnan)
Attachment #8721948 - Flags: review?(nalexander)
Attachment #8721948 - Flags: review+
Comment on attachment 8721948 [details] [diff] [review]
suppress-lint-permission-check

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

Can I get some context on why it's okay to suppress the check?  I assume we are requesting the permission (or have it in the manifest) and this is guarded by some feature flag?
Attachment #8721948 - Flags: review?(nalexander)
(Reporter)

Comment 15

3 years ago
Hi Nick, 
Nick,

It is safe to add the suppress lint annotations for both GeckoAppShell and StumblerService. 
* In GeckoAppShell, we explicitly check for the permission using PermissionBlock [1]
* In case of StumblerService, it is not intuitive at first glance, but the permissions are checked for in StumblerService which instantiate the GpsScanner [2]. 

Please let me know if this needs to be improved

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#509
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java#183
(In reply to Vivek Balakrishnan[:vivek] from comment #15)
> Hi Nick, 
> Nick,
> 
> It is safe to add the suppress lint annotations for both GeckoAppShell and
> StumblerService. 
> * In GeckoAppShell, we explicitly check for the permission using
> PermissionBlock [1]
> * In case of StumblerService, it is not intuitive at first glance, but the
> permissions are checked for in StumblerService which instantiate the
> GpsScanner [2]. 
> 
> Please let me know if this needs to be improved
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/GeckoAppShell.java?from=GeckoAppShell.java#509
> [2]
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/
> org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java#183

Very good -- perhaps add comments to this effect in the patch?  Roll on!
(Assignee)

Comment 17

3 years ago
Sure I'll add comments and update the patch.
(Assignee)

Comment 18

3 years ago
Created attachment 8726703 [details] [diff] [review]
suppress-lint-permission-check

added comments to annotations
Attachment #8726703 - Flags: review?(nalexander)
(Assignee)

Updated

3 years ago
Attachment #8721948 - Attachment is obsolete: true
Comment on attachment 8726703 [details] [diff] [review]
suppress-lint-permission-check

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

lgmt.  Thanks, Maurya!
Attachment #8726703 - Flags: review?(nalexander) → review+
(Assignee)

Comment 20

3 years ago
Thanks Nick! Would you be able to vouch for me so that I can get Level 1 (try server) access?

https://bugzilla.mozilla.org/show_bug.cgi?id=1254409

You'd need to add a comment saying that you support my application. (See https://www.mozilla.org/en-US/about/governance/policies/commit/)
Flags: needinfo?(nalexander)
Vouched!  Clearing NI.
Flags: needinfo?(nalexander)
(Assignee)

Comment 23

3 years ago
Nick, would you be able to assign this to me before marking as resolved!

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c4c637484ba
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(Assignee)

Comment 25

3 years ago
Nick, can you add me to the "Assigned To" field?
Flags: needinfo?(nalexander)
Drive-by assign! :)
Assignee: nobody → maurya1985
Flags: needinfo?(nalexander)
(Assignee)

Comment 27

3 years ago
Thanks Sebastian :)
(In reply to Maurya Talisetti from comment #25)
> Nick, can you add me to the "Assigned To" field?

Maurya -- thanks for all your help with these lint issues.  I've given you canconfirm and editbugs Bugzilla rights.  That'll let you assign yourself, and also change most other fields.  Use your new powers wisely :)
(Assignee)

Comment 29

3 years ago
Awesome, thank you very much Nick :)
Summary: [Lint] Missing check for Permissions → [Lint: MissingPermission] Missing check for Permissions
You need to log in before you can comment on or make changes to this bug.