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

replace pyramid.require_default_csrf setting with config.set_default_csrf_options #2518

Merged
merged 1 commit into from Apr 20, 2016

Conversation

Projects
None yet
2 participants
@mmerickel
Member

mmerickel commented Apr 18, 2016

I realized that the csrf options should not really change per-environment and were more similar to default permissions. I'm removing the INI setting and adding config.set_default_csrf_options instead. Bonus aspect is that it will allow changing the default header and safe methods if desired.

I don't currently plan to remove the pyramid.csrf_trusted_origins setting but it's also an option. I do expect the origins to be different per-environment (dev vs prod) but people could handle it themselves by passing it to set_default_csrf_options.

Thoughts?

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 18, 2016

Leaving pyramid.csrf_trusted_origins makes sense, this way configuration can still be defined with the rest of the configuration.

@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 18, 2016

I'm going back and forth on how deep the integration should go. I'd like to have check_csrf_token use these options automatically for every call to it similar to how check_csrf_origin works right now (always checks the setting unless overridden). That way the new defaults are always used unless you explicitly specify them. That's a minor backward-incompatibility but does require opt-in (by calling config.set_default_csrf_options from the user to trigger so it feels like a win to me.

If I went that route then I would set the signature like config.set_default_csrf_options(autocheck=True, token='csrf_token', header='X-CSRF-Token', autocheck_ignore_safe_request_methods=['GET', 'HEAD', 'TRACE', 'OPTIONS']). A user could then set autocheck=False but still influence the token/header being set.

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 18, 2016

I like the idea that I as a user can influence the header/token name from a setting. Other than that, I really have no real preference either way.

@mmerickel mmerickel changed the title from [wip] replace pyramid.require_default_csrf setting with config.set_default_csrf_options to replace pyramid.require_default_csrf setting with config.set_default_csrf_options Apr 19, 2016

@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 19, 2016

Alright I've overhauled the automatic csrf api to be simpler.

  • I removed the pyramid.require_default_csrf setting.
  • I removed the ability to specify a token name to add_view(require_csrf=...).
  • add_view(require_csrf=...) may now be True, False, or None.
  • If the value is None then it falls through to the setting used in config.set_default_csrf_options(require_csrf=..).
  • The token name, header and safe methods can only be overridden globally via the new set_default_csrf_options.

Please review!

@bertjwregeer bertjwregeer added this to the 1.7 milestone Apr 19, 2016

if resolved_val is True:
resolved_val = 'csrf_token'
explicit_val = info.options.get('require_csrf')
defaults = info.registry.queryUtility(IDefaultCSRFOptions)

This comment has been minimized.

@bertjwregeer

bertjwregeer Apr 19, 2016

Member

Wouldn't it be simpler to always register a default utility, that can be overridden, rather than having to redefine the defaults here?

If we ever decide to change the safe_methods or the header, we need to change it in two places.

This comment has been minimized.

@mmerickel

mmerickel Apr 19, 2016

Member

Pyramid tends to be fairly lazy about this stuff. I think no matter what you will have 2 spots unless I move things out of the signature of set_default_csrf_options.

This comment has been minimized.

@bertjwregeer

bertjwregeer Apr 19, 2016

Member

No worries :-)

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 19, 2016

Besides a minor nitpick about duplication, this looks great. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment