Python bindings HOWTO proof reader request

[Justus Winter]

Hi Ben,

thanks for working on this.

Ben McGinnes <ben at adversary.org> writes:
> https://files.gnupg.net/file/data/ossmg4ung2hcpyyuks6j/PHID-FILE-xgbofmytge7fzn3u5kuc/GPGMEpythonHOWTOen.org
>
> [...]
>
> I'm more interested in being sure that the example code works (it
> should, I was running it as I was writing the thing) and that the
> corresponding text descriptions actually help to clarify what's going
> on in that code.

I find many examples not very elegant, as in they are not very pythonic
and they promote the use of low-level interfaces (op_*).

For example:

>   #+begin_src python
>     import gpg
>     import os
>
>     rkey = "0x12345678DEADBEEF"
>     text = """
>     Some plain text to test with.  Obtained from any input source Python can read.
>
>     It makes no difference whether it is string or bytes, but the bindings always
>     produce byte output data.  Which is useful to know when writing out either the
>     encrypted or decrypted results.
>
>     """
>
>     plain = gpg.core.Data(text)
>     cipher = gpg.core.Data()

In almost all cases, explicit wrapping with core.Data is no longer
necessary and provides no benefit.

>     c = gpg.core.Context()
>     c.set_armor(1)

c = gpg.core.Context(armor=True)

>     c.op_keylist_start(rkey, 0)
>     r = c.op_keylist_next()
>
>     if r == None:
>	 print("""The key for user "{0}" was not found""".format(rkey))

r = list(c.keylist(rkey))
if not r:
    print("The key for user {0!r} was not found".format(rkey))

>     else:
>	 try:

I see no value in wrapping this in a try-block for demonstration
purposes.

>	     c.op_encrypt([r], 1, plain, cipher)
>	     cipher.seek(0, os.SEEK_SET)
>	     del(text)
>	     del(plain)

In general, there is no need to del(..) anything.

>	     afile = open("secret_plans.txt.asc", "wb")
>	     afile.write(cipher.read())
>	     afile.close()
>	 except gpg.errors.GPGMEError as ex:
>	     print(ex.getstring())

c. encrypt(text.encode(), r, sink=open("secret_plans.txt.asc", "w"))

>   #+end_src

Or:

>    #+begin_src python
>      import gpg
>
>      text = b"""Oh look, another test message.
>
>      The same rules apply as with the previous example and more likely
>      than not, the message will actually be drawn from reading the
>      contents of a file or, maybe, from entering data at an input()
>      prompt.
>
>      Since the text in this case must be bytes, it is most likely that
>      the input form will be a separate file which is opened with "rb"
>      as this is the simplest method of obtaining the correct data
>      format.
>      """
>
>      c = gpg.Context(armor=True)
>      rpattern = list(c.keylist(pattern="@gnupg.org", secret=False))
>      logrus = []
>
>      for i in range(len(rpattern)):
>	  if rpattern[i].can_encrypt == 1:
>	      logrus.append(rpattern[i])

r = [r
     for r in c.keylist(pattern="@gnupg.org", secret=False)
     if r.can_encrypt]

Also, I don't get 'logrus'.

>      cipher = c.encrypt(text, recipients=logrus, sign=False, always_trust=True)
>
>      afile = open("secret_plans.txt.asc", "wb")
>      afile.write(cipher[0])
>      afile.close()

Again, sink= is better.  But if you do want to get results from
e.g. .encrypt(..), please use destructuring assignment to get the
returned values, e.g.:

cipher, _, _ = c.encrypt(...)

>    #+end_src

>   #+begin_src python
>     import os.path
>     import gpg
>
>     if os.path.exists("/path/to/secret_plans.txt.asc") is True:
>	 ciphertext = "/path/to/secret_plans.txt.asc"

1/ I don't see why example code wouldn't just assume one variant, or
maybe even better, take the path as command line argument.

2/ some_predicate is True =^= some_predicate

>     elif os.path.exists("/path/to/secret_plans.txt.gpg") is True:
>	 ciphertext = "/path/to/secret_plans.txt.gpg"
>     else:
>	 ciphertext = None
>
>     if ciphertext is not None:

Just fail if the input does not exist.

>	 afile = open(ciphertext, "rb")
>	 plaintext = gpg.Context().decrypt(afile)
>	 afile.close()
>	 newfile = open("/path/to/secret_plans.txt", "wb")
>	 newfile.write(plaintext[0])
>	 newfile.close()
>	 print(plaintext[0])
>	 plaintext[1]
>	 plaintext[2]

Statements without effect.

>	 del(plaintext)

Don't del stuff.

>     else:
>	 pass

if pred:
    pass
else:
    pass

=^=

if pred:
    pass

>   #+end_src

Also, I feel like this whole example should be a oneliner:

gpg.core.Context().decrypt(sys.stdin, sink=sys.stdout)

>     signed = c.sign(text, mode=0)

Destructuring assignment,  please don't use magic values, use symbolic
modes instead.

>      text = text0.encode("utf-8")

UTF-8 is the default for .encode() and .decode().


Cheers,
Justus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 487 bytes
Desc: not available
URL: <https://lists.gnupg.org/pipermail/gnupg-devel/attachments/20180315/70d41e66/attachment.sig>