This should be used to replace existing uses of the SettingsAPI to set a lockscreen. It's currently used The interface I'm thinking of is very small: Allow just setting and checking. set(): Takes a string and returns a promise. This will generate a new salt on every invocation and store salt, iteration count and algorithm (has to be SHA1 until bug 554827 lands. Should be SHA-256 afterwards) in the Settings. check(): Takes a string and returns true or false.

It would be good to talk to some lockscreen guys about this. Adding Greg to the cc here.

Yeah, feedback welcome! Though please not that I plan to cover migrating old code to the helper in a follow-up (which I planned to tack onto the meta bug 1100943, once this here is done).

I think it's good for me to see this happens. However, I may need some real possible code changes to know the proposal better. For example, what component would be placed in System/LockScreen to hash the passcode.

This should do as a first idea of a PasscodeHelper. I'm definitely looking for feedback as to how I could make lines 70-73 and 88-100 prettier!

Comment on attachment 8525375 [details] initial proposal of the passcodehelper The overall strategy generally seems sane to me. I would be interested in seeing the performance hit here, I think we are generally very sensitive to unlock perf, so we need to make sure we don't regress it. Is our long-term strategy to use sha-256? If so it might be worth waiting until we have bug 554827 - I think trying to avoid an algorithm migration here makes sense, maybe we'd be signing up for a lot of pain otherwise?

Not to regress for unlock performenace means we have to make some device-specific measurements for PBKDF2 iterations, but I have no access to a low-spec device. Can you help with this, Kevin? I'd prefer not to just pick a very low number ;) About SHA-256, bug 554827 has been inactive for quite some time. I have tried to create extra attention in there.

Comment on attachment 8525375 [details] initial proposal of the passcodehelper I've no concerns about what the patch is doing, but some coding style issues occur. Please take a look and fix them, thanks.

Comment on attachment 8525375 [details] initial proposal of the passcodehelper I have addressed your coding style comments. Can you take an other look please? I am wondering what kind of test I should write and would make sense. I was looking at ./apps/system/test/unit/settings_listener_test.js but wasn't quite sure. Do we have docs for this?

Comment on attachment 8525375 [details] initial proposal of the passcodehelper Hi, I've left some comments about 1. Promises is too deep 2. Large anonymous function at return statement Please take a look and to see if they're fix-able, thanks. And for the test: unfortunately I don't know we have any doc for writing a unit test...usually we refer others' test and write one. Sometimes this sucks because you need to get pain first before you learn something.

And for Promise in unit test it can be very tricky. If you encounter any issue you can ask me or Tim, who encountered the issues in Keyboard's unit tests. But I don't know what files in keyboard you can refer to, since I'm not familiar with the component.

I didn't write tests yet, but I reduced the nesting by moving more things into their own function. See https://github.com/mozilla-b2g/gaia/pull/26301 For tests, I will look into other helpers in shared/, as well as the tagged_test.js unit test

Comment on attachment 8525375 [details] initial proposal of the passcodehelper Hi, I've left several issues on the PR page. The major issue is we shouldn't use literal object as a stateful instance with `this`. This pattern had made us suffered since it's hard to write test and debugging with. Instead of, please consider to use the plain constructor-prototype style.

Comment on attachment 8525375 [details] initial proposal of the passcodehelper Thank you for your patience, that review was super helpful. I've tried to address all of your comments. Can you review again, please?

Comment on attachment 8525375 [details] initial proposal of the passcodehelper You're almost there! Few key comments: 1. Please use 'prototype.method' instead of 'this.method' as I commented on the PR page. 2. To set review you need provide a unit test. See 'apps/sharedtest' for more information. If you have any trouble about writing promised-based unit test you may take a look at 'lockscreen_inputpad_test.js' under the folder of System unit tests. 3. (Just ask) Do you have following plan (and bug) to use this practically in Gaia? Or would you do it in this bug?

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #14) > Comment on attachment 8525375 [details] > initial proposal of the passcodehelper > > You're almost there! Few key comments: > > 1. Please use 'prototype.method' instead of 'this.method' as I commented on > the PR page. > Will do, thanks. > 2. To set review you need provide a unit test. See 'apps/sharedtest' for > more information. If you have any trouble about writing promised-based unit > test you may take a look at 'lockscreen_inputpad_test.js' under the folder > of System unit tests. > Coming :) > 3. (Just ask) Do you have following plan (and bug) to use this practically > in Gaia? Or would you do it in this bug? I am planning to adopt this in all instances that deal with the Passcode (currently: System, Settings & FindMyDevice). I was thinking of doing it in a follow-up bug. Would you advise differently?

For LockScreen part (System) you can set review to me. Other parts you need other reviews.

Comment on attachment 8525375 [details] initial proposal of the passcodehelper The latest push should hopefully address your comments. I've also added some tests which might look a bit shallow or high-level, but this is why I do it like that: The helper is a bit probabilistic. The same password will always result in a different hash. This is because of a random salt, that protects against dictionary attacks. To test the cryptography in a deterministic way, I am setting the salt and digest to a pre-computed value in the test setup. This makes it deterministic again. The first test then just has to perform a checkPassword() call for the correct PIN. To test the whole thing in total, the second test case embraces the probabilistic nature of the helper and sets a password with proper randomness and checks if a password check will still work out. (Since the actual uses of passcodes is spread among many modules that needs a collection of reviews, I'll do in a follow-up bug instead of here).

Comment on attachment 8525375 [details] initial proposal of the passcodehelper OK I think it's good except some final nits. Feel free to land the squashed commit with green TBPL result after you fix them. Good works.

green on try: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=0c33f52bdc00

Autolander requires a different commit message syntax. Trying again :-)

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list. Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list. Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Disabling autolander and waiting for manual check-in now.. :)

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list. Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list. Note: Until bug 1095028 lands, the patch *must* have a review by a suggested reviewer. If you are the patch author, you can leave an additional R+ on the attachment for autolander to process it.

Kevin, Autolander needs either Greg in the list of Shared peers or you to r+, what do you advise?

Sure that makes sense because it's not in a lockscreen component, though it is specific to lockscreen. I don't mind throwing an R+ on here, but before I do - I just wanted to understand why the code is in shared/ instead of in the system/ app? Are we planning on using this outside of the system/lockscreen app?

I think the code, if we think about its essence, is certainly a 'library', and thus to put it in Shared is more reasonable than in LockScreen. (Pardon me, you just hit the point, so I couldn't stop the comment here...) I'm always worried about that we sometimes may be too focus on the immediate questions like "what's the use case" or "why you do this", rather than organize code according to their roles and natures in our whole project. IMO It's important because if we organize our code according to that, we adopt the happened use cases and avoid the possible problems that already implicitly concluded in the design patterns and architecture, and thus we needn't to hit into the wall again when we finally discovered that the lack of current use case doesn't mean it's really in-existent. Yes I know this sounds like not so Mozilla, since it looks like most of people cares the over-design issues , so that we push the burden of necessities to the patcher. However, the current problem in our (Gecko and Gaia) codebase, which also makes we suffer since at least v1.2, is always the actually happening 'under-design' crisis. This crisis, at least from the code I've read and tracked, is majorly caused by the convention of only patching those broken parts, plus the naïve design principles to stockpile features, which brought us 3K LOC monstrous single closure with the horror of unable to test and even find the bug of it, not to mention the 16K LOC colossal JS file with multiple patterns that people just checked-in and seems never thought about how to organize the mess. So I don't think it's a sin to forecast the current unseen necessities. Moreover, I think to ask use case first is one of the reasons why we have had 16+ duplicated 'padLeft' functions in our code base, because if I implement a function or library that by its nature is sharable, but I need to conquer the difficulties of convincing people why this is sharable while the current use case is my component only, I wouldn't bother myself to put it in the right place. And then, when others encounter their own use cases similar to it, people would do the duplicated version so naturally since they're all the first and only the use case, and would encounter the same difficulty to convince the jury the patch is not guilty!

(And I need to clarify that I don't intend to usurp the reviewer title. I just found there are issues in the patch as you can track them on the PR page, and if the flag is wrong please change it as a f+)

I'm totally fine if you want to press the land button and go with your review - but this one library in particular did not seem fitting for the shared/ directory. I think this in particular is a different problem than the padLeft problem. I just didn't see much value in using this code outside of lockscreen code, but if there is a use-case for that, then it should absolutely be in shared/. If not, then it should probably be in the system app, or in an external repo pulled in by bower/npm.

(In reply to Kevin Grandon :kgrandon [INACTIVE - heads down on Gij Issue] from comment #27) > Are we planning on using this outside of the system/lockscreen app? passcode handling is already spread over multiple modules (system, settings, findmydevice), and I want to refactor all existing code to use PasscodeHelper. This allows us to transition towards sensible password hashing instead of storing it in plain text. The long term goal is to have one centralistic function that handles all passcode interactions (setting and checking), so that we can easily improve passcode handling without touching three distinct apps (i.e., three distinct modules and three reviews). If you still want this moved somewhere else, please advise where it should belong.

Comment on attachment 8525375 [details] initial proposal of the passcodehelper Sounds good to me then, thanks for the clarification.

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/123f83ce7ae176b61c89c11862d9001d97daeb9b

First steps can be found in https://timtaubert.de/blog/2015/05/implementing-a-pbkdf2-based-password-storage-scheme-for-firefox-os/ and bug 181662 comment 15 (I am not sure how we document software that we wrote to be used within Firefox OS. I know we have good docs for our internal C/C++ interfaces in Gecko, but I think there's much, much less for shared libraries in Gaia. Hoping Chris can help out (CCing)) I can start providing the documentation myself once I am back from PTO, leaving a needinfo as a reminder.

(In reply to Frederik Braun [:freddyb] (on PTO until Sep 4th) from comment #34) > First steps can be found in > https://timtaubert.de/blog/2015/05/implementing-a-pbkdf2-based-password- > storage-scheme-for-firefox-os/ and bug 181662 comment 15 > > (I am not sure how we document software that we wrote to be used within > Firefox OS. I know we have good docs for our internal C/C++ interfaces in > Gecko, but I think there's much, much less for shared libraries in Gaia. > Hoping Chris can help out (CCing)) > > I can start providing the documentation myself once I am back from PTO, > leaving a needinfo as a reminder. I agree - there is precious little about Gaia internal libraries. I would love to include more, but I found it difficult to find the information or the time, and things seem to change so fast in Gaia that it looked like it made more sense to just point to the Github repo readme's than try to document it on MDN. The closest I ever got to this was: https://developer.mozilla.org/en-US/Firefox_OS/Platform/Gaia/Gaia_apps But this is out of date now, hence the note at the top of the article. Say this though, if there is interest in documenting internal libraries on MDN, I would be happy to help out. Maybe we could provide a section for this underneath https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia ?

Thanks for providing the input, Chris. I am clearing the needinfo, as I have created a first draft at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/PasscodeHelper_Internals. I think we can discuss further edits through IRC or e-mail