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.

Sign up for GitHub

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

Jump to bottom

Make compatible with Django 1.10 #30

Merged
merged 1 commit into from
Sep 30, 2016
Merged

Make compatible with Django 1.10 #30

merged 1 commit into from
Sep 30, 2016

Conversation

pcraston
Copy link
Contributor

@pcraston pcraston commented Sep 5, 2016

replace _get_new_csrf_key with _get_new_csrf_string

@moggers87
Copy link
Contributor

A couple of things:

  • We should maintain compatibility with at least Django 1.8 and 1.9
  • You need to add Django 1.10 to .travis.yml

@pcraston pcraston force-pushed the master branch 5 times, most recently from 11097ec to 3dd5fd7 Compare September 6, 2016 08:48
@pcraston
Copy link
Contributor Author

pcraston commented Sep 6, 2016

Hi @moggers87
Tests are passing for 1.8 and 1.9 but still have two tests failing for 1.10. I'm struggling to understand the point of test_massive_anon_cookie and test_surprising_characters at https://github.com/mozilla/django-session-csrf/blob/master/session_csrf/tests.py#L337 . Those tests still failing. Any ideas off the top of your head why the warner is getting called twice with django 1.10 and not with previous versions? (can't spot anything relevant in the release notes)

@moggers87
Copy link
Contributor

warner is a mock, so you could go in with PDB and check warner.call_args_list

@pcraston pcraston force-pushed the master branch 3 times, most recently from 0332902 to 65af05c Compare September 7, 2016 14:38
@pcraston
Copy link
Contributor Author

pcraston commented Sep 7, 2016 •
edited

thanks for the pointer, turned out there were quite a few changes required but it should all be backwards compatible for Django 1.8 and 1.9

@moggers87 let me know if you need any other modifications in order to merge this

@pcraston pcraston mentioned this pull request Sep 21, 2016
Closed
@pcraston
Copy link
Contributor Author

I've been testing my branch with our project and it's working fine. Let me know if you find any other issues that are preventing this PR from being merged.

@glogiotatidis
Copy link
Contributor

glogiotatidis commented Sep 28, 2016 •
edited

I confirm that this is working and a csrf string gets generated.

Django made changes in the first place to fight BREACH type attacks and added salt encrypt of each request to do so. See Django's Ticket and Git Commit. The changes here don't take into account that but I'm not sure if that's required given the different approach session-csrf takes. Even if we need to implement salting probably we shouldn't block this pull request.

@moggers87 can you please take another look now that the tests pass?

edited to update GitHub link

@glogiotatidis glogiotatidis mentioned this pull request Sep 28, 2016
Merged
@moggers87
Copy link
Contributor

Visual inspection looks good.

With regard to BREACH, we already use functions from Django to produce our tokens. In theory all this package does is change token storage from a cookie to the session.

@glogiotatidis
Copy link
Contributor

glogiotatidis commented Sep 29, 2016 •
edited

With regard to BREACH, we already use functions from Django to produce our tokens. In theory all this package does is change token storage from a cookie to the session.

Indeed but Django is handling differently the tokens now to fight BREACH. Instead of just generating tokens -with the same function we use- it's also salting and unsalting tokens -with a function that we don't use-.

django/django@5112e65#diff-a3be722ce2831a8d11438021d44cedf1R91

@moggers87
Copy link
Contributor

I think that would be better tackled in a separate issue - would be nice to vender in a copy of those functions for 1.8 and 1.9 users.

@glogiotatidis
Copy link
Contributor

Agreed this can be part of another PR. Filed ticket #32 to track this.

@peterbe
Copy link
Contributor

peterbe commented Sep 29, 2016

So this breaks support for Django <= 1.7. I'm fine with that.
What appears to be missing is a change here: https://github.com/mozilla/django-session-csrf/blob/master/setup.py#L9 I would make that 0.7.0.

peterbe
peterbe reviewed Sep 29, 2016
View reviewed changes
Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

Just putting in some question marks here. If @glogiotatidis thinks this is good enough I'm all for merging it.
Don't forget to update the setup.py version too.

.travis.yml
- DJANGO="Django==1.9.4"
- DJANGO="Django==1.8.14"
- DJANGO="Django==1.9.9"
- DJANGO="Django==1.10.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these still the latest and greatest for each?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

session_csrf/__init__.py
try:
from django.middleware.csrf import _get_new_csrf_key as django_get_new_csrf_string
except ImportError:
from django.middleware.csrf import _get_new_csrf_string as django_get_new_csrf_string
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really no public function available in django.middleware.csrf to get a token string out?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just had another look at nothing there as far as I can tell

session_csrf/__init__.py
if DJANGO_VERSION < (1, 10, 0):
return request.user.is_authenticated()
else:
return request.user.is_authenticated
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

session_csrf/__init__.py
else:
return request.user.is_authenticated

class CsrfMiddleware(deprecation.MiddlewareMixin if DJANGO_VERSION >= (1, 10, 0) else object):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually not familiar with what deprecation.MiddlewareMixin but I feel like this class definition now deserves a comment that explains why we do what we do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment

@pcraston
Make compatible with Django 1.10. Replace _get_new_csrf_key with _get…
29d5030 
…_new_csrf_string, use Middleware Deprecation Mixin, fix urlpatterns
@peterbe peterbe merged commit 5a220fd into mozilla:master Sep 30, 2016
@peterbe
Copy link
Contributor

peterbe commented Sep 30, 2016

Hurray! I'll push a release to pypi

@peterbe
Copy link
Contributor

peterbe commented Sep 30, 2016

There https://pypi.python.org/pypi/django-session-csrf/0.7.0

Please test it. If it works, I'll tweet about it.

Also, @glogiotatidis we need to update your PR to up this in the requirements.

@glogiotatidis
Copy link
Contributor

Great stuff, thanks all!

@peterbe peterbe mentioned this pull request Oct 3, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterbe peterbe peterbe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pcraston @moggers87 @glogiotatidis @peterbe