1099419 - [Lockscreen][Camera] App permission screen stays after locking and unlocking device

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20141114_Overlay.txt

Description:
When the user locks the screen from App Permission overlay on Camera and unlocks the device, the overlay is still displayed.
   
Repro Steps:
1) Update a Flame device to BuildID: 20141114001204
2) Go to Settings > Screen Lock > enable Lock Screen and Passcode Lock.
3) Restart the device.
4) On the lockscreen, swipe left to the camera icon.
5) When App Permission overlay is displayed, lock the device by pressing power button.
6) Unlock the device.
  
Actual:
App Permission overlay is displayed.
  
Expected: 
Lockscreen is displayed.
  
Environmental Variables:
Device: Flame 2.1 (319mb, KK, Shallow Flash)
BuildID: 20141114001204
Gaia: af6533781356acc62b0f40c9e040aa5b47d3b709
Gecko: 551326425826
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Repro frequency: 100%
See attached: logcat, video
http://youtu.be/t4u-tqSloIY

Comment 1

[Yeojin Chung [:YeojinC]]

This issue also reproduces on Flame 2.2.

Result: App Permission overlay does not get dismissed by locking and unlocking the device.

Device: Flame 2.2 (319mb, KK, Shallow Flash)
BuildID: 20141114040205
Gaia: 1e300eac2e56d98ad51d414766d031db7d33221f
Gecko: bbb68df450c2
Version: 36.0a1 (2.2) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

==================================================
This issue does NOT reproduce on Flame 2.0.

Result: App Permission overlay gets dismissed by locking and unlocking the device.

Device: Flame 2.0 (319mb, KK, Shallow Flash)
BuildID: 20141114040205
Gaia: 1e300eac2e56d98ad51d414766d031db7d33221f
Gecko: bbb68df450c2
Version: 36.0a1 (2.2) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Comment 2

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

[Blocking Requested - why for this release]:

Nominating this to block 2.1. This is poor user flow. The user should always be able to view the lockscreen and unlock the phone, after waking the screen up.

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

Unable to obtain a window because for over a month the device wouldn't prompt user for app permission following the STR, and after this behavior the next change is reproducing this bug 1099419. We don't really have a 'working/no repro' state here so a window is not possible.

Comment 4

[bhavana bajaj [:bajaj]]

[Blocking Requested - why for this release]:

John, can you help with this one , i tend to block on it as we are compromising user security here. Not sure if you need any UX input (feel free to NI ROB if so)

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Oh~~~

We have 3 John in Taipei. You are finding LockScreen/Keyboard John who is not me. He is John Lu.

Comment 6

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Greg, I guess it's your call here. Feel free to ping me if you need any assistance :)

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

As Comment 4, NI Rob to make sure the expected result is what UX want. Also, whether the first prompt should be banned, too? I've heard that if in LockScreen with passcode, the dialog should not prompt (which means accept the default setting silently).

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Setting blocking flag per comment 4. I believe nominating for 2.0 was an accident here.

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

Fixing ni? flag :)

Comment 10

[Rob MacDonald [:robmac]]

(In reply to Yeojin Chung [:YeojinC] from comment #0)
> Description:
> When the user locks the screen from App Permission overlay on Camera and
> unlocks the device, the overlay is still displayed.
>
> Expected: 
> Lockscreen is displayed.

Thanks for catching this one. Definitely a blocking issue as Bhavana pointed out. The lock screen should be displayed. Once the phone is unlocked, the prompt would then be in the foreground.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #7)
> As Comment 4, NI Rob to make sure the expected result is what UX want. Also,
> whether the first prompt should be banned, too? I've heard that if in
> LockScreen with passcode, the dialog should not prompt (which means accept
> the default setting silently).

Greg - I talked with Tiffanie (Camera UX owner) about this. We propose  removing the "Remember my choice" option from the dialog if a passcode is enabled and the user is accessing the camera directly from the lock screen slider. We want to avoid the scenario where someone who is not the owner of the phone inadvertently accepts a permission on behalf of the phone owner. This would apply if the user has not checked the "Remember my choice" previously. If this isn't feasible in the 2.1 timeframe we're open to discussing alternative solutions.

I've NI'd Tif on this bug as an FYI. Please flag her and myself (robmac) if you have any questions. 

Thanks!
Rob

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Rob MacDonald [:robmac] from comment #10)
> Greg - I talked with Tiffanie (Camera UX owner) about this. We propose 
> removing the "Remember my choice" option from the dialog if a passcode is
> enabled and the user is accessing the camera directly from the lock screen
> slider. We want to avoid the scenario where someone who is not the owner of
> the phone inadvertently accepts a permission on behalf of the phone owner.
> This would apply if the user has not checked the "Remember my choice"
> previously. If this isn't feasible in the 2.1 timeframe we're open to
> discussing alternative solutions.

That would couple the permission dialog with lock state -- I would not say it's not doable but it poses significant risk.

Comment 12

[howie [:howie]]

Hi Rob, the UX proposal is risky to do at this point, do you have any other suggestion? thanks.

Comment 13

[howie [:howie]]

Hi Tiffanie , can you provide feedback on this one? thank you.

Comment 14

[Tiffanie Shakespeare [:tif]]

Could we disable the remember check box so it can't be checked?

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

Hi Tiffanie:

That probably isn't a proper change for v2.1 (to remove the checkbox in such case). What we proposed is, on LockScreen to hide the dialog when screen is off (by pressing power button). So the flow would be:

0. Set passcode for LockScreen
1. User locks the device
2. Slide to left to invoke secure camera
3. The secure camera shows
4. The dialog prompts
5. Turn the screen off
6. Turn the screen on: the dialog should disappear --> the change we want to make

May you confirm this is what UX want? Or you have any other opinions?

Comment 16

[Tiffanie Shakespeare [:tif]]

I think the concern with that is an evil doer (since a passcode isn't needed) would be able to flip the "always remember" toggle to always record the location. This is why Rob and I are asking about removing the toggle or disabling it.

Is there anything we can do that prevents this security hole?

Comment 17

[Tiffanie Shakespeare [:tif]]

Clearing Rob's NI - he's out for the next few weeks.

Comment 18

[Greg Weng [:snowmantw][:gweng][:λ]]

NI myself for reminding a v2.1 fix.

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

The v2.2 bug is Bug 1118172

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch v2.1

Fixed: I patched it to hide the prompt when screen is off. In my manual test, this works as expected.

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8544931 [details] [review]
Patch v2.1

r+ if change hide to discard. Thanks.

Comment 22

[Greg Weng [:snowmantw][:gweng][:λ]]

Changed, thanks.

Comment 23

[Greg Weng [:snowmantw][:gweng][:λ]]

Waiting TBPL for new patch.

Comment 24

[Greg Weng [:snowmantw][:gweng][:λ]]

TBPL is green: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=cac7d09d7668

Comment 25

[Greg Weng [:snowmantw][:gweng][:λ]]

v2.1 https://github.com/mozilla-b2g/gaia/commit/932f8276ef2306d859bd1e6c2548958c7258c685

Please follow the Bug 1118172 if there is any further discussions.

Comment 26

[Greg Weng [:snowmantw][:gweng][:λ]]

Wait... I forgot to submit approval again (sorry). Because this bug is originally a 2.2 bug...

Comment 27

[Greg Weng [:snowmantw][:gweng][:λ]]

I now back it out.

Comment 28

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8544931 [details] [review]
Patch v2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Dialog prompt over LockScreen and doesn't dismiss
[User impact] if declined: No UX fix
[Testing completed]: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=cac7d09d7668
[Risk to taking this patch] (and alternatives if risky):No
[String changes made]: No

Comment 29

[Greg Weng [:snowmantw][:gweng][:λ]]

The new PR is here:

https://github.com/mozilla-b2g/gaia/pull/27223

The same patch. So I don't make another attachment to prevent getting confused.

Comment 30

[Greg Weng [:snowmantw][:gweng][:λ]]

It failed with a intermittent error. I now re-trigger it and waiting for landing.
And I would land a same 2.2 bug to hide the window at the branch, too.
Keep NI to remind me.

Comment 31

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch v2.1 rev2

The v2.1 patch got approval after I reverted it.

Comment 32

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch

For master.

Comment 33

[Greg Weng [:snowmantw][:gweng][:λ]]

TBPL is green for master patch: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=7b328edb5cc7

Comment 34

[Greg Weng [:snowmantw][:gweng][:λ]]

v2.1 is only with some intermittent errors:

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=d6e689f3cc7b

Comment 35

[Greg Weng [:snowmantw][:gweng][:λ]]

master: https://github.com/mozilla-b2g/gaia/commit/737ef4d7a8494bc6ece4bc3f37b4de977f01333c

Comment 36

[Greg Weng [:snowmantw][:gweng][:λ]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/1975241ac29f723479e6c60b2bf74ebed54da91a

Comment 37

[SandKing]

Attachment: Verify video: verify video.mp4

This issue has been verified successfully on Flame2.1&2.2
Verify video:"verify video.mp4".

Flame 2.1
Gaia-Rev        1975241ac29f723479e6c60b2bf74ebed54da91a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0863fe4b75c3
Build-ID        20150112001215
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150112.035023
FW-Date         Mon Jan 12 03:50:34 EST 2015
Bootloader      L1TC000118D0

Flame 2.2
Gaia-Rev        7c5b27cad370db377b18a742d3f3fdb0070e899f
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2c37b89bdd86
Build-ID        20150112153951
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150112.194842
FW-Date         Mon Jan 12 19:48:52 EST 2015
Bootloader      L1TC000118D0

Comment 38

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Tiffanie Shakespeare from comment #16)
> I think the concern with that is an evil doer (since a passcode isn't
> needed) would be able to flip the "always remember" toggle to always record
> the location. This is why Rob and I are asking about removing the toggle or
> disabling it.
> 
> Is there anything we can do that prevents this security hole?

I don't like removing the toggle here:
- hiding the "remember me" on special occasions is surprising to the user, and violates the standard pattern
- actually hiding remember me doesn't do all that much - when you grant permission, the permission is persisted until the device is reset, so even without this the "attack" is still possible
- What is the actual "attack" scenario here? An attacker with access to your locked phone can change the permission setting of geolocation from "ask" to "grant". How can the attacker actually benefit from this? They would need to coerce the user into sending them a photo, and we already do things like strip exif location when responding to web activity pick requests in the gallery.

To actually improve the current situation, I think the solution is providing better indication to the user when a photo has a location embedded. For example, we could show the geolocation information in the gallery when, when the user clicks on the "information" button. We could even provide a button to "strip location data from photo". What do you think? Shall I raise a seperate bug for this?