KidFox: Restricted profiles - Check global restrictions in isAllowed() (Hide all Sync features and related UI)

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jchaulk, Assigned: sebastian)

Tracking

Trunk
Firefox 42
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Blocks: FFB
OS: Windows 8.1 → Android
Hardware: x86_64 → All
Version: unspecified → Trunk
Note that we can't easily disable the Android settings UI for Sync, but we can do some stuff in Settings.
Component: Profile Handling → Settings and Preferences
(Reporter)

Updated

4 years ago
Blocks: kidfox-v1

Updated

4 years ago
No longer blocks: kidfox-v1

Updated

4 years ago
Blocks: kidfox-v1
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
(In reply to Richard Newman [:rnewman] from comment #1)
> Note that we can't easily disable the Android settings UI for Sync, but we
> can do some stuff in Settings.

The good news is that Android does this for us. I can't add an account from the settings UI when using a restricted profile.
Posted patch 1125293-hide-sync.patch (obsolete) — Splinter Review
This was easier than I thought. We already have the code in place to hide the sync UI if the DISALLOW_MODIFY_ACCOUNTS restriction is enforced by the system. We just need to look into the 'global' user restrictions whenever we check a restriction for a restricted profile.
Attachment #8639909 - Flags: review?(ally)
Comment on attachment 8639909 [details] [diff] [review]
1125293-hide-sync.patch

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

So I gave this a quick run and it works. Yay. That said, since this code doesnt quite match the title of the bug, please update the bug title to more accurately reflect what hte patch actually does; ie check the global restrictions

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +697,5 @@
>  <!ENTITY restriction_disallow_customize_home_title "Disallow customizing home">
>  <!ENTITY restriction_disallow_customize_home_description "Disallow customizing home panels.">
>  <!ENTITY restriction_disallow_private_browsing_title "Disallow private browsing">
>  <!ENTITY restriction_disallow_private_browsing_description "Disallow private browsing mode.">
> +

nit: added whitespace

::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java
@@ +30,5 @@
>      }
>  
>      @Override
>      public boolean isAllowed(Restriction restriction) {
> +        boolean isAllowed = !getAppRestrictions(context).getBoolean(restriction.name, DEFAULT_RESTRICTIONS.contains(restriction));

y'know we have a lot of ! showing up in our patches for this. part of me wonders if it would have been better if we didnt have to invert the values all the time. That said, I'm not about to advocate changing horses midstream this close to the deadline(s).

@@ +31,5 @@
>  
>      @Override
>      public boolean isAllowed(Restriction restriction) {
> +        boolean isAllowed = !getAppRestrictions(context).getBoolean(restriction.name, DEFAULT_RESTRICTIONS.contains(restriction));
> +

nit of nits: is you use isAllowed right way, get rid of this extra newline?
Attachment #8639909 - Flags: review?(ally) → review+
Depends on: 1187260
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #4) 
> y'know we have a lot of ! showing up in our patches for this. part of me
> wonders if it would have been better if we didnt have to invert the values
> all the time. That said, I'm not about to advocate changing horses midstream
> this close to the deadline(s).

Oh yeah, I totally agree. This is mostly because in browser code we ask whether it's allowed to use a specific feature and internally we check if some restrictions are enforced (allowed = !restricted). Also constructs like isAllowed(Restriction.DISALLOW_*) are not really something that is easy to understand. I'll create a follow-up bug to clean that up in 43.
Updated patch to address the nits.
Attachment #8639909 - Attachment is obsolete: true
Attachment #8641010 - Flags: review+
See Also: → 1189336
Summary: KidFox: Restricted profiles - Hide all Sync features and related UI → KidFox: Restricted profiles - Check global restrictions in isAllowed() (Hide all Sync features and related UI)
url:        https://hg.mozilla.org/integration/fx-team/rev/9372bf0fd83e8fdd3c1229874f2e84dc53c3500d
changeset:  9372bf0fd83e8fdd3c1229874f2e84dc53c3500d
user:       Sebastian Kaspari <s.kaspari@gmail.com>
date:       Thu Jul 30 20:00:16 2015 +0200
description:
Bug 1125293 - Restricted profiles: Check global restrictions in isAllowed(). r=ally

This effectively hides all sync features and related UI for restricted profiles
because this is controlled by the DISALLOW_MODIFY_ACCOUNTS restriction of the
system.
https://hg.mozilla.org/mozilla-central/rev/9372bf0fd83e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.