Added support for callable credential validator #3490

Closed
wants to merge 21 commits into from

6 participants

@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 weierophinney and 1 other commented on an outdated diff Jan 21, 2013
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;
@weierophinney
Zend Framework member

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

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

@weierophinney
Zend Framework member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney and 1 other commented on an outdated diff Jan 21, 2013
library/Zend/Authentication/Adapter/DbTable.php
@@ -378,23 +403,27 @@ protected function _authenticateSetup()
*/
protected function _authenticateCreateSelect()
{
- // build credential expression
- if (empty($this->credentialTreatment) || (strpos($this->credentialTreatment, '?') === false)) {
- $this->credentialTreatment = '?';
- }
+ $tableColumns = array('*');
+ if (!is_callable($this->credentialValidationCallback)) {
+ // build credential expression
+ if (empty($this->credentialTreatment) || (strpos($this->credentialTreatment, '?') === false)) {
@weierophinney
Zend Framework member

Why are you treating $credentialTreatment and $credentialValidationCallback as mutually exclusive? Could we not have both? If not, why not?

credentialTreatment is only used to get a 1 or 0 response on the credential match.
Since the the credential match is an extra overhead if the check is going to be done in code, there is no need for the credential treatment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on an outdated diff Jan 21, 2013
library/Zend/Authentication/Adapter/DbTable.php
- unset($resultIdentity['zend_auth_credential_match']);
+ unset($resultIdentity['zend_auth_credential_match']);
+ }
@weierophinney
Zend Framework member

I'd move this into a separate method, to make the current method more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney
Zend Framework member

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

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

@ralphschindler
Zend Framework member

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
Zend Framework member

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
Zend Framework member
@ralphschindler
Zend Framework member

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
Zend Framework member

@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
@weierophinney
Zend Framework member

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.

JustInVTime Refactored #3490. Split DbTable into 2 different (CallbackCheck and C…
…redentialTreatment) adapters
d392e6e
@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 weierophinney added a commit that referenced this pull request Mar 27, 2013
@weierophinney weierophinney Merge branch 'feature/3490' into develop
Close #3490
41bc48e
@weierophinney
Zend Framework member

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)

@ghost Unknown added a commit that referenced this pull request Jul 14, 2013
JustInVTime Refactored #3490. Split DbTable into 2 different (CallbackCheck and C…
…redentialTreatment) adapters
d7b125a
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'feature/3490' into develop
Close #3490
048264f
@weierophinney weierophinney pushed a commit to zendframework/zend-authentication that referenced this pull request May 14, 2015
JustInVTime Refactored zendframework/zendframework#3490. Split DbTable into 2 dif…
…ferent (CallbackCheck and CredentialTreatment) adapters
14b1457
@weierophinney weierophinney added a commit to zendframework/zend-authentication that referenced this pull request May 14, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#3490 from youngguns-nl…
…/zf-3371

Added support for callable credential validator

Conflicts:
	library/Zend/Authentication/Adapter/DbTable.php
4361af7
@weierophinney weierophinney added a commit to zendframework/zend-authentication that referenced this pull request May 14, 2015
@weierophinney weierophinney Merge branch 'feature/3490' into develop 92baf0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment