As a follow-up of bug 1100945, I want to migrate existing code not to handle the "lockscreen.passcode-lock.code" setting manually, but to use the PasscodeHelper in shared/. The benefit of the PasscodeHelper is that it stores and computes a hashed passcode that can not be read off of the device as plaintext.

Comment on attachment 8571387 [details] [gaia] mozfreddyb:bug-1123325-migrate-passcode-settings > mozilla-b2g:master I'm trying to use the new (more secure) passcode library for the lockscreen, but I am stuck with a race condition: With the new helper, screenLockPasscode.handleEvent may perform a passcode check asynchronously, even though it is currently always called synchronously. I'm wondering if there is some sort of "wait for condition" helper to test asynchronously (with a timeout). Alternatively, I could attempt rewriting 'handleEvent()' so that it always returns a promise. The problem is that this would involve rewriting other things down the line: For example '_enablePasscode()', because it interacts with settings and passcode checks as well. What would you suggest? The line that triggers this problem can be seen here: https://github.com/mozilla-b2g/gaia/pull/27918/files#diff-b60537412a9af8b1959313fda9b3b800R243

Comment on attachment 8571387 [details] [gaia] mozfreddyb:bug-1123325-migrate-passcode-settings > mozilla-b2g:master Looks good to me! Thanks. Note that we also need to do migration in `system/js/migrators/settings_migrator.js`.

(In reply to Arthur Chen [:arthurcc] from comment #3) > Comment on attachment 8571387 [details] > [gaia] mozfreddyb:bug-1123325-migrate-passcode-settings > mozilla-b2g:master > > Looks good to me! Thanks. Note that we also need to do migration in > `system/js/migrators/settings_migrator.js`. What kind of migration do you suggest? The PasscodeHelper itself creates four new settings keys for its data (https://github.com/mozilla-b2g/gaia/blob/f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/shared/js/passcode_helper.js#L6), but leaves the old ones untouched. Should we use the SettingsMigrator to set a Passcode using the new method and then removing the old ones? NB: I have yet to change FindMyDevice and the PrivacyPanel, before this can land. All the bugs will have to land simultaneously at the same time.

(In reply to Frederik Braun [:freddyb] (Off between Mar 6th-15th) from comment #4) > (In reply to Arthur Chen [:arthurcc] from comment #3) > > Comment on attachment 8571387 [details] > > [gaia] mozfreddyb:bug-1123325-migrate-passcode-settings > mozilla-b2g:master > > > > Looks good to me! Thanks. Note that we also need to do migration in > > `system/js/migrators/settings_migrator.js`. > > What kind of migration do you suggest? If the users have passcodes being set (which are stored in the old settings key), we have to ensure the passcodes still work after switching to use PasscodeHelper. The whole thing should be transparent to users. > The PasscodeHelper itself creates four new settings keys for its data > (https://github.com/mozilla-b2g/gaia/blob/ > f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/shared/js/passcode_helper.js#L6), > but leaves the old ones untouched. > > Should we use the SettingsMigrator to set a Passcode using the new method > and then removing the old ones? Yes, that's what I meant. > > > NB: I have yet to change FindMyDevice and the PrivacyPanel, before this can > land. All the bugs will have to land simultaneously at the same time.

The migration itself seems easy, but I'm not sure how I'm supposed to require the PasscodeHelper in settings_migrator.js. There's no require.js or setup.js for this file, can I just use require()?

You can use `LazyLoader.load` to load the script. Please load the file only when the migration is required.

Comment on attachment 8581519 [details] pull request for settings app carrying over f+

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests Can you review the lockscreen part of this pull request, arthur?

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests The patch is looking good to me, thanks. I would also like to have Greg working on lock screen take a look at the patch.

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests Dave, you seem like a good review candidate for the changes to the gaia-ui lockscreen tests in this pull request. Can you take a look?

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests I'd like to see if we can reduce some of the duplication in the Python tests, and I'm also concerned about an assert after changing the passcode via the UI.

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests I have some concerns, which are marked on the PR page. Maybe you could take a look, thanks.

Yeah, thanks a lot! I replied inline on GitHub and pushed out a fix that addresses your concerns. The new commit also includes a rebase from master to pick up the fix for bug 1147731. Sorry if this makes readability a bit difficult.

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests This one's green with your issues addressed. Can you please review again?

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests Just nits, r=me with those addressed.

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests For me I suggest to add some comment to describe why 'set_passcode_to_1337' could be a general 'set_passcode_to(String)'. Or other coders couldn't have the clue. Others are fine, thanks.

Comment on attachment 8581519 [details] pull request for settings app Arthur, thanks for the initial feedback+. Now that the other places have taken shape, can you do a full review?

Comment on attachment 8583830 [details] pull request for findmydevice app Nits aside, r+ from me.

Comment on attachment 8581519 [details] pull request for settings app Thanks for taking the effort. Please check my comments regarding promise chain and unit test in github. Alive, I'm requesting a review from you as we have to add new logic in SettingsMigrator in this case unfortunately. I know the plan is to move all of the migration process to gecko but in this case it involves encryption and I don't think we should duplicate such logic in gecko. A migraor in gaia is required, but we have to make sure it can only be used in this kind of cases.

Comment on attachment 8581519 [details] pull request for settings app r+ iff we decide to put this in gaia.

After discussed with Tim, the logic in SettingsMigrator will still be moved to gecko without exception. However, for this time being I think we can just land as it is.

Comment on attachment 8581519 [details] pull request for settings app (In reply to Arthur Chen [:arthurcc] from comment #23) > Thanks for taking the effort. Please check my comments regarding promise > chain and unit test in github. Thank you very much for the useful comments you gave me in this and former reviews. The example on how to mock something out with a custom requite context helped a lot! :-) You already gave me approval, but I just squashed all commits and finally addressed all of your comments. Feel free to look over it again, if you wish.

Comment on attachment 8581519 [details] pull request for settings app Please check my comments in github. Most of them are regarding how to simplify the code of setting mocks for testing. Let me know should any problems, thanks!

Comment on attachment 8593292 [details] pull request for lockscreen/system app and gaia-unit-tests Can you take a look again? I moved a lot of things around. The only things I couldn't do was remove the "Passcodehelper = MockPasscodeHelper" line and rename the global `PasscodeHelper`, because it was apparently used in code deeper down the call chain.

Comment on attachment 8581519 [details] pull request for settings app I think this is the patch that requires my review. r=me with a few nits addressed, thanks!

Hi Frederik, We have another major changes on this part (bug 1166495) and I would like this land first to save the rebasing effort. Is this patch ready to land?

Yes, I really want this to land. But the problematic thing about these patches is that we need to land all of them at once and I want to avoid a crash land. I'll push all three in a new pull-request to see how things go.

Combined https://github.com/mozilla-b2g/gaia/pull/30474

I see a lot of intermittent failures, all of them unrelated. The only thing that seems caused by my patches is a failure in test_settings_passcode.py TestSettingsPasscode.test_set_passcode_by_settings, that I can *not* reproduce locally. I've tried since early this morning and I'm giving up for now.

Made substantial progress. I have to investigate a gaia integration test failure tomorrow, which I couldn't finish today. See the oranges at https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=8a5b3ca57b4d0804265d8442ed2af12f08d52980

I carried over r+ for most of the nits I added to reduce/fix test failures, but the following is possibly worth a longer discussion: I noticed some errors in the lockscreen integration test (apps/settings/test/marionette/tests/screen_lock_settings_test.js) I fixed this by "mocking" the previous behavior and keeping a plain-text copy of the lock screen for the test. I intend to address this in a follow-up, for the sake of progress. Arthur, is it OK to take this and polish it in a follow-up bug? The commit is at https://github.com/mozfreddyb/gaia/commit/13a970b165325c311b37f8afeec6f6ca22aed957 The pull request for a combination of all patches is at https://github.com/mozilla-b2g/gaia/pull/30498

I think it is okay to focus on the state of the screen lock (whether it is enabled/disable) without checking the real value of the passcode in UI tests. With that in mind, perhaps we can simply move the checks from the UI tests to unit tests(screen_lock_passcode_test.js) and that would make things a lot easier. This could be done by adding a test case ensuring `PasscodeHelper.set` is called with the correct value when enabling the screen lock.

Thanks, Arthur! I removed the checks from the UI test. The unit test at screen_lock_passcode_test.js (also in this pull request) already mocks the PasscodeHelper and check that it's called with the correct value.

This attachment links to a pull request that contains *all* the three patches, as they need to be checked in together. Carrying over r+. The previous attachments (marked as obsolete) contain the review flags from the respective module owners.

Master: https://github.com/mozilla-b2g/gaia/commit/35c33cca1934211674f16d923f46be3ca7f4bd31

Reverted for causing smoketest blocker, bug 1175910: https://github.com/mozilla-b2g/gaia/commit/4d7401b3c86e3fe7d5a63ce2271f41ac93b2d379

Note, that this pull request had 5 commits (doesn't look like the commit was squashed), and the one that is reverted was just one of them, the one in the middle. With the only 91f4fa2 reverted, one side effect (there could be many) is that python marionette test of https://github.com/mozilla-b2g/gaia/blob/47952b1058381737ec2deb98f5e442bbe843943f/tests/python/gaia-ui-tests/gaiatest/tests/functional/lockscreen/test_lockscreen_unlock_to_camera_with_passcode.py fails to unlock.

Thank you for raising this, No-Jun! The functionality is spread across many apps (e.g. many reviewers), that's why I did not squash the commits. But if you back-out single commits, some apps will use a different settings value to check and set the unlock code than others. We need all of them backed-out. Ryan, can you please help and back out the remaining commits?

Backouts are the responsibility of the patch author, then module peers, QA, then our team, in that order. I'm about to get on a plane, so you'll need to see if one of the aforementioned others can assist.

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #40) > Reverted for causing smoketest blocker, bug 1175910: > https://github.com/mozilla-b2g/gaia/commit/ > 4d7401b3c86e3fe7d5a63ce2271f41ac93b2d379 |git revert -m 1 35c33cca1934211674f16d923f46be3ca7f4bd31| would have worked a lot better for you here, FYI.

I do not have commit rights and there are multiple module peers involved. I will try making noise on IRC.

Backed out in https://github.com/mozilla-b2g/gaia/commit/c46c77ad85ab0a62d95f1488adc4bd10ad13fa73

Hi Frederik, It seems this part[1] should also be modified to support the missing case. [1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/lock_screen_passcode_validator.js#L101

Yeah, I have addressed this in https://bugzilla.mozilla.org/show_bug.cgi?id=1176180 and asked Greg for feedback. But yours is also very much appreciated, Arthur.

Please refresh the pull request.

new pull request (carrying over r+)

Master: https://github.com/mozilla-b2g/gaia/commit/cbcfa736f626e3bbd6164aaaad925edf1433ac1d