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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Comment 1

[Richard Newman [:rnewman]]

Note that we can't easily disable the Android settings UI for Sync, but we can do some stuff in Settings.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1125293-hide-sync.patch

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.

Comment 4

[Allison Naaktgeboren :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?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1125293-hide-sync-v2.patch

Updated patch to address the nits.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/9372bf0fd83e8fdd3c1229874f2e84dc53c3500d
changeset:  9372bf0fd83e8fdd3c1229874f2e84dc53c3500d
user:       Sebastian Kaspari <[e-mail]>
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.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9372bf0fd83e