1042715 - Add support for Restricted Profiles

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Profile Handling, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Firefox should be aware that it's being run from a Restricted Profile on Android. We can then make other runtime changes to the application.

Restricted profiles:
https://developer.android.com/about/versions/android-4.3.html#RestrictedProfiles

Possible runtime changes:
* Implement nsIParentalControls which will control:
  * Send the "Prefer: safe" header
  * Block downloading files
* Block add-on installation
* Block webapp installation
* Block access to add-ons, downloads, apps about: pages

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

We can also disable Sync using code like this:

@TargetApi(18)
public static boolean shouldDisableSync(Context context) {
    if (Build.VERSION.SDK_INT >= 18) {
        UserManager um = (UserManager) context.getSystemService(Context.USER_SERVICE);
        Bundle restrictions = um.getUserRestrictions();

        return restrictions.getBoolean(UserManager.DISALLOW_MODIFY_ACCOUNTS, false);
    }
    return false;
}

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: restricted-profiles v0.1

This patch lands the basics, like the OS X support already in the tree:
* Add JNI helpers for getting restriction info
* Add an Android impl of nsIParentalControlsService that uses the helpers
* Add a simple test to verify that nsIParentalControlsService is available and functional on Android

This is enough to send the "Prefer: safe" header, as verified in a restricted profile. The service and JNI helpers could be used to add more functional it in other places of the app too. I'll leave that for followup bugs.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fennec-restricted-headers.png

Shows the header being sent

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I forgot to mention. This patch requires VERSION.SDK_INT >= 18, so we'll need the builders updated. I think there is already a bug filed for that.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8461117 [details] [diff] [review]
restricted-profiles v0.1

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

This looks fine, with the exception of the API19 dependency; f+ waiting for that.

Automated tests for this are hard. I'm not convinced that it's worth the mocking effort to make it testable.

::: mobile/android/base/GeckoAppShell.java
@@ +2522,5 @@
>      }
>  
> +    @WrapElementForJNI
> +    public static boolean isUserRestricted() {
> +        if (Build.VERSION.SDK_INT >= 18) {

Invert, early return.

@@ +2526,5 @@
> +        if (Build.VERSION.SDK_INT >= 18) {
> +            UserManager mgr = (UserManager)getContext().getSystemService(Context.USER_SERVICE);
> +            Bundle restrictions = mgr.getUserRestrictions();
> +
> +            return (restrictions.keySet().size() > 0);

I think you can just do

  return !restrictions.isEmpty();

@@ +2534,5 @@
> +
> +    @WrapElementForJNI
> +    public static String getUserRestrictions() {
> +        JSONObject json = new JSONObject();
> +        if (Build.VERSION.SDK_INT >= 18) {

if (Build.VERSION.SDK_INT < 18) {
    return "{}";
}

@@ +2541,5 @@
> +
> +            Set<String> keys = restrictions.keySet();
> +            for (String key : keys) {
> +                try {
> +                    json.put(key, JSONObject.wrap(restrictions.get(key)));

JSONObject.wrap was added in API 19. You need to find another way.

You should also check for JSONObject.wrap() == JSONObject.NULL here, and avoid adding the mapping.

::: toolkit/components/parentalcontrols/nsParentalControlsServiceAndroid.cpp
@@ +42,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsParentalControlsService::Log(int16_t aEntryType,
> +                               bool blocked,

aBlocked?

@@ +51,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsParentalControlsService::RequestURIOverride(nsIURI *aTarget,
> +                                              nsIInterfaceRequestor *aCocoadowContext,

aCocoadowContext!?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #5)

> Automated tests for this are hard. I'm not convinced that it's worth the
> mocking effort to make it testable.

I have an idea for that I can try tonight

> ::: mobile/android/base/GeckoAppShell.java

> > +    public static boolean isUserRestricted() {
> > +        if (Build.VERSION.SDK_INT >= 18) {
> 
> Invert, early return.

Done

> > +            return (restrictions.keySet().size() > 0);
> 
> I think you can just do
> 
>   return !restrictions.isEmpty();

Done

> > +    public static String getUserRestrictions() {
> > +        JSONObject json = new JSONObject();
> > +        if (Build.VERSION.SDK_INT >= 18) {
> 
> if (Build.VERSION.SDK_INT < 18) {
>     return "{}";
> }

Done

> > +                    json.put(key, JSONObject.wrap(restrictions.get(key)));
> 
> JSONObject.wrap was added in API 19. You need to find another way.
> 
> You should also check for JSONObject.wrap() == JSONObject.NULL here, and
> avoid adding the mapping.

I just went with a simple:
json.put(key, restrictions.get(key));

Which won't handle Collections or Maps, but the bundle should only hold simple types anyway. Worst case, we drop this method. It's just a helper for JS code to get the restrictions.


> ::: toolkit/components/parentalcontrols/nsParentalControlsServiceAndroid.cpp
> > +nsParentalControlsService::Log(int16_t aEntryType,
> > +                               bool blocked,
> 
> aBlocked?
> 
> > +nsParentalControlsService::RequestURIOverride(nsIURI *aTarget,
> > +                                              nsIInterfaceRequestor *aCocoadowContext,
> 
> aCocoadowContext!?

Comment 7

[Richard Newman [:rnewman]]

> aCocoadowContext!?

Hah.

aWindowContext
:%s,Win,Cocoa

Someone a long time ago failed to double-check a patch.

Might as well fix this while you're here, rather than cargo-culting it into the future.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: restricted-profiles v0.2

* Updated based on comments
* I tried to add another test, using a Mock nsIParentalControlsService. It worked for any code getting an instance of the service after I injected the Mock, but the nsHttpHandler code is initialized very early and caches the enabled state before my Mock could take affect. Oh well.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Try looks green:
https://tbpl.mozilla.org/?tree=Try&rev=386a0c2f9130

https://hg.mozilla.org/integration/fx-team/rev/fbb338297264

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbb338297264

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8461275 [details] [diff] [review]
restricted-profiles v0.2

Approval Request Comment

[Feature/regressing bug #]:
New.

[User impact if declined]:
Don't get the feature.

[Describe test coverage new/current, TBPL]:
Adds a test.

[Risks and why]: 
Finkle and I discussed uplifting this a week or so ago. If it's not giving us any trouble, let's get this into 33. It's partner-relevant.

[String/UUID change made/needed]:
None that I can see.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8461275 [details] [diff] [review]
restricted-profiles v0.2

I spoke with Kar and there is a good reason to take this desktop parity feature in 33. Beta+

Kevin - This is a feature uplift. Is there an existing restricted profile test plan that you can use to test this feature in beta2? Can you also please request that your test do some exploratory testing of this feature this week so that we can try to flush out any issues earlier in beta?

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Adds support for Prefer:Safe HTTP header.
[Suggested wording]: Added support for Prefer:Safe HTTP header
[Links (documentation, blog post, etc)]:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for beta uplift.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Needs rebasing for beta uplift.

I rebased the patch and I'm making a test build.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Looks like Mark landed this on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/080344c7c80b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/fcf16a67fed4

https://tbpl.mozilla.org/php/getParsedLog.php?id=47629536&tree=Mozilla-Beta

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Backed out for bustage.
> https://hg.mozilla.org/releases/mozilla-beta/rev/fcf16a67fed4
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47629536&tree=Mozilla-Beta

Fx33 (Beta) is not using API version 20. It's using 17:
http://mxr.mozilla.org/mozilla-beta/source/mobile/android/config/mozconfigs/common#18

I don't feel like bumping the SDK version on Beta. Let's leave this in Fx34 (Aurora) to ride the trains.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Mark Finkle (:mfinkle) from comment #18)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> > Backed out for bustage.
> > https://hg.mozilla.org/releases/mozilla-beta/rev/fcf16a67fed4
> > 
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=47629536&tree=Mozilla-Beta
> 
> Fx33 (Beta) is not using API version 20. It's using 17:
> http://mxr.mozilla.org/mozilla-beta/source/mobile/android/config/mozconfigs/
> common#18
> 
> I don't feel like bumping the SDK version on Beta. Let's leave this in Fx34
> (Aurora) to ride the trains.

I appreciate you taking the safe approach here Mark. 

cc Karen, who should know that this feature will not ship in 33.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Added support for Prefer:Safe HTTP header" as wording.