756297 - receiptcheck {status: invalid} is too vague

Status: RESOLVED FIXED

(Marketplace Graveyard :: Payments/Refunds, defect, P4)

Description

[Ian Bicking (:ianbicking)]

For the receipt-checking client I'm trying to categorize various errors into cases that could be retried later, and cases when the receipt is just wrong.  I believe {"status": "invalid"} should mean that the receipt is permanently invalid.  If the server gets something it doesn't expect, I would expect that to return a 500 error, or perhaps we can define some new status.  This could be tried later, perhaps logged differently.

Right now the receipt check service seems to have some overly aggressive error catching that turns into invalid responses.  I would much rather these simply become 500 (or maybe better 400) responses, and leave "invalid" as a positive assertion that the receipt should not be respected.  Over time we can hunt down those exceptions and turn them into invalid if they represent new ways that we see invalid receipts come about.  Especially to begin with when we return invalid it's more likely to be an error with the receipt checking service than with the receipt.

Arguably we should also create a distinct status for "not a purchase" and "syntactically not a receipt".  Invalid seems to be both of these.  Or we can add a code to the invalid response to give better detail - syntactically invalid jwt, invalid signature, invalid verify URL, etc.

Comment 1

[Kumar McMillan [:kumar]]

Agreed. I think we can add some error types to the output to indicate why things are invalid yet still keep the service secure.

Comment 2

[Wil Clouser [:clouserw]]

Ryan says he worked on this, just needs to prepare a proper pull request.  Bumping to next week for reviews.  Note the low priority - no payments code is active on Aug 30th push.

Comment 3

[Wil Clouser [:clouserw]]

Is this bug still valid?

Comment 4

[Andy McKay]

Yes.

Comment 5

[Andy McKay]

The only real difference I can see is between when something changes on the server. In that scenario you've got a receipt that is "invalid" but something might change on the server to make it "valid". But if the data within the receipt is never going to work, it's invalid.

Would expanding the spec to return:

{'status': 'invalid', 'reason': 'RECEIPT_MISSING_USER'}

Seem ok? We could document errors. For an idea of what they are, here's all the logs I send right now:

            log_info('Valid receipt, not premium')
            log_info('Error decoding receipt')
            log_info('No directed-identifier supplied')
            log_info('Receipt type not in %s' % ','.join(types))
            log_info('Receipt had invalid domain')
            log_info('Receipt had the wrong path')
            log_info('No user in receipt')
            log_info('Invalid store data')
            log_info('No entry in users_install for uuid: %s' % uuid)
            log_info('Invalid receipt, no purchase')
            log_info('Valid receipt, but refunded')
            log_info('Valid receipt')
            log_info('Valid receipt, but invalid contribution')
            log_info('Error with expiry in the receipt')
            log_info('This receipt has expired: %s UTC < %s UTC'

Clean these up and make codes out of them and we have most of the codes.

I ran this by @rforbes and he seemed ok with it.

Comment 6

[Kumar McMillan [:kumar]]

+1, this sounds good to me. I don't think we're relying on security by obscurity for anything so it should be fine to provide an error reason.

Comment 7

[Andy McKay]

https://github.com/mozilla/zamboni/commit/e32439