Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not require a captcha when using the API #931

Merged
merged 3 commits into from Nov 24, 2021
Merged

Conversation

almet
Copy link
Member

@almet almet commented Nov 22, 2021

This was trickier than expected, due to some side effects : when the
captcha is set to True via configuration, it doesn't change the
behavior directly of the ProjectForm class, but does so only when the
project form is used in the web.py module.

So, when just using the API (and not using the web.py module, for
instance during tests — manual or functional), no problem was shown,
and everything was working properly.

But at soon as somebody sees the "/" endpoint, the captcha was
required, by both the API and the web.py module.

This fixes it by adding a way to bypass the captcha with a new
bypass_captcha property on the form.

This was trickier than expected, due to some side effects : when the
captcha is set to `True` via configuration, it doesn't change the
behavior directly of the ProjectForm class, but does so only when the
project form is used in the `web.py` module.

So, when just using the API (and not using the web.py module, for
instance during tests — manual or functional), no problem was shown,
and everything was working properly.

But at soon as somebody sees the "/" endpoint, the captcha was
required, by both the API and the `web.py` module.

This fixes it by adding a way to bypass the captcha with a new
`bypass_captcha` property on the form.
@zorun
Copy link
Collaborator

zorun commented Nov 22, 2021

Wow, this is a good heisenbug! Maybe we should get rid of that enable_captcha class method to avoid this kind of very subtle issues in the future? Isn't there another way to do this, maybe with form inheritance?

@almet
Copy link
Member Author

almet commented Nov 22, 2021

Hmm, that's correct. I believe we should merge this and change the way we handle this captcha thing in another pull request. I opened a new bug for this.

@almet
Copy link
Member Author

almet commented Nov 23, 2021

Actually, I changed the way things were implemented for the Captcha. It's a lot more clear this way, thanks @zorun for the review 👍

@zorun
Copy link
Collaborator

zorun commented Nov 23, 2021

Great, thanks for the change, it looks good :) I think you just need to reformat

Prior to this commit, things were done by activating or deactivating a
"captcha" property on the class on-the-fly, which caused side-effects.

This is now using subclasses, which makes the code simpler to
understand, and less prone to side-effects.

Thanks @zorun for the idea.
@almet almet merged commit 1698841 into master Nov 24, 2021
@almet almet deleted the almet/fix-api-captcha branch November 24, 2021 23:44
TomRoussel pushed a commit to TomRoussel/ihatemoney that referenced this pull request Mar 2, 2024
* Do not require a captcha when using the API

This was trickier than expected, due to some side effects : when the
captcha is set to `True` via configuration, it doesn't change the
behavior directly of the ProjectForm class, but does so only when the
project form is used in the `web.py` module.

So, when just using the API (and not using the web.py module, for
instance during tests — manual or functional), no problem was shown,
and everything was working properly.

But at soon as somebody sees the "/" endpoint, the captcha was
required, by both the API and the `web.py` module.

This fixes it by adding a way to bypass the captcha with a new
`bypass_captcha` property on the form.

Prior to this commit, things were done by activating or deactivating a
"captcha" property on the class on-the-fly, which caused side-effects.

This is now using subclasses, which makes the code simpler to
understand, and less prone to side-effects.

Thanks @zorun for the idea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants