If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox OS master

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: freddyb, Assigned: freddyb)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
FxOS-S2 (10Jul)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(b2g-master fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
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

3 years ago
Created attachment 8571387 [details] [review]
[gaia] mozfreddyb:bug-1123325-migrate-passcode-settings > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
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
Attachment #8571387 - Flags: feedback?(arthur.chen)
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`.
Attachment #8571387 - Flags: feedback?(arthur.chen) → feedback+
(Assignee)

Comment 4

3 years ago
(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.
(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.
(Assignee)

Comment 6

3 years ago
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 8

3 years ago
Created attachment 8581519 [details] [review]
pull request for settings app
(Assignee)

Comment 9

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

carrying over f+
Attachment #8581519 - Flags: feedback+
(Assignee)

Updated

3 years ago
Attachment #8571387 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8581519 - Attachment description: [gaia] mozfreddyb:bug-1123325-migrate-passcode-settings > mozilla-b2g:master → pull request for settings app

Comment 10

3 years ago
Created attachment 8583830 [details] [review]
pull request for findmydevice app
(Assignee)

Updated

3 years ago
Attachment #8583830 - Attachment description: [gaia] mozfreddyb:bug-1123325-PasscodeHelper-in-FindMyDevice > mozilla-b2g:master → pull request for findmydevice app
Attachment #8583830 - Flags: review?(mgoodwin)

Comment 11

3 years ago
Created attachment 8593292 [details] [review]
pull request for lockscreen/system app and gaia-unit-tests
(Assignee)

Comment 12

3 years ago
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?
Attachment #8593292 - Attachment description: [gaia] mozfreddyb:bug-1123325_passcodehelper_lockscreen > mozilla-b2g:master → pull request for lockscreen/system app and gaia-unit-tests
Attachment #8593292 - Flags: review?(arthur.chen)
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.
Attachment #8593292 - Flags: review?(gweng)
Attachment #8593292 - Flags: review?(arthur.chen)
Attachment #8593292 - Flags: review+
(Assignee)

Comment 14

3 years ago
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?
Attachment #8593292 - Flags: review?(dave.hunt)
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.
Attachment #8593292 - Flags: review?(dave.hunt) → review-
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.
Attachment #8593292 - Flags: review?(gweng)
(Assignee)

Comment 17

3 years ago
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.
(Assignee)

Comment 18

3 years ago
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?
Attachment #8593292 - Flags: review?(gweng)
Attachment #8593292 - Flags: review?(dave.hunt)
Attachment #8593292 - Flags: review-
Comment on attachment 8593292 [details] [review]
pull request for lockscreen/system app and gaia-unit-tests

Just nits, r=me with those addressed.
Attachment #8593292 - Flags: review?(dave.hunt) → review+
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.
Attachment #8593292 - Flags: review?(gweng) → review+
(Assignee)

Comment 21

3 years ago
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?
Attachment #8581519 - Flags: review?(arthur.chen)
Comment on attachment 8583830 [details] [review]
pull request for findmydevice app

Nits aside, r+ from me.
Attachment #8583830 - Flags: review?(mgoodwin) → review+
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.
Attachment #8581519 - Flags: review?(arthur.chen) → review?(alive)
Comment on attachment 8581519 [details] [review]
pull request for settings app

r+ iff we decide to put this in gaia.
Attachment #8581519 - Flags: review?(alive) → review+
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.
(Assignee)

Comment 26

2 years ago
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.
Attachment #8581519 - Flags: feedback?(arthur.chen)
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!
Attachment #8581519 - Flags: feedback?(arthur.chen)
(Assignee)

Comment 28

2 years ago
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.
Attachment #8593292 - Flags: review+ → review?(arthur.chen)
Attachment #8593292 - Flags: review?(arthur.chen)
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!
Attachment #8581519 - Flags: review+
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?
Flags: needinfo?(fbraun)
(Assignee)

Comment 31

2 years ago
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.
Flags: needinfo?(fbraun)
(Assignee)

Comment 32

2 years ago
Combined https://github.com/mozilla-b2g/gaia/pull/30474
(Assignee)

Comment 33

2 years ago
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.
(Assignee)

Comment 34

2 years ago
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
(Assignee)

Comment 35

2 years ago
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
Flags: needinfo?(arthur.chen)
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.
Flags: needinfo?(arthur.chen)
(Assignee)

Comment 37

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8581519 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8583830 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8593292 - Attachment is obsolete: true
(Assignee)

Comment 38

2 years ago
Created attachment 8623638 [details] [review]
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.
Attachment #8623638 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Whiteboard: [review flags granted on previous attachments, see comment 38]
Master: https://github.com/mozilla-b2g/gaia/commit/35c33cca1934211674f16d923f46be3ca7f4bd31
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-b2g-master: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [review flags granted on previous attachments, see comment 38]
Target Milestone: --- → FxOS-S1 (26Jun)
Reverted for causing smoketest blocker, bug 1175910: https://github.com/mozilla-b2g/gaia/commit/4d7401b3c86e3fe7d5a63ce2271f41ac93b2d379
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1175910
(Assignee)

Updated

2 years ago
Depends on: 1176180

Updated

2 years ago
Depends on: 1176279
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.

Updated

2 years ago
Blocks: 1176349
(Assignee)

Comment 42

2 years ago
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?
Flags: needinfo?(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.
status-b2g-master: fixed → affected
Flags: needinfo?(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.
(Assignee)

Comment 45

2 years ago
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
Flags: needinfo?(fbraun)
(Assignee)

Comment 48

2 years ago
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.
Flags: needinfo?(fbraun)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Please refresh the pull request.
Flags: needinfo?(fbraun)
Keywords: checkin-needed
Target Milestone: FxOS-S1 (26Jun) → FxOS-S2 (10Jul)
(Assignee)

Updated

2 years ago
Attachment #8623638 - Attachment is obsolete: true
Flags: needinfo?(fbraun)
(Assignee)

Comment 50

2 years ago
Created attachment 8630470 [details] [review]
github pull request (including follow-up for lockscreen)

new pull request (carrying over r+)
Attachment #8630470 - Flags: review+
Master: https://github.com/mozilla-b2g/gaia/commit/cbcfa736f626e3bbd6164aaaad925edf1433ac1d
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-b2g-master: affected → fixed
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Depends on: 1181565

Updated

2 years ago
Depends on: 1182317
You need to log in before you can comment on or make changes to this bug.