diff mbox series

[v2,2/3] Include the responsible actor in applicable events

Message ID 20191007221645.13186-3-johan@herland.net
State Superseded
Headers show
Series Store the 'actor' responsible for events | expand

Commit Message

Johan Herland Oct. 7, 2019, 10:16 p.m. UTC
We want to use the events as an audit log. An important part of this is
recording _who_ made the changes that the events represent.

To accomplish this, we need to know the current user (aka. request.user)
at the point where we create the Event instance. Event creation is
currently triggered by signals, but neither the signal handlers, nor the
model classes themselves have easy access to request.user.

For Patch-based events (patch-created, patch-state-changed,
patch-delegated, patch-completed), we can do the following hack:
The relevant events are created in signal handlers that are all hooked
up to either the pre_save or post_save signals sent by Patch.save().
But before calling Patch.save(), Patchwork must naturally query
Patch.is_editable() to ascertain whether the patch can in fact be
changed by the current user. Thus, we only need a way to communicate
the current user from Patch.is_editable() to the signal handlers that
create the resulting Events. The Patch object itself is available in
both places, so we simply add an ._edited_by attribute to the instance
(which fortunately is not detected as a persistent db field by Django).

The series-completed event is also triggered by Patch.save(), so uses
the same trick as above.

For the check-created event the current user always happens to be the
same as the .user field recorded in the Check object itself.

The remaining events (cover-created, series-created) are both triggered
by incoming emails, hence have no real actor as such, so we simply leave
the actor as None/NULL.

Closes: #73
Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 patchwork/models.py            |  6 +++++-
 patchwork/signals.py           | 10 ++++++++--
 patchwork/tests/test_events.py |  7 +++++++
 3 files changed, 20 insertions(+), 3 deletions(-)

Comments

Daniel Axtens Oct. 9, 2019, 6:04 a.m. UTC | #1
Johan Herland <johan@herland.net> writes:

> We want to use the events as an audit log. An important part of this is
> recording _who_ made the changes that the events represent.
>
> To accomplish this, we need to know the current user (aka. request.user)
> at the point where we create the Event instance. Event creation is
> currently triggered by signals, but neither the signal handlers, nor the
> model classes themselves have easy access to request.user.
>
> For Patch-based events (patch-created, patch-state-changed,
> patch-delegated, patch-completed), we can do the following hack:
> The relevant events are created in signal handlers that are all hooked
> up to either the pre_save or post_save signals sent by Patch.save().
> But before calling Patch.save(), Patchwork must naturally query
> Patch.is_editable() to ascertain whether the patch can in fact be
> changed by the current user. Thus, we only need a way to communicate
> the current user from Patch.is_editable() to the signal handlers that
> create the resulting Events. The Patch object itself is available in
> both places, so we simply add an ._edited_by attribute to the instance
> (which fortunately is not detected as a persistent db field by Django).
>
> The series-completed event is also triggered by Patch.save(), so uses
> the same trick as above.

Aren't patch-created and series-completed usually triggered by incoming
emails also? (like with cover-created)

What happens if you use parsemail or parsearchive to load mail in? Is
the actor always NULL for patch-created in this case?

Regards,
Daniel

>
> For the check-created event the current user always happens to be the
> same as the .user field recorded in the Check object itself.
>
> The remaining events (cover-created, series-created) are both triggered
> by incoming emails, hence have no real actor as such, so we simply leave
> the actor as None/NULL.
>
> Closes: #73
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  patchwork/models.py            |  6 +++++-
>  patchwork/signals.py           | 10 ++++++++--
>  patchwork/tests/test_events.py |  7 +++++++
>  3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/patchwork/models.py b/patchwork/models.py
> index b43c15a..f37572e 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -498,9 +498,13 @@ class Patch(Submission):
>              return False
>  
>          if user in [self.submitter.user, self.delegate]:
> +            self._edited_by = user
>              return True
>  
> -        return self.project.is_editable(user)
> +        if self.project.is_editable(user):
> +            self._edited_by = user
> +            return True
> +        return False
>  
>      @property
>      def combined_check_state(self):
> diff --git a/patchwork/signals.py b/patchwork/signals.py
> index 666199b..987cc5a 100644
> --- a/patchwork/signals.py
> +++ b/patchwork/signals.py
> @@ -77,6 +77,7 @@ def create_patch_created_event(sender, instance, created, raw, **kwargs):
>          return Event.objects.create(
>              category=Event.CATEGORY_PATCH_CREATED,
>              project=patch.project,
> +            actor=getattr(patch, '_edited_by', None),
>              patch=patch)
>  
>      # don't trigger for items loaded from fixtures or new items
> @@ -93,6 +94,7 @@ def create_patch_state_changed_event(sender, instance, raw, **kwargs):
>          return Event.objects.create(
>              category=Event.CATEGORY_PATCH_STATE_CHANGED,
>              project=patch.project,
> +            actor=getattr(patch, '_edited_by', None),
>              patch=patch,
>              previous_state=before,
>              current_state=after)
> @@ -116,6 +118,7 @@ def create_patch_delegated_event(sender, instance, raw, **kwargs):
>          return Event.objects.create(
>              category=Event.CATEGORY_PATCH_DELEGATED,
>              project=patch.project,
> +            actor=getattr(patch, '_edited_by', None),
>              patch=patch,
>              previous_delegate=before,
>              current_delegate=after)
> @@ -139,6 +142,7 @@ def create_patch_completed_event(sender, instance, raw, **kwargs):
>          return Event.objects.create(
>              category=Event.CATEGORY_PATCH_COMPLETED,
>              project=patch.project,
> +            actor=getattr(patch, '_edited_by', None),
>              patch=patch,
>              series=patch.series)
>  
> @@ -184,6 +188,7 @@ def create_check_created_event(sender, instance, created, raw, **kwargs):
>          return Event.objects.create(
>              category=Event.CATEGORY_CHECK_CREATED,
>              project=check.patch.project,
> +            actor=check.user,
>              patch=check.patch,
>              created_check=check)
>  
> @@ -219,10 +224,11 @@ def create_series_completed_event(sender, instance, raw, **kwargs):
>      # exists in the wild ('PATCH 5/n'), so we probably want to retest a series
>      # in that case.
>  
> -    def create_event(series):
> +    def create_event(series, actor):
>          return Event.objects.create(
>              category=Event.CATEGORY_SERIES_COMPLETED,
>              project=series.project,
> +            actor=actor,
>              series=series)
>  
>      # don't trigger for items loaded from fixtures, new items or items that
> @@ -241,4 +247,4 @@ def create_series_completed_event(sender, instance, raw, **kwargs):
>      # we can't use "series.received_all" here since we haven't actually saved
>      # the instance yet so we duplicate that logic here but with an offset
>      if (instance.series.received_total + 1) >= instance.series.total:
> -        create_event(instance.series)
> +        create_event(instance.series, getattr(instance, '_edited_by', None))
> diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py
> index e5c40b5..415237f 100644
> --- a/patchwork/tests/test_events.py
> +++ b/patchwork/tests/test_events.py
> @@ -110,8 +110,10 @@ class PatchChangedTest(_BaseTestCase):
>          patch = utils.create_patch(series=None)
>          old_state = patch.state
>          new_state = utils.create_state()
> +        actor = utils.create_maintainer(project=patch.project)
>  
>          patch.state = new_state
> +        self.assertTrue(patch.is_editable(actor))
>          patch.save()
>  
>          events = _get_events(patch=patch)
> @@ -120,6 +122,7 @@ class PatchChangedTest(_BaseTestCase):
>          self.assertEqual(events[1].category,
>                           Event.CATEGORY_PATCH_STATE_CHANGED)
>          self.assertEqual(events[1].project, patch.project)
> +        self.assertEqual(events[1].actor, actor)
>          self.assertEventFields(events[1], previous_state=old_state,
>                                 current_state=new_state)
>  
> @@ -127,10 +130,12 @@ class PatchChangedTest(_BaseTestCase):
>          # purposefully setting series to None to minimize additional events
>          patch = utils.create_patch(series=None)
>          delegate_a = utils.create_user()
> +        actor = utils.create_maintainer(project=patch.project)
>  
>          # None -> Delegate A
>  
>          patch.delegate = delegate_a
> +        self.assertTrue(patch.is_editable(actor))
>          patch.save()
>  
>          events = _get_events(patch=patch)
> @@ -139,6 +144,7 @@ class PatchChangedTest(_BaseTestCase):
>          self.assertEqual(events[1].category,
>                           Event.CATEGORY_PATCH_DELEGATED)
>          self.assertEqual(events[1].project, patch.project)
> +        self.assertEqual(events[1].actor, actor)
>          self.assertEventFields(events[1], current_delegate=delegate_a)
>  
>          delegate_b = utils.create_user()
> @@ -175,6 +181,7 @@ class CheckCreatedTest(_BaseTestCase):
>          self.assertEqual(events.count(), 1)
>          self.assertEqual(events[0].category, Event.CATEGORY_CHECK_CREATED)
>          self.assertEqual(events[0].project, check.patch.project)
> +        self.assertEqual(events[0].actor, check.user)
>          self.assertEventFields(events[0])
>  
>  
> -- 
> 2.19.2
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Johan Herland Oct. 9, 2019, 4:31 p.m. UTC | #2
On Wed, Oct 9, 2019 at 8:04 AM Daniel Axtens <dja@axtens.net> wrote:
> Johan Herland <johan@herland.net> writes:
> > We want to use the events as an audit log. An important part of this is
> > recording _who_ made the changes that the events represent.
> >
> > To accomplish this, we need to know the current user (aka. request.user)
> > at the point where we create the Event instance. Event creation is
> > currently triggered by signals, but neither the signal handlers, nor the
> > model classes themselves have easy access to request.user.
> >
> > For Patch-based events (patch-created, patch-state-changed,
> > patch-delegated, patch-completed), we can do the following hack:
> > The relevant events are created in signal handlers that are all hooked
> > up to either the pre_save or post_save signals sent by Patch.save().
> > But before calling Patch.save(), Patchwork must naturally query
> > Patch.is_editable() to ascertain whether the patch can in fact be
> > changed by the current user. Thus, we only need a way to communicate
> > the current user from Patch.is_editable() to the signal handlers that
> > create the resulting Events. The Patch object itself is available in
> > both places, so we simply add an ._edited_by attribute to the instance
> > (which fortunately is not detected as a persistent db field by Django).
> >
> > The series-completed event is also triggered by Patch.save(), so uses
> > the same trick as above.
>
> Aren't patch-created and series-completed usually triggered by incoming
> emails also? (like with cover-created)

I'm sure you are right (I'm still finding my way around the code base
and don't know all the sources of these events). In those cases the
'actor' is probably left as NULL (since I suspect is_editable() is
never called before .save()). IMHO this is correct and perfectly fine:
The event simply does not have an 'actor', i.e there is no user that
intentionally caused this thing to happen.

> What happens if you use parsemail or parsearchive to load mail in? Is
> the actor always NULL for patch-created in this case?

I suspect so. Depends on whether parsemail/parsearchive runs with some
kind of request.user set, and if is_editable() is ever called from
that code. Again, the audit log here is (in our view, at least)
primarily there to document a wayward user (or their script)
mistakenly updating patch states and delegates. Recording a non-NULL
actor is only important for events that are triggered manually, not
for events that automatically/naturally arise from incoming emails.

Have fun!
...Johan
Joe Hershberger Oct. 9, 2019, 8:08 p.m. UTC | #3
On Mon, Oct 7, 2019 at 5:18 PM Johan Herland <johan@herland.net> wrote:
>
> We want to use the events as an audit log. An important part of this is
> recording _who_ made the changes that the events represent.
>
> To accomplish this, we need to know the current user (aka. request.user)
> at the point where we create the Event instance. Event creation is
> currently triggered by signals, but neither the signal handlers, nor the
> model classes themselves have easy access to request.user.
>
> For Patch-based events (patch-created, patch-state-changed,
> patch-delegated, patch-completed), we can do the following hack:
> The relevant events are created in signal handlers that are all hooked
> up to either the pre_save or post_save signals sent by Patch.save().
> But before calling Patch.save(), Patchwork must naturally query
> Patch.is_editable() to ascertain whether the patch can in fact be
> changed by the current user. Thus, we only need a way to communicate
> the current user from Patch.is_editable() to the signal handlers that
> create the resulting Events. The Patch object itself is available in
> both places, so we simply add an ._edited_by attribute to the instance
> (which fortunately is not detected as a persistent db field by Django).
>
> The series-completed event is also triggered by Patch.save(), so uses
> the same trick as above.
>
> For the check-created event the current user always happens to be the
> same as the .user field recorded in the Check object itself.
>
> The remaining events (cover-created, series-created) are both triggered
> by incoming emails, hence have no real actor as such, so we simply leave
> the actor as None/NULL.
>
> Closes: #73
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Johan Herland <johan@herland.net>

This is excellent! I've been missing this type of log for a long time.
Always wondering if a patch was delegated as a guess by a helpful 3rd
party or intentionally by the custodian. Thank you!

Cheers,
-Joe
diff mbox series

Patch

diff --git a/patchwork/models.py b/patchwork/models.py
index b43c15a..f37572e 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -498,9 +498,13 @@  class Patch(Submission):
             return False
 
         if user in [self.submitter.user, self.delegate]:
+            self._edited_by = user
             return True
 
-        return self.project.is_editable(user)
+        if self.project.is_editable(user):
+            self._edited_by = user
+            return True
+        return False
 
     @property
     def combined_check_state(self):
diff --git a/patchwork/signals.py b/patchwork/signals.py
index 666199b..987cc5a 100644
--- a/patchwork/signals.py
+++ b/patchwork/signals.py
@@ -77,6 +77,7 @@  def create_patch_created_event(sender, instance, created, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_CREATED,
             project=patch.project,
+            actor=getattr(patch, '_edited_by', None),
             patch=patch)
 
     # don't trigger for items loaded from fixtures or new items
@@ -93,6 +94,7 @@  def create_patch_state_changed_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_STATE_CHANGED,
             project=patch.project,
+            actor=getattr(patch, '_edited_by', None),
             patch=patch,
             previous_state=before,
             current_state=after)
@@ -116,6 +118,7 @@  def create_patch_delegated_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_DELEGATED,
             project=patch.project,
+            actor=getattr(patch, '_edited_by', None),
             patch=patch,
             previous_delegate=before,
             current_delegate=after)
@@ -139,6 +142,7 @@  def create_patch_completed_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_COMPLETED,
             project=patch.project,
+            actor=getattr(patch, '_edited_by', None),
             patch=patch,
             series=patch.series)
 
@@ -184,6 +188,7 @@  def create_check_created_event(sender, instance, created, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_CHECK_CREATED,
             project=check.patch.project,
+            actor=check.user,
             patch=check.patch,
             created_check=check)
 
@@ -219,10 +224,11 @@  def create_series_completed_event(sender, instance, raw, **kwargs):
     # exists in the wild ('PATCH 5/n'), so we probably want to retest a series
     # in that case.
 
-    def create_event(series):
+    def create_event(series, actor):
         return Event.objects.create(
             category=Event.CATEGORY_SERIES_COMPLETED,
             project=series.project,
+            actor=actor,
             series=series)
 
     # don't trigger for items loaded from fixtures, new items or items that
@@ -241,4 +247,4 @@  def create_series_completed_event(sender, instance, raw, **kwargs):
     # we can't use "series.received_all" here since we haven't actually saved
     # the instance yet so we duplicate that logic here but with an offset
     if (instance.series.received_total + 1) >= instance.series.total:
-        create_event(instance.series)
+        create_event(instance.series, getattr(instance, '_edited_by', None))
diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py
index e5c40b5..415237f 100644
--- a/patchwork/tests/test_events.py
+++ b/patchwork/tests/test_events.py
@@ -110,8 +110,10 @@  class PatchChangedTest(_BaseTestCase):
         patch = utils.create_patch(series=None)
         old_state = patch.state
         new_state = utils.create_state()
+        actor = utils.create_maintainer(project=patch.project)
 
         patch.state = new_state
+        self.assertTrue(patch.is_editable(actor))
         patch.save()
 
         events = _get_events(patch=patch)
@@ -120,6 +122,7 @@  class PatchChangedTest(_BaseTestCase):
         self.assertEqual(events[1].category,
                          Event.CATEGORY_PATCH_STATE_CHANGED)
         self.assertEqual(events[1].project, patch.project)
+        self.assertEqual(events[1].actor, actor)
         self.assertEventFields(events[1], previous_state=old_state,
                                current_state=new_state)
 
@@ -127,10 +130,12 @@  class PatchChangedTest(_BaseTestCase):
         # purposefully setting series to None to minimize additional events
         patch = utils.create_patch(series=None)
         delegate_a = utils.create_user()
+        actor = utils.create_maintainer(project=patch.project)
 
         # None -> Delegate A
 
         patch.delegate = delegate_a
+        self.assertTrue(patch.is_editable(actor))
         patch.save()
 
         events = _get_events(patch=patch)
@@ -139,6 +144,7 @@  class PatchChangedTest(_BaseTestCase):
         self.assertEqual(events[1].category,
                          Event.CATEGORY_PATCH_DELEGATED)
         self.assertEqual(events[1].project, patch.project)
+        self.assertEqual(events[1].actor, actor)
         self.assertEventFields(events[1], current_delegate=delegate_a)
 
         delegate_b = utils.create_user()
@@ -175,6 +181,7 @@  class CheckCreatedTest(_BaseTestCase):
         self.assertEqual(events.count(), 1)
         self.assertEqual(events[0].category, Event.CATEGORY_CHECK_CREATED)
         self.assertEqual(events[0].project, check.patch.project)
+        self.assertEqual(events[0].actor, check.user)
         self.assertEventFields(events[0])