On 2013-09-14 16:35:30, Philip Jägenstedt wrote: > This is useful so that the signer can see exactly what was sent > and makes it possible to resend/import/upload the signatures > later without signing again (risking duplicate signatures).
Thanks for the patch! Here's a quick review...
> diff --git a/monkeysign/gpg.py b/monkeysign/gpg.py
> index 88769b0..351e2cd 100644
> --- a/monkeysign/gpg.py
> +++ b/monkeysign/gpg.py
> @@ -377,12 +377,15 @@ class Keyring():
> raise GpgProtocolError(self.context.returncode,
> _('unexpected GPG exit code in list-keys: %d') % self.context.returncode)
> return keys
>
> - def encrypt_data(self, data, recipient):
> + def encrypt_data(self, data, recipients):
> """encrypt data using asymetric encryption
>
> - returns the encrypted data or raise a GpgRuntimeError if it fails
> + returns the encrypted data or raises a GpgRuntimeError if it failed.
> """
> - self.context.call_command(['recipient', recipient, '--encrypt'],
> data)
> + command = ['encrypt']
> + for recipient in recipients:
> + command += ['--recipient', recipient]
> + self.context.call_command(command, data)
> if self.context.returncode == 0:
> return self.context.stdout
> else:
This makes sense, conceptually - we should be able to encryp to multiple
recipients.
However, the patch looks a little strange to me - there is a typo fix
that is unrelated, and we change the order of arguments.
I wonder if a list comprehension wouldn't be more readable than a loop
here:
command = ['--encrypt] + [ ['--recipient', recipient ] for recipient in
recipients ]
> @@ -391,9 +394,9 @@ class Keyring():
> def decrypt_data(self, data):
> """decrypt data using asymetric encryption
>
> - returns the plaintext data or raise a GpgRuntimeError if it failed.
> + returns the plaintext data or raises a GpgRuntimeError if it failed.
> """
> - self.context.call_command(['--decrypt'], data)
> + self.context.call_command(['decrypt'], data)
> if self.context.returncode == 0:
> return self.context.stdout
> else:
Again, this seems like an unrelated change...
> diff --git a/monkeysign/ui.py b/monkeysign/ui.py
> index 19fdef8..9166c77 100644
> --- a/monkeysign/ui.py
> +++ b/monkeysign/ui.py
[...]
> @@ -394,21 +394,24 @@ yourself. With GnuPG this can be done using:
> Regards,
> """)
>
> - def __init__(self, keydata, keyfpr, recipient, mailfrom, mailto):
> + def __init__(self, signingkeydata, signingkeyfpr, keydata, keyfpr,
> recipient, mailfrom, mailto):
> """email constructor
so yeah, I agree with your later comment that this is getting a little
ugly. :)
it was a little weird in the first place to pass the fpr *and* the
keydata - we shouldn't need to pass duplicate data like this. I think
the reason why I did this is that i wanted testing of the class to be
easier, but maybe we can pass a gpg context that contains the relevant
data instead?
i am thinking we could have this constructor instead:
(context, signedkeyfpr, mailfrom, mailto, extrarecipient = None)
the signedkey data is exported from context based on signedkeyfpr.
if extrarecipient is specified, it is assumed it can be exported from context.
however, I wonder if we really need to go through all this
trouble. first of all, this is a change in the gpg.py API, and while I
don't think there are any consumers to the interface, this is something
that I am hesitant in changing, especially since we don't add unit tests
for that new functionality in the above patch.
second, it seems to me we could simply show / export the signed key to
the user if desired before encrypting it. that *is* basically what we
need, don't we?
A.
--
L'ennui avec la grande famille humaine, c'est que tout le monde veut
en être le père.
- Mafalda
pgpoGtrbWoSvP.pgp
Description: PGP signature
