Closed Bug 1389413 Opened 7 years ago Closed 7 years ago

[Form Autofill] Decrypt credit card number for filtering when MasterPassword is disabled

Categories

(Toolkit :: Form Manager, enhancement, P1)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4][ETA:8/18])

Attachments

(2 files)

Bug 1389413 - [Form Autofill] Part 1: Handle credit card number decryption while calling getByFilter.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1389413 - Part 2: Refactor getRecords unit test after getByFilter API removed.
59 bytes, text/x-review-board-request
lchang
: review+
Details 
This bug is Bug 1388238's follow up that handle the decryption. We might need to move the getByFilter to parent and let the parent to handle the different cases.
Assignee: nobody → schung
Decryption is only supposed to happen once a user chooses to fill the credit card so it shouldn't be decrypted as part of fetching from storage. The reason is because we can't trigger a MP dialog while the autocomplete popup is opening as the MP dialog will cause it to close. We should have a helper that can be used by the fill code to do decryption only at fill time.
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #2)
> Decryption is only supposed to happen once a user chooses to fill the credit
> card so it shouldn't be decrypted as part of fetching from storage. The
> reason is because we can't trigger a MP dialog while the autocomplete popup
> is opening as the MP dialog will cause it to close. We should have a helper
> that can be used by the fill code to do decryption only at fill time.

Right, so the decryption will only apply for the non-masterpassword case(while previewing). We'll return all creditcard profiles wo decryption while filtering if user set mp already.
This bug is actually for live filtering credit card numbers while user types in a field. Change the title to make it clearer.
Summary: [Form Autofill] Handle credit card number decryption while calling getByFilter → [Form Autofill] Decrypt credit card number for filtering when MasterPassword is disabled
Comment on attachment 8899786 [details]
Bug 1389413 - [Form Autofill] Part 1: Handle credit card number decryption while calling getByFilter.

https://reviewboard.mozilla.org/r/171126/#review176646
Attachment #8899786 - Flags: review?(lchang) → review+
Comment on attachment 8900140 [details]
Bug 1389413 - Part 2: Refactor getRecords unit test after getByFilter API removed.

https://reviewboard.mozilla.org/r/171530/#review176650

Looks good. Thanks.

::: browser/extensions/formautofill/test/unit/test_getRecords.js:177
(Diff revision 1)
> +  sinon.stub(collection, "getAll");
> +  collection.getAll.returns(mockCreditCards);
> +
> +  let testCases = [
> +    {
> +      description: "If the search string could match 1 creditCard(wo masterpassword)",

nit: "w/o" or "without". Also, one space before the bracket.

::: browser/extensions/formautofill/test/unit/test_getRecords.js:222
(Diff revision 1)
> +        searchString: "1",
> +      },
> +      expectedResult: [TEST_CREDIT_CARD_1, TEST_CREDIT_CARD_2],
> +    },
> +    {
> +      description: "If the search string could match 1 creditCard(masterpassword set)",

nit: "with masterPassword".
Attachment #8900140 - Flags: review?(lchang) → review+
Thanks for the review!
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6db77d081806
[Form Autofill] Part 1: Handle credit card number decryption while calling getByFilter. r=lchang
https://hg.mozilla.org/integration/autoland/rev/a950b17ce21e
Part 2: Refactor getRecords unit test after getByFilter API removed. r=lchang
Keywords: checkin-needed
Backed out for eslint failures in browser/extensions/formautofill/test/unit/test_getRecords.js:

https://hg.mozilla.org/integration/autoland/rev/6710896c6687c920389c257b1c60f668a6d27508
https://hg.mozilla.org/integration/autoland/rev/1990adc115e7f1992f7f99da42d0fa7cda0ded13

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a950b17ce21ea9465d3c115249cd1657f416aa12&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=125121840&repo=autoland

[task 2017-08-23T10:10:19.596619Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/extensions/formautofill/test/unit/test_getRecords.js:92:35 | ["addresses"] is better written in dot notation. (dot-notation)
[task 2017-08-23T10:10:19.596803Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/
Flags: needinfo?(schung)
Lint error fixed.
Flags: needinfo?(schung)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c9e603f39a87
[Form Autofill] Part 1: Handle credit card number decryption while calling getByFilter. r=lchang
https://hg.mozilla.org/integration/autoland/rev/2b4436f2b379
Part 2: Refactor getRecords unit test after getByFilter API removed. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9e603f39a87
https://hg.mozilla.org/mozilla-central/rev/2b4436f2b379
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: