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

adjust how session protection attribute and config interact #699

Open
davidism opened this issue Jul 25, 2022 · 2 comments
Open

adjust how session protection attribute and config interact #699

davidism opened this issue Jul 25, 2022 · 2 comments

Comments

@davidism
Copy link
Collaborator

davidism commented Jul 25, 2022

Currently, login_manager.session_protection is only considered if app.config does not have the SESSION_PROTECTION key. Session protection is the only config that works this way, and this is not a standard pattern for Flask config. A more common pattern would be to use the attribute if the config is set to some default value, but that doesn't work in this case because None has meaning (probably should be False instead).

As part of #696, I also wanted to set all available config to default values in init_app rather than using config.get with defaults spread out across the code. However, this doesn't work for session protection.

At minimum, I think init_app should do app.config.setdefault("SESSION_PROTECTION", self.session_protection). Requiring the extension to be configured before registering it on the app is a standard pattern in Flask, as you can't guarantee in general that config changed afterwards will take effect anyway.

An extra step would be to deprecate then remove the attribute and only use the config. I think this makes sense because session protection is very dependent on how the app is deployed, which is what app.config is for.

@davidism
Copy link
Collaborator Author

Somewhat interesting, the tests already never set the attribute, they always set the config.

@davidism
Copy link
Collaborator Author

davidism commented Nov 2, 2023

Considering it again, I think it still makes sense to configure this at the extension level. Then when init_app is called, set the value in app.config with the attribute as a default, and from then on read app.config only. In other words, do what I described first, not the second part about deprecating it.

@davidism davidism self-assigned this Nov 2, 2023
@davidism davidism removed their assignment Apr 1, 2024
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

No branches or pull requests

1 participant