Merge lp:~stevenk/launchpad/auditor-for-packageupload into lp:launchpad

[Steve Kowalik]

Add a feature flag 'auditor.enabled', write an AuditorClient class that will talk to the configured auditor instance and have PackageUpload call into it on accept.

Write an AuditorClient class, which matches up to the AuditorServer test fixture. It will connect to the configured auditor instance and send or receive data. I have also added a feature flag 'auditor.enabled' which model/browser code can use to check if they should call into the auditor service.

[Robert Collins (lifeless)]

Surely auditorclient should be in its own package, not part of LP ?
Other things than LP will want to use Auditor.

[j.c.sackett (jcsackett)]

> Surely auditorclient should be in its own package, not part of LP ?
> Other things than LP will want to use Auditor.

It is: see https://launchpad.net/auditorclient. The subclassing of it here seems sensible since there's no need for the enterpriseid stuff to be migrated into the auditorclient package.

I assume the import changes in enterpiseid.py are b/c of a circular import issue. May be worth it to add a comment indicating such.

Otherwise, this looks good to me. Thanks, Steve.

review: Approve

[Robert Collins (lifeless)]

+ for entry in logs['log-entries']:
+ entry['actor'] = enterpriseid_to_object(entry['actor'])
+ entry['object'] = enterpriseid_to_object(entry['object'])

- this is late evaluation pain waiting to happen. Depending on how
many entries you get back, of course. May need to make a multi-object
version of enterpriseid_to_object.

[Colin Watson (cjwatson)]

Would it be worth fixing the spelling of "receive" (not "recieve") before it gets baked into too many method names and becomes hard to change?

[Colin Watson (cjwatson)]

Oh, and would it be easy to audit rejections at the same time? We occasionally do get questions of the form "who rejected this upload? they didn't tell me why", and we have no way to tell.

=== added file 'lib/lp/services/auditor/client.py'
--- lib/lp/services/auditor/client.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/auditor/client.py	2012-08-06 04:49:24 +0000
@@ -0,0 +1,43 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Client that will send and receive audit logs to an auditor instance."""
+
+__metaclass__ = type
+__all__ = [
+    'AuditorClient',
+    ]
+
+from auditorclient.client import Client
+
+from lp.services.config import config
+from lp.services.enterpriseid import (
+    enterpriseid_to_object,
+    object_to_enterpriseid,
+    )
+
+
+class AuditorClient(Client):
+
+    def __init__(self):
+        super(AuditorClient, self).__init__(
+            config.auditor.host, config.auditor.port)
+
+    def send(self, obj, operation, actorobj, comment=None, details=None):
+        return super(AuditorClient, self).send(
+            object_to_enterpriseid(obj), operation,
+            object_to_enterpriseid(actorobj), comment, details)
+
+
+    def receive(self, obj=None, operation=None, actorobj=None, limit=None):
+        if obj:
+            obj = object_to_enterpriseid(obj)
+        if actorobj:
+            actorobj = object_to_enterpriseid(actorobj)
+        logs = super(AuditorClient, self).receive(
+            obj, operation, actorobj, limit)
+        # Process the actors and objects back from enterprise ids.
+        for entry in logs['log-entries']:
+            entry['actor'] = enterpriseid_to_object(entry['actor'])
+            entry['object'] = enterpriseid_to_object(entry['object'])
+        return logs['log-entries']

=== added file 'lib/lp/services/auditor/tests/test_client.py'
--- lib/lp/services/auditor/tests/test_client.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/auditor/tests/test_client.py	2012-08-06 04:49:24 +0000
@@ -0,0 +1,27 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from lp.services.auditor.client import AuditorClient
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import AuditorLayer
+
+
+class TestAuditorClient(TestCaseWithFactory):
+
+    layer = AuditorLayer
+
+    def test_send_and_receive(self):
+        # We can use .send() and .receive() on AuditorClient to log.
+        actor = self.factory.makePerson()
+        pu = self.factory.makePackageUpload()
+        client = AuditorClient()
+        result = client.send(pu, 'packageupload-accepted', actor)
+        self.assertEqual('Operation recorded.', result)
+        result = client.receive(obj=pu)
+        del result[0]['date'] # Ignore the date.
+        expected = [{
+            u'comment': u'', u'details': u'', u'actor': actor,
+            u'operation': u'packageupload-accepted', u'object': pu}]
+        self.assertContentEqual(expected, result)

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2012-07-26 14:31:45 +0000
+++ lib/lp/services/config/schema-lazr.conf	2012-08-06 04:49:24 +0000
@@ -33,6 +33,11 @@
 run_parts_location: none
 
 
+[auditor]
+host: localhost
+port: none
+
+
 [binaryfile_expire]
 dbuser: binaryfile-expire
 

=== modified file 'lib/lp/services/enterpriseid.py'
--- lib/lp/services/enterpriseid.py	2012-03-22 23:21:24 +0000
+++ lib/lp/services/enterpriseid.py	2012-08-06 04:49:24 +0000
@@ -11,15 +11,9 @@
 
 import os
 
-from lp.registry.model.person import Person
 from lp.services.config import config
 
 
-known_types = {
-    'Person': Person,
-    }
-
-
 def object_to_enterpriseid(obj):
     """Given an object, convert it to SOA Enterprise ID."""
     otype = obj.__class__.__name__
@@ -33,8 +27,15 @@
 
 def enterpriseid_to_object(eid):
     """Given an SOA Enterprise ID, return the object that it references."""
+    # Circular imports.
+    from lp.registry.model.person import Person
+    from lp.soyuz.model.queue import PackageUpload
     scheme = eid.split(':')
     if not scheme[0].startswith('lp'):
         raise TypeError
+    known_types = {
+        'PackageUpload': PackageUpload,
+        'Person': Person,
+    }
     klass = known_types[scheme[1]]
     return klass.get(scheme[2])

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-07-31 00:08:37 +0000
+++ lib/lp/services/features/flags.py	2012-08-06 04:49:24 +0000
@@ -294,6 +294,12 @@
      '',
      '',
      ''),
+    ('auditor.enabled',
+     'boolean',
+     'If true, send audit data to an auditor instance.',
+     '',
+     '',
+     ''),
     ])
 
 # The set of all flag names that are documented.

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2012-07-09 12:32:23 +0000
+++ lib/lp/soyuz/browser/queue.py	2012-08-06 04:49:24 +0000
@@ -443,7 +443,7 @@
 
     def queue_action_accept(self, queue_item):
         """Reject the queue item passed."""
-        queue_item.acceptFromQueue()
+        queue_item.acceptFromQueue(user=self.user)
 
     def queue_action_reject(self, queue_item):
         """Accept the queue item passed."""

=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2012-07-09 12:32:23 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2012-08-06 04:49:24 +0000
@@ -358,8 +358,9 @@
         """
 
     @export_write_operation()
+    @call_with(user=REQUEST_USER)
     @operation_for_version("devel")
-    def acceptFromQueue(logger=None, dry_run=False):
+    def acceptFromQueue(logger=None, dry_run=False, user=None):
         """Call setAccepted, do a syncUpdate, and send notification email.
 
          * Grant karma to people involved with the upload.

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-07-24 16:12:10 +0000
+++ lib/lp/soyuz/model/queue.py	2012-08-06 04:49:24 +0000
@@ -50,6 +50,7 @@
 from lp.archiveuploader.tagfiles import parse_tagfile_content
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.services.auditor.client import AuditorClient
 from lp.services.config import config
 from lp.services.database.bulk import load_referencing
 from lp.services.database.constants import UTC_NOW
@@ -64,6 +65,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.features import getFeatureFlag
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.librarian.interfaces.client import DownloadFailed
 from lp.services.librarian.model import LibraryFileAlias
@@ -625,7 +627,7 @@
         # Give some karma!
         self._giveKarma()
 
-    def acceptFromQueue(self, logger=None, dry_run=False):
+    def acceptFromQueue(self, logger=None, dry_run=False, user=None):
         """See `IPackageUpload`."""
         assert not self.is_delayed_copy, 'Cannot process delayed copies.'
 
@@ -633,6 +635,9 @@
             self._acceptNonSyncFromQueue(logger, dry_run)
         else:
             self._acceptSyncFromQueue()
+        if bool(getFeatureFlag('auditor.enabled')):
+            client = AuditorClient()
+            client.send(self, 'packageupload-accepted', user)
 
     def acceptFromCopy(self):
         """See `IPackageUpload`."""

=== modified file 'lib/lp/testing/layers.py'
--- lib/lp/testing/layers.py	2012-07-05 10:38:03 +0000
+++ lib/lp/testing/layers.py	2012-08-06 04:49:24 +0000
@@ -707,44 +707,6 @@
         pass
 
 
-class AuditorLayer(BaseLayer):
-
-    auditor = AuditorServer()
-
-    _is_setup = False
-
-    @classmethod
-    @profiled
-    def setUp(cls):
-        cls.auditor.setUp()
-        cls.config_fixture.add_section(
-            cls.auditor.config.service_config)
-        cls.appserver_config_fixture.add_section(
-            cls.auditor.config.service_config)
-        cls._is_setup = True
-
-    @classmethod
-    @profiled
-    def tearDown(cls):
-        if not cls._is_setup:
-            return
-        cls.auditor.cleanUp()
-        cls._is_setup = False
-        # Can't pop the config above, so bail here and let the test runner
-        # start a sub-process.
-        raise NotImplementedError
-
-    @classmethod
-    @profiled
-    def testSetUp(cls):
-        pass
-
-    @classmethod
-    @profiled
-    def testTearDown(cls):
-        pass
-
-
 # We store a reference to the DB-API connect method here when we
 # put a proxy in its place.
 _org_connect = None
@@ -1389,6 +1351,42 @@
         disconnect_stores()
 
 
+class AuditorLayer(LaunchpadFunctionalLayer):
+
+    auditor = AuditorServer()
+
+    _is_setup = False
+
+    @classmethod
+    @profiled
+    def setUp(cls):
+        cls.auditor.setUp()
+        cls.config_fixture.add_section(cls.auditor.service_config)
+        cls.appserver_config_fixture.add_section(cls.auditor.service_config)
+        cls._is_setup = True
+
+    @classmethod
+    @profiled
+    def tearDown(cls):
+        if not cls._is_setup:
+            return
+        cls.auditor.cleanUp()
+        cls._is_setup = False
+        # Can't pop the config above, so bail here and let the test runner
+        # start a sub-process.
+        raise NotImplementedError
+
+    @classmethod
+    @profiled
+    def testSetUp(cls):
+        pass
+
+    @classmethod
+    @profiled
+    def testTearDown(cls):
+        pass
+
+
 class GoogleLaunchpadFunctionalLayer(LaunchpadFunctionalLayer,
                                      GoogleServiceLayer):
     """Provides Google service in addition to LaunchpadFunctionalLayer."""

=== modified file 'setup.py'
--- setup.py	2012-07-02 04:21:44 +0000
+++ setup.py	2012-08-06 04:49:24 +0000
@@ -27,6 +27,7 @@
     # used in zcml.
     install_requires=[
         'ampoule',
+        'auditorclient',
         'auditorfixture',
         'BeautifulSoup',
         'bzr',

=== modified file 'versions.cfg'
--- versions.cfg	2012-07-23 21:05:14 +0000
+++ versions.cfg	2012-08-06 04:49:24 +0000
@@ -8,8 +8,9 @@
 amqplib = 1.0.2
 anyjson = 0.3.1
 argparse = 1.2.1
-auditor = 0.0.1
-auditorfixture = 0.0.1
+auditor = 0.0.2
+auditorclient = 0.0.2
+auditorfixture = 0.0.3
 BeautifulSoup = 3.1.0.1
 bson = 0.3.2
 # The source for this version of bzr is at lp:~benji/bzr/bug-998040