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

Prepare v4 release #312

Merged
merged 11 commits into from
Nov 14, 2023
Merged

Conversation

Stormheg
Copy link
Contributor

Reasoning for new major release: we dropped support for unsupported Django and Python versions, which is a breaking change according to SemVer.

I'll fill in the release date once we actually release this.

@Stormheg
Copy link
Contributor Author

#311 should be merged before we release v4.

@alexrobertson-muckrack
Copy link
Contributor

If you're looking at a V4 release, you might drop the default for the V3 action.

@Stormheg
Copy link
Contributor Author

Hi @Andrew-Chen-Wang, any chance you are able to review this?

@Stormheg
Copy link
Contributor Author

Thanks for reviewing @Andrew-Chen-Wang! I'll continue preparing the release when I have time.

Regarding your comment @alexrobertson-muckrack

If you're looking at a V4 release, you might drop the default for the V3 action.

I'm not sure what the benefit of this breaking change would be. action=form has been the default for over 5 years.
I'm all for keeping the transition to v4 as smooth as possible. Is there a benefit I'm not aware of?

@alexrobertson-muckrack
Copy link
Contributor

@Stormheg it would require users of V3 ReCAPTCHA to provide individual action values for their forms rather than assume they should all be grouped under the form action. Its not necessary but might promote better use of the V3 API.

@Stormheg
Copy link
Contributor Author

Got it, any thoughts on this @Andrew-Chen-Wang?

@Andrew-Chen-Wang
Copy link
Contributor

It just needs a guide inside of our docs (eg "Migrating to v4" section). Otherwise, many are using the defaults and providing a small hindrance to promote security is beneficial

@koutoftimer
Copy link
Contributor

What left to be done for release to happen?

@Stormheg
Copy link
Contributor Author

Stormheg commented Nov 2, 2023

@koutoftimer don't worry, I'll get to this soon.

@koutoftimer
Copy link
Contributor

I'm not sure, but it looks like closing issue about naming conflict will be beneficial #291

@Stormheg
Copy link
Contributor Author

Stormheg commented Nov 4, 2023

Marking as draft pending the renaming of the captcha namespace to django_recaptcha. See #317.

@Stormheg Stormheg marked this pull request as draft November 4, 2023 14:35
@enzedonline
Copy link

It would be useful to be able to add the action as an attribute (with 'form' being the default value) and required score also (settings.RECAPTCHA_REQUIRED_SCORE being the default). It would allow distinguishing between usages (e.g. 'contact', 'signup' etc) and use specific scores in those places.
e.g. ReCaptchaField(label="", widget=ReCaptchaV3(), action="signup", required_score=0.85)

@Stormheg
Copy link
Contributor Author

It would be useful to be able to add the action as an attribute (with 'form' being the default value) and required score also (settings.RECAPTCHA_REQUIRED_SCORE being the default). It would allow distinguishing between usages (e.g. 'contact', 'signup' etc) and use specific scores in those places. e.g. ReCaptchaField(label="", widget=ReCaptchaV3(), action="signup", required_score=0.85)

Hey @enzedonline, would you mind creating an issue out of this? From what I understand you want to be able to a specific required score for a specific type of form. This is an interesting idea and I'd love to discuss it further in a separate issue.

@Stormheg
Copy link
Contributor Author

I've rebased and updated the PR with some more changes. Most controversial is dropping the action='form' default entirely. Allow me to motivate why:

  • Preserving the previous behaviour of always specifying action=form isn't that useful. The power in the actions feature is specifying a different action for each form to allow analysing abuse with the added context of which form is being abused. Thus always specifying form as a default would have the same result as passing no action at all. You can't distinguish between forms.
  • We could require always passing an action but what would be the point of that? It does not yield sizeable security benefits as this is used for analytics only. I think this increases friction, let's keep it simple.
  • And finally; Actions are an optional feature in the reCAPTCHA v3 API. Wouldn't it be weird to make this required on our side?

I think the best way forward is to make actions optional, document it and encourage people to use it. I've added documentation that suggests this, feedback on wording welcome and appreciated!


Some more changes include:

  • Converted all reStructuredText to Markdown. This is something I was planning to do anyway. When I wrote the upgrade considerations I got fed up enough with reStructuredText to finally do this.
  • Wrote upgrade considerations. I expect people to look in the changelog of the package they are upgrading to discover what has changed and what they need to do to upgrade. I believe the required changes and changes to be clear. Again, feedback welcome!
  • Updated the author information in setup.py. This was still listing Praekelt Consulting.

@Stormheg Stormheg marked this pull request as ready for review November 11, 2023 13:11
@Andrew-Chen-Wang
Copy link
Contributor

And finally; Actions are an optional feature in the reCAPTCHA v3 API. Wouldn't it be weird to make this required on our side?

It is for backwards compatibility as reCaptchav2 is still widely used I believe (yes, even with multi modal LLMs, they're expensive, and any form of captchas/locks are preventable to a certain degree); and imo, v2 is still very good.

But this is assuming the backwards compat is actually true. Please let me know.

Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be busy next few days. I believe everything LGTM (besides the point above which requires further discussion if needed), and if not, I will be available to approve any new review requests immediately since there are a lot of breaking changes and plenty of patches may come ahead.

@Stormheg
Copy link
Contributor Author

Thanks for reviewing @Andrew-Chen-Wang 👍

As to to your comment: the reCAPTCHA v2 API does not support actions. It is exclusive to reCAPTCHA v3 (source, reCAPTCHA v3 introduces a new concept: actions).

Dropping the default action does not introduce any backwards incompatible changes to the RecaptchaV2 widgets.

If there are no further comments or concerns I'll get this merged and a release out 🙌

@Stormheg Stormheg merged commit 57de001 into django-recaptcha:main Nov 14, 2023
7 checks passed
@Stormheg Stormheg deleted the chore/prepare-v4-release branch November 14, 2023 17:18
@Stormheg
Copy link
Contributor Author

For all involved with this PR: I tried to create a release which should trigger the publish.yml workflow but this did not happen for some reason. I noticed the workflow is referencing a PyPI token which does not exist, so even if the workflow did trigger it wouldn't be able to upload to PyPI. First time we use this workflow, the previous release was uploaded manually by me but in the interest of security I no longer have the necessary access to do so.

I'm getting in touch with the relevant folks at Torchbox to sort out the PyPI situation as only they have access.

@enzedonline
Copy link

It would be useful to be able to add the action as an attribute (with 'form' being the default value) and required score also (settings.RECAPTCHA_REQUIRED_SCORE being the default). It would allow distinguishing between usages (e.g. 'contact', 'signup' etc) and use specific scores in those places. e.g. ReCaptchaField(label="", widget=ReCaptchaV3(), action="signup", required_score=0.85)

Hey @enzedonline, would you mind creating an issue out of this? From what I understand you want to be able to a specific required score for a specific type of form. This is an interesting idea and I'd love to discuss it further in a separate issue.

@Stormheg Sorry, just saw this - added to the ideas list #320

@Stormheg
Copy link
Contributor Author

I have been entrusted with PyPI access and have set up Trusted Publishing (791f833)

"Trusted publishing" is our term for using the OpenID Connect (OIDC) standard to exchange short-lived identity tokens between a trusted third-party service and PyPI. This method can be used in automated environments and eliminates the need to use username/password combinations or manually generated API tokens to authenticate with PyPI when publishing.

As a result, v4.0.0 is now available on PyPI!

I'd like to thank all contributors and @Andrew-Chen-Wang for reviewing my PRs 🚀

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

5 participants