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