Find My Device has a command that enables screen lock, pass code, and sets a server-provided pass code, and then it sends a 'lockscreen.lock-immediately' to lock up the device before the thief can access it. However, when a pass code timeout has already been set, 'lockscreen.lock-immediately' will lock the screen, but won't require a password before the timeout has passed. Steps to reproduce: 1. Set screen lock and pass code with a long timeout 2. Connect WebIDE debugger to Find My Device process 3. Issue following command on console: - Commands._commands.lock.bind(Commands, "thief!", "0000", function(){})(); What should happen: - Device should lock immediately, locking out the user until pass code 0000 is entered What actually happens: - Thief keeps using the phone until it runs into the timeout

I see two obvious approaches to fix this: 1. FindMyDevice sets pass code timeout to 0 ("immediately)" before firing the 'lockscreen.lock-immediately' event 2. Make the 'lockscreen.lock-immediately' listener enforce that the pass code is required immediately. I strongly prefer the second approach because there are other uses for 'lockscreen.lock-immediately'-type functionality. I found this bug while working on a new 'lock-immediately' use case.

The proposed patch follows the second approach: Making 'ls.lock-immediately' lock immediately This fixes a bug where 'lockscreen.lock-immediately' as sent by FindMyDevice doesn't lock the device immediately with a passcode when a pass code timeout has been set. On unlock, LockScreen.checkPassCodeTimeout() only asks for the pass code when either .fetchLockedInterval() or .fetchUnlockedInterval() exceed the pass code timeout. Setting both _lastLockedTimeStamp and _lastUnlockedTimeStamp to something deep in the past will enforce pass code entry on unlock and avoid a potential race condition. But I'm not sure if it is an acceptable approach to make LockScreenWindowManager call window.lockScreen methods directly. I'm also wondering whether resetting _lastLocked/UnlockedTimeStamp has any averse side effects.

Given the fix it and comment 1, this looks like a Lockscreen issue.

Created https://github.com/mozilla-b2g/gaia/pull/31060 which is identical to the attached patch.

Hi Christiane: I think your analyses are correct. However, since LockScreen itself is listening the 'lockscreen.lock-immediately' at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/lockscreen/js/lockscreen.js#L437 so you just need to add the callback at that section. Although it's an unfortunate legacy, but that means there is no need to call LockScreen from LockScreenWindowManager. The principle is LockScreenWindowManager should only open and close the window, and it should not deal with other internal logic of LockScreen itself. If you have discovered other side-effects or concerns made you couldn't patch it like that, we can have some following discussions.

Oh, and the method name: you can just call it as 'resetTimeoutForcibly' and add some comments about that. Since the method does no real unlocking like changing UI, it just reset the timeouts.

Right, I missed the handler in lockscreen.js. Amended the pull request accordingly.

FindMyDevice and consequently LockScreen's flawed 'lockscreen.lock-immediately' handling were introduced in b2g-v2.0. Prior branches are not affected unless FindMyDevice was backported. Wondering about our current 2.0+ partner branches... I guess that Stingray isn't affected. Who knows about them? Please ni.

For LockScreen and System app your patch must be with an new or modified unit test to get a r+. You can set it as feedback? to get granted without tests first, but if you want to land it you still need to have a r+ by other peers/owners with a patch and test. Can you reset the flag and add the test? We can follow the normal process to land it soon, since your change is simple and the test would not be difficult.

Sorry, Greg, I was trying to request review, not grant it. It's my first testing work in gaia, but I'll give it a shot.

Added two new tests for the new functionality and a fix the old broken test to the pull request. This is the main commit: Add and fix lock-immediately tests This extends lockscreen_test.js with two new tests that check whether resetting timeouts will trigger pass code check, and whether lock-immediately triggers timeout reset. This also fixes the old lock-immediately test which didn't trigger the observer callback and effectively tested whether a locked lockscreen would be locked, which resulted in a broken test that never fails.

Christiane, I've took the liberty to comment your PR to make sure we do not introduce back any mozSettings observer leak within the system app. More on this topic in bug 1105639.

Greg, I rebased the PR to current master. Please review. If OK, can you take care of landing this while I'm on PTO?

Okay, will do. But I couldn't set review flag to myself, could you do that for me?

Comment on attachment 8638937 [details] [gaia] cr:immediatelockfix > mozilla-b2g:master Setting review+ as per :gweng's request in Comment 15

This is an important security issue and will have regulartory impacts for the 2.2r branch.

Comment on attachment 8638937 [details] [gaia] cr:immediatelockfix > mozilla-b2g:master Rebased, local tests passing, manual check according to STR is positive now. Please review.

Since this patch is integrated with the new state manager, I propose not to backport, but to apply a hotfix to FindMyDevice instead (setting passcode timeout setting to immediately).

Comment on attachment 8638937 [details] [gaia] cr:immediatelockfix > mozilla-b2g:master Okay. It's now only with one comment nit. LGTM.

Please re-read that comment. It is actually still valid, so I'm leaving it. Fixing the signalling will not be dealt with in this bug.