moin 1.9.x recaptcha patch

adds recaptchas to moin 1.9.x to keep spammers away

[Thomas.J.Waldmann, 2014-03-01 17:14:29]

hi steve,

i think this could go into upstream if some small issues get fixed and the stuff
gets tested afterwards again.

cheers,

thomas

https://codereview.appspot.com/70400043/diff/1/MoinMoin/action/newaccount.py
File MoinMoin/action/newaccount.py (right):

https://codereview.appspot.com/70400043/diff/1/MoinMoin/action/newaccount.py#...
MoinMoin/action/newaccount.py:156: if recaptcha:
recaptcha is an object, so what do you intend to do with this "if"?

https://codereview.appspot.com/70400043/diff/1/MoinMoin/security/sec_recaptch...
File MoinMoin/security/sec_recaptcha.py (right):

https://codereview.appspot.com/70400043/diff/1/MoinMoin/security/sec_recaptch...
MoinMoin/security/sec_recaptcha.py:7: @copyright: 2011 by Steve McIntyre
maybe update the year if you do changes

maybe give email or wiki page reference for contacting you

like MoinMoin:SteveMcIntyre (or whatever your wiki homepage is)

https://codereview.appspot.com/70400043/diff/1/MoinMoin/security/sec_recaptch...
MoinMoin/security/sec_recaptcha.py:13: import sys
it is convention that stdlib imports come first.
also, it should not crash if recaptcha support lib isn't there (like when not
using the debian package).

like:

import sys

try:
    from recaptcha.client import captcha
except ImportError:
    captcha = None

from MoinMoin import log
logging = log.getLogger(__name__)

(then other moin imports)

later, before you use "captcha" you have to check "if captcha is not None:".

https://codereview.appspot.com/70400043/diff/1/MoinMoin/security/sec_recaptch...
MoinMoin/security/sec_recaptcha.py:38: self.private_key = None
this exception handling is rather dirty. :)

never use "except:", you should catch a more specific exception, like
AttributeError.

also, if this goes into moin upstream, you could just add the None defaults to
MoinMoin.config.multiconfig module, so you would always have the attribute and
could just use it and not need this exception handling at all.

then, it is either None (builtin value from multiconfig) or the value configured
by the admin.

https://codereview.appspot.com/70400043/diff/1/MoinMoin/security/sec_recaptch...
MoinMoin/security/sec_recaptcha.py:43: if (self.public_key and
self.private_key):
no parens needed here

https://codereview.appspot.com/70400043/diff/1/MoinMoin/security/sec_recaptch...
MoinMoin/security/sec_recaptcha.py:45: return False
"if expression: something with True; (else) something with False" is a known
antipattern.

if you compute a bolean already within the if-expression, you can just directly
use that.

In this case:

return bool(self.public_key and self.private_key)

The bool() is recommended here to force it to True/False.

https://codereview.appspot.com/70400043/diff/1/MoinMoin/security/sec_recaptch...
MoinMoin/security/sec_recaptcha.py:70: result =
captcha.displayhtml(self.public_key, use_ssl = True)
use_ssl=True   <- no blanks around "=" in this case

(PEP8 code formatting convention is widely accepted)

we even have tests for pep8, you can run them from the toplevel dir of the repo
checkout by:

./pytest

Not sure if it detects all pep8 fails, though.

[Thomas.J.Waldmann, 2014-05-04 12:52:59]

Just stumbled over this CR again when catching up on the wiki, did you read my
feedback?

[tfoote, 2016-01-14 01:36:20]

We were having some bad spam problems on our wiki (http://wiki.ros.org) and were
interested in ReCaptcha support. So we took your patch, expanded it a bit, and
things seem to be working well now.  Our complete diff is posted at:
https://gist.github.com/tfoote/675b98df53369e199dea