Closed Bug 1042715 Opened 5 years ago Closed 5 years ago

Add support for Restricted Profiles

Categories

(Firefox for Android :: Profile Handling, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
relnote-firefox --- 34+

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Component: General → Profile Handling
OS: Linux → Android
Hardware: x86_64 → All
See Also: → 995070, 995430
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;
}
Attached patch restricted-profiles v0.1 (obsolete) — Splinter Review
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.
Assignee: nobody → mark.finkle
Attachment #8461117 - Flags: review?(rnewman)
Shows the header being sent
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.
Status: NEW → ASSIGNED
Depends on: 1030265, 1042829
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!?
Attachment #8461117 - Flags: review?(rnewman) → feedback+
(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!?
> 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.
* 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.
Attachment #8461117 - Attachment is obsolete: true
Attachment #8461275 - Flags: review?(rnewman)
Attachment #8461275 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/fbb338297264
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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.
Attachment #8461275 - Flags: approval-mozilla-beta?
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?
Attachment #8461275 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(kbrosnan)
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)]:
relnote-firefox: --- → ?
Needs rebasing for beta uplift.
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
Flags: needinfo?(kbrosnan)
(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.
Added to the release notes with "Added support for Prefer:Safe HTTP header" as wording.
You need to log in before you can comment on or make changes to this bug.