RFC: Setup authentication through method calling (bonus fluent interface)

[inoas]

Reason

I would love to see stuff to fail early. It should be easy to know where and why. The current array configuration is maybe a bit error-prone and fails silently.

I would love to be notified when something is wrong and right now when you pass some array structure that is malformed it is silently ignored and I think it would save countless hours if you were notified.

The current array-based configuration also adds a lot of newbie questions on the social channels that could be caught early with this feature!

Suggestion

Could an interface like this work (and/or be improved)?:

class Application extends BaseApplication
{
    public function middleware($middleware)
    {
        // Instantiate the authentication service and configure authenticators
        $service = new AuthenticationService()
            ->addIdentifier('Orm'),
            // TODO handle assignments - like username, password, finders (scopes) - here
            ->addAuthenticator('Form')
            ->addAuthenticator('Session');

        // Add it to the authentication middleware
        $authentication = new AuthenticationMiddleware($service);

        // Add the middleware to the middleware stack
        $middleware->add($authentication);
    }
}

[ADmad]

As mentioned here configuring the service class using fluent interface is already possible.

[inoas]

Setting username, password fields, custom finders etc, should also be possible through a fluent interface - is it?

Could you update the example to see if it flows good?

[burzum]

@inoas please review the code before making suggestions.

Setting username, password fields, custom finders etc, should also be possible through a fluent interface - is it?

AuthenticationService::loadAuthenticator() is already doing what you propose, 2nd arg is the config. It just returns the authenticator not the current objects instance. So in theory a change would be trivial.

We could add a loadIdentifier method but this would just be a proxy to the collection object. So you can already do $service->identifiers()->load('Identifier');

What is the advantage of chaining here that would justify the change?

[inoas]

Chaining isn't the key - just would/is nice to have.

The array way of setting up component should be discouraged unless the new code throws very explicit warnings including warnings about array keys/values that were not expected.

The examples should feature the new way first, the old way last.

If the setting is done through single calls or chains matters little to me, though fluent is just nicer.

[burzum]

@inoas I've updated the documentation with the OOP style config. See the readme.md in the persistence branch: https://github.com/cakephp/authentication/tree/persistence

@markstory @ADmad can we close this ticket?

[inoas]

So what happens if someone enters this:

        $service->identifiers()->load('Authentication.Orm', [
            'fields' => [
                'username' => 'email',
                'passwort' => 'pass'
                     // ^
            ]
        ]);

Could these be method calls like setUsernameField() and setPasswordField()?

[burzum]

Nothing happens then. Unit test your login. :)

[markstory]

Adding methods to set every config option seems like it would get unwieldy at least on the surface. We could add checking for typos in configuration keys, and raise exceptions when unexpected keys are provided.

[burzum]

@markstory this becomes a ton of checks for some config arrays. Especially the nested ones like the fields.

[markstory]

Wouldn't it also be a ton of methods?

[ADmad]

It's not praction to have tons of method nor array key checks. At some point devs just have to be careful.

In AuthComponent you would end up with a dozen keys in config array making typos difficult to spot but here having separate methods for setting up identifiers and authentication means each function call would have only 2-3 keys making typos easy to spot.

[burzum]

We should remove the possibility to pass the whole config in a giant array in the constructor then? This would force people to use the two methods.

[ADmad]

I would keep it. Let people choose their preferred way.

[dereuromark]

I agree. Some might have it configured and pass the Configure::read() stuff or sth.

[burzum]

OK, so we can close this and keep it as it is because it already features both flavours?

[inoas]

@markstory wrote:

We could add checking for typos in configuration keys, and raise exceptions when unexpected keys are provided.

How about we consume each supported key (read out then unset it). If anything is left afterwards, the intention was unclear and an exception would be thrown listing (dotnotation) some path/paths could not be consumed?

That would also keep the ability to set stuff via Configure::write() as @dereuromark pointed out.

[burzum]

I still think it is kind of error prone. This is the moment things change and I'd like to see less error-prone-ness.

While I agree, just test your code. :) You should get failing tests for your login if you have such typos.

How about we read out every key then unset it. If anything is left afterwards, the intention was unclear and an exception would be thrown listing (dotnotation) the path that could not be consumed?

@inoas This has been discussed before and the idea was to have config objects. Basically something like an entity that can validate it's config and the config be read from. However, if you're interested in chasing this idea any further please open a ticket in the cakephp repository.

You might show how other frameworks do it (I don't have the time for the research) and might come up with a sample implementation? A BC compatible way would be a config object that implements ArrayAccess. So we can do checks on it while staying BC when passing it. This could be even done as a plugin. If you implement getters and setters like the entity class you can implement custom checks on each of the array values. So while calling $config->password() it would write to $config['fields']['password'].

$config = new Config($existingArray);
$config->validate(); // Throw exception if something is wrong
$config['fields']['password'] = 'foo'; // Should set password
$config->password('foo'); // Should set password, throws exception if invalid value(s) is passed
echo $config['password']; // Should return foo

Classes receiving that could check if they receive an array or config object and call validate() when it's an object. This could replace the InstanceConfigTrait as well I think.

This here should pretty much do what you want:

class Config extends ArrayObject {

	protected $_defaultConfig = [
		'defaultValue' => 'my default value'
	];

	public function __construct(array $input = []) {
		$input = array_merge($this->_defaultConfig, $input);
		parent::__construct($input, $flags = 0, $iteratorClass = 'ArrayIterator');
	}

	public function __call($func, $argv) {
		if (substr($func, 0, 6) === 'array_') {
			return call_user_func_array($func, array_merge([$this->getArrayCopy()], $argv));
		}

		if (count($argv) === 0) {
			return $this[$func];
		}

		$this[$func] = $argv[0];;
		return $this;
	}
}

Then do your checks inside your methods, you need to implement them.