1123325 - [B2G][Settings[Lock Screen] Migrate existing Passcode checks to the Passcode helper

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Frederik Braun [:freddy]]

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 1

[GitHub Autolander Bot]

Attachment: [gaia] mozfreddyb:bug-1123325-migrate-passcode-settings > mozilla-b2g:master

Comment 2

[Frederik Braun [:freddy]]

Comment on attachment 8571387 [details] [review]
[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 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8571387 [details] [review]
[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`.

Comment 4

[Frederik Braun [:freddy]]

(In reply to Arthur Chen [:arthurcc] from comment #3)
> Comment on attachment 8571387 [details] [review]
> [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.

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

(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] [review]
> > [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.

Comment 6

[Frederik Braun [:freddy]]

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()?

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Comment 8

[GitHub Autolander Bot]

Attachment: pull request for settings app

Comment 9

[Frederik Braun [:freddy]]

Comment on attachment 8581519 [details] [review]
pull request for settings app

carrying over f+

Comment 10

[GitHub Autolander Bot]

Attachment: pull request for findmydevice app

Comment 11

[GitHub Autolander Bot]

Attachment: pull request for lockscreen/system app and gaia-unit-tests

Comment 12

[Frederik Braun [:freddy]]

Comment on attachment 8593292 [details] [review]
pull request for lockscreen/system app and gaia-unit-tests

Can you review the lockscreen part of this pull request, arthur?

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8593292 [details] [review]
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 14

[Frederik Braun [:freddy]]

Comment on attachment 8593292 [details] [review]
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 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8593292 [details] [review]
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 16

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

Comment on attachment 8593292 [details] [review]
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.

Comment 17

[Frederik Braun [:freddy]]

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 18

[Frederik Braun [:freddy]]

Comment on attachment 8593292 [details] [review]
pull request for lockscreen/system app and gaia-unit-tests

This one's green with your issues addressed. Can you please review again?

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8593292 [details] [review]
pull request for lockscreen/system app and gaia-unit-tests

Just nits, r=me with those addressed.

Comment 20

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

Comment on attachment 8593292 [details] [review]
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 21

[Frederik Braun [:freddy]]

Comment on attachment 8581519 [details] [review]
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 22

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8583830 [details] [review]
pull request for findmydevice app

Nits aside, r+ from me.

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8581519 [details] [review]
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 24

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

Comment on attachment 8581519 [details] [review]
pull request for settings app

r+ iff we decide to put this in gaia.

Comment 25

[Arthur Chen [:arthurcc][inactive after 6/30]]

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 26

[Frederik Braun [:freddy]]

Comment on attachment 8581519 [details] [review]
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 27

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8581519 [details] [review]
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 28

[Frederik Braun [:freddy]]

Comment on attachment 8593292 [details] [review]
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 29

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8581519 [details] [review]
pull request for settings app

I think this is the patch that requires my review. r=me with a few nits addressed, thanks!

Comment 30

[Arthur Chen [:arthurcc][inactive after 6/30]]

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?

Comment 31

[Frederik Braun [:freddy]]

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.

Comment 32

[Frederik Braun [:freddy]]

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

Comment 33

[Frederik Braun [:freddy]]

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.

Comment 34

[Frederik Braun [:freddy]]

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

Comment 35

[Frederik Braun [:freddy]]

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

Comment 36

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 37

[Frederik Braun [:freddy]]

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.

Comment 38

[Frederik Braun [:freddy]]

Attachment: final pull request (green on try, rebased to master)

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.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Johan Lorenzo [:jlorenzo]]

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

Comment 41

[No-Jun Park [:njpark]]

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.

Comment 42

[Frederik Braun [:freddy]]

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?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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

Comment 45

[Frederik Braun [:freddy]]

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

Comment 46

[Doug Sherk (:drs) (inactive)]

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

Comment 47

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Comment 48

[Frederik Braun [:freddy]]

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.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

Please refresh the pull request.

Comment 50

[Frederik Braun [:freddy]]

Attachment: github pull request (including follow-up for lockscreen)

new pull request (carrying over r+)

Comment 51

[Ryan VanderMeulen [:RyanVM]]

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