Support migrate credit card data to the Rust store
Categories
(Toolkit :: Form Autofill, task, P2)
Tracking
()
People
(Reporter: dimi, Assigned: dimi)
Details
Attachments
(3 files, 4 obsolete files)
|
Assignee | |
Comment 1•6 days ago
|
||
With the pref on, copy the profile's credit cards into the Rust store, check
field by field that they survived, and serve them from there. Off by default,
so this changes nothing yet.
The JSON collection becomes a migration target first. addManyWithMeta,
updateManyWithMeta, removeMany and #recordForMigration were added to Addresses
by bug 2058441, but nothing in them is address-specific -- they use only _data,
_findIndexByGUID, _findByGUID, remove, _stripComputedFields and computeFields,
all of which AutofillRecords already provides -- so they move to the base class
rather than being copied. What differs per collection is what
_stripComputedFields and computeFields do, which the subclasses already
override. Addresses inherits exactly what it declared before.
Mirrors the address migration landed in bug 2058441, on its own four prefs:
- creditCards.storage.rust.enabled is the intent, watched rather than only
read at startup - creditCards.storage.rust.active is where the cards actually are, set by
Firefox once a copy has completed - creditCards.storage.rust.runMigrationTest runs the copy only to measure it
and then wipes it, recorded on its own generation counter so it cannot be
mistaken for the real thing - creditCards.storage.rust.migrationAttempts bounds the retries
The two collections migrate independently -- a profile can have moved its
addresses and not its cards -- so the prefs, the switch promise and the Glean
events are all separate rather than shared.
The number is decrypted out of one store and encrypted into the other, rather
than the ciphertext being carried across. Each store owns its own scheme, so
neither has to understand the other's, and when the Rust store gains its own key
only #encryptNumber changes. This is the shape LoginStorageMigrator uses:
_recordForMigrationExport hands a record over with its number in the clear, and
the receiving store encrypts it on the way in.
That needs the OS key store readable, and #creditCardKeyStoreReady settles it
once before any copying starts rather than discovering it per record -- a copy
that stops halfway leaves the profile split across two stores, and a key store
locked for the first card is locked for all of them. It does not reauth: this
runs at startup, before the user has asked for anything, so a locked key store
defers the migration to a launch where it is unlocked. A deferral is not counted
against the retry budget, because nothing was attempted.
A record whose number will not decrypt is refused and reported, not copied. The
alternative is what CreditCardsBase._stripComputedFields does for an edit, where
swallowing the failure is deliberate so the rest of the card stays editable: the
number is left masked, and _encryptNumber then encrypts the mask and writes
asterisks over cc-number. That is survivable for one card a user is editing and
can retry. It is not survivable for an unattended copy of the whole wallet,
because most decrypt failures are transient -- a locked keychain, a denied
prompt, an OS crypto error -- and the ciphertext it destroys was recoverable on
the next launch. Refusing the record leaves the source untouched, counts it as
failed, and so leaves the profile where it is.
cc-number-encrypted is therefore compared by presence rather than exactly: the
target writes its own ciphertext, and encryption is not deterministic. cc-number
is compared by its last four digits, which both stores keep in the clear and
which catch a card landing under the wrong guid or a number truncated in
transit.
Tests follow separately, as they did for addresses.
|
|
Assignee | |
Comment 2•6 days ago
|
||
Covers the copy in both directions, and what is specific to cards: that the
number crosses without ever being decrypted.
test_the_number_is_re_encrypted_by_the_target asserts the ciphertext after a
copy is NOT the one handed over, and that it decrypts to the number that was
saved. What has to survive a migration is the number, not the bytes.
test_a_decrypt_failure_refuses_rather_than_copies is the regression test for the
copy path. Every decrypt fails for the whole of the copy, the way a keychain
locking mid-run would, and it asserts the profile stays where it is, the run
reports the record as failed, and the source record is byte-identical
afterwards -- so the next launch can still copy it properly.
test_a_locked_key_store_defers_the_migration covers the other half: a key store
that will not open at all means nothing is attempted, nothing is reported, the
retry budget is untouched, and the next launch migrates normally.
The decrypt failure is transient in both, on purpose. A permanently lost key is
lost whichever way the copy is written; only a failure that would have resolved
later distinguishes refusing the record from copying it without its number, and
that is the case where the damage would have been permanent.
Depends on D323539
|
||
Comment 3•3 days ago
|
||
Authored by https://github.com/DimiDL
https://github.com/mozilla/application-services/commit/e1f6fe836d1df033e1fda13a0f3face77e9247c0
[main] Add credit card bulk-import APIs for store migration (#7585)
|
|
Assignee | |
Comment 4•2 days ago
|
||
With the pref on, copy the profile's credit cards into the Rust store, check
field by field that they survived, and serve them from there. Off by default,
so this changes nothing yet.
The JSON collection becomes a migration target first. addManyWithMeta,
updateManyWithMeta, removeMany and #recordForMigration were added to Addresses
by bug 2058441, but nothing in them is address-specific -- they use only _data,
_findIndexByGUID, _findByGUID, remove, _stripComputedFields and computeFields,
all of which AutofillRecords already provides -- so they move to the base class
rather than being copied. What differs per collection is what
_stripComputedFields and computeFields do, which the subclasses already
override. Addresses inherits exactly what it declared before.
Mirrors the address migration landed in bug 2058441, on its own four prefs:
- creditCards.storage.rust.enabled is the intent, watched rather than only
read at startup - creditCards.storage.rust.active is where the cards actually are, set by
Firefox once a copy has completed - creditCards.storage.rust.runMigrationTest runs the copy only to measure it
and then wipes it, recorded on its own generation counter so it cannot be
mistaken for the real thing - creditCards.storage.rust.migrationAttempts bounds the retries
The two collections migrate independently -- a profile can have moved its
addresses and not its cards -- so the prefs, the switch promise and the Glean
events are all separate rather than shared.
The number is decrypted out of one store and encrypted into the other, rather
than the ciphertext being carried across. Each store owns its own scheme, so
neither has to understand the other's, and when the Rust store gains its own key
only #encryptNumber changes. This is the shape LoginStorageMigrator uses:
_recordForMigrationExport hands a record over with its number in the clear, and
the receiving store encrypts it on the way in.
That needs the OS key store readable, and #creditCardKeyStoreReady settles it
once before any copying starts rather than discovering it per record -- a copy
that stops halfway leaves the profile split across two stores, and a key store
locked for the first card is locked for all of them. It does not reauth: this
runs at startup, before the user has asked for anything, so a locked key store
defers the migration to a launch where it is unlocked. A deferral is not counted
against the retry budget, because nothing was attempted.
A record whose number will not decrypt is refused and reported, not copied. The
alternative is what CreditCardsBase._stripComputedFields does for an edit, where
swallowing the failure is deliberate so the rest of the card stays editable: the
number is left masked, and _encryptNumber then encrypts the mask and writes
asterisks over cc-number. That is survivable for one card a user is editing and
can retry. It is not survivable for an unattended copy of the whole wallet,
because most decrypt failures are transient -- a locked keychain, a denied
prompt, an OS crypto error -- and the ciphertext it destroys was recoverable on
the next launch. Refusing the record leaves the source untouched, counts it as
failed, and so leaves the profile where it is.
cc-number-encrypted is therefore compared by presence rather than exactly: the
target writes its own ciphertext, and encryption is not deterministic. cc-number
is compared by its last four digits, which both stores keep in the clear and
which catch a card landing under the wrong guid or a number truncated in
transit.
Tests follow separately, as they did for addresses.
|
|
Assignee | |
Comment 5•2 days ago
|
||
Covers the copy in both directions, and what is specific to cards: that the
number crosses without ever being decrypted.
test_the_number_is_re_encrypted_by_the_target asserts the ciphertext after a
copy is NOT the one handed over, and that it decrypts to the number that was
saved. What has to survive a migration is the number, not the bytes.
test_a_decrypt_failure_refuses_rather_than_copies is the regression test for the
copy path. Every decrypt fails for the whole of the copy, the way a keychain
locking mid-run would, and it asserts the profile stays where it is, the run
reports the record as failed, and the source record is byte-identical
afterwards -- so the next launch can still copy it properly.
test_a_locked_key_store_defers_the_migration covers the other half: a key store
that will not open at all means nothing is attempted, nothing is reported, the
retry budget is untouched, and the next launch migrates normally.
The decrypt failure is transient in both, on purpose. A permanently lost key is
lost whichever way the copy is written; only a failure that would have resolved
later distinguishes refusing the record from copying it without its number, and
that is the case where the damage would have been permanent.
Depends on D324309
|
||
Updated•2 days ago
|
|
|
||
Updated•2 days ago
|
|
|
Assignee | |
Comment 6•1 day ago
|
||
Credit cards need the same copy addresses already have, and the copy does not
depend on which collection it is copying: the same seven methods, differing
only in the prefs that budget the run, the Glean category it reports to, and how
two records are compared.
So AddressStorageMigrator becomes AutofillStorageMigrator, which holds all of
it, plus a per-collection class at the foot of the file that supplies the rest.
No behaviour change: the address configuration reproduces what the file did
before, down to the log strings, and its fieldDiffers is the !== the old
#verify used inline.
_report moves to the shared class, so the address migration test stubs it
there. Assigning to the subclass prototype would shadow the real method and
restoring would leave undefined in its place.
|
|
Assignee | |
Comment 7•1 day ago
|
||
Serves credit cards from the Rust store once a profile has copied them across,
behind extensions.formautofill.creditCards.storage.rust.enabled, with the same
shape addresses use: a dry run that measures the copy and throws it away, a
real run budgeted to three attempts, and a switch that copies back the other
way when the pref is turned off.
The copy itself is the shared AutofillStorageMigrator from P1; this adds the
credit card configuration next to the address one. Two pieces of it are new
rather than a rename of the address version:
- fieldDiffers goes through creditCardFieldDiffers, because neither of the
number's two fields compares as a string. cc-number is a mask, rebuilt at a
fixed width here and written at the card's own length by the JSON store, so
the last four digits are what get compared; cc-number-encrypted compares by
presence, encryption not being deterministic. - #migrate now asks the source to export each record before copying it, so a
store holding a value only it can read hands the number over in the clear
and the target stores it under its own scheme instead of inheriting
ciphertext it cannot read. A record that will not export is counted failed
rather than copied without its number. Addresses have nothing to convert
and get the record back unchanged.
Tests follow in P3, as they did for addresses.
Depends on D324550
|
|
Assignee | |
Comment 8•1 day ago
|
||
Covers the copy in both directions, and what is specific to cards: that the
number crosses without ever being decrypted.
test_the_number_is_re_encrypted_by_the_target asserts the ciphertext after a
copy is NOT the one handed over, and that it decrypts to the number that was
saved. What has to survive a migration is the number, not the bytes.
test_a_decrypt_failure_refuses_rather_than_copies is the regression test for the
copy path. Every decrypt fails for the whole of the copy, the way a keychain
locking mid-run would, and it asserts the profile stays where it is, the run
reports the record as failed, and the source record is byte-identical
afterwards -- so the next launch can still copy it properly.
test_a_locked_key_store_defers_the_migration covers the other half: a key store
that will not open at all means nothing is attempted, nothing is reported, the
retry budget is untouched, and the next launch migrates normally.
The decrypt failure is transient in both, on purpose. A permanently lost key is
lost whichever way the copy is written; only a failure that would have resolved
later distinguishes refusing the record from copying it without its number, and
that is the case where the damage would have been permanent.
Depends on D324551
|
|
||
Updated•1 day ago
|
|
|
||
Updated•1 day ago
|
|
||
Comment 9•1 day ago
|
||
A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)
|
|
||
Updated•1 day ago
|
|
|
||
Updated•1 day ago
|
|
|
||
Updated•1 day ago
|
Description
•