Skip to content

remove install from receipts (bug 968474) #1820

Merged
merged 1 commit into from Mar 11, 2014

4 participants

@andymckay
Mozilla member
  • removes use of installed from receipt verification
  • moves uuid from installed onto addon-purchase
  • what to do for receipts that don't have addon-purchase (eg developer or reviewer installs, just empty?)

tests pass, but want to think about this one a little bit

r? @robhudson, @kumar303

@kumar303 kumar303 and 1 other commented on an outdated diff Feb 28, 2014
mkt/receipts/utils.py
+ :params app: the app record.
+ :params user: the UserProfile record.
+ """
+ try:
+ return app.addonpurchase_set.get(user=user).uuid
+ except ObjectDoesNotExist:
+ return str(uuid.uuid4())
+
+
+def create_receipt(webapp, user, uuid, flavour=None):
+ """
+ Creates a receipt for use in payments.
+
+ :params app: the app record.
+ :params user: the UserProfile record.
+ :params uuid: a user uuid for this purchase.
@kumar303
Mozilla member
kumar303 added a note Feb 28, 2014

I don't think this is a "user uuid" anymore

@andymckay
Mozilla member
andymckay added a note Mar 1, 2014

I don't think it either was before, it was just a uuid for that install row. The spec says: "Instead, the suggested implementation is to randomly generate a GUID for each transaction. The store should retain the GUID, and use it to link receipts to accounts, but it should be an unusable opaque identifier to all others."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhudson robhudson commented on the diff Feb 28, 2014
apps/market/models.py
type = models.PositiveIntegerField(default=amo.CONTRIB_PURCHASE,
choices=do_dictsort(amo.CONTRIB_TYPES),
db_index=True)
+ user = models.ForeignKey(UserProfile)
+ uuid = models.CharField(max_length=255, db_index=True, unique=True)
@robhudson
Mozilla member
robhudson added a note Feb 28, 2014

Is this to not leak user IDs outside of zamboni?

@andymckay
Mozilla member
andymckay added a note Mar 1, 2014

Yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kumar303 kumar303 commented on the diff Feb 28, 2014
services/verify.py
self.cursor.execute(sql, {'addon_id': self.addon_id,
- 'user_id': self.user_id})
+ 'uuid': self.uuid})
@kumar303
Mozilla member
kumar303 added a note Feb 28, 2014

How does this work when uuid does not point to a row in app.addonpurchase_set ? i.e. when the create receipt code did uuid.uuid4()

@andymckay
Mozilla member
andymckay added a note Mar 1, 2014

I think in this case, and this is something I have to check, we have a test, reviewer or developer receipt. They get verified somewhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhudson robhudson commented on an outdated diff Feb 28, 2014
mkt/receipts/utils.py
@@ -13,11 +15,32 @@
from lib.crypto.receipt import sign
-def create_receipt(installed, flavour=None):
+def get_uuid(app, user):
+ """
+ Returns a users uuid suitable for use in the receipt, by looking up
+ the purchase table. Otherwise it just returns a random uuid.
@robhudson
Mozilla member
robhudson added a note Feb 28, 2014

A random one? Or a newly generated one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kumar303 kumar303 and 1 other commented on an outdated diff Feb 28, 2014
mkt/receipts/utils.py
@@ -13,11 +15,32 @@
from lib.crypto.receipt import sign
-def create_receipt(installed, flavour=None):
+def get_uuid(app, user):
+ """
+ Returns a users uuid suitable for use in the receipt, by looking up
+ the purchase table. Otherwise it just returns a random uuid.
+
+ :params app: the app record.
+ :params user: the UserProfile record.
+ """
+ try:
+ return app.addonpurchase_set.get(user=user).uuid
+ except ObjectDoesNotExist:
+ return str(uuid.uuid4())
@kumar303
Mozilla member
kumar303 added a note Feb 28, 2014

I don't see where the random UUID is stored so it can be looked up later

@andymckay
Mozilla member
andymckay added a note Mar 1, 2014

Its not, should we even bother in this case? The use cases for getting a receipt without a user id are for test, developer and reviewer receipts. In these cases they haven't bought it and the app developer doesn't really care who the user is anyway, perhaps we should just put null in the receipt?

@andymckay
Mozilla member
andymckay added a note Mar 1, 2014

Right so developer and reviewer receipts don't use this path: https://github.com/mozilla/zamboni/blob/master/services/verify.py#L99
and neither does a test-receipt https://github.com/mozilla/zamboni/blob/master/services/verify.py#L114. They don't call check_purchase https://github.com/mozilla/zamboni/blob/master/services/verify.py#L91 which is where the uuid lives.

@andymckay
Mozilla member
andymckay added a note Mar 4, 2014

I've checked and developer receipts, if asked for from the consumer pages, will hit check_purchase and fail.

@andymckay
Mozilla member
andymckay added a note Mar 4, 2014

Nope, i was wrong - developer, reviewer and test receipts all go to receipt.verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kumar303 kumar303 commented on an outdated diff Feb 28, 2014
migrations/756-migrate-uuid.sql
@@ -0,0 +1,6 @@
+ALTER TABLE addon_purchase ADD COLUMN `uuid` varchar(255) NULL UNIQUE;
+
+UPDATE addon_purchase INNER JOIN users_install
+ON addon_purchase.user_id = users_install.user_id
@kumar303
Mozilla member
kumar303 added a note Feb 28, 2014

sort of weird to read this without indentation

@kumar303
Mozilla member
kumar303 added a note Feb 28, 2014

is this join correct? Seems like there would be multiple purchases and multiple installs linked to the same user_id. Maybe you meant this? (addon_purchase.user_id = users_install.user_id AND addon_purchase.addon_id = users_install.addon_id)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kumar303 kumar303 commented on an outdated diff Feb 28, 2014
migrations/756-migrate-uuid.sql
@@ -0,0 +1,6 @@
+ALTER TABLE addon_purchase ADD COLUMN `uuid` varchar(255) NULL UNIQUE;
+
+UPDATE addon_purchase INNER JOIN users_install
+ON addon_purchase.user_id = users_install.user_id
+AND addon_purchase.addon_id = users_install.addon_id
+SET addon_purchase.uuid = users_install.uuid;
@kumar303
Mozilla member
kumar303 added a note Feb 28, 2014

after the migration, shouldn't this make the uuid column not null?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhudson robhudson commented on the diff Feb 28, 2014
services/verify.py
@@ -205,28 +201,15 @@ def check_db(self):
log_info('Invalid store data')
raise InvalidReceipt('WRONG_STOREDATA')
- sql = """SELECT id, user_id, premium_type FROM users_install
- WHERE addon_id = %(addon_id)s
- AND uuid = %(uuid)s LIMIT 1;"""
- self.cursor.execute(sql, {'addon_id': self.addon_id,
- 'uuid': uuid})
- result = self.cursor.fetchone()
- if not result:
- # We've got no record of this receipt being created.
- log_info('No entry in users_install for uuid: %s' % uuid)
- raise InvalidReceipt('WRONG_USER')
@robhudson
Mozilla member
robhudson added a note Feb 28, 2014

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kumar303 kumar303 commented on the diff Feb 28, 2014
services/verify.py
@@ -205,28 +201,15 @@ def check_db(self):
log_info('Invalid store data')
raise InvalidReceipt('WRONG_STOREDATA')
- sql = """SELECT id, user_id, premium_type FROM users_install
- WHERE addon_id = %(addon_id)s
- AND uuid = %(uuid)s LIMIT 1;"""
- self.cursor.execute(sql, {'addon_id': self.addon_id,
- 'uuid': uuid})
- result = self.cursor.fetchone()
- if not result:
- # We've got no record of this receipt being created.
- log_info('No entry in users_install for uuid: %s' % uuid)
- raise InvalidReceipt('WRONG_USER')
-
- pk, self.user_id, self.premium = result
-
def check_purchase(self):
"""
Verifies that the app has been purchased.
"""
sql = """SELECT id, type FROM addon_purchase
@kumar303
Mozilla member
kumar303 added a note Feb 28, 2014

In thinking about in-app purchases, could this query the contributions table instead? Just thinking out loud.

@andymckay
Mozilla member
andymckay added a note Mar 1, 2014

Yeah this will all have to change again for when we land in-app purchases, perhaps addon-purchase changes, growing another foreign key?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andymckay andymckay and 1 other commented on an outdated diff Mar 1, 2014
migrations/756-migrate-uuid.sql
@@ -0,0 +1,6 @@
+ALTER TABLE addon_purchase ADD COLUMN `uuid` varchar(255) NULL UNIQUE;
+
+UPDATE addon_purchase INNER JOIN users_install
+ON addon_purchase.user_id = users_install.user_id
+AND addon_purchase.addon_id = users_install.addon_id
@andymckay
Mozilla member
andymckay added a note Mar 1, 2014

Doesn't the AND here do that?

@kumar303
Mozilla member
kumar303 added a note Mar 4, 2014

oh right, my bad. The indentation threw me off. I thought it was a where clauses or maybe I just missed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cvan cvan and 1 other commented on an outdated diff Mar 4, 2014
mkt/receipts/utils.py
+ :params user: the UserProfile record.
+ """
+ try:
+ return app.addonpurchase_set.get(user=user).uuid
+ except ObjectDoesNotExist:
+ return str(uuid.uuid4())
+
+
+def create_receipt(webapp, user, uuid, flavour=None):
+ """
+ Creates a receipt for use in payments.
+
+ :params app: the app record.
+ :params user: the UserProfile record.
+ :params uuid: a user uuid for this purchase.
+ :params flavor: None|'developer'|'reviewer', the type of receipt.
@cvan
Mozilla member
cvan added a note Mar 4, 2014

you call it flavour but flavor here. do me a favour or favor and be consistent.

@andymckay
Mozilla member
andymckay added a note Mar 4, 2014

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cvan cvan commented on an outdated diff Mar 4, 2014
mkt/receipts/views.py
@@ -22,7 +22,7 @@
from lib.cef_loggers import receipt_cef
import mkt
from mkt.constants import apps
-from mkt.receipts.utils import create_test_receipt
+from mkt.receipts.utils import get_uuid, create_receipt, create_test_receipt
@cvan
Mozilla member
cvan added a note Mar 4, 2014

🍟 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andymckay
Mozilla member

feedback from review done, plus some import cleaning @kumar303

@andymckay
Mozilla member

I think this is good to go, but I'm too close to tag time. I'll push this on Tuesday next week (when I'm back from PTO).

@andymckay andymckay merged commit 46ec49e into mozilla:master Mar 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.