Closed Bug 1074496 Opened 6 years ago Closed 6 years ago

Disable import from Android in Guest mode


(Firefox for Android :: General, defect)

Not set



Firefox 36
Tracking Status
firefox35 --- ?
firefox36 --- verified
fennec 35+ ---


(Reporter: wesj, Assigned: wesj)




(1 file)

This is a potential security breach on the lock screen. I'm pretty close to saying lets just disable all settings:
Setting tracking-35, 'cos that's where lockscreen shipped.
tracking-fennec: --- → 35+
OS: Mac OS X → Android
Hardware: x86 → All
Attached patch PatchSplinter Review
This blocks this pref and the remote debugging pref. Looking through other settings:

  Home/Search/Tabs/Updates (why is Updates shown on release?)
  Nothing looks too dangerous to me here

  Test size/Title bar/Scroll title/Char encoding/Plugins
  Only plugins sounds dangerous. Not dangerous enough I'm worried though.

  Tracking/Cookies/Remember passwords/Master password/Clear private data
  Master password seems kinda useless here, but nothing dangerous.

  Nothing bad?

  AboutFirefox/FAQS/Feedback/Product announcements/Telemetry/Crash reporter/Health Report
  I thought about disabling Telemetry or health report, but I guess they're kinda useful to us?

Dev tools
  Paint flashing
Attachment #8497160 - Flags: review?(liuche)
Attachment #8497160 - Flags: review?(liuche) → review?(rnewman)
Comment on attachment 8497160 [details] [diff] [review]

Review of attachment 8497160 [details] [diff] [review]:

This looks good to me.

::: mobile/android/base/preferences/
@@ +19,5 @@
>  import android.util.Log;
>  class AndroidImportPreference extends MultiPrefMultiChoicePreference {
>      static final private String LOGTAG = "AndroidImport";
> +    public static final String PREF = "android.not_a_preference.import_android";

Nit: a better name than PREF? PREF_NAME perhaps?

@@ +28,5 @@
> +        public boolean setupPref(Context context, Preference pref) {
> +            return RestrictedProfiles.isAllowed(Restriction.DISALLOW_IMPORT_SETTINGS);
> +        }
> +
> +        public void onChange(Context context, Preference pref, Object newValue) { }

Nit: single space inside braces.
Attachment #8497160 - Flags: review?(rnewman) → review+
Comment on attachment 8497160 [details] [diff] [review]

Review of attachment 8497160 [details] [diff] [review]:

::: mobile/android/base/
@@ +85,1 @@
>          if (GeckoAppShell.getGeckoInterface().getProfile().inGuestMode()) {

Your tree is out of date. I already fixed this :D

::: mobile/android/base/preferences/
@@ +25,5 @@
>      private Context mContext;
> +    public static class Handler implements GeckoPreferences.PrefHandler {
> +        public boolean setupPref(Context context, Preference pref) {
> +            return RestrictedProfiles.isAllowed(Restriction.DISALLOW_IMPORT_SETTINGS);

Is there a reason for doing this here (creating a new inner class) rather than just continuing with the switch statement removal approach?
No. I just hate that all of our pref handling is all over one massive file full of switch statements. I wanted to pull them all apart at one point in bug 1022103 (also bug 967376 never got finished). If you hate the idea, I'd love to know :)
I'd like a better approach.

(I've thought about just loading prefs screens from different files (or pre-processed files), rather than selectively hiding bits and pieces of a single master screen. That would allow us to do stuff like lift out lonely orphan preferences if a subtree isn't needed, etc. etc.)

That said, 80% of a sub-optimal approach and 20% of a good one is perhaps worse than being consistent with the sub-optimal one, but I trust your judgment!
Assignee: nobody → wjohnston
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Target Milestone: Firefox 35 → Firefox 36
Is this wanted for Fennec 35?
Flags: qe-verify+
Wes: if you want to uplift this to 35, you'll need to apply relevant bits of Bug 1078395 on top of your patch. That's being uplifted first, so some of its changes don't (yet) apply to Aurora.

(If you don't, it won't build!)
Flags: needinfo?(wjohnston)
Verified as fixed in:
Build: Firefox for Android 36.a01 (2014-11-09)
Device: Nexus 4 (Android 4.4.4)
Flags: needinfo?(wjohnston)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.