1437871 - Release and Beta share granted runtime permissions

Status: RESOLVED INVALID

(GeckoView :: General, defect, P5)

Description

[Sorina Florean [:sflorean]]

Environment: 
Device:  Samsung Galaxy Tab S3 (Android 7.0), Google Pixel (Android 8.1.0), Samsung Galaxy S8 (Android 7.0);
Build: 59.0b9;

Steps to reproduce:
1. Have a build of Beta installed on device;
2. Enable all the permisions camera, storage;
3. Uninstall Firefox;
4. Install again Firefox;
5. Download a file; or check permissions in phone settings.

Expected result:
Permissions are not enabled and user is giving access to storage if he wants to download some files.

Actual result:
Permissions remain enabled. The prompt is not displayed and the build has access to storage.

Notes:release build is installed on the devices.
see video: https://drive.google.com/open?id=1tJXxppm6kMSJukFJyLLmdNgVA17MIv_q

Comment 1

[Nick Alexander :nalexander [he/him]]

My guess is that this is part of Google's breaking changes to builds that have an android:sharedUserId.  grisha will likely know more.

I think we need to reach out to Google and figure out if there's a path away from sharedUserId for Firefox for Android, 'cuz they're breaking us pretty hard.  snorp: any ideas who we could talk to?

Comment 2

[Jan Henning [:JanH]]

No need to guess, as far as I know the Android permissions system has always worked based on UIDs: https://groups.google.com/forum/#!topic/android-security-discuss/eLPuV6uLQw8
So runtime permissions behaving this way is simply a natural consequence of that and I wouldn't call that a breaking change - or is there anything I'm not aware of?

(Also Firefox 60 will be affected, too, as soon as it reaches Beta).

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah I think this is because of the shared UID, but as JanH says that's probably just be how it's supposed to work. I don't know of anyone at Google that could help us with this, but I think the burden is going to fall entirely on us to figure it out. Didn't we use to need sharedUserId for Sync? Is that no longer true? What will break if we just remove sharedUserId in 60?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> Yeah I think this is because of the shared UID, but as JanH says that's
> probably just be how it's supposed to work. I don't know of anyone at Google
> that could help us with this, but I think the burden is going to fall
> entirely on us to figure it out. Didn't we use to need sharedUserId for
> Sync?

Correct.  The idea was to be able to migrate Sync details from the initial Firefox for Android beta (Fennec 14?) to the very first native Release (Fennec 15, IIRC).

> Is that no longer true?

Correct -- there's no advantage to the sharedUserId at this point.

 What will break if we just remove sharedUserId
> in 60?

Everything :(  To the best of my knowledge, changing sharedUserId is not possible: we'd need every existing user in the channel that is changed (presumably, Beta, since it has fewer users) to reinstall.  This is why we should talk to Google -- perhaps there's a way to do this that I don't know about, or there's some other mechanism through the Google Play Store that can help us.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Nick Alexander :nalexander from comment #4)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
>  What will break if we just remove sharedUserId
> > in 60?
> 
> Everything :(  To the best of my knowledge, changing sharedUserId is not
> possible: we'd need every existing user in the channel that is changed
> (presumably, Beta, since it has fewer users) to reinstall.  This is why we
> should talk to Google -- perhaps there's a way to do this that I don't know
> about, or there's some other mechanism through the Google Play Store that
> can help us.

To be specific, you believe changing this would allocate a new user ID and data directory upon update, losing all of the user's data/settings/etc? I'll try to find someone to ask about this.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Oh no, it's even worse -- Android allocates a UID but does not give you a new data directory, so you don't have permission to read/write anything there. Sigh....

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

The bug here is looking pretty bleak, like most of the stuff filed in the Android tracker: https://issuetracker.google.com/issues/36905922

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] This only affects users with Release and Beta installed, which I expect is a small part of our audience (though this is pretty serious if not).

Comment 10

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

Looks like this might still be relevant.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Agi Sferro | :agi | ⏰ PST | he/him from comment #11)

Looks like this might still be relevant.

No, I don't think it is. This was specifically about android:sharedUserId and permissions that would ideally have been per-Android package but which, to allow upgrade from org.mozilla.firefox_beta to org.mozilla.firefox, where not scoped per-Android package.

None of this applies to GeckoView. Sadly, some of these mistakes needed to be perpetuated in Fenix -- see, for example, the android:sharedUserId in https://searchfox.org/mozilla-mobile/rev/0473592082a68f23da6ce6623be976c421ccab11/fenix/app/src/migration/AndroidManifest.xml#3. But the per-Android package permissions, I think, have been avoided.