Opened 9 years ago

Closed 7 years ago

#1836 closed task (fixed)

bridgedb captcha needs review

Reported by: aagbsn Owned by: nickm
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords:
Cc: aagbsn@… Actual Points:
Parent ID: #5481 Points:
Reviewer: Sponsor:

Description

Please review bridgedb recaptcha support:

http://github.com/aagbsn/bridgedb/tree/recaptcha

Child Tickets

Attachments (1)

recaptcha.patch (12.2 KB) - added by aagbsn 8 years ago.
patch of diff between master and branch recaptcha

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by aagbsn

Cc: aagbsn@… added
Status: newneeds_review

comment:2 Changed 9 years ago by aagbsn

Owner: set to nickm
Status: needs_reviewassigned

comment:3 Changed 9 years ago by aagbsn

Status: assignedneeds_review

comment:4 Changed 9 years ago by nickm

It looks like this will now probably conflict with the i18n code?

Also, Main.py was tracked already, in lib/bridgedb/Main.py. Why add a new copy in ./ ?

Did you really mean to add values for RECAPTCHA_PUB_KEY and RECAPTCHA_PRIV_KEY in the source code?

I don't see a configuration option to turn recaptcha support on and off; there should probably be one. Also, the RECAPTCHA_PUB/PRIV_KEY values probably belong there too, not in the source code: editing source files in order to configure a program is very 1990. ;)

comment:5 in reply to:  4 Changed 9 years ago by aagbsn

Replying to nickm:

It looks like this will now probably conflict with the i18n code?

Also, Main.py was tracked already, in lib/bridgedb/Main.py. Why add a new copy in ./ ?

Oops. Fixed.

Did you really mean to add values for RECAPTCHA_PUB_KEY and RECAPTCHA_PRIV_KEY in the source code?

This was a result of a poor design. The values now live in bridgedb.conf, and the default testing configuration does not enable recaptcha support.

I don't see a configuration option to turn recaptcha support on and off; there should probably be one. Also, the RECAPTCHA_PUB/PRIV_KEY values probably belong there too, not in the source code: editing source files in order to configure a program is very 1990. ;)

Very true. I added configuration options RECAPTCHA_ENABLED, RECAPTCHA_PRIV_KEY, RECAPTCHA_PUB_KEY to bridgedb.conf

comment:6 Changed 8 years ago by karsten

Code looks good. Two comments:

  • When rebasing your branch to origin/master, I'm running into conflicts. I'd rather not break anything there. Can you rebase to origin/master and make a new branch with only the recaptcha changes? Feel free to squash commits if that facilitates rebasing.
  • Can you tell me what libraries I need and what files I should modify to run the recaptcha code? I'm running Debian Lenny.

Thanks!

comment:7 in reply to:  6 Changed 8 years ago by aagbsn

Replying to karsten:

Code looks good. Two comments:

  • When rebasing your branch to origin/master, I'm running into conflicts. I'd rather not break anything there. Can you rebase to origin/master and make a new branch with only the recaptcha changes? Feel free to squash commits if that facilitates rebasing.

ah, squashing commits breaks stuff for people (nick) tracking the repo. I did rebase prior to submitting this for review but git is replaying the entire branch on top of master each time I rebase. What is the best course of action here? I have also attached a patch.

  • Can you tell me what libraries I need and what files I should modify to run the recaptcha code? I'm running Debian Lenny.

you'll need recaptcha-client (pip install recaptcha-client) or:
http://pypi.python.org/pypi/recaptcha-client

and BeautifulSoup (pip install BeautifulSoup) or:
http://pypi.python.org/pypi/BeautifulSoup/3.0.4

and a recaptcha.net api key (it's IP-specific)

The following bridgedb.conf options are required:
RECAPTCHA_ENABLED = True
RECAPTCHA_PUB_KEY = 'your_public_key_here'
RECAPTCHA_PRIV_KEY = 'your_private_key_here'

Thanks!

Thanks for your feedback!

Changed 8 years ago by aagbsn

Attachment: recaptcha.patch added

patch of diff between master and branch recaptcha

comment:8 Changed 8 years ago by karsten

So, I found a BRIDGEDB_TEXT[14] that should be a BRIDGEDB_TEXT[15] and otherwise didn't get it running. I think I did all the steps you said in your last comment, and it doesn't give me a warning. But when asking for bridges via HTTPS, BridgeDB still returns three bridges to me without showing me a captcha. I also didn't see an error or something. Any idea what I could be missing?

comment:9 Changed 8 years ago by kaner

I reviewed 55e25a97f on https://github.com/aagbsn/bridgedb/tree/fix_recaptcha today and I'd say it overall looks ok. There seem to be a few minor format and other things. Could you run pylint (or similar) over it? - I got errors running 'python setup.py install'. Thanks.

comment:10 in reply to:  9 Changed 8 years ago by aagbsn

Replying to kaner:

I reviewed 55e25a97f on https://github.com/aagbsn/bridgedb/tree/fix_recaptcha today and I'd say it overall looks ok. There seem to be a few minor format and other things. Could you run pylint (or similar) over it? - I got errors running 'python setup.py install'. Thanks.

Done.

comment:11 Changed 8 years ago by karsten

Milestone: BridgeDB Upgrades Phase 1
Parent ID: #4380

Assigning as a child ticket to the project ticket that replaces the "BridgeDB Upgrades Phase 1" milestone.

comment:12 Changed 7 years ago by karsten

Isn't this code already reviewed and merged? If so, please close the ticket.

comment:13 Changed 7 years ago by karsten

Parent ID: #4380#5481

Making this a child ticket of #5481 where it's already listed as required substep.

comment:14 Changed 7 years ago by aagbsn

Resolution: fixed
Status: needs_reviewclosed

yes, this is merged.

Note: See TracTickets for help on using tickets.