-
Notifications
You must be signed in to change notification settings - Fork 13
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
Allow case-sensitive check for token #4
Conversation
@@ -6,7 +6,7 @@ | |||
http://doctrine-project.org/schemas/orm/doctrine-mapping.xsd"> | |||
|
|||
<mapped-superclass name="ZfrOAuth2\Server\Entity\AbstractToken"> | |||
<id name="token" type="string" length="40" /> | |||
<id name="token" type="string" column-definition="VARCHAR(40) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not portable, and will break any storage layer that isn't MySQL. No-go IMO.
Instead, fetch the token and compare it after fetch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: doctrine/dbal#245
So options={"collation"="utf8_bin"} is portable? |
@bakura10 no, but at least it won't crash DDL statements for other storage layers. The only portable approach is most probably by fetching and checking PHP-side |
Allow case-sensitive check for token
@bakura10 this is still not fixed btw - it needs checks in the repository as well. |
@bakura10 you just broke my entire project with this 👎 SQLITE does not support it thus integration tests fails :/ |
Really? Even with the "options" thing? @Ocramius told us it should not break anything. |
@bakura10 I was honestly expecting this to be ignored on sqlite =_= Yeah, so I guess the right fix is really fetching and comparing |
That's a bit strange, ZfrOAuth2 is unit-tested with SQLite, and all tests are passing. |
@bakura10 Any chance you can revert this with a notice for now so that it does not break my application.
|
Ok so :/. Please see the other PR. I'll merge that in a few minutes. |
I'm still curious about why ZfrOAuth2 unit tests do not fail :/. |
Different versions of sqlite perhaps ? |
@macnibblet probably an old sqlite version? Also, this PR should be reverted |
|
I have version 3.7.13 too... :/. |
Wtf |
nvm. You're right. Actually there is no functional tests in ZfrOAuth2. I experienced the same issue in my code. |
ping @Ocramius @mac_nibblet
I realized that by default, SQL do case-insenstivie checks for token. This can be an issue as tokens are fetched by the token itself.
I couldn't find anything in Doctrine 2 mapping to force having utf8_bin (which makes case-senstivie checks instead of default utf8_general), so I used a columnDefinition.