Merge lp:~michael.nelson/ubuntu-webcatalog/1040523-oops-creating-nonce into lp:ubuntu-webcatalog

[Michael Nelson]

Nonce is unique per consumer/token rather than unique for the table.

Overview
========

See the pre-imp discussion with myself on the bug :P. The code was checking whether a nonce existed for a given consumer/token, and if not creating the nonce, even though the nonce may not be unique itself - causing nearly 4k oopses per day.

This branch relaxes the unique constraint on Nonce.nonce so that it is required to be unique for the consumer/token only, matching the actual check itself.

`fab test`

[Anthony Lenton (elachuni)]

Good idea noodles!

I'd love to see this applied to rnr, sca and rec too at some point, though for some reason apps.u.c had many more of these oopses than the other services.

review: Approve

=== added file 'src/webcatalog/migrations/0028_nonce_unique_together.py'
--- src/webcatalog/migrations/0028_nonce_unique_together.py	1970-01-01 00:00:00 +0000
+++ src/webcatalog/migrations/0028_nonce_unique_together.py	2012-08-23 10:17:22 +0000
@@ -0,0 +1,174 @@
+# encoding: utf-8
+import datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        
+        # Removing unique constraint on 'Nonce', fields ['nonce']
+        db.delete_unique('webcatalog_nonce', ['nonce'])
+
+        # Adding unique constraint on 'Nonce', fields ['nonce', 'token', 'consumer']
+        db.create_unique('webcatalog_nonce', ['nonce', 'token_id', 'consumer_id'])
+
+
+    def backwards(self, orm):
+        
+        # Removing unique constraint on 'Nonce', fields ['nonce', 'token', 'consumer']
+        db.delete_unique('webcatalog_nonce', ['nonce', 'token_id', 'consumer_id'])
+
+        # Adding unique constraint on 'Nonce', fields ['nonce']
+        db.create_unique('webcatalog_nonce', ['nonce'])
+
+
+    models = {
+        'auth.group': {
+            'Meta': {'object_name': 'Group'},
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
+            'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
+        },
+        'auth.permission': {
+            'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'},
+            'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
+        },
+        'auth.user': {
+            'Meta': {'object_name': 'User'},
+            'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}),
+            'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
+            'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
+            'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
+        },
+        'contenttypes.contenttype': {
+            'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
+            'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
+        },
+        'webcatalog.application': {
+            'Meta': {'ordering': "('-wilson_score', 'name')", 'unique_together': "(('distroseries', 'package_name'),)", 'object_name': 'Application'},
+            'app_type': ('django.db.models.fields.CharField', [], {'max_length': '32', 'blank': 'True'}),
+            'architectures': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'archive_id': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '64', 'null': 'True', 'blank': 'True'}),
+            'categories': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'channel': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'comment': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'debtags': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'departments': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['webcatalog.Department']", 'symmetrical': 'False', 'blank': 'True'}),
+            'description': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
+            'distroseries': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['webcatalog.DistroSeries']"}),
+            'icon': ('django.db.models.fields.files.ImageField', [], {'max_length': '200', 'null': 'True', 'blank': 'True'}),
+            'icon_name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'imported_from_sca': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'is_latest': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'keywords': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'license': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}),
+            'mimetype': ('django.db.models.fields.CharField', [], {'max_length': '2048', 'blank': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+            'package_name': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'popcon': ('django.db.models.fields.IntegerField', [], {'null': 'True', 'blank': 'True'}),
+            'price': ('django.db.models.fields.DecimalField', [], {'null': 'True', 'max_digits': '7', 'decimal_places': '2', 'blank': 'True'}),
+            'ratings_average': ('django.db.models.fields.DecimalField', [], {'null': 'True', 'max_digits': '3', 'decimal_places': '2', 'blank': 'True'}),
+            'ratings_histogram': ('django.db.models.fields.CharField', [], {'max_length': '128', 'blank': 'True'}),
+            'ratings_total': ('django.db.models.fields.IntegerField', [], {'null': 'True', 'blank': 'True'}),
+            'section': ('django.db.models.fields.CharField', [], {'max_length': '32', 'blank': 'True'}),
+            'version': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}),
+            'wilson_score': ('django.db.models.fields.FloatField', [], {'db_index': 'True', 'null': 'True', 'blank': 'True'})
+        },
+        'webcatalog.applicationmedia': {
+            'Meta': {'ordering': "('url',)", 'unique_together': "(('application', 'url'),)", 'object_name': 'ApplicationMedia'},
+            'application': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['webcatalog.Application']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'media_type': ('django.db.models.fields.CharField', [], {'max_length': '16'}),
+            'url': ('django.db.models.fields.URLField', [], {'max_length': '200'})
+        },
+        'webcatalog.consumer': {
+            'Meta': {'object_name': 'Consumer'},
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'key': ('django.db.models.fields.CharField', [], {'max_length': '64'}),
+            'secret': ('django.db.models.fields.CharField', [], {'default': "'YduzTarbGEAUckidRHwqIvXkbMOIWW'", 'max_length': '255', 'blank': 'True'}),
+            'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}),
+            'user': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'oauth_consumer'", 'unique': 'True', 'to': "orm['auth.User']"})
+        },
+        'webcatalog.department': {
+            'Meta': {'object_name': 'Department'},
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '64'}),
+            'parent': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['webcatalog.Department']", 'null': 'True', 'blank': 'True'}),
+            'slug': ('django.db.models.fields.SlugField', [], {'unique': 'True', 'max_length': '50', 'db_index': 'True'})
+        },
+        'webcatalog.distroseries': {
+            'Meta': {'object_name': 'DistroSeries'},
+            'code_name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '20', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'prerelease': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'version': ('django.db.models.fields.CharField', [], {'max_length': '10', 'blank': 'True'})
+        },
+        'webcatalog.exhibit': {
+            'Meta': {'object_name': 'Exhibit'},
+            'banner_url': ('django.db.models.fields.CharField', [], {'max_length': '1024'}),
+            'click_url': ('django.db.models.fields.URLField', [], {'default': "''", 'max_length': '200'}),
+            'date_created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'display': ('django.db.models.fields.NullBooleanField', [], {'null': 'True', 'blank': 'True'}),
+            'distroseries': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['webcatalog.DistroSeries']", 'symmetrical': 'False'}),
+            'html': ('django.db.models.fields.TextField', [], {}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'package_names': ('django.db.models.fields.CharField', [], {'max_length': '1024'}),
+            'published': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'sca_id': ('django.db.models.fields.IntegerField', [], {}),
+            'weight': ('django.db.models.fields.IntegerField', [], {'default': '0'})
+        },
+        'webcatalog.machine': {
+            'Meta': {'unique_together': "(('owner', 'uuid'),)", 'object_name': 'Machine'},
+            'hostname': ('django.db.models.fields.CharField', [], {'max_length': '64'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'logo_checksum': ('django.db.models.fields.CharField', [], {'max_length': '56', 'blank': 'True'}),
+            'owner': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}),
+            'package_list': ('django.db.models.fields.TextField', [], {}),
+            'packages_checksum': ('django.db.models.fields.CharField', [], {'max_length': '56'}),
+            'uuid': ('django.db.models.fields.CharField', [], {'max_length': '32', 'db_index': 'True'})
+        },
+        'webcatalog.nonce': {
+            'Meta': {'unique_together': "(('nonce', 'token', 'consumer'),)", 'object_name': 'Nonce'},
+            'consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['webcatalog.Consumer']"}),
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'nonce': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+            'token': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['webcatalog.Token']"})
+        },
+        'webcatalog.reviewstatsimport': {
+            'Meta': {'object_name': 'ReviewStatsImport'},
+            'distroseries': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['webcatalog.DistroSeries']", 'unique': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'last_import': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.utcnow'})
+        },
+        'webcatalog.token': {
+            'Meta': {'object_name': 'Token'},
+            'consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['webcatalog.Consumer']"}),
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
+            'token': ('django.db.models.fields.CharField', [], {'default': "'oHbbjpYHzIoCMTZpBVYAeMYiogLeMYUWeHyabzbvrPRqEzijjY'", 'max_length': '50', 'primary_key': 'True'}),
+            'token_secret': ('django.db.models.fields.CharField', [], {'default': "'QdhhAEWTjdtjTMnLoBEdnEIEzgFowyDRwkQjGDVtAlFpKgxvmK'", 'max_length': '50'}),
+            'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'})
+        }
+    }
+
+    complete_apps = ['webcatalog']

=== modified file 'src/webcatalog/models/oauthtoken.py'
--- src/webcatalog/models/oauthtoken.py	2012-06-06 16:38:11 +0000
+++ src/webcatalog/models/oauthtoken.py	2012-08-23 10:17:22 +0000
@@ -135,7 +135,7 @@
 class Nonce(models.Model):
     token = models.ForeignKey(Token)
     consumer = models.ForeignKey(Consumer)
-    nonce = models.CharField(max_length=255, unique=True, editable=False)
+    nonce = models.CharField(max_length=255, editable=False)
     created_at = models.DateTimeField(auto_now_add=True)
 
     @classmethod
@@ -149,6 +149,7 @@
 
     class Meta:
         app_label = 'webcatalog'
+        unique_together = ('nonce', 'token', 'consumer')
 
 
 class DataStore(OAuthDataStore):

=== modified file 'src/webcatalog/tests/__init__.py'
--- src/webcatalog/tests/__init__.py	2012-06-28 18:26:25 +0000
+++ src/webcatalog/tests/__init__.py	2012-08-23 10:17:22 +0000
@@ -23,6 +23,7 @@
 from .test_forms import *
 from .test_handlers import *
 from .test_models import *
+from .test_models_oauth import *
 from .test_managers import *
 from .test_migrations import *
 from .test_pep8 import *

=== modified file 'src/webcatalog/tests/factory.py'
--- src/webcatalog/tests/factory.py	2012-08-20 15:36:56 +0000
+++ src/webcatalog/tests/factory.py	2012-08-23 10:17:22 +0000
@@ -225,13 +225,16 @@
             token.save()
         return token, consumer
 
-    def make_nonce(self, token=None, consumer=None, created_at=None):
+    def make_nonce(self, token=None, consumer=None, created_at=None,
+                   nonce_string=None):
         if token is None or consumer is None:
             assert token is None and consumer is None
             token, consumer = self.make_oauth_token_and_consumer()
+        if nonce_string is None:
+            nonce_string = self.get_unique_string(prefix='nonce-')
+
         nonce = Nonce.objects.create(
-            token=token, consumer=consumer,
-            nonce=self.get_unique_string(prefix='nonce-'))
+            token=token, consumer=consumer, nonce=nonce_string)
         if created_at:
             nonce.created_at = created_at
             nonce.save()

=== added file 'src/webcatalog/tests/test_models_oauth.py'
--- src/webcatalog/tests/test_models_oauth.py	1970-01-01 00:00:00 +0000
+++ src/webcatalog/tests/test_models_oauth.py	2012-08-23 10:17:22 +0000
@@ -0,0 +1,65 @@
+#   -*- coding: utf-8 -*-
+#   This file is part of the Apps Directory
+#   Copyright (C) 2011 Canonical Ltd.
+#
+#   This program is free software: you can redistribute it and/or modify
+#   it under the terms of the GNU Affero General Public License as
+#   published by the Free Software Foundation, either version 3 of the
+#   License, or (at your option) any later version.
+#
+#   This program is distributed in the hope that it will be useful,
+#   but WITHOUT ANY WARRANTY; without even the implied warranty of
+#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#   GNU Affero General Public License for more details.
+#
+#   You should have received a copy of the GNU Affero General Public License
+#   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test cases for oauth models."""
+
+from webcatalog.models import (
+    DataStore,
+    Nonce,
+    )
+from webcatalog.tests.factory import TestCaseWithFactory
+
+
+class DataStoreTestCase(TestCaseWithFactory):
+
+    def test_lookup_nonce_creates_nonce(self):
+        token, consumer = self.factory.make_oauth_token_and_consumer()
+
+        result = DataStore().lookup_nonce(
+            consumer.oauth_consumer(), token.oauth_token(),
+            'unique-nonce')
+
+        self.assertFalse(result)
+        self.assertEqual(1, Nonce.objects.filter(
+            token=token, consumer=consumer,
+            nonce='unique-nonce').count())
+
+    def test_lookup_nonce_exists(self):
+        nonce = self.factory.make_nonce(nonce_string='unique')
+        token = nonce.token
+        consumer = nonce.consumer
+
+        result = DataStore().lookup_nonce(
+            consumer.oauth_consumer(), token.oauth_token(),
+            'unique')
+
+        self.assertTrue(result)
+
+    def test_lookup_nonce_uniquish(self):
+        # If a nonce for the given consumer/token doesn't
+        # exist, it's considered uniquish and valid.
+        nonce = self.factory.make_nonce(nonce_string='uniquish-nonce')
+        token, consumer = self.factory.make_oauth_token_and_consumer()
+
+        result = DataStore().lookup_nonce(
+            consumer.oauth_consumer(), token.oauth_token(),
+            'uniquish-nonce')
+
+        self.assertFalse(result)
+        self.assertEqual(1, Nonce.objects.filter(
+            token=token, consumer=consumer,
+            nonce='uniquish-nonce').count())