ObjectRepository: don't always require credentialProperty about doctrinemodule HOT 10 CLOSED

So you basically would want to skip the entire credential property logic if a callable is set if I get it right?

The idea behind the credential callable is to match against encrypted data, such as hashing the provided $credentialValue and match it against the retrieved value.

from doctrinemodule.

No, the above code does not skip the credential property logic if a callable is set. It first checks if a callable is set and returns true. If not (for instance, if it returns a hashed value), the credential property is still used as before.

from doctrinemodule.

@Pietr, as @Ocramius indicates, it's not exactly how the callable was envisaged to be used, however it's not a BC break, and seems to make sense to me. Although, it sounds like your callable is doing the actual authenticaiton, rather than jush doing a hash. If the callable is actually doing the work, do you really need the ObjectRepository at all? Would it be simpler to turn your callable into an auth adaper in it's own right?

from doctrinemodule.

@superdweebie I'm using Phpass to do the actual hashing, which uses a per-user salt encoded in the hash string. Hence I cannot have the credential callable return a correct new hashing of the plaintext, without extracting the salt first (which Phpass does not directly support). Phpass does provide a CheckPassword function, which behaves like a credential callable that returns true/false, hence my request for this change.

You indicate this is not how the callable was envisaged to be used, but the current implementation does already support callables that return true/false (it just queries the credential property as well, without actually needing its value). Indeed I could create my own AdapterInterface, but it would be structured similarly to the ObjectRepository, so it feels like reinventing (a tiny bit of) the wheel.

If it is decided not to make this change, I would suggest to remove the support for callables that return a boolean, because it is a bit confusing to provide this functionality while also requiring the credential property to be set in any case.

from doctrinemodule.

@Pietr I now understand better. I'm supportive of the change. Put in a PR :)

(Note I'm, not a commiter, so others will have to want it also)

from doctrinemodule.

I'm fine with changing this behavior so that you can use a callback to actually generate the credential value and avoid access to the object itself, but do not mix $credentialValue assuming that boolean true is a special case.
Let's use a different variable for that. It may sound horrible, but maybe a byRef parameter can do the trick (like with Zend\Cache)

from doctrinemodule.

I agree that changing behavior depending on whether $credentialValue returns a boolean or a string is not very nice. I see three options:

1. Add another property such as $booleanCredentialCallable to ObjectRepository and try to call that callable in the validateIdentity function, if it is set.

2. Instead of providing a single ObjectRepository that tries to handle both types of credential callables, split the cases into different AdapterInterface implementations. The common functionality can be placed in an abstract super class. An alternative but similar design is to keep a single ObjectRepository but to add an 'IdentitityValidationStrategy' to it. There could be a BooleanIdentityValidationStrategy and a StringIdentityValidationStrategy (I know, they aren't good names but you know what I mean...)

3. Only support credential callables that return a boolean indicating correct/incorrect password. Any callable that returns a string (the hashed password) can be easily rewritten to return such a boolean, by moving the string comparison that is currently in the ObjectRepository to the callable.

Solution 1 is the only solution that doesn't really break the current interface for people using a callable that returns a string. However, this solution adds more complexity to the ObjectRepository and, in my opinion, makes that class handle too many cases.

from doctrinemodule.

@Pietr I think I like option 2a best.

from doctrinemodule.

I also agree on the second approach. Injecting some kind of strategy is
fine, by keeping the default one being something like return $credentialValue === $entity->getCredentialValue(); (or similar approach).

I am not 100% sure about all this, but it would be possible to split the
repository auth adapter into a simple find operation + a default strategy
checking the credential property. It still sounds very elaborate and
complex anyway, and it probably wouldn't make things easier for the end
user (who would be faster in implementing his own auth adapter at that
point).

Breaking the interface is not really a problem as long as we do it after
tagging (which will happen as soon as ZF 2.0.0 final is out) and
documenting the change.

Marco Pivetta

http://twitter.com/Ocramius

http://marco-pivetta.com

On 1 September 2012 11:27, Tim Roediger [email protected] wrote:

@Pietr https://github.com/Pietr I think I like option 2a best.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/81#issuecomment-8211425.

from doctrinemodule.

Closing this one as there's no further feedback and the current approach is portable enough (IMO).
I will re-open if there is a clear decision about an approach to take

from doctrinemodule.