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

Add Python 3 support for FacebookAuthorization.parse_signed_data #493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stianpr
Copy link

@stianpr stianpr commented Oct 22, 2014

json.loads was expecting a string, but in python 3 base64decode() return bytes and that is why it was 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. Read more about this on python docs. Though this is only available from python 2.7.7.

Fixes #491.

@stianpr
Copy link
Author

stianpr commented Oct 22, 2014

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.

@troygrosfield
Copy link
Collaborator

@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.

@stianpr
Copy link
Author

stianpr commented Nov 10, 2014

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.

@neokeats
Copy link

hi,
they are still issues with python3.
for example : open_facebook\api.py Line 209 refers to the method that no longer exists in py3 : unicode.

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

@stianpr
Copy link
Author

stianpr commented Nov 17, 2014

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?

@neokeats
Copy link

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.

https://docs.python.org/3.0/whatsnew/3.0.html

@stianpr
Copy link
Author

stianpr commented Nov 17, 2014

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 except clause work?

Anyway you should create a separat issue for this.

@neokeats
Copy link

Oh i see,
they are not the lines with problem.
In the patch and the trunk, they are actually missing so it can't work.
I just provided the modifications needed in order to get it to actually work with python3.

`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.
@stianpr stianpr force-pushed the py3-support-parsed-signed-data branch from c899a8e to eaf88d1 Compare January 27, 2015 11:35
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.

the JSON object must be str, not 'bytes'
3 participants