password_hash is unicode, needs to be encoded

[mmerickel]

I think SQLAlchemy must have changed its behavior. Maybe someone could confirm this is an issue but it was failing for me while trying to reproduce Pylons/pyramid_debugtoolbar#261

[mmerickel]

@vincent-ferotin Can I ask if you ran into this issue while following the wiki2 tutorial? I had to add these fixes prior to being able to reproduce your issue with the toolbar.

[digitalresistor]

If the password_hash is supposed to be UTF-8, then the field type in the database should not be Text, but instead should be Unicode.

See http://docs.sqlalchemy.org/en/latest/core/type_basics.html#sqlalchemy.types.Unicode and http://docs.sqlalchemy.org/en/latest/core/type_basics.html#sqlalchemy.types.Text

However bcrypt and friends should instead be receiving byte strings instead, as the API denotes on PyPi: https://pypi.python.org/pypi/bcrypt/2.0.0

The users password for example should not be encoded/decoded to UTF-8 at all.

[mmerickel]

@bertjwregeer what are you suggesting? Are you proposing updating the tutorial to use a BLOB type then?

[digitalresistor]

The output from bcrypt should be a standard ASCII string (60 characters long IIRC) so a CHAR(60) would be fine for storage, with encoding of latin1, if the database is explicitly set to UTF-8, then yes, it should potentially be stored as a blob.

The encode on the password I guess would be required since the browser is sending UTF-8... I hate encoding.

[vincent-ferotin]

Hi Michael.

I think you switched my email with this of Vinicius Assef <
viniciusban@gmail.com>
for his problem with "SQLAlchemy + URL dispatch wiki tutorial in Pyramid
1.7b4", which is described in an other thread (
https://groups.google.com/forum/#!topic/pylons-discuss/upiQDqpLxbk).

-- Vincent

2016-05-16 6:33 GMT+02:00 Michael Merickel notifications@github.com:

@vincent-ferotin https://github.com/vincent-ferotin Can I ask if you
ran into this issue while following the wiki2 tutorial? I had to add these
fixes prior to being able to reproduce your issue with the toolbar.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2576 (comment)

[mmerickel]

Thanks @vincent-ferotin, I obviously relied too much on github's autocomplete. ping @viniciusban

[viniciusban]

Yes, @mmerickel, I was following this step in wiki2 tutorial: http://docs.pylonsproject.org/projects/pyramid/en/1.7-branch/tutorials/wiki2/authentication.html

Then, as suggested by @stevepiercy, I cloned the Pyramid repo to get this code https://github.com/Pylons/pyramid/tree/1.7-branch/docs/tutorials/wiki2/src/authentication and the problem was showed again.

As I wrote in Pylons/pyramid_debugtoolbar#261 the error occurs in debugtoolbar 2.5 and 3.0.

[mmerickel]

@viniciusban I think you misunderstand. I'm asking if you ran into this unicode issue (this PR) specifically.

[viniciusban]

Sorry @mmerickel.

No, I didn't get this unicode issue.

[jvanasco]

The problem is because of pysqlite.

http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html?highlight=sqlite#unicode

Unicode
The pysqlite driver only returns Python unicode objects in result sets, never plain strings, and accommodates unicode objects within bound parameter values in all cases. Regardless of the SQLAlchemy string type in use, string-based result values will by Python unicode in Python 2. The Unicode type should still be used to indicate those columns that require unicode, however, so that non-unicode values passed inadvertently will emit a warning. Pysqlite will emit an error if a non-unicode string is passed containing non-ASCII characters.

No matter what SqlAlchemy column/field you use, it will be a Unicode value under sqlite*

A quick fix is to just adjust the password function: models/user.py L28

•        actual_hash = bcrypt.hashpw(pw.encode('utf8'), expected_hash)
  

•       actual_hash = bcrypt.hashpw(pw.encode('utf8'), expected_hash.encode('utf8'))
  

note: * one could use a custom column type that does post/preprocessing, but that will confuse people. Just expect it to be unicode.

[jvanasco]

Sorry, I did not explain my comment above enough -- both arguments to the bcrypt function need to be string. That's why the issue is happening and the current fix doesn't fully address it.

[mmerickel]

@jvanasco The current fix is exactly the same as your suggested fix. What do you mean it doesn't fully address it?

[digitalresistor]

Current fix would address the pysqlite issues. Which IMHO is a broken model... wow.

[digitalresistor]

LGTM. Now that I understand what is going on...

[jvanasco]

@mmerickel i read a line wrong and thought you encoded a different variable Sometimes i get dyslexic looking at code blocks..

Yeah, the pysqlite thing is annoying. The same underlying issue recently tripped me up.

[mmerickel]

Well this is just a demo but it'd be nice if it didn't break for anyone. Who wants to switch this to a LargeBinary such that it's always a byte string, versus keeping it as Text and relying on the underlying driver? I'm leaning toward the binary route myself.

[mmerickel]

Okay, gonna merge this for now but if someone wants to improve this, the door is open!