Closed Bug 1538270 Opened 7 months ago Closed 5 months ago

Fennec allows upload of files after denying runtime storage permissions

Categories

(Firefox for Android :: General, enhancement, P3)

Firefox 68
enhancement

Tracking

()

VERIFIED FIXED
Firefox 69
Tracking Status
firefox67 --- wontfix
firefox68 --- verified
firefox69 --- verified

People

(Reporter: colee, Assigned: brad.arant)

References

(Regression)

Details

Attachments

(2 files, 1 obsolete file)

On the latest Fennec Nightly, attempting to upload a file while explicitly denying storage permissions still allows the file selection and upload to proceed as normal if chosen from internal storage or a 3rd party app, even on Oreo, Pie, and Android Q. While this fits the Android permissions model, it's confusing to users.

Also occurs in Firefox Fennec 65.0.1, Pixel 3, Pie.

Summary: Fennec Nightly allows upload of files after denying runtime storage permissions → Fennec allows upload of files after denying runtime storage permissions

This behavior was introduced by 1362919.

Blocks: 1362919

While this fits the Android permissions model, it's confusing to users.

So you're saying, this is how it works with other apps?

Andreas, what do you think? Is this something you think is acceptable or should we change the behavior (or messaging to users)?

Flags: needinfo?(abovens)
No longer blocks: 1362919
Priority: -- → P2
Regressed by: 1362919
Flags: needinfo?(abovens)

Video for the two situations - https://drive.google.com/file/d/1aC-p961cInOPzoq7EzrpZ2TYpaKA2yxv

  • if permissions are denied - we fallback to the default file picker that only allows uploading existing files
  • if permissions are granted - we offer user all possible options to upload files, even from camera, sound recorder, etc.

As Liz said maybe just presenting a Toast with a message similar to "Permissions denied: Fallback to system file picker" would help users understand what's happening.
I see both Chrome and Opera fallback to the default file picker. Without informing the users about this in any way.

As Liz said maybe just presenting a Toast with a message similar to "Permissions denied: Fallback to system file picker" would help users understand what's happening.

Sure, let's try that messaging approach please.

Discussed today, setting as lower priority as this is how other apps behave, though.

Type: defect → enhancement
Priority: P2 → P3
Assignee: nobody → brad.arant

In file FilePicker.java line 92 there is code that runs when there is an authorization failure that displays the default file picker. I inserted the toast message:

      "Access denied to device. Showing default selection."

As indicated in previous messages a toast message will be added as the user enters the default file picker.

I have created bug 1552729 to provide for text finalization and translation.

Also, text changed to "Permissions denied: Fallback to system file picker" per request.

Updated text per request: Required permissions not granted, reverting to the system file picker.

Keywords: checkin-needed

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81e2959af690
Show toast for default file picker when permissions denied.;r=VladBaicu

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69

Why is there a trailing space in the string?

More important, is Fennec 69 a thing? My understanding is that Fennec would stop at 68.

https://docs.google.com/document/d/1oRPkwP3l7QzdQYj0Wn7d_3EfTZaakocA_i_pGKlG0dI/edit?usp=sharing says that stopping at 68 is the plan, but that under unforeseen circumstances the whole plan may yet be abandoned as long as the Firefox 71 development cycle hasn't yet started.

So my interpretation of that would be that up until that point, all Fennec changes should land normally in mozilla-central in case the transition is aborted again, but of course also need to be uplifted to 68 in order to actually ship in case the transition goes ahead as planned.

@Brad:

  1. Flod's question regarding the string.
  2. Once that is sorted out, this'll need to be uplifted to Beta 68.
Flags: needinfo?(brad.arant)

Submitted new patch since old one was approved and applied. Removal of space at end of message.

Flags: needinfo?(brad.arant)

We'll want to uplift this fix to Fennec 68 Beta. Fennec will move to the ESR 68 channel so we need to uplift Fennec fixes from 69 to 68 if we want to ship them to users.

Attachment #9066119 - Attachment is obsolete: true

Please also publish D32411 to correct extra space in message.

Keywords: checkin-needed

Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20709a491a1e
Removal of extra space at end of message.;r=VladBaicu

Keywords: checkin-needed

Brad, can you please request uplift of your patches to Fennec 68 Beta? We'll be supporting Fennec 68 in ESR until mid-2020 so it would be nice to have your fix.

Flags: needinfo?(brad.arant)

Comment on attachment 9066566 [details]
Bug 1538270 - Show toast for default file picker when permissions denied.;r?VladBaicu

Beta/Release Uplift Approval Request

  • User impact if declined: Continued user confusion when default file picker is displayed.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Go to page where file can be uploaded.
    Upload file.
    When asked for permissions to storage deny the request.
    Default picker is displayed.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just a toast message. No logic change.
  • String changes made/needed:
Flags: needinfo?(brad.arant)
Attachment #9066566 - Flags: approval-mozilla-beta?
Attachment #9067222 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

This adds new strings, so uplift requires approval from l10n.

Flags: needinfo?(francesco.lodolo)

At this point, please consider Fennec changes whitelisted, they all need to be uplifted to beta (and esr in the future)

Flags: needinfo?(francesco.lodolo)

Comment on attachment 9066566 [details]
Bug 1538270 - Show toast for default file picker when permissions denied.;r?VladBaicu

fennec fix, for beta68

Attachment #9066566 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9067222 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified as fixed on the latest Nightly 68.0a1 (2019-06-05) using Samsung Galaxy S8 (Android 9) and Nokia 6 (Android 7.1.1). I'll let the qe-verify + till the verification on Beta, thanks.

Verified as fixed on Nightly 69.0a1 (2019-06-10), Beta 68.0b9 with Nexus 6P (Android 8.1.0).
Due to my findings, I'll mark this issue as verified on Firefox 68 and 69. Thanks.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.