Add BlowfishAuthenticate adapter. #775

Merged
merged 1 commit into from Aug 23, 2012

3 participants

@sitedyno

Mostly copied FormAuthenticate and it's tests. Happy to make any changes necessary :)

@markstory
CakePHP member

So much copy paste 👎 . It looks like extending FormAuthenticate and re-factoring both classes so only the password comparison is different would be easily doable.

@markstory
CakePHP member

With that said, I think its a great idea to have this in core :D

@sitedyno

I "need" to change _findUser though. It seems like changing its behavior in BaseAuthenticate might break existing implementations that rely on its current behavior?

@markstory
CakePHP member

Couldn't you blowfish hash the user's password before calling _findUser though?

@sitedyno

You need the existing salt from the database to check the password. Each password hash has its own salt.

@markstory
CakePHP member

Much nicer 👍 :D

@sitedyno

I'm not sure what to do with the tests though. I'm inclined to leave it as is for thoroughness, but...

@ADmad
CakePHP member

Instead of adding a param to BaseAuthenticate::_findUser() how about we just change it to BaseAuthenticate::_findUser($conditions = array())

@sitedyno

Would that meant it should be pulled against 3.0? This would be a BC break.

@ADmad
CakePHP member

Can still maintain BC by checking type of $conditions argument and if its string check for existence of second function argument to get password.

@sitedyno

Squashed commits. Happy to make any other changes necessary.

@ADmad
CakePHP member

You missed the point of my suggestion of having just a single param $conditions for _findUser(). If $conditions is an array its used as conditions for the find() to get user. You are treating it as an options array. My reason for having $conditions was to avoid the ignorePassword option/argument.

@sitedyno

@ADmad sorry about that. Better?

@ADmad ADmad and 1 other commented on an outdated diff Aug 18, 2012
...ke/Controller/Component/Auth/BlowfishAuthenticate.php
+ $fields = $this->settings['fields'];
+ if (!$this->_checkFields($request, $model, $fields)) {
+ return false;
+ }
+ $user = $this->_findUser(
+ array(
+ $model . '.' . $fields['username'] => $request->data[$model][$fields['username']],
+ )
+ );
+ if (!$user) {
+ return false;
+ }
+ $password = Security::hash(
+ $request->data[$model][$fields['password']],
+ 'blowfish',
+ $user['password']
@ADmad
CakePHP member
ADmad added a note Aug 18, 2012

You are hard coding column name here which is bad. Plus if your column name is password wouldn't it already get unset in _findUser() ?

Edit: Okay the "if" check for fields in conditions will avoid unsetting of password field. But its not fool proof. Someone might decide to pass conditions without prefixing the fieldname with model name.

@ADmad
CakePHP member
ADmad added a note Aug 18, 2012

Yes i realized the password field won't be unset and edited my comment but github doesn't send email notifications for comment edits :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sitedyno

Ok last time I ever reply to github email xD

@sitedyno

Squashed commits.

@ADmad
CakePHP member

@markstory how does this look now?

@ceeram ceeram and 1 other commented on an outdated diff Aug 20, 2012
lib/Cake/Controller/Component/Auth/BaseAuthenticate.php
* @return Mixed Either false on failure, or an array of user data.
*/
- protected function _findUser($username, $password) {
@ceeram
CakePHP member
ceeram added a note Aug 20, 2012

Wont this cause 5.4 strict errors for existing adapters extending this with 2 arguments?

@ADmad
CakePHP member
ADmad added a note Aug 20, 2012

If this is approved we would change all core usage to pass conditions array. If someone has overridden method in their extended class and get errors they can fix it. With debug off things should work without issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on an outdated diff Aug 20, 2012
...ke/Controller/Component/Auth/BlowfishAuthenticate.php
+ *
+ * CakePHP(tm) : Rapid Development Framework (http://cakephp.org)
+ * Copyright 2005-2012, Cake Software Foundation, Inc. (http://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * Redistributions of the files must retain the above copyright notice.
+ *
+ * @copyright Copyright 2005-2012, Cake Software Foundation, Inc. (http://cakefoundation.org)
+ * @link http://cakephp.org CakePHP(tm) Project
+ * @license MIT License (http://www.opensource.org/licenses/mit-license.php)
+ */
+
+App::uses('FormAuthenticate', 'Controller/Component/Auth');
+
+/**
+ * An authentication adaptor for AuthComponent. Provides the ability to authenticate using POST data using Blowfish
@markstory
CakePHP member

Should be 'adapter'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on an outdated diff Aug 20, 2012
...ke/Controller/Component/Auth/BlowfishAuthenticate.php
+ * CakePHP(tm) : Rapid Development Framework (http://cakephp.org)
+ * Copyright 2005-2012, Cake Software Foundation, Inc. (http://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * Redistributions of the files must retain the above copyright notice.
+ *
+ * @copyright Copyright 2005-2012, Cake Software Foundation, Inc. (http://cakefoundation.org)
+ * @link http://cakephp.org CakePHP(tm) Project
+ * @license MIT License (http://www.opensource.org/licenses/mit-license.php)
+ */
+
+App::uses('FormAuthenticate', 'Controller/Component/Auth');
+
+/**
+ * An authentication adaptor for AuthComponent. Provides the ability to authenticate using POST data using Blowfish
+ * hashing. Can be used by configuring AuthComponent to use it via the AutheComponent::$authenticate setting.
@markstory
CakePHP member

Should be 'AuthComponent'

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

Other than the few typos I think this looks good. The strict errors could be solved by adding a second argument to _findUser(). It might complicate the argument checking, but not too much.

@sitedyno

Added $password parameter. Will remove the commit if you guys tell me to :)

@ADmad ADmad and 1 other commented on an outdated diff Aug 21, 2012
lib/Cake/Controller/Component/Auth/BaseAuthenticate.php
* @return Mixed Either false on failure, or an array of user data.
*/
- protected function _findUser($username, $password) {
+ protected function _findUser($conditions = array(), $password = false) {
@ADmad
CakePHP member
ADmad added a note Aug 21, 2012

Default value of null would be better than false for $password, as it better denotes "no value".

I was looking at in in the sense of "A password argument wasn't supplied" than "The password had no value".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sitedyno

Squashed commits.

@ceeram ceeram commented on an outdated diff Aug 22, 2012
lib/Cake/Controller/Component/Auth/BaseAuthenticate.php
@@ -66,19 +66,28 @@ public function __construct(ComponentCollection $collection, $settings) {
/**
* Find a user record using the standard options.
*
- * @param string $username The username/identifier.
- * @param string $password The unhashed password.
+ * The $conditions parameter can be a (string)username or an array containing conditions for Model::find('first'). If
+ * the password field is not included in the conditions the password will be returned.
+ *
+ * @param string $conditions The username/identifier, or an array of find conditions.
@ceeram
CakePHP member
ceeram added a note Aug 22, 2012

This docblock needs to match the new mixed string|array type, also maybe better not to have a default value for $conditions param?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sitedyno

Sqashed commits.

@markstory markstory merged commit fe3d99c into cakephp:2.3 Aug 23, 2012
@ADmad
CakePHP member

I think this BlowfishAuthenticate needs some more work. Internally Security::hash() uses php's crypt() function for blowfish hashing which requires the salt value to be of a specific format with prefix value having particular significance. It is highly unlikely that the configuration value Security.salt used for system wide hashing would be that particular format. So BlowfishAuthenticate should perhaps take the salt value as option or just the prefix string with fallback to some sane default prefix string in case it's not provided by the developer.

@sitedyno

I'm not sure I'm understanding your concern correctly but, the developer generally shouldn't provide a salt for blowfish. When the user registers (or the blowfish hash is first generated) no salt should be supplied. In that case a unique salt is generated by Security::_crypt(). Each password hash should have a unique salt, so the notion of a sane default does not apply for blowfish hashing.

Perhaps the docblocks need to be clarified, but I believe the code uses best practices.

@ADmad
CakePHP member

Yup sorry, my fault. I didn't realize that when generating hash for a new user you are not supposed to supply a salt.

Some more info related to this in docblocks or manual would be nice. Earlier today someone in the IRC channel was struggling to setup and use the BlowfishAuthenticate class.

@sitedyno

I'll try to get manual and docblock pull requests done this weekend to make things clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment