mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
179 stars 80 forks source link

Fix #197: Use AJAX for submitting assertions instead of a hidden form. #208

Closed Osmose closed 10 years ago

Osmose commented 10 years ago

This is a large change, and breaks backwards compatibility in a lot of ways, including new JSON responses from Verify, a new JavaScript API, and moved URLs.

Sorry for the giant changeset, but the changes just sort've piled up as I went along. The removal of the browserid_info could maybe be split out, as could the test updates if anyone thinks that's worth it, but if you can just handle mega-PR you'd be my best friend. :D

Osmose commented 10 years ago

Let's see... @willkg and @peterbe, I would appreciate it if you at least skim over this to let me know if you have any objections at a high level on the API or design.

@jgmize, would you be willing to do a detailed r? This PR will make adding admin login support trivial, which translates to making my work for Nucleus trivial and quick. :D

If not, let me know, I can find someone else.

jgmize commented 10 years ago

@Osmose there's a failing test in Travis CI:

ERROR: test_http_method_not_allowed (django_browserid.tests.test_views.JSONViewTests)

Traceback (most recent call last):

File "/home/travis/build/mozilla/django-browserid/django_browserid/tests/test_views.py", line 26, in test_http_method_not_allowed response = TestView().http_method_not_allowed() File "/home/travis/build/mozilla/django-browserid/django_browserid/views.py", line 24, in http_method_not_allowed response['Allow'] = ', '.join(self._allowed_methods()) AttributeError: 'TestView' object has no attribute '_allowed_methods'

jgmize commented 10 years ago

Sorry to be the bearer of bad news, but in addition to the py26_dj14 failure on travis I mentioned earlier, I'm also seeing 15 errors in the py33_dj15 environment when I run tox. Unfortunately I don't have py32 installed on this machine, but it looks like that would be affected as well, because they are all the same "TypeError: can't use a string pattern on a bytes-like object" happening when trying to load the json.

Osmose commented 10 years ago

Crap, I had finished this up on the plane but completely forgot that my tox tests failed and I had meant to go back and fix the issues. Doh!

Consider this incomplete and ignore me. Sorry!

jgmize commented 10 years ago

No worries, @Osmose -- easy thing to forget, and it's going to take me a while to go through this anyway, so feel free to take your time and enjoy your PTO

Osmose commented 10 years ago

@jgmize Pushed fixes, tests are passing everywhere now. :D

Osmose commented 10 years ago

Pushed a small fix; retrieving the CSRF token using django.middleware.csrf failed if the site used session_csrf, which doesn't store the token in the same place. Instead, I changed it to rely on the context processors being configured correctly, as both CSRF implementations make the token available in the csrf_token template variable.

It now retrieves the CSRF token by creating a RequestContext and extracting the token from it. Test was updated and I've manually tried it on a site using session_csrf (Nucleus, in this case) and a site using the normal Django CSRF (my personal django-browserid test site).

willkg commented 10 years ago

OMG! I forgot about this. What happened was that my todo list is actually a stack and over the last two weeks I've been pushing so many things off the stack that this item fell into the murky depths.

I'll try to get to it before Thursday.

willkg commented 10 years ago

I'm (finally) starting to work through this now. Really sorry it's taken me so long.

willkg commented 10 years ago

I'm getting this when trying to compile the docs:

Exception occurred:
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/conf/__init__.py", line 132, in __init__
    % (self.SETTINGS_MODULE, e)
ImportError: Could not import settings 'django_browserid.tests.settings' (Is it on sys.path? Is there an import error in the settings file?): cannot import name authenticate
The full traceback has been saved in /tmp/sphinx-err-2mVkSu.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
Either send bugs to the mailing list at <http://groups.google.com/group/sphinx-dev/>,
or report them in the tracker at <http://bitbucket.org/birkenfeld/sphinx/issues/>. Thanks!
make: *** [html] Error 1

Key part seems to be "cannot import name authenticate".

That's with the newly released Django 1.6 (which gets picked up because requirements.txt says >=1.3), but also with Django 1.5.4.

willkg commented 10 years ago

That covers my initial pass. I really want to read through the docs to wrap my head around the changes. I need to work on other things now, but if someone hasn't figured out the docs building issue, then I'll look at it later.

willkg commented 10 years ago
# Sphinx version: 1.1.3
# Python version: 2.7.5
# Docutils version: 0.11 release
# Jinja2 version: 2.7.1
Traceback (most recent call last):
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/sphinx/cmdline.py", line 188, in main
    warningiserror, tags)
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/sphinx/application.py", line 102, in __init__
    confoverrides or {}, self.tags)
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/sphinx/config.py", line 216, in __init__
    exec code in config
  File "/home/willkg/mozilla/django-browserid/docs/conf.py", line 28, in <module>
    from django_browserid import __version__
  File "/home/willkg/mozilla/django-browserid/docs/../django_browserid/__init__.py", line 10, in <module>
    from django_browserid.auth import BrowserIDBackend  # NOQA
  File "/home/willkg/mozilla/django-browserid/docs/../django_browserid/auth.py", line 22, in <module>
    from django.contrib.auth.models import User
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/contrib/auth/__init__.py", line 5, in <module>
    from django.middleware.csrf import rotate_token
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/middleware/csrf.py", line 16, in <module>
    from django.utils.cache import patch_vary_headers
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/utils/cache.py", line 26, in <module>
    from django.core.cache import get_cache
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/core/cache/__init__.py", line 70, in <module>
    if DEFAULT_CACHE_ALIAS not in settings.CACHES:
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/conf/__init__.py", line 53, in __getattr__
    self._setup(name)
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/conf/__init__.py", line 48, in _setup
    self._wrapped = Settings(settings_module)
  File "/home/willkg/.virtualenvs/browserid/lib/python2.7/site-packages/django/conf/__init__.py", line 134, in __init__
    raise ImportError("Could not import settings '%s' (Is it on sys.path?): %s" % (self.SETTINGS_MODULE, e))
ImportError: Could not import settings 'django_browserid.tests.settings' (Is it on sys.path?): cannot import name authenticate

That's the full stack trace. Looks like conf.py imports django_browserid to get __version__, but that imports some stuff which imports some stuff which tries to import settings ... but settings is django_browserid.tests.settings and thus we hit a circular import problem and everything dies.

Possible fixes:

  1. Change conf.py to do what setup.py does and "parse" the version information manually instead of importing.
  2. Point the docs at a different set of settings. Building the documentation shouldn't require the test settings.
Osmose commented 10 years ago

Turns out I encountered this and fixed it in a branch that was, ironically, waiting for this PR to be merged: https://github.com/Osmose/django-browserid/commit/bd36782f201e0cfdb0dfab278b448a97ae53328c

Feel free to checkout the fix-docs-again branch from my repo to get the docs to build.

willkg commented 10 years ago

Updating here. That worked super and I've built the docs and I'm reading through them now.

willkg commented 10 years ago

This looks good as far as I can tell. I don't know if that's because it actually is good or if I'm not being very imaginative in regards to how the new architecture affects usage. I'm going to be positive and assume it's because it is good.

One thing is that the documentation is very nice. It looks really easy to implement. I like that.

Osmose commented 10 years ago

@willkg Updated. I moved around the tests for the verify view a bit so that I could better test the specific things I wanted to test, and I updated the sanity check method to return a boolean based on whether it ran the checks or not. Other than that I just directly followed your input.

If you hand me an r+ I can merge and we can finally move on with our lives. :D

willkg commented 10 years ago

I kind of wished you had just pushed a new commit rather than amending the previous one--it makes it impossible to see the differences.

I re-skimmed the patch, but I'm not sure doing that had much value other than reminding me that Christmas is around the corner and I'm behind in my shopping. r+

Osmose commented 10 years ago

I kind of wished you had just pushed a new commit rather than amending the previous one--it makes it impossible to see the differences.

Doh! Force of habit, I shouldn't have done that on such a massive patch. Sorry! I'll see if I can recover that info, otherwise we'll just wing it. Sorry again. :(

willkg commented 10 years ago

No worries! Unless you're really good with git, I wouldn't try to undo the amend, for that way lies folly and danger and I definitely don't want you to lose what you did.

willkg commented 10 years ago

Oh! I have the previous code. I can get this as a patch, compare the two and that'll show me the differences.

I'll do that tomorrow. That's easy peasy and then you don't have to fiddle with things.

willkg commented 10 years ago

Two things:

  1. I'm feeling pretty smart right now on account of the idea in my last comment working fantastically.
  2. Looks great! r+