Closed Bug 756297 Opened 12 years ago Closed 11 years ago

receiptcheck {status: invalid} is too vague

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
2013-09-17

People

(Reporter: ianbicking, Assigned: andy+bugzilla)

References

Details

(Whiteboard: u=dev p=2 [qa-])

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.
Agreed. I think we can add some error types to the output to indicate why things are invalid yet still keep the service secure.
Priority: -- → P3
Blocks: 766201
No longer blocks: 752013
Blocks: 766199
Assignee: nobody → rtilder
Blocks: 782711
No longer blocks: 766199
Target Milestone: --- → 2012-08-23
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.
Target Milestone: 2012-08-23 → 2012-08-30
Target Milestone: 2012-08-30 → ---
Is this bug still valid?
No longer blocks: 782711
Yes.
Priority: P3 → P4
Version: 1.0 → 1.1
Whiteboard: u=dev p=2
Version: 1.1 → 1.2
Version: 1.2 → 1.3
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.
+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.
https://github.com/mozilla/zamboni/commit/e32439
Assignee: rtilder → amckay
Target Milestone: --- → 2013-09-17
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: u=dev p=2 → u=dev p=2 [qa-]
You need to log in before you can comment on or make changes to this bug.