This repository has been archived by the owner on Jan 8, 2020. It is now read-only.
Fix 6428 - authenticate() always fails on IBMi when using DB table-based authentication #6510
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
52422c4
ZF-6428: authenticate() always fails on IBMi when using DB table-based
clarkphp 4c80350
Merge branch 'fix-6428' of https://github.com/clarkphp/zf2 into fix-6428
clarkphp 1218c2b
Correct a PSR-2 Code Style error I had missed before issuing PR.
clarkphp 5d8e023
Format source code to better conform to ZF2 Coding Standards:
clarkphp File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm wondering if this can be pushed down to
Zend\Db
instead (providing the actual requested column names when the DB layer scrambles the returned ones)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.
To be more specific, while I understand why this is happening (I think informix/firebase also do this mess) I don't like that we push these hacks into layers with other responsibilities.
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.
I see where you're coming from, and initially, I thought the same, but technically, the ZF2 DB layer is not scrambling anything; rather, it is abiding by the column-casing set by the value of the driver option db2_attr_case, which can be one of DB2_CASE_LOWER, DB2_CASE_UPPER, or DB2_CASE_NATURAL. The default is DB2_CASE_UPPER, and is used by nearly every IBM shop we at Zend have worked with.
Since CredentialTreatmentAdapter::authenticateValidateResult() is always looking for a lowercase key, the value of db2_attr_case might or might not allow authenticate() to behave as expected, depending on the setting of db2_attr_case for a given application.
Should this really go somewhere in the DB layer, or should this go in an IBM-specific class overriding CredentialTreatmentAdapter::authenticateQuerySelect(), or should it stay where it is?
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.
That by far means that DB2 in general is doing it right :-)
The fix is simple, so I won't be too picky about it, but you may as well introduce it as
@todo
ifZend\Db
can't abstract this ugly detail of the RDBMS away from us right now (and by abstracting, I mean reading thedb2_attr_case
and doing something about it).I think the keys should just match the input queries, and so should the return values.
If you don't plan to fix that in this PR (an additional
if
condition is acceptable over a huge rework ofZend\Db
), then please open an issue about it and reference it in a@todo
.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.
Perhaps I'm thinking too hard about it, but the more I consider it, the less I see that Zend\Db needs any change. It is doing exactly what it is supposed to do: return column names cased according to db2_attr_case, regardless of the case used when the query was built.
The values returned are whatever was stored in the table; db2_attr_case affects only the column names, and thus the keys of our associative arrays.
I could be totally missing your point, and if so, I apologize, but is your preference special code in Zend\Db that looks for a ZEND_AUTH_CREDENTIAL_MATCH column (either upper or lower case) in a resultset, and forces it lower case, so that the authentication code sees the lowercase array key it is looking for?