-
Notifications
You must be signed in to change notification settings - Fork 541
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
Add Python 3 support for FacebookAuthorization.parse_signed_data #493
base: master
Are you sure you want to change the base?
Add Python 3 support for FacebookAuthorization.parse_signed_data #493
Conversation
I successfully run most of the tests in python 2.7.8, though I'm not sure I should branch out of master because master has 2 breaking tests there? I manually tested the change in python 3 and it seems to work fine. It would be nice to have more people on Django 1.7 with python 3 verify that it works. |
@stianpr, I've been using the app on py3.4.x, django 1.7.x without issues. Your change above looks correct, but the bigger issue here is trying to get travis running successfully again. The django 1.7 issues have to do with south and migrations. I'll open up additional bugs relating to those and we'll track there. It's hard to accept these changes when the tests are still failing. Let's get those working first then get this change merged in once the tests are passing. |
cf8b0d1
to
c899a8e
Compare
I'm missing one test that we should have and that's where the cookie parsing fails. It should not throw an exception on those circumstances. I will add that ASAP. |
hi, so the file is missing at the top try:
unicode = unicode
except NameError:
unicode = str the same goes for django_facebook\connect.py and django_facebook\utils.py try :
from functools import reduce
except:
pass is needed in django_facebook\model_managers.py i had to modify the hash_key method in django_facebook\utils.py : def hash_key(key):
import hashlib
if six.PY3:
key = key.encode('utf-8')
hashed = hashlib.md5(key).hexdigest()
return hashed |
I don't see why you get any exceptions on these lines of code. They don't look pretty but should be valid in python3? |
the unicode method is no longer present in python3 so by doing unicode=unicode we get exception in python 3 and set the method to be the str one in python3 the method reduce was removed too because : Removed reduce(). Use functools.reduce() if you really need it; however, 99 percent of the time an explicit for loop is more readable. |
Yeah I know that they are not present in python 3. And they will trigger an exception but they are guarded from doing that in your examples. Don't the Anyway you should create a separat issue for this. |
Oh i see, |
`json.loads` was expecting a string, but in python 3 `base64decode()` return bytes and that is why it bugged. We fix this by making sure the decoded payload data is in string and that `hmac.new()` is provided with arguments in bytes. `open_facebook.utils.smart_str` will do that job correctly in python 2 and 3. We also use `hmac.compare_digest()` which is the preferred way to compare those kinds of data to prevent timing analysis. If not `hmac.compare_digest` is available (python 2.7.7+) then we just compare logically. Fixes tschellenbach#491.
c899a8e
to
eaf88d1
Compare
json.loads
was expecting a string, but in python 3base64decode()
return bytes and that is why it was bugged. We fix this by making sure the decoded payload data is in string and thathmac.new()
is provided with arguments in bytes.open_facebook.utils.smart_str
will do that job correctly in python 2 and 3.We also use
hmac.compare_digest()
which is the preferred way to compare those kinds of data to prevent timing analysis. Read more about this on python docs. Though this is only available from python 2.7.7.Fixes #491.