[PATCH] parser: Unmangle From: headers that have been mangled for DMARC purposes
To avoid triggering spam filters due to failed signature validation, many
mailing lists mangle the From header to change the From address to be the
address of the list, typically where the sender's domain has a strict DMARC
policy enabled.
In this case, we should try to unmangle the From header.
Add support for using the X-Original-Sender or Reply-To headers, as used by
Google Groups and Mailman respectively, to unmangle the From header when
necessary.
Closes: #64 ("Incorrect submitter when using googlegroups")
Reported-by: Alexandre Belloni
Reported-by: Stephen Rothwell
Signed-off-by: Andrew Donnellan
---
patchwork/parser.py| 75 ++
patchwork/tests/test_parser.py | 68 --
2 files changed, 130 insertions(+), 13 deletions(-)
diff --git a/patchwork/parser.py b/patchwork/parser.py
index 7dc66bc05a5b..79beac5617b1 100644
--- a/patchwork/parser.py
+++ b/patchwork/parser.py
@@ -321,12 +321,7 @@ def find_series(project, mail, author):
return _find_series_by_markers(project, mail, author)
-def get_or_create_author(mail):
-from_header = clean_header(mail.get('From'))
-
-if not from_header:
-raise ValueError("Invalid 'From' header")
-
+def split_from_header(from_header):
name, email = (None, None)
# tuple of (regex, fn)
@@ -355,6 +350,65 @@ def get_or_create_author(mail):
(name, email) = fn(match.groups())
break
+return (name, email)
+
+
+# Unmangle From addresses that have been mangled for DMARC purposes.
+#
+# To avoid triggering spam filters due to failed signature validation, many
+# mailing lists mangle the From header to change the From address to be the
+# address of the list, typically where the sender's domain has a strict
+# DMARC policy enabled.
+#
+# Unfortunately, there's no standardised way of preserving the original
+# From address.
+#
+# Google Groups adds an X-Original-Sender header. If present, we use that.
+#
+# Mailman preserves the original address by adding a Reply-To, except in the
+# case where the list is set to either reply to list, or reply to a specific
+# address, in which case the original From is added to Cc instead. These corner
+# cases are dumb, but we try and handle things as sensibly as possible by
+# looking for a name in Reply-To/Cc that matches From. It's inexact but should
+# be good enough for our purposes.
+def get_original_sender(mail, name, email):
+if name and ' via ' in name:
+# Mailman uses the format " via "
+# Google Groups uses "'' via "
+stripped_name = name[:name.rfind(' via ')].strip().strip("'")
+
+original_sender = clean_header(mail.get('X-Original-Sender', ''))
+if original_sender:
+new_email = split_from_header(original_sender)[1].strip()[:255]
+return (stripped_name, new_email)
+
+addrs = []
+reply_to_headers = mail.get_all('Reply-To') or []
+cc_headers = mail.get_all('Cc') or []
+for header in reply_to_headers + cc_headers:
+header = clean_header(header)
+addrs = header.split(",")
+for addr in addrs:
+new_name, new_email = split_from_header(addr)
+if new_name:
+new_name = new_name.strip()[:255]
+if new_email:
+new_email = new_email.strip()[:255]
+if new_name == stripped_name:
+return (stripped_name, new_email)
+
+# If we can't figure out the original sender, just keep it as is
+return (name, email)
+
+
+def get_or_create_author(mail, project=None):
+from_header = clean_header(mail.get('From'))
+
+if not from_header:
+raise ValueError("Invalid 'From' header")
+
+name, email = split_from_header(from_header)
+
if not email:
raise ValueError("Invalid 'From' header")
@@ -362,6 +416,9 @@ def get_or_create_author(mail):
if name is not None:
name = name.strip()[:255]
+if project and email.lower() == project.listemail.lower():
+name, email = get_original_sender(mail, name, email)
+
# this correctly handles the case where we lose the race to create
# the person and another process beats us to it. (If the record
# does not exist, g_o_c invokes _create_object_from_params which
@@ -1004,7 +1061,7 @@ def parse_mail(mail, list_id=None):
if not is_comment and (diff or pull_url): # patches or pull requests
# we delay the saving until we know we have a patch.
-author = get_or_create_author(mail)
+author = get_or_create_author(mail, project)
delegate = find_delegate_by_header(mail)
if not delegate and diff:
@@ -1099,7 +1156,7 @@ def parse_mail(mail, list_id=None):
is_cover_letter = True
if is_cover_letter:
-author = get_or_create_author(mail)
+author = get_or_create_author(mail, project)
# we don't use 'find_series' here as a cover letter will
Re: Deduplication of patchwork mail content?
Hi Daniel, > While I'm at it, it occurred to me that for both the ozlabs and > kernel.org instances, there are a lot of mails that are sent across > multiple projects. ATM the entire contents of the mail - content, > headers, diff, what have you, will be stored in full for each project. The headers will be different, as they've gone through different lists. This may not be too relevant to the actual purpose of patchwork though. The comments (apart from the first) may diverge, depending on whether responders keep both lists on CC. The diffs will be the same, so we could deduplicate those, if it's worth your trouble: patchwork=# select sum(dup_size) from (select octet_length(diff) * (n-1) as dup_size, a.msgid, n from (select msgid, count(msgid) as n, min(id) as id from patchwork_submission group by msgid having count(msgid) > 1) as a inner join patchwork_patch on patchwork_patch.submission_ptr_id = a.id) as b; sum --- 221334709 (1 row) and: patchwork=# select sum(octet_length(diff)) from patchwork_patch; sum 6261083055 (1 row) So 221MB out of 6.2GB is duplicate; around 3.5%. Cheers, Jeremy ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH] Use secrets and fall back to random.SystemRandom for keys
On Thu, Oct 10, 2019 at 09:30:50AM +1100, Daniel Axtens wrote: > Hi Jeremy, > > > The random module uses the Mersenne Twister pseudorandom number > > generator and is not a cryptographically secure random number > > generator[0]. The secrets[1] module is intended for generating > > cryptographically strong random numbers, so recommend using that to > > generate the secret key. It's new in Python 3, so if it's unavailable > > fall back to using the ``os.urandom()`` backed implementation of random. > > > > [0] https://docs.python.org/3/library/random.html > > [1] https://docs.python.org/3/library/secrets.html > > > > Thanks for your patch. > > I agree that correctly generated randomness is the right way to go. > > Do you think we need to advise existing implementations to roll their > secret? My feeling is that given the way the twister has been seeded > since Python 2.4 (os.urandom if available), existing installations are > probably OK, but I'd be interested in your take. > If I were running the service I would, but I do agree that given it's likely it was seeded with os.urandom existing installations are probably fine. > > Signed-off-by: Jeremy Cline > > --- > > docs/deployment/installation.rst | 10 -- > > patchwork/settings/production.example.py | 12 +--- > > ...andom.SystemRandom-for-keys-9ceb496919a1bb6f.yaml | 5 + > > 3 files changed, 22 insertions(+), 5 deletions(-) > > create mode 100644 > > releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml > > > > diff --git a/docs/deployment/installation.rst > > b/docs/deployment/installation.rst > > index d422573..f477a11 100644 > > --- a/docs/deployment/installation.rst > > +++ b/docs/deployment/installation.rst > > @@ -254,9 +254,15 @@ This should be a random value and kept secret. You can > > generate and a value for > > > > .. code-block:: python > > > > - import string, random > > + import string > > + try: > > + import secrets > > + except ImportError: # Python < 3.6 > > + import random > > + secrets = random.SystemRandom() > > We're dropping Python 2 soon, not in the next version coming out Real > Soon Now, but in the version after that. Would it be worth holding this > patch until then so as we can avoid this messy try:import? I have a > topic branch for this and I'd be happy to include this patch in it. > An alternate approach here would be to just not use the secrets API. CPython is just proxying the calls to secrets.choice to random.SystemRandom().choice() so it comes to the same thing. I just prefer the secrets API since its promise is to be fit for cryptographic purposes and if they find a bug they'll fix it. In my own projects I prefer to use the newer APIs when possible and fall back as I don't find the clean-up to be terribly difficult (I just grep for ImportErrors), but I do get that it's ugly. I don't have a strong opinion about doing just random.SystemRandom().choice(), just secrets.choice() in a deferred branch, or this, so I'll leave it up to you. Thanks, Jeremy ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH] Use secrets and fall back to random.SystemRandom for keys
On Thu, Oct 10, 2019 at 09:32:13AM +1100, Daniel Axtens wrote: > > diff --git > > a/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml > > > > b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml > > new file mode 100644 > > index 000..7b101cb > > --- /dev/null > > +++ > > b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml > > @@ -0,0 +1,5 @@ > > +--- > > +security: > > + - | > > +Change the recommended method for generating the Django secret key to > > use a > > +cryptographically secure random number generator. > > Oh, while I remember, I think I've had trouble with the security: > section before. Have you been able to verify that this shows up in the > docs? (I build mine with `docker-compose run web tox -e docs`) > Ah, you caught me. I noticed the request for a release note late in the game and didn't actually check that it worked in the docs. You're right, it doesn't show up. Some quick debugging indicates that the reno config is excluding the section, something like: diff --git a/releasenotes/config.yaml b/releasenotes/config.yaml index cd31940..6c7d622 100644 --- a/releasenotes/config.yaml +++ b/releasenotes/config.yaml @@ -11,3 +11,4 @@ sections: - [fixes, Bug Fixes] - [api, API Changes] - [other, Other Notes] + - [security, Security Notes] fixes it. I can re-roll the patch if you like. ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH] Use secrets and fall back to random.SystemRandom for keys
> diff --git > a/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml > > b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml > new file mode 100644 > index 000..7b101cb > --- /dev/null > +++ > b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml > @@ -0,0 +1,5 @@ > +--- > +security: > + - | > +Change the recommended method for generating the Django secret key to > use a > +cryptographically secure random number generator. Oh, while I remember, I think I've had trouble with the security: section before. Have you been able to verify that this shows up in the docs? (I build mine with `docker-compose run web tox -e docs`) Regards, Daniel > -- > 2.21.0 > > ___ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH] Use secrets and fall back to random.SystemRandom for keys
Hi Jeremy,
> The random module uses the Mersenne Twister pseudorandom number
> generator and is not a cryptographically secure random number
> generator[0]. The secrets[1] module is intended for generating
> cryptographically strong random numbers, so recommend using that to
> generate the secret key. It's new in Python 3, so if it's unavailable
> fall back to using the ``os.urandom()`` backed implementation of random.
>
> [0] https://docs.python.org/3/library/random.html
> [1] https://docs.python.org/3/library/secrets.html
>
Thanks for your patch.
I agree that correctly generated randomness is the right way to go.
Do you think we need to advise existing implementations to roll their
secret? My feeling is that given the way the twister has been seeded
since Python 2.4 (os.urandom if available), existing installations are
probably OK, but I'd be interested in your take.
> Signed-off-by: Jeremy Cline
> ---
> docs/deployment/installation.rst | 10 --
> patchwork/settings/production.example.py | 12 +---
> ...andom.SystemRandom-for-keys-9ceb496919a1bb6f.yaml | 5 +
> 3 files changed, 22 insertions(+), 5 deletions(-)
> create mode 100644
> releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
>
> diff --git a/docs/deployment/installation.rst
> b/docs/deployment/installation.rst
> index d422573..f477a11 100644
> --- a/docs/deployment/installation.rst
> +++ b/docs/deployment/installation.rst
> @@ -254,9 +254,15 @@ This should be a random value and kept secret. You can
> generate and a value for
>
> .. code-block:: python
>
> - import string, random
> + import string
> + try:
> + import secrets
> + except ImportError: # Python < 3.6
> + import random
> + secrets = random.SystemRandom()
We're dropping Python 2 soon, not in the next version coming out Real
Soon Now, but in the version after that. Would it be worth holding this
patch until then so as we can avoid this messy try:import? I have a
topic branch for this and I'd be happy to include this patch in it.
> +
> chars = string.ascii_letters + string.digits + string.punctuation
> - print(repr("".join([random.choice(chars) for i in range(0,50)])))
> + print("".join([secrets.choice(chars) for i in range(50)]))
>
> Once again, store this in ``production.py``.
Regards,
Daniel
>
> diff --git a/patchwork/settings/production.example.py
> b/patchwork/settings/production.example.py
> index c6aa2f2..8058537 100644
> --- a/patchwork/settings/production.example.py
> +++ b/patchwork/settings/production.example.py
> @@ -21,9 +21,15 @@ from .base import * # noqa
> # You'll need to replace this to a random string. The following python code
> can
> # be used to generate a secret key:
> #
> -# import string, random
> -# chars = string.letters + string.digits + string.punctuation
> -# print repr("".join([random.choice(chars) for i in range(0,50)]))
> +# import string
> +# try:
> +# import secrets
> +# except ImportError: # Python < 3.6
> +# import random
> +# secrets = random.SystemRandom()
> +#
> +# chars = string.ascii_letters + string.digits + string.punctuation
> +# print("".join([secrets.choice(chars) for i in range(50)]))
>
> SECRET_KEY = os.environ['DJANGO_SECRET_KEY']
>
> diff --git
> a/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
>
> b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
> new file mode 100644
> index 000..7b101cb
> --- /dev/null
> +++
> b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
> @@ -0,0 +1,5 @@
> +---
> +security:
> + - |
> +Change the recommended method for generating the Django secret key to
> use a
> +cryptographically secure random number generator.
> --
> 2.21.0
>
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
[PATCH] Use secrets and fall back to random.SystemRandom for keys
The random module uses the Mersenne Twister pseudorandom number
generator and is not a cryptographically secure random number
generator[0]. The secrets[1] module is intended for generating
cryptographically strong random numbers, so recommend using that to
generate the secret key. It's new in Python 3, so if it's unavailable
fall back to using the ``os.urandom()`` backed implementation of random.
[0] https://docs.python.org/3/library/random.html
[1] https://docs.python.org/3/library/secrets.html
Signed-off-by: Jeremy Cline
---
docs/deployment/installation.rst | 10 --
patchwork/settings/production.example.py | 12 +---
...andom.SystemRandom-for-keys-9ceb496919a1bb6f.yaml | 5 +
3 files changed, 22 insertions(+), 5 deletions(-)
create mode 100644
releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
diff --git a/docs/deployment/installation.rst b/docs/deployment/installation.rst
index d422573..f477a11 100644
--- a/docs/deployment/installation.rst
+++ b/docs/deployment/installation.rst
@@ -254,9 +254,15 @@ This should be a random value and kept secret. You can
generate and a value for
.. code-block:: python
- import string, random
+ import string
+ try:
+ import secrets
+ except ImportError: # Python < 3.6
+ import random
+ secrets = random.SystemRandom()
+
chars = string.ascii_letters + string.digits + string.punctuation
- print(repr("".join([random.choice(chars) for i in range(0,50)])))
+ print("".join([secrets.choice(chars) for i in range(50)]))
Once again, store this in ``production.py``.
diff --git a/patchwork/settings/production.example.py
b/patchwork/settings/production.example.py
index c6aa2f2..8058537 100644
--- a/patchwork/settings/production.example.py
+++ b/patchwork/settings/production.example.py
@@ -21,9 +21,15 @@ from .base import * # noqa
# You'll need to replace this to a random string. The following python code can
# be used to generate a secret key:
#
-# import string, random
-# chars = string.letters + string.digits + string.punctuation
-# print repr("".join([random.choice(chars) for i in range(0,50)]))
+# import string
+# try:
+# import secrets
+# except ImportError: # Python < 3.6
+# import random
+# secrets = random.SystemRandom()
+#
+# chars = string.ascii_letters + string.digits + string.punctuation
+# print("".join([secrets.choice(chars) for i in range(50)]))
SECRET_KEY = os.environ['DJANGO_SECRET_KEY']
diff --git
a/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
new file mode 100644
index 000..7b101cb
--- /dev/null
+++
b/releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
@@ -0,0 +1,5 @@
+---
+security:
+ - |
+Change the recommended method for generating the Django secret key to use a
+cryptographically secure random number generator.
--
2.21.0
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2 2/3] Include the responsible actor in applicable events
On Mon, Oct 7, 2019 at 5:18 PM Johan Herland 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 > Signed-off-by: Johan Herland 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 ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH] Fix issue with delegation of patch via REST API
On Wed, 9 Oct 2019 at 12:58, Bjorn Helgaas wrote: > > On Thu, Oct 3, 2019 at 4:22 PM Bjorn Helgaas wrote: > > > > I can't get patchwork delegation via git-pw to work either on ozlabs > > or kernel.org. Any hints on where to look or more data to collect? > > This magically started working on patchwork.kernel.org. I don't know > what changed, but thank you to whoever fixed it! The 2.1.5 upgrade was the magical change. :) -K ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH] Fix issue with delegation of patch via REST API
On Thu, Oct 3, 2019 at 4:22 PM Bjorn Helgaas wrote:
>
> I can't get patchwork delegation via git-pw to work either on ozlabs
> or kernel.org. Any hints on where to look or more data to collect?
This magically started working on patchwork.kernel.org. I don't know
what changed, but thank you to whoever fixed it!
> On Wed, Sep 25, 2019 at 01:51:20PM -0500, Bjorn Helgaas wrote:
> > On Wed, Sep 25, 2019 at 04:33:35PM +0100, Stephen Finucane wrote:
> > > On Tue, 2019-09-24 at 14:12 -0500, Bjorn Helgaas wrote:
> > > > On Tue, Sep 24, 2019 at 09:45:16AM +0100, Stephen Finucane wrote:
> > > > > On Mon, 2019-09-23 at 15:39 -0500, Bjorn Helgaas wrote:
> > > > > > On Sat, Sep 21, 2019 at 07:30:46PM +0100, Stephen Finucane wrote:
> > > > > > > There have been reports of people being unable to delegate
> > > > > > > patches to
> > > > > > > themselves, despite being a maintainer or the project to which
> > > > > > > the patch
> > > > > > > is associated.
> > > > > > > ...
> > > > > > I tried the instance at
> > > > > > https://patchwork.kernel.org/project/linux-pci/list/
> > > > > > to see if it was new enough to work without this fix. But it also
> > > > > > fails, slightly differently:
> > > > > >
> > > > > > $ git config -l | grep "^pw"
> > > > > > pw.server=https://patchwork.kernel.org/api/1.1
> > > > > > pw.project=linux-pci
> > > > > > pw.token=...
> > > > > >
> > > > > > $ git-pw patch update --delegate helgaas 11151519
> > > > > > More than one delegate found: helgaas
> > > > > >
> > > > > > Is this another manifestation of the same bug or something else?
> > > > >
> > > > > This is a different issue and, unlike the other one, is more feature
> > > > > than bug. This is happening because the search for a particular user
> > > > > is
> > > > > returning multiple matches. We match on username, first name, last
> > > > > name
> > > > > and email, so I imagine you have multiple user accounts on the
> > > > > instance
> > > > > and there might be a conflict between an email address of one account
> > > > > and a username of another? (Let me know if this isn't the case). The
> > > > > easy solution is to use a more specific match. I'd suggest just using
> > > > > the email address associated with your user account ([1] suggests this
> > > > > is 'bhelg...@google.com'). We could also support lookup by user ID
> > > > > (which would guarantee a single match) but I haven't added that to
> > > > > git-
> > > > > pw yet since it didn't seem that usable.
> > > >
> > > > Still no workey. I'm cursed. The web "Delegate to" dropdown menu
> > > > includes "bhelgaas" and my profile seems to be associated with
> > > > "bhelgaas" (at least, that's the name in the upper right of the web
> > > > page when I'm logged in).
> > > >
> > > > $ git-pw patch update --delegate bhelgaas 11151519
> > > > {u'delegate': [u"User 'bhelgaas' is not a maintainer for project
> > > > 'Linux PCI development list'"]}
> > > > $ git-pw patch update --delegate bhelg...@google.com 11151519
> > > > {u'delegate': [u"User 'bhelgaas' is not a maintainer for project
> > > > 'Linux PCI development list'"]}
> > > > $ git-pw patch update --delegate bj...@helgaas.com 11151519
> > > > No matching delegates found: bj...@helgaas.com
> > > >
> > > > https://patchwork.kernel.org/user/ also claims I'm a maintainer for
> > > > https://patchwork.kernel.org/project/linux-pci/list/ and shows both
> > > > bhelg...@google.com and bj...@helgaas.com as email addresses.
> > >
> > > I'll have a look into this but I've no ideas off the top of my head. If
> > > you're comfortable with Python, could you add a couple of print
> > > statements to log what we're requesting from the API and what we're
> > > getting back and share them here? If not, I'll try look into this next
> > > week.
> >
> > I am able to change the *state*, e.g.,
> >
> > $ git-pw patch update --state accepted 11151519
> > $ git-pw patch update --state new 11151519
> >
> > seem to work fine.
> >
> > Not sure if this has enough information to be useful to you, but this
> > is the output from:
> >
> > git-pw --debug patch update --delegate bhelg...@google.com 11151519
> >
> > 2019-09-25 13:43:04,838 - git_pw.patch - DEBUG - Updating patch:
> > id=11151519, commit_ref=None, state=None, archived=None
> > 2019-09-25 13:43:04,843 - git_pw.config - DEBUG - Retrieved 'server'
> > setting from git-config
> > 2019-09-25 13:43:04,843 - git_pw.config - DEBUG - Retrieved 'server'
> > setting from cache
> > 2019-09-25 13:43:04,847 - git_pw.config - DEBUG - Retrieved 'project'
> > setting from git-config
> > 2019-09-25 13:43:04,847 - git_pw.config - DEBUG - Retrieved 'project'
> > setting from cache
> > 2019-09-25 13:43:04,847 - git_pw.config - DEBUG - Retrieved 'project'
> > setting from cache
> > 2019-09-25 13:43:04,847 - git_pw.config - DEBUG - Retrieved 'project'
> > setting from cache
> > 2019-09-25 13:43:04,847 - git_pw.api - DEBUG - GET
> > https://patchwork.kernel.org/ap
Re: [PATCH v2 3/3] /api/events: Add 'actor' field to generated JSON
On Wed, Oct 9, 2019 at 7:59 AM Daniel Axtens wrote:
> Johan Herland writes:
> > Cc: Mauro Carvalho Chehab
> > Signed-off-by: Johan Herland
> > ---
> > docs/api/schemas/latest/patchwork.yaml | 6 ++
> > docs/api/schemas/patchwork.j2 | 8
> > docs/api/schemas/v1.2/patchwork.yaml | 6 ++
>
> I think you might also need to edit docs/usage/overview.rst, sorry.
Ah yes, an obvious oversight on my part. Will fix in v3.
Before I post v3, I'll wait for some more responses to the discussion
on patch 2/3 about the best way (or rather: least horrible way) to
determine the active user and forward it into the event creation.
> Apart from that, lgtm:
> Acked-by: Daniel Axtens
Thanks for reviewing!
...Johan
> Regards,
> Daniel
>
> > patchwork/api/event.py | 10 +++---
> > patchwork/tests/api/test_event.py | 24
> > 5 files changed, 51 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/api/schemas/latest/patchwork.yaml
> > b/docs/api/schemas/latest/patchwork.yaml
> > index 45a6118..58d5d44 100644
> > --- a/docs/api/schemas/latest/patchwork.yaml
> > +++ b/docs/api/schemas/latest/patchwork.yaml
> > @@ -1495,6 +1495,12 @@ components:
> >type: string
> >format: iso8601
> >readOnly: true
> > +actor:
> > + type: object
> > + title: The user that caused/created this event
> > + readOnly: true
> > + allOf:
> > +- $ref: '#/components/schemas/UserEmbedded'
> > payload:
> >type: object
> > EventCoverCreated:
> > diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
> > index 16d85a3..1f1a7bd 100644
> > --- a/docs/api/schemas/patchwork.j2
> > +++ b/docs/api/schemas/patchwork.j2
> > @@ -1510,6 +1510,14 @@ components:
> >type: string
> >format: iso8601
> >readOnly: true
> > +{% if version >= (1, 2) %}
> > +actor:
> > + type: object
> > + title: The user that caused/created this event
> > + readOnly: true
> > + allOf:
> > +- $ref: '#/components/schemas/UserEmbedded'
> > +{% endif %}
> > payload:
> >type: object
> > EventCoverCreated:
> > diff --git a/docs/api/schemas/v1.2/patchwork.yaml
> > b/docs/api/schemas/v1.2/patchwork.yaml
> > index 3a96aa3..2aaf393 100644
> > --- a/docs/api/schemas/v1.2/patchwork.yaml
> > +++ b/docs/api/schemas/v1.2/patchwork.yaml
> > @@ -1495,6 +1495,12 @@ components:
> >type: string
> >format: iso8601
> >readOnly: true
> > +actor:
> > + type: object
> > + title: The user that caused/created this event
> > + readOnly: true
> > + allOf:
> > +- $ref: '#/components/schemas/UserEmbedded'
> > payload:
> >type: object
> > EventCoverCreated:
> > diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> > index c0d973d..33ea104 100644
> > --- a/patchwork/api/event.py
> > +++ b/patchwork/api/event.py
> > @@ -23,6 +23,7 @@ from patchwork.models import Event
> > class EventSerializer(ModelSerializer):
> >
> > project = ProjectSerializer(read_only=True)
> > +actor = UserSerializer()
> > patch = PatchSerializer(read_only=True)
> > series = SeriesSerializer(read_only=True)
> > cover = CoverLetterSerializer(read_only=True)
> > @@ -50,7 +51,7 @@ class EventSerializer(ModelSerializer):
> > data = super(EventSerializer, self).to_representation(instance)
> > payload = OrderedDict()
> > kept_fields = self._category_map[instance.category] + [
> > -'id', 'category', 'project', 'date']
> > +'id', 'category', 'project', 'date', 'actor']
> >
> > for field in [x for x in data]:
> > if field not in kept_fields:
> > @@ -65,10 +66,13 @@ class EventSerializer(ModelSerializer):
> >
> > class Meta:
> > model = Event
> > -fields = ('id', 'category', 'project', 'date', 'patch', 'series',
> > - 'cover', 'previous_state', 'current_state',
> > +fields = ('id', 'category', 'project', 'date', 'actor', 'patch',
> > + 'series', 'cover', 'previous_state', 'current_state',
> >'previous_delegate', 'current_delegate', 'created_check')
> > read_only_fields = fields
> > +versioned_fields = {
> > +'1.2': ('actor', ),
> > +}
> >
> >
> > class EventList(ListAPIView):
> > diff --git a/patchwork/tests/api/test_event.py
> > b/patchwork/tests/api/test_event.py
> > index 8816538..5e47ff3 100644
> > --- a/patchwork/tests/api/test_event.py
> > +++ b/patchwork/tests/api/test_event.py
> > @@ -35,11 +35,16 @@ class TestEventAPI(utils.APITestCase):
> > def assertSerialized(self, event_obj, event_json):
> > self.assertEqual(event_obj.id, event_json['id'])
> > self.
Re: [PATCH v2 2/3] Include the responsible actor in applicable events
On Wed, Oct 9, 2019 at 8:04 AM Daniel Axtens wrote: > Johan Herland 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 ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
