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

Switch to sha512 in AuthTktAuthenticationPolicy #2496

Merged
merged 5 commits into from
Apr 15, 2016

Conversation

digitalresistor
Copy link
Member

@digitalresistor digitalresistor added this to the 1.7 milestone Apr 14, 2016
@mmerickel
Copy link
Member

As I said in #2362 I don't want to do this until 2.0.

@mmerickel mmerickel modified the milestones: 2.0, 1.7 Apr 14, 2016
@digitalresistor
Copy link
Member Author

Okay :-) Missed that.

@tseaver
Copy link
Member

tseaver commented Apr 14, 2016

@mmerickel Why is something we've warned about for three major releases a 2.0-only change?

@mmerickel
Copy link
Member

@tseaver My concern is that the warnings don't actually show up for people. Python 2.7 changed the default behavior to not show DeprecationWarnings unless you explicitly set an environ variable or interpreter flag. It's ridiculous.

For this reason, and that the behavior of this change is something that will affect people who upgrade by logging out every user of their website if they didn't see the warnings... It feels like a bad idea.

We should do a better job with our deprecation warnings, we've never changed how it worked (the current system worked great on Python 2.6) but simply doesn't work well now.

@tseaver
Copy link
Member

tseaver commented Apr 14, 2016

@bertjwregeer pep8 failure:

pyramid/authentication.py:8:1: F401 'warnings' imported but unused

@digitalresistor
Copy link
Member Author

@tseaver Yeah, noticed that. But unless we agree to ship this soon, I was not planning on fixing this until 2.0.

@mmerickel
Copy link
Member

The deprecation is weird because it's changing the default. It's not removing a feature where user's code will error but rather just flipping it to something else. Super subtle.

@tseaver
Copy link
Member

tseaver commented Apr 14, 2016

IMHO, having user logins evaporate during a "major" software upgrade (and I would count Pyramid 1.6.x -> 1.7 as such) doesn't sound like a big enough issue to warrant deferring the change until 2.0. We won't be "breaking" users software, in the sense that they will be forced to make code changes to get up and running: users who care will be able to a) change the code ahead of time such that they pass an explicit hashalg, and thus won't see the cookies disappear; b) schedule the upgrade for a time when they deem that having login cookies disappear is tolerable.

@mmerickel
Copy link
Member

Alright, I'm satisfied since we've at least discussed the issue. 2.0 was my conservative feeling as no one had expressed interest otherwise until now. We'll have to make the release notes very clear here.

@digitalresistor
Copy link
Member Author

@mmerickel Updated CHANGES.txt with specific text. Also, re-organised it a little (forgot that I was on a feature branch).

@digitalresistor digitalresistor modified the milestones: 1.7, 2.0 Apr 15, 2016
@mmerickel
Copy link
Member

hashalg docstring is not updated on the policy

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.

3 participants