1703977 - Extend Form Autofill credit card saving support for the GeckoView storage

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, task, P2)

Description

[Eugen Sawin [:esawin]]

In bug 1691821 we've enabled support for Form Autofill GeckoView storage backend fetching.
In this bug, we extend that support to saving to storage.

We need to identify the correct endpoint in Gecko Form Autofill component to call into GeckoViewAutocomplete.onCreditCardSave.

Once support is established, we can enable the GV API (bug 1703976).

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Dimi, do you know what the status of this is? is this something on your radar at all? (hopefully you're the right person for this question :) )

Comment 2

[Dimi Lee [:dimi]]

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #1)

Dimi, do you know what the status of this is? is this something on your radar at all? (hopefully you're the right person for this question :) )

Hi Agi,
Yes, this is on our radar, but we haven't begun to work on this yet. Do you have a timeline or roadmap when geckoview plans to ship this feature?
We are now planing the work for H2 so its a good timing to discuss with the team to see when we can start to work on this.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

Amoya should be able to answer roadmap questions.

Comment 4

[Amedyne Moya [:amoya]]

Hi Dimi,
The request from Fenix product team is to have this within 3Q2021. I know 91 is on the way right now, is there a probability for this to be planned for 92?

Amedyne

Comment 5

[Dimi Lee [:dimi]]

hi, ethan, I think this is something we should discuss in the immediate roadmap planning meeting.

Comment 6

[Ethan Tseng [:ethan]]

(In reply to Dimi Lee [ooo until 7/4] from comment #5)

hi, ethan, I think this is something we should discuss in the immediate roadmap planning meeting.

For the record, in a roadmap meeting on June 16, we agreed this bug is a top priority for our team in H2 2021.

Comment 7

[Ethan Tseng [:ethan]]

(In reply to amoya from comment #4)

The request from Fenix product team is to have this within 3Q2021. I know 91 is on the way right now, is there a probability for this to be planned for 92?

Also for the record - Because our team is wrapping up our current projects, plus the Form Autofill component is new to us that we'll need some ramp-up time, we are targeting Firefox 93 (soft freeze on 2021-09-02) for now.

Comment 8

[Tim Giles [:tgiles]]

Hi :agi, I've done some initial investigation and have some questions. Hopefully you, or someone you can point to, can help clear these questions up. Form autofill is also still pretty new to me, so I'm trying to figure out the difference between the browser/extensions version and the toolkit version, so apologies if my lack of understanding makes my questions a bit confusing! Feel free to call out any assumptions, correct or incorrect, that I'm making about GeckoView and what it needs from toolkit.

1. What does GeckoView need from Gecko in order to save credit cards?
   I don't have a lot of experience knowing what needs to be set up at the toolkit level so that Desktop/Android can create their own implementations or know how to call into Gecko. As far as I can tell, we have FormAutofillStorage which has some implementation available for Android. Obviously I don't understand how storage works on mobile, since on Desktop we call await gFormAutofillStorage.creditCards.add(data.creditcard)...but it appears as if this message path isn't used on Desktop. This leads into my second question...

2. Does Gecko need to hook up the FormAutofill:SaveCreditCard message path so that SaveCreditCard is not only called by tests?
   If we had this message path set up, would that benefit GeckoView? Again, since I'm not sure what the process normally looks like, I'm not sure what toolkit needs to do for GeckoView.

3. Why can't GeckoView implement the _save() function in its implementation of FormAutofillStorage?
   From my perspective, it looks like if GeckoView could do a call like this.creditCards._saveRecord(creditCard) or this.creditCards.add(creditCard) then everything should "just work"™. I'm sure I'm wrong about this, but I don't know enough about this interaction to say otherwise.

4. Why does GeckoView have to implement _save() in the GeckoViewStorage class?
   This is mainly for my own understanding. All I seem to remember from Eugen debrief is that GeckoView needs to be stateless...but I could be misremembering or applying that memory to the wrong part of the stack.

5. Why can't the _save() function utilize GeckoViewAutocomplete.onCreditCardSave()?
   Couldn't the _save() function iterate over the currently stored credit cards from super.data and call onCreditCardSave(super.data[i] or something to that effect? Seems like a batch save operation would be beneficial here but I don't know if that's what the bug was originally asking for.

6. If we can't utilize GeckoViewAutocomplete.onCreditCardSave() in the _save() function, then what does the _save() function need to call in order to resolve this bug?

I'll continue researching and I'll update/add new comments if I happen to answer my questions before you have a chance to respond.

Feel free to answer the questions in any order and to merge questions if they happen to have the same answer. Thanks for the help!

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Tim Giles [:tgiles] from comment #8)

Hi :agi, I've done some initial investigation and have some questions. Hopefully you, or someone you can point to, can help clear these questions up. Form autofill is also still pretty new to me, so I'm trying to figure out the difference between the browser/extensions version and the toolkit version, so apologies if my lack of understanding makes my questions a bit confusing! Feel free to call out any assumptions, correct or incorrect, that I'm making about GeckoView and what it needs from toolkit.

Hey Tim! I'm gonna try to answer your questions

1. What does GeckoView need from Gecko in order to save credit cards?
   I don't have a lot of experience knowing what needs to be set up at the toolkit level so that Desktop/Android can create their own implementations or know how to call into Gecko. As far as I can tell, we have FormAutofillStorage which has some implementation available for Android. Obviously I don't understand how storage works on mobile, since on Desktop we call await gFormAutofillStorage.creditCards.add(data.creditcard)...but it appears as if this message path isn't used on Desktop. This leads into my second question...

I believe calling .creditCards.add should be sufficient for us

2. Does Gecko need to hook up the FormAutofill:SaveCreditCard message path so that SaveCreditCard is not only called by tests?
   If we had this message path set up, would that benefit GeckoView? Again, since I'm not sure what the process normally looks like, I'm not sure what toolkit needs to do for GeckoView.

I believe that's the case, if you can call FormAutofill:SaveCreditCard whenever needed everything else should be already wired up (until the Java layer, but we can take care of that).

3. Why can't GeckoView implement the _save() function in its implementation of FormAutofillStorage?
   From my perspective, it looks like if GeckoView could do a call like this.creditCards._saveRecord(creditCard) or this.creditCards.add(creditCard) then everything should "just work"™. I'm sure I'm wrong about this, but I don't know enough about this interaction to say otherwise.

Is _save() synchronous? That might be the problem. GeckoView doesn't save data in it, it will forward the save request to the embedder (Fenix) that actually stores the data. This is inevitably an asynchronous operation so if we rely on _save being synchronous that might be the reason why we cannot implement it.

4. Why does GeckoView have to implement _save() in the GeckoViewStorage class?
   This is mainly for my own understanding. All I seem to remember from Eugen debrief is that GeckoView needs to be stateless...but I could be misremembering or applying that memory to the wrong part of the stack.

I believe that's correct, we cannot synchronously save the data (and in general, we're not storing the data inside a JSON file, even though we pretend to be one) so we cannot really save anything.

5. Why can't the _save() function utilize GeckoViewAutocomplete.onCreditCardSave()?
   Couldn't the _save() function iterate over the currently stored credit cards from super.data and call onCreditCardSave(super.data[i] or something to that effect? Seems like a batch save operation would be beneficial here but I don't know if that's what the bug was originally asking for.

That wouldn't work I think. As I understand it, GeckoView's implementation of FormAutofillStorage is just a "in memory" representation of the credit card storage (which lives in the embedder, outside of GeckoView's control). We cannot "save" because if we iterated through all the credit cards in the storage we would create duplicates, since we don't really keep track of what credit cards were added and what already exist in the storage. I guess we could change that, but, from what I can tell, the code wasn't meant to be used that way.

6. If we can't utilize GeckoViewAutocomplete.onCreditCardSave() in the _save() function, then what does the _save() function need to call in order to resolve this bug?

The save function would be always empty in the GV implementation, if I'm understanding it correctly. We keep track of what was added in add and just keep the storage around for an in memory cache.

I'll continue researching and I'll update/add new comments if I happen to answer my questions before you have a chance to respond.

Feel free to answer the questions in any order and to merge questions if they happen to have the same answer. Thanks for the help!

Sure! Feel free to grab some time on my calendar if you think it would be helpful to chat in person!

Comment 10

[Tim Giles [:tgiles]]

Hey Agi, any updates on your side for this bug?

I also have another question that you might be able to answer.

1. Do we need to add a GeckoViewPrompter function like what we do for login saving? I'm not sure if Fenix handles the prompter, the credit card prompter, in this case. Maybe this is outside the scope of this bug, not 100% sure.

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

Sorry I got a little bit sidetracked. I finally was able to test it locally, looks like one of the problems is that we use FormAutofillDoorHanger which seems to be designed for desktop (not sure why it's in toolkit):

https://searchfox.org/mozilla-central/rev/a9851cca236e03d088d7528dd27445080475a68e/toolkit/components/formautofill/FormAutofillParent.jsm#725

Which is actually the answer to your question:

Do we need to add a GeckoViewPrompter function like what we do for login saving? I'm not sure if Fenix handles the prompter, the credit card prompter, in this case. Maybe this is outside the scope of this bug, not 100% sure.

Yes, on mobile, we should create a prompt instead of using FormAutofillDoorHanger.

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

Another problem that I'm seeing is that, if you do have credit cards saved in Fenix, then this will fail:

      let decrypted = await OSKeyStore.decrypt(
        creditCard["cc-number-encrypted"],
        false
      );

https://searchfox.org/mozilla-central/rev/a9851cca236e03d088d7528dd27445080475a68e/toolkit/components/formautofill/FormAutofillStorageBase.jsm#1933-1936

because we don't encrypt the credit card number (because it's a memory-only representation) so cc-number-encrypted is not defined.

Comment 13

[Tim Giles [:tgiles]]

Thanks for that update Agi, that helps me out a ton!

Okay, so it looks like we need to move the current implementation of FormAutofillDoorHanger into a Desktop specific implementation and create a mobile implementation of FormAutofillDoorHanger that, instead of using desktop XUL, uses the GeckoViewPrompter.asyncShowPrompter() and then delegates that message to GeckoView via GeckoViewAutocomplete.onCreditCardSave(). I think that's what we need to do on the toolkit side of things. Feel free to correct me if this seems incorrect.

As for the getDuplicateGuid issue, about the unencrypted credit card numbers, I'll move that base implementation into the Desktop implementation and add an implementation that does not use the decrypt function on Mobile...that should resolve that issue.

Comment 14

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Tim Giles [:tgiles] from comment #13)

Okay, so it looks like we need to move the current implementation of FormAutofillDoorHanger into a Desktop specific implementation and create a mobile implementation of FormAutofillDoorHanger that, instead of using desktop XUL, uses the GeckoViewPrompter.asyncShowPrompter() and then delegates that message to GeckoView via GeckoViewAutocomplete.onCreditCardSave(). I think that's what we need to do on the toolkit side of things. Feel free to correct me if this seems incorrect.

Sounds good to me, it should be pretty similar to the implementation of LoginStorageDelegate.promptToSavePassword: https://searchfox.org/mozilla-central/rev/5227b2bd674d49c0eba365a709d3fb341534f361/mobile/android/components/geckoview/LoginStorageDelegate.jsm#49-71

I wanna note that, similar to promptToSavePassword, I don't think that the generic method should have DoorHanger in the name and should instead be named promptToSaveCreditCard as we don't show a DoorHanger on mobile :) it's a nit but figured I should mention it.

Comment 15

[Tim Giles [:tgiles]]

Hey Agi, thanks for the update and calling out that nit, I'll definitely take it into consideration when writing the patch. I was wondering if it was possible for you to write a test or two that I could use to verify my implementation of the mobile prompter, since I've been unable to get mobile to build on my machine. If you have some other ideas on how I can verify my implementation, I'm also open to them. I'm trying to not take all your time trying to verify my patch :)

Comment 16

[[ex-Mozilla] Agi Sferro | :agi]

You could get a Linux VM and test with an artifact build? it shouldn't take too long to build.

Comment 17

[Tim Giles [:tgiles]]

Attachment: Bug 1703977 - Extend credit card saving support for GeckoView storage. r=agi,zbraniecki,dimi

Comment 18

[Tim Giles [:tgiles]]

Hey Agi, I've made some progress in getting the prompter set up and all that, but I've run into a few issues that, I hope, you may have an answer to. As always, thanks for the time and the help!

1. The GeckoViewPrompter should work in the GeckoView Example application on the Android 7.0 emulator right? I've called the synchronous and asynchronous "showPrompt" functions and I haven't been able to get a prompt to appear yet. I figured it was because the credit card side of things isn't set up in Android yet (Bug 1703976), so I tried with a login prompt which seems to be all connected. Unfortunately, I wasn't able to get this prompt to appear either which always caused the prompt's returned value to be null. What am I missing on the GeckoViewPrompter end of things?

2. It looks like the "Autocomplete:Save:CreditCard" type prompt is not available in central. It looks like Bug 1703976 implements this prompt. Should I base my work off this previously mentioned bug or should I assume that the save credit card prompt works as expected? Is there some other prompt I should test instead to ensure the capture flow works as expected?

3. In FormAutoFillParent on Line 803-805, we call the notifyUsed() function which then tries to find the saved credit card via GUID. Should we be trying this find by GUID approach on mobile, or use something like GeckoViewAutocomplete's fetchCreditCards and do some kind of look up logic based on the fetched result?

4. Currently mergeToStorage throws a ERROR_NOT_IMPLEMENTED exception for credit cards (and probably for addresses to). After showing the prompt, if we don't have a certain state, we end up going down a path that calls this mergeToStorage function, which will throw that previous exception. Does it make sense to make mergeToStorage for mobile credit cards a no-op instead?

5. This one is pretty nebulous so apologies, but I still have no idea what is supposed to happen with this _save() function in FormAutofillStorage. Maybe with my previous questions and your current understanding, this question makes more sense now...but I'm not entirely sure.

Comment 19

[[ex-Mozilla] Agi Sferro | :agi]

Sorry for the late reply! got sidetracked

(In reply to Tim Giles [:tgiles] from comment #18)

Hey Agi, I've made some progress in getting the prompter set up and all that, but I've run into a few issues that, I hope, you may have an answer to. As always, thanks for the time and the help!

1. The GeckoViewPrompter should work in the GeckoView Example application on the Android 7.0 emulator right? I've called the synchronous and asynchronous "showPrompt" functions and I haven't been able to get a prompt to appear yet. I figured it was because the credit card side of things isn't set up in Android yet (Bug 1703976), so I tried with a login prompt which seems to be all connected. Unfortunately, I wasn't able to get this prompt to appear either which always caused the prompt's returned value to be null. What am I missing on the GeckoViewPrompter end of things?

GeckoViewExample's implementation of those prompts is here: https://searchfox.org/mozilla-central/source/mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/BasicGeckoViewPrompt.java#56 I believe none of the ones you tried are actually implemented, that's probably the reason why you're not seeing the prompts. You can try adding a very simple implementation like:

    @Override
    public GeckoResult<PromptResponse> onCreditCardSave(...) {
        Log.i(LOGTAG, "onCreditCardSelect was called!");
    }

and then look at the device logs using adb logcat.

Another option (my favorite one!) would be writing a test here: https://searchfox.org/mozilla-central/rev/352b525ab841278cd9b3098343f655ef85933544/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AutocompleteTest.kt#114

But in any case if you get as far as calling us in the GeckoViewPrompter that should be enough and we can take it from there.

2. It looks like the "Autocomplete:Save:CreditCard" type prompt is not available in central. It looks like Bug 1703976 implements this prompt. Should I base my work off this previously mentioned bug or should I assume that the save credit card prompt works as expected? Is there some other prompt I should test instead to ensure the capture flow works as expected?

if you want to test it, it would be nice if you added those patches to your stack to check that everything is working.

3. In FormAutoFillParent on Line 803-805, we call the notifyUsed() function which then tries to find the saved credit card via GUID. Should we be trying this find by GUID approach on mobile, or use something like GeckoViewAutocomplete's fetchCreditCards and do some kind of look up logic based on the fetched result?

Does the code not work as written? We have the storage in memory so we should already be able to find the saved credit card with no modifications to the code (unless I'm missing something)

4. Currently mergeToStorage throws a ERROR_NOT_IMPLEMENTED exception for credit cards (and probably for addresses to). After showing the prompt, if we don't have a certain state, we end up going down a path that calls this mergeToStorage function, which will throw that previous exception. Does it make sense to make mergeToStorage for mobile credit cards a no-op instead?

If I'm understanding correctly that method is used when we're trying to merge a submitted credit card to another one already in storage? If that is so I think we should support that on Android too? From what I can see the base implementation should be enough as long as we make update work on android too: https://searchfox.org/mozilla-central/rev/352b525ab841278cd9b3098343f655ef85933544/toolkit/components/formautofill/FormAutofillStorageBase.jsm#472. From what I can see in add we have common code until we call _saveRecord that is overridden on Android to send the record to the embedder, maybe we can do something like that in update too? (maybe we already do that but I can't find it right now)

5. This one is pretty nebulous so apologies, but I still have no idea what is supposed to happen with this _save() function in FormAutofillStorage. Maybe with my previous questions and your current understanding, this question makes more sense now...but I'm not entirely sure.

I believe nothing is needed there. We will use CreditCards and Addresses methods to actually save records, so the _save() method is not needed (for context, on an ordinary JSONFile object, the _save method is called on a timer after a change to the underlying in-memory object to sync data to disk).

let me know if any of this is unclear

Comment 20

[Dimi Lee [:dimi]]

Tim is taking a week off, I'll steal the work from him.

Comment 21

[Dimi Lee [:dimi]]

Attachment: Bug 1703977 - P1. Extend Form Autofill credit card saving support for the GeckoView storage

Comment 22

[Dimi Lee [:dimi]]

Attachment: Bug 1703977 - P2. Do not decrypt credit card number in getDuplicateGuild function in the android platform.

Depends on D121633

Comment 23

[Dimi Lee [:dimi]]

Attachment: Bug 1703977 - P3. Rename FormAutofillPrompter show func to _showCreditCardCaptureDoorhanger

In order to sync the naming with the password manager.

Depends on D121634

Comment 24

[Dimi Lee [:dimi]]

Attachment: Bug 1703977 - P4. Remove unused comment in FormAutofillStorage

_save() is only used by _saveImmediately(), which is test only code.
"Save" action in the android platform occurs in FormAutofillPrompter.promptToSaveCreditCard

Depends on D121635

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17226fa8618a
P1. Extend Form Autofill credit card saving support for the GeckoView storage r=geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/40915609cbd8
P2. Do not decrypt credit card number in getDuplicateGuild function in the android platform. r=agi
https://hg.mozilla.org/integration/autoland/rev/e5af3f0446fa
P3. Rename FormAutofillPrompter show func to _showCreditCardCaptureDoorhanger r=agi
https://hg.mozilla.org/integration/autoland/rev/25e5608dad37
P4. Remove unused comment in FormAutofillStorage r=agi

Comment 26

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9cb7b946de8a
Backed out 4 changesets for causing geckoview failures in AutocompleteTest#creditCardSelectAndFill

Comment 27

[Alexandru Michis [:malexandru]]

Backed out 4 changesets (Bug 1703977) for causing geckoview failures in AutocompleteTest#creditCardSelectAndFill
Backout link: https://hg.mozilla.org/integration/autoland/rev/9cb7b946de8acd9bab6d99100a6df51b5718687e
Push with failures, failure log.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/308c86a4efa3
P1. Extend Form Autofill credit card saving support for the GeckoView storage r=geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/94d4d7434dfc
P2. Do not decrypt credit card number in getDuplicateGuild function in the android platform. r=agi
https://hg.mozilla.org/integration/autoland/rev/d50b4a5928d1
P3. Rename FormAutofillPrompter show func to _showCreditCardCaptureDoorhanger r=agi
https://hg.mozilla.org/integration/autoland/rev/0e452fe4837d
P4. Remove unused comment in FormAutofillStorage r=agi

Comment 29

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/308c86a4efa3
https://hg.mozilla.org/mozilla-central/rev/94d4d7434dfc
https://hg.mozilla.org/mozilla-central/rev/d50b4a5928d1
https://hg.mozilla.org/mozilla-central/rev/0e452fe4837d