Added support for callable credential validator

[youngguns-nl]

This is an update for #3371, mainly by @BinaryKitten

It provides

• tests
• usage of a callback instead of a closure
• maintain BC

[weierophinney]

This should check $validationCallback against is_callable(), and raise an exception if it is not.

[BinaryKitten]

The original had callable as a typehint which mean that it didn't need the check. This should have been caught in cf6755f

[weierophinney]

Except that the callable type hint is only available starting in 5.4, and
we support 5.3...
On Jan 21, 2013 12:35 PM, "Kathryn Reeve" [email protected] wrote:

In library/Zend/Authentication/Adapter/DbTable.php:

@@ -192,6 +205,18 @@ public function setCredentialTreatment($treatment)
$this->credentialTreatment = $treatment;
return $this;
}

• /**

• \* setCredentialValidationCallback() - allows the developer to use a callback as a way of checking the
  

• \* credential.
  

• *
  

• \* @param  callable $validationCallback
  

• \* @return DbTable Provides a fluent interface
  

• */
  

• public function setCredentialValidationCallback($validationCallback)
• {

•    $this->credentialValidationCallback = $validationCallback;
  

The original had callable as a typehint which mean that it didn't need the
check. This should have been caught in cf6755fhttps://github.com/zendframework/zf2/commit/cf6755f

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3490/files#r2716396.

[weierophinney]

@youngguns-nl Can you rebase against current master, please?

Also, @BinaryKitten -- is this ready? I know you were collaborating on it...

[ralphschindler]

At which point do you think it would make more sense to just create a separate adapter, with a different constructor signature?

[youngguns-nl]

@ralphschindler At the point you don't need a database as your password storage?
I think it's a bit overkill to create a second dbAdapter because you want to validate the password with a php callback instead of a database function.

[ralphschindler]

I'm sorry, I didn't mean a different database adapter, I meant a Zend\Authentication adapter. So basically, instead of shoehorning this feature into the existing DbTable authentication adapter which makes some features mutually exclusive, I mean create a CallbackDbTable adapter that extends DbTable adapter, but remove the credentialTreatment in the constructor in favor of a callback.

That is what I was getting at, not a different Db adapter. ;)

[weierophinney]

@ralphschindler The idea as last discussed is to introduce a validation
callback at either the AuthenticationService level, or by adding an
interface that adapters could implement. I think it makes sense for the db
apter to support both current credential treatment and a validator
callback; it would simplify usage not needing to know which adapter did
which.
On Feb 17, 2013 8:23 PM, "Ralph Schindler" [email protected] wrote:

I'm sorry, I didn't mean a different database adapter, I meant a
Zend\Authentication adapter. So basically, instead of shoehorning this
feature into the existing DbTable authentication adapter which makes it
some features mutually exclusive, I mean create a CallbackDbTable adapter
that extends DbTable adapter, but remove the credentialTreatment in the
constructor in favor of a callback.

That is what I was getting at, not a different Db adapter. ;)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3490#issuecomment-13703983.

[ralphschindler]

I've given this some thought and I still feel like this really should be done in a separate class. Here is my reasoning:

Currently, the goal of the DbTableAdapter is to take an identity and single credential and match them with SQL. Extensions to this philosophy are to allow fragments of SQL, usually functions like MD5 or PASSWORD to be specified to treat credentials to be able to help match an existing row. Matches are represented as a special column in the result called zend_auth_credential_match. This patch attempts to subvert matching done in the database for matching done in userland, so really, you no longer attempting to authenticate in the database, you only want to retrieve matching identities from the database.

The only checking done in userland is this, as the rest of the object assumes everything was matched in the db:

if ($resultIdentity['zend_auth_credential_match'] != '1') {

This is subjective, but the constructor is growing beyond a manageable point with regards to number of arguments. What's less subjective is that the last two arguments are now mutually exclusive, thus, depending on your usage scenario, one is valid while the other is not. If we were to accept this feature as part of this class, we should at least throw an exception when callbacks are supplied with credential treatments.

I might call this class Zend\Authentication\Adapter\DbCallbackAdapter, which would underscore that matching of the credential would be done in userland.

Matthew, thoughts, or do you think we should move forward with the feature as it is here?

[weierophinney]

@ralphschindler Considering that they could extend from the same base, I think having two different adapters makes sense. If you start with credential treatment, and then later decide you want to do userland hashing, you'd simply change adapters. This would simplify the logic -- no need to test for one or the other condition -- and leverage the existing code.

@youngguns-nl and @BinaryKitten -- thoughts? Would you be willing to go that route?

[BinaryKitten]

Hi @weierophinney, @ralphschindler, @youngguns-nl

I am happy with multiple adapter route.

I can look at this later this week for completion if that makes sense.

From what I gather there will be an AbstractDbAdapter which will have the
main crux of code
That will be extended into

• CredentialTreatmentAdapter - aka current Db adapter
• CallbackCheckAdapter - uses a callback to process the password

Names to still be refined and stated but recommend that we split the
adapters and have a bare extends class replace the original

thoughts?

[weierophinney]

From what I gather there will be an AbstractDbAdapter which will have the
main crux of code
That will be extended into

• CredentialTreatmentAdapter - aka current Db adapter
• CallbackCheckAdapter - uses a callback to process the password

We'll have to keep the original DbTable adapter around, as removing it would break BC; however, we can have it extend the new CredentialTreatment adapter, and mark it as @deprecated. I'm assuming you'd put these new classes under a DbTable subnamespace, correct?

[BinaryKitten]

@weierophinney Yep that's my thoughts/
Means that DbTable would just be a proxy to the new name one

[JustInVTime]

sorry for my late response. I turned the @youngguns-nl account into an organization account and apparently I'm not getting e-mail notifications anymore.

Two different adapters sounds fine to me. @BinaryKitten any updates so far? Tomorrow I have time to work on this as well. Looking forward to finally get this into the next release!

[BinaryKitten]

@JustInVTime I've not started this yet, as above was aiming to work on it later this week (probably this evening)

Grab me in IRC at some point @JustInVTime, @youngguns-nl

[JustInVTime]

I have a bunch of work done now.

I created the DbTable subnamespace, with the CallbackCheckAdapter and CredentialTreatmentAdapter.
Both extends DbTable/AbstractAdapter (which in his turn extends the Adapter/AbstractAdapter) which contains most of the original methods. It contains 2 abstract methods, _authenticateValidateResult and _authenticateCreateSelect.

The 'old' Adapter/DbTable class extends the DbTable/CredentialTreatmentAdapter as suggested, so BC is maintained here.

Should the CallbackCheckAdapter have a default build-in callback, like the CredentialTreatmentAdapter also has? This could be a strict comparison. Thoughts? If not, then this adapter is only useful with a callback and in this way differs from his CredentialTreatment brother.

[BinaryKitten]

Why the _ names?

[JustInVTime]

@BinaryKitten Cause that's in the original code base?

[JustInVTime]

as @weierophinney mentioned on IRC: we don't apply BC concerns to non-public API. That means protected and private can be altered whenever.

I'll rename those function names to non '_' prefixed ones.

[weierophinney]

Merged to develop, for release with 2.2.0.

[jbaez]

Shouldn't in CallbackCheckAdapter::authenticateValidateResult the hash and credential be reversed?
At the moment the callback is done with the parameters in this order: (hash, credential)
if they are reversed then they would match the Zend\Crypt\Password\PasswordInterface::verify(credential, hash)

[BinaryKitten]

@jbaez this PR has been closed and merged. Please do open a new one if you wish to add more to it (all contributions gratefully received)