800511 - Remove redundant validation from navigator.mozPay()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kumar McMillan [:kumar]]

The navigator.mozPay() code currently checks that a JWT maps to a whitelisted payment provider. This should be the only check needed since all other validation happens on the provider's server. This bug is to remove the client side validation that happens such as checking the price, the currency, and the country -- http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#314 -- since it is redundant. 

Furthermore, we have already discussed and agreed on an API change [1] where country should not be part of the price because there is no possible way to map country codes to prices. I think some future API changes like this might happen so it would be easiest to manage validation of the payment JWT server side. It is also cumbersome to validate JWTs client side because there is no UI to present the errors. The payment provider has a UI to present errors.

This does not block app purchases or in-app payments but it will make development confusing for in-app payments since an unused country code will have to be passed into nav.pay() until this is fixed.

[1] https://groups.google.com/d/msg/mozilla.dev.webapps/A5U2L2B012U/q8BYTaJzy9UJ

Comment 1

[Kumar McMillan [:kumar]]

This entire block that checks the formation of refund parameters and payment name, description, etc, is what I feel to be redundant: http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#279 The hosted provider is better equipped to validate these and present errors to the app developer who sent the malformed request.

Comment 2

[[:fabrice] Fabrice Desré]

In general, the client side can not trust the sever side too much, so I would tend to keep client side validation.

Comment 3

[Kumar McMillan [:kumar]]

The formation of the JWT request only concerns two parties though: the app initiating the request and the server fulfilling the request. As for the trust issue, the relationship (IMO) is simply that the client has a whitelist of providers and therefore it trusts each provider in that list to receive and process payment JWTs.

The main use case I'm concerned about is this:

When developers are experimenting with in-app payments they will make mistakes. We can provide them with a test payment provider to allow them to experiment and see the results of their mistakes with a verbose error message. If they make a mistake in production we do not want to present the real user with the verbose error message -- we want to say "the app has made a malformed payment request. Contact their support at...".

It is not possible to fulfill that use case with client side validation. How will we know if the developer is in testing mode or not? That's where it gets complicated on the client but easy on the server. How will we present the errors to the developer? The error codes in the callbacks are not very helpful. If we decide that is the way to go then we need to not only return error codes in the callback but also verbose explanations of what they did wrong.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> In general, the client side can not trust the sever side too much, so I
> would tend to keep client side validation.

+1 on this.

(In reply to Kumar McMillan [:kumar] from comment #3)
> The formation of the JWT request only concerns two parties though: the app
> initiating the request and the server fulfilling the request. As for the
> trust issue, the relationship (IMO) is simply that the client has a
> whitelist of providers and therefore it trusts each provider in that list to
> receive and process payment JWTs.
> 
> The main use case I'm concerned about is this:
> 
> When developers are experimenting with in-app payments they will make
> mistakes. We can provide them with a test payment provider to allow them to
> experiment and see the results of their mistakes with a verbose error
> message. If they make a mistake in production we do not want to present the
> real user with the verbose error message -- we want to say "the app has made
> a malformed payment request. Contact their support at...".

So the use case being talked about here is when the app makes screws up on a mozPay call to the marketplace prod payment provider, right? And having a useful message, right? 

If the "screw" up is a commonality across general JWT checks (general generic errors), then I'd be more inclined to keep things more on the client side. We just make sure the error code is useful. With exposed APIs, as long as the caller knows what each error code means, then I think they should be okay. The problem I think being described here though sounds more like a dev docs problem, not an implementation problem.

> 
> It is not possible to fulfill that use case with client side validation. How
> will we know if the developer is in testing mode or not? That's where it

A sandbox test server for the payment provider vs. a marketplace prod payment provider? And support both in the Gaia profile? Why couldn't that work?

> gets complicated on the client but easy on the server. How will we present
> the errors to the developer? The error codes in the callbacks are not very
> helpful. If we decide that is the way to go then we need to not only return
> error codes in the callback but also verbose explanations of what they did
> wrong.

Why wouldn't dev docs help reduce this problem? If we provide error codes with explanations that we can point developers to, then that should be sufficient, right?

Comment 5

[Jason Smith [:jsmith]]

cc the MDN apps folks for input

Comment 6

[Kumar McMillan [:kumar]]

(In reply to Jason Smith [:jsmith] from comment #4)
> Why wouldn't dev docs help reduce this problem? If we provide error codes
> with explanations that we can point developers to, then that should be
> sufficient, right?

No, this would only get us half of the way there (i.e. convey info to developers). The other half is showing the user what went wrong and giving them an option to retrieve support from the developer. This is only something the pay provider server can do because that's who has a relationship with the developer. For example, we can look up in our DB on our server and show the support email to contact the developer when there is a problem.

Client side validation is going to create a lot of problems. The main problems are:
1) We cannot show users a helpful error message when the app developer makes a malformed in-app payment. Instead, they will see a blank screen or whatever because the error code will be in the error callback (the payment screen is not shown at all).
2) We cannot iterate quickly on the JWT format because all validation of the JWT is stuck in the client. The format has and will continue to change. We do not have a firm grasp on refunds yet, for example.
3) We cannot show developers an on-screen explanation of the error in the payment window. They will get an error code in their callback and will have to look it up in the docs.

However, if the consensus is still to keep all validation in the client then I will close this and create these new bugs:
- fix the outdated JWT validation (we no longer use country code)
- add documentation for all the possible error codes since there is none
- add additional error codes for all the possible validation failures since right now only debug statements explain what is happening.

I still think this API does not require client side validation at all -- the JWT is just an artifact of communication to make a payment and if we want to keep the API flexible we would let the two parties that are in communication with each other deal with the JWT themselves. The navigator.mozPay() API is simply a way to securely connect a merchant with a payment provider. That's it.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Kumar McMillan [:kumar] from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #4)
> > Why wouldn't dev docs help reduce this problem? If we provide error codes
> > with explanations that we can point developers to, then that should be
> > sufficient, right?
> 
> No, this would only get us half of the way there (i.e. convey info to
> developers). The other half is showing the user what went wrong and giving
> them an option to retrieve support from the developer. This is only
> something the pay provider server can do because that's who has a
> relationship with the developer. For example, we can look up in our DB on
> our server and show the support email to contact the developer when there is
> a problem.

In most past DOM arguments I've been involved with, I would imagine that would be the responsibility of the app developer to be able to handle the error fired back and provide useful context to the user. The developer also has knowledge of what payment server they are hitting, so worst case, they could point the user to the support channel for that payment provider.

If the problem happens after the payment provider starts showing information in the trusted UI, then there's the possibility to provide useful errors at the control of the payment provider.

So I actually do think the option is there. Note that the more we put on the server, the more network activity has to take place. This is concerning, because we know network connectivity isn't great in Brazil.

> 
> Client side validation is going to create a lot of problems. The main
> problems are:
> 1) We cannot show users a helpful error message when the app developer makes
> a malformed in-app payment. Instead, they will see a blank screen or
> whatever because the error code will be in the error callback (the payment
> screen is not shown at all).

I'd imagine the app developer could handle this if they have the right error code to produce resulting content to the user on what to do as a result. Forcing it can give negative effects, as we remove areas of freedom the developer has in how they want to handle a specific error case.

> 2) We cannot iterate quickly on the JWT format because all validation of the
> JWT is stuck in the client. The format has and will continue to change. We
> do not have a firm grasp on refunds yet, for example.

True, although that same story has happened with the app manifest. The advantage of client-side validation is perf to catch immediate errors (malformed JWT) so that responsiveness back to the developer is quick and consistency (we establish consistency across any payment provider on a standardized JWT, rather than an arbitrary one). I would hope we could aim for something standardized down the line, but if we move the format checking to the payment provider, than I could imagine a problem starting where JWT format loses standardization along with the perf benefits of client-side validation.

> 3) We cannot show developers an on-screen explanation of the error in the
> payment window. They will get an error code in their callback and will have
> to look it up in the docs.

During the process of when the trusted UI content is being shown, I would imagine there would be the possibility to show errors within the trusted UI context. So the control at that point would be at the responsibility of the payment provider.

As for the error code, that's quite typical for DOM APIs for having to handle it. I don't see why this is any different of a scenario than a typical web developers for handling errors fired back from a DOM API function call.

> 
> However, if the consensus is still to keep all validation in the client then
> I will close this and create these new bugs:
> - fix the outdated JWT validation (we no longer use country code)
> - add documentation for all the possible error codes since there is none
> - add additional error codes for all the possible validation failures since
> right now only debug statements explain what is happening.

That sounds reasonable.

> 
> I still think this API does not require client side validation at all -- the
> JWT is just an artifact of communication to make a payment and if we want to
> keep the API flexible we would let the two parties that are in communication
> with each other deal with the JWT themselves. The navigator.mozPay() API is
> simply a way to securely connect a merchant with a payment provider. That's
> it.

At this point, I almost feel like we're trying to make mozPay be the moz market pay API, when in fact there are significant differences how a DOM API vs. an exposed API on the web is built. 

Jonas - Thoughts?

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

The JWT validation on the client side was introduced to provide to the user enough information about the payment requests to be fired, as requested in [1]. We need this client side validation to be sure that the user makes her choice based on what we agreed to be the necessary information to make a secure choice about sending or not the payment request.

After [1], we agreed to remove the payment request confirmation step [2] when there is only one JWT, which is going to be the case for v1. We did that on the Gaia side [3] leaving the platform code as it is.

Despite we are currently not exposing the payment request information to the user before sending it to the payment provider, we might need to do it after v1. So, we probably need to do this client side validation, to be sure that we have enough and valid information to let the user choose between payment requests. Unless we consider that the only needed information is the payment provider (JWT 'typ' parameter), as it was before [1].

[1] https://groups.google.com/forum/#!msg/mozilla.dev.b2g/YGITHnnjh0M/OnWiUgJG5IgJ
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=793811
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=793811#c7

Comment 9

[Kumar McMillan [:kumar]]

We really need to keep moving on the client side if we aren't going to remove its validation so I created bug 805132, bug 805130, and bug 805123

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't have super strong opinions here. If we are absolutely sure that we're only going to be shipping with one payment provider(which it sounds like we are), and given that we've removed the selector UI, then the client side validation isn't buying us a whole lot.

But Fernando is totally correct in that we'll have to add this back once we support multiple payment providers. So at some point we'll have to standardize the JWT format and stop tweaking it.

But I'm totally fine with removing the validation given the current state of things.

Comment 11

[Kumar McMillan [:kumar]]

Jonas, I am a little unclear (and concerned) about how much code we're about to freeze on the phone. For example, the JWT format will most likely need to change soon to a price tier model - this is the only way the mkt team can envision supporting multiple prices/currencies per region and carrier. If we do not remove the client side validation for the current format (an array of prices/currencies) then will we be stuck with it? If so, for how long? 

Ultimately I too would like to settle on a standard JWT format so that future providers know what to implement but I think we would be able to iterate on that format quicker using server side validation of the JWT format.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't know what more I can say in addition to comment 10?

> But I'm totally fine with removing the validation given the current state of
> things.


If you need anything in addition to that from me, please detail what.

Comment 13

[Jason Smith [:jsmith]]

Removing this from tracking until I see a strong argument to pull the validation out. Right now, personally, I don't think it's worth the churn to spend time to pulling the code out IMO, unless there's serious stability and churn issues.

Comment 14

[Jason Smith [:jsmith]]

Over to Lucas for a decision.

Comment 15

[Jason Smith [:jsmith]]

Actually, I've got an idea that might make everyone happy. I'd say let's do this:

Land a patch that allows the validation piece to be easily prefed on and off (about:config pref?) in this bug. 

Worst case, if we're churning, seeing stability issues or plainly just end up deciding we want it off temporarily for some other random strong reason, then we can quickly land a patch to switch the pref off by default and move validation to the server.

I think this meets the combination of concerns stated above, as that will allow us to balance the churn concern Kumar has raised, keeping existing client-side validation towards the longer-term solution, and address a middle ground on what Jonas has stated.

Kumar - Does that work?

Comment 16

[Kumar McMillan [:kumar]]

The scenario I am most concerned about is freezing client validation on Firefox OS devices that we ship to users -- app developers will want to painlessly support in-app payments on *all* of those devices. If deploying config changes to disable client validation is easy in that scenario then I am in support of it.

Comment 17

[Jason Smith [:jsmith]]

(In reply to Kumar McMillan [:kumar] from comment #16)
> The scenario I am most concerned about is freezing client validation on
> Firefox OS devices that we ship to users -- app developers will want to
> painlessly support in-app payments on *all* of those devices. If deploying
> config changes to disable client validation is easy in that scenario then I
> am in support of it.

Okay on the deploying of the config changes to allow easy pref off.

As for that scenario, that's easy to work around to follow the same philosophy the app manifest follows - evolve the specification carefully to follow something like:

- New parameters are optional only, with fallback to defaults
- Careful how we manage existing parameters in how we modify them (as when we change these, we could regress existing consumers of the API)

Comment 18

[Lucas Adamski [:ladamski]]

We talked through this on IRC and it does sound like trying to do any client verification would add little to no value, while potentially limiting our payment implementation options in the future.

Comment 19

[Jason Smith [:jsmith]]

(In reply to Lucas Adamski from comment #18)
> We talked through this on IRC and it does sound like trying to do any client
> verification would add little to no value, while potentially limiting our
> payment implementation options in the future.

Yup, I heard and agreed. Let's do this then. Noming to block.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 22

[Kumar McMillan [:kumar]]

Comment on attachment 678194 [details] [diff] [review]
WIP

Review of attachment 678194 [details] [diff] [review]:
-----------------------------------------------------------------

drive-by r+ from me :)

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

I would love to write some tests for this, but I am currently block because of a missing function ('atob') in xpcshell that is required for the tests. I'll file a follow up bug for them.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/222f4185ecb5

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/222f4185ecb5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e658006870b6

Comment 27

[Jason Smith [:jsmith]]

Updated test plan to reflect what test cases are removed in spreadsheet being tracked to remove validation tests for testing the payment API as an independent entity.