Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add authentication storage factory #95

Merged
merged 2 commits into from Oct 4, 2012

Conversation

Projects
None yet
3 participants
Contributor

superdweebie commented Sep 30, 2012

An attempt to make creating an AuthenticationService a one liner, whilst still providing lots of configuration flexibility. AuthenticationService can now be created with:

$authService = new AuthenticationService($serviceLocator->get('doctrine.authenticationstorage.odm_default'), $serviceLocator->get('doctrine.authenticationadapter.odm_default'))

In doing this a few more options have been added to thee Authentication options class. The options class has also been renamed from AuthenticationAdapter to Authentication. Furthermore, the construction signature of Storage\ObjectRepository has changed. I think these changes bring more power, flexibility and consistency. However, they are BC breaks, so please discuss before commit.

Tests for the adapter factory and the storage factory have been added to the odm module.

@superdweebie superdweebie referenced this pull request in doctrine/DoctrineMongoODMModule Sep 30, 2012

Merged

add authentication storage factory config and tests #41

Member

bakura10 commented Sep 30, 2012

Perfect, this one was missing :). However I'm not sure you need to store metadata in the Authentication options, as you can retrieve it from the ObjectManager !

@Ocramius Ocramius commented on an outdated diff Sep 30, 2012

...ineModule/Authentication/Storage/ObjectRepository.php
- /**
- * Metadata factory
- *
- * @var ClassMetadataFactory
- */
- protected $metadataFactory;
+ protected $options;
@Ocramius

Ocramius Sep 30, 2012

Owner

Type hinting please :)

@Ocramius Ocramius commented on an outdated diff Sep 30, 2012

...eTest/Authentication/Storage/ObjectRepositoryTest.php
@@ -60,7 +60,13 @@ public function testCanRetrieveEntityFromObjectRepositoryStorage()
->with($this->equalTo('DoctrineModuleTest\Authentication\Adapter\TestAsset\IdentityObject'))
->will($this->returnValue($metadata));
- $storage = new ObjectRepositoryStorage($objectRepository, $metadataFactory, new SessionStorage());
+
+ $storage = new ObjectRepositoryStorage();
+ $storage->setOptions(array(
+ 'objectRepository' => $objectRepository,
+ 'classMetadataFactory' => $metadataFactory,
+ 'storage' => new SessionStorage()
@Ocramius

Ocramius Sep 30, 2012

Owner

Careful, you should use https://github.com/zendframework/zf2/blob/master/library/Zend/Authentication/Storage/NonPersistent.php instead. Otherwise you're polluting the test environment :)

@Ocramius Ocramius commented on an outdated diff Sep 30, 2012

...eTest/Authentication/Storage/ObjectRepositoryTest.php
@@ -60,7 +60,13 @@ public function testCanRetrieveEntityFromObjectRepositoryStorage()
->with($this->equalTo('DoctrineModuleTest\Authentication\Adapter\TestAsset\IdentityObject'))
->will($this->returnValue($metadata));
- $storage = new ObjectRepositoryStorage($objectRepository, $metadataFactory, new SessionStorage());
+
+ $storage = new ObjectRepositoryStorage();
+ $storage->setOptions(array(
@Ocramius

Ocramius Sep 30, 2012

Owner

Can you use directly the constructor?

Owner

Ocramius commented Sep 30, 2012

Nice additions! I'd anyway add a test for the newly introduced factory class (you can check DoctrineModuleTest\Service\EventManagerFactoryTest for that) As the complexity is really low it should just be 2 tests.

Member

bakura10 commented Sep 30, 2012

I just realized that DoctrineORMModule did not have any key in Module.php. Please could you add

doctrine.authenticationadapter.orm_default
doctrine.authenticationstorage.orm_default

in DoctrineORMModule too ?

Member

bakura10 commented Oct 1, 2012

Ok to merge this one as soon as he added the keys for DoctrineORMModule.

Contributor

superdweebie commented Oct 1, 2012

@bakura10 I'll add the keys to DoctrineORMModule when I've finished with this one, and we are all happy.

Also, following up your suggestion of removing the ClassMetadataFactory from options, I've changed it to ClassMetadat, but kept it, so that the option of providing a ObjectRepository, and not an ObjectManager is still possible.

Contributor

superdweebie commented Oct 1, 2012

@bakura10 and @Ocramius please review, and then I think it's ready for merge. When you think it's ready, I'll add a PR to DoctrineORMModule.

Authentication service can be grabbed with $serviceLocator->get('doctrine.authenticationservice.odm_default'). The authentication service is extended with the convienice methods login and logout.

Storage has been given one extra method readKeyOnly which will grab the object key stored in session without the db call - may be a helpful effiecency in some instances. It is also supported by the extension method getIdentityKey on the new authentication service.

Controller plugin and view helper can both be used with the simplicity of $this->identity() or $this->identity->getIdentityKey()

@Ocramius Ocramius and 2 others commented on an outdated diff Oct 2, 2012

...ctrineModule/Authentication/AuthenticationService.php
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace DoctrineModule\Authentication;
+
+use Zend\Authentication\AuthenticationService as ZendAuthenticationService;
+
+/**
+ * Authentication service that adds login and logout methods.
+ *
+ * @license MIT
+ * @link http://www.doctrine-project.org/
+ * @since 0.5.0
+ * @author Tim Roediger <superdweebie@gmail.com>
+ */
+class AuthenticationService extends ZendAuthenticationService
@Ocramius

Ocramius Oct 2, 2012

Owner

Don't think we need this class, nor shouldn't provide it...
The way ZF does it has always been

  1. get the adapter
  2. set credentials into adapter
  3. set adapter in auth service (if it hasn't been fetched from the auth service itself)
  4. authenticate

(see my example on the auth docs)

@bakura10

bakura10 Oct 2, 2012

Member

Agree. This is weird.

Envoyé de mon iPhone

Le 2 oct. 2012 à 11:46, Marco Pivetta notifications@github.com a écrit :

In src/DoctrineModule/Authentication/AuthenticationService.php:

get the adapter
set credentials into adapter
set adapter in auth service (if it hasn't been fetched from the auth service itself)
authenticate
(see my example on the auth docs)


Reply to this email directly or view it on GitHub.

@superdweebie

superdweebie Oct 2, 2012

Contributor

I'm happy if you say that this is outside the scope of DoctrineModule. However, I think the code has it's place, even if not in DoctrineModule. Why should every dev write the same lines of code when most of them just want to create a function login($username, $password)?

@Ocramius

Ocramius Oct 2, 2012

Owner

Because an authentication adapter does not assume anything about identity value or credential value. It may be an adapter using the current OS user running the PHP process (for example)...

@bakura10

bakura10 Oct 2, 2012

Member

Just add a login function to the base authentication service class.

Envoyé de mon iPhone

Le 2 oct. 2012 à 12:52, Tim Roediger notifications@github.com a écrit :

In src/DoctrineModule/Authentication/AuthenticationService.php:

  • * http://www.doctrine-project.org.
  • /
    +
    +namespace DoctrineModule\Authentication;
    +
    +use Zend\Authentication\AuthenticationService as ZendAuthenticationService;
    +
    +/
    *
  • * Authentication service that adds login and logout methods.
  • * @license MIT
  • * @link http://www.doctrine-project.org/
  • * @SInCE 0.5.0
  • * @author Tim Roediger superdweebie@gmail.com
  • */
    +class AuthenticationService extends ZendAuthenticationService
    I'm happy if you say that this is outside the scope of DoctrineModule. However, I think the code has it's place, even if not in DoctrineModule. Why should every dev write the same lines of code when most of them just want to create a function login($username, $password)?


Reply to this email directly or view it on GitHub.

@Ocramius

Ocramius Oct 2, 2012

Owner

@bakura10 nope, that won't be accepted in ZF2 imo. The credential concept is something specific to the auth adapter, not the service itself.

@Ocramius Ocramius and 1 other commented on an outdated diff Oct 2, 2012

...eModule/Authentication/Controller/Plugin/Identity.php
+ */
+
+namespace DoctrineModule\Authentication\Controller\Plugin;
+
+use DoctrineModule\Authentication\AuthenticationService;
+use Zend\Mvc\Controller\Plugin\AbstractPlugin;
+
+/**
+ * Controller plugin to fetch the authenticated identity.
+ *
+ * @license MIT
+ * @link http://www.doctrine-project.org/
+ * @since 0.5.0
+ * @author Tim Roediger <superdweebie@gmail.com>
+ */
+class Identity extends AbstractPlugin
@Ocramius

Ocramius Oct 2, 2012

Owner

Same here... Even if I love the idea, I think it is not DoctrineModule's scope. ZfcUser, which aims at providing auth utilities, has it.

This may instead be suggested for ZF2 itself...

@superdweebie

superdweebie Oct 2, 2012

Contributor

Ok, will remove.

@Ocramius Ocramius and 1 other commented on an outdated diff Oct 2, 2012

...octrineModule/Authentication/View/Helper/Identity.php
+ */
+
+namespace DoctrineModule\Authentication\View\Helper;
+
+use DoctrineModule\Authentication\AuthenticationService;
+use Zend\View\Helper\AbstractHelper;
+
+/**
+ * View helper plugin to fetch the authenticated identity.
+ *
+ * @license MIT
+ * @link http://www.doctrine-project.org/
+ * @since 0.5.0
+ * @author Tim Roediger <superdweebie@gmail.com>
+ */
+class Identity extends AbstractHelper
@Ocramius

Ocramius Oct 2, 2012

Owner

Same comments as those for the controller plugin

@superdweebie

superdweebie Oct 2, 2012

Contributor

Ok, will remove.

@Ocramius Ocramius and 1 other commented on an outdated diff Oct 2, 2012

...trineModule/Service/Authentication/ServiceFactory.php
+ */
+class ServiceFactory extends AbstractFactory
+{
+ /**
+ *
+ * @param \Zend\ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @return \Zend\Authentication\AuthenticationService
+ */
+ public function createService(ServiceLocatorInterface $serviceLocator)
+ {
+ $options = $this->getOptions($serviceLocator, 'authentication');
+ if (is_string($options->getObjectManager())) {
+ $options->setObjectManager($serviceLocator->get($options->getObjectManager()));
+ }
+
+ if ($options->getStoreOnlyKeys()){
@Ocramius

Ocramius Oct 2, 2012

Owner

I think storeOnlyKeys needs a better naming. Actually, I'm quite against this one, the user should instead override the factory for such a particular use case imo...
He can still use our auth adapter and then his own storage produced by his factory... I'd completely skip this part if possible then...

@superdweebie

superdweebie Oct 2, 2012

Contributor

If the whole AuthenticationService goes, this is a moot point.

@Ocramius Ocramius and 1 other commented on an outdated diff Oct 2, 2012

...le/Service/Authentication/ControllerPluginFactory.php
+ */
+namespace DoctrineModule\Service\Authentication;
+
+use DoctrineModule\Authentication\Controller\Plugin\Identity;
+use DoctrineModule\Service\AbstractFactory;
+use Zend\ServiceManager\ServiceLocatorInterface;
+
+/**
+ * Factory to create controller plugin that can fetch the authenticated identity.
+ *
+ * @license MIT
+ * @link http://www.doctrine-project.org/
+ * @since 0.1.0
+ * @author Tim Roediger <superdweebie@gmail.com>
+ */
+class ControllerPluginFactory extends AbstractFactory
@Ocramius

Ocramius Oct 2, 2012

Owner

Same comments as those for the controller plugin

@superdweebie

superdweebie Oct 2, 2012

Contributor

It's going

@Ocramius Ocramius commented on an outdated diff Oct 2, 2012

...neModule/Service/Authentication/ViewHelperFactory.php
+ */
+namespace DoctrineModule\Service\Authentication;
+
+use DoctrineModule\Authentication\View\Helper\Identity;
+use DoctrineModule\Service\AbstractFactory;
+use Zend\ServiceManager\ServiceLocatorInterface;
+
+/**
+ * Factory to create view helper that can fetch the authenticated identity.
+ *
+ * @license MIT
+ * @link http://www.doctrine-project.org/
+ * @since 0.1.0
+ * @author Tim Roediger <superdweebie@gmail.com>
+ */
+class ViewHelperFactory extends AbstractFactory
@Ocramius

Ocramius Oct 2, 2012

Owner

Same comments as those for the controller plugin

@Ocramius Ocramius and 1 other commented on an outdated diff Oct 2, 2012

...trineModule/Service/Authentication/ServiceFactory.php
+
+use DoctrineModule\Authentication\AuthenticationService;
+use DoctrineModule\Authentication\Adapter\ObjectRepository as Adapter;
+use DoctrineModule\Authentication\Storage\ObjectRepository as Storage;
+use DoctrineModule\Service\AbstractFactory;
+use Zend\ServiceManager\ServiceLocatorInterface;
+
+/**
+ * Factory to create authentication service object.
+ *
+ * @license MIT
+ * @link http://www.doctrine-project.org/
+ * @since 0.1.0
+ * @author Tim Roediger <superdweebie@gmail.com>
+ */
+class ServiceFactory extends AbstractFactory
@Ocramius

Ocramius Oct 2, 2012

Owner

ServiceFactory -> AuthenticationServiceFactory ?

@superdweebie

superdweebie Oct 2, 2012

Contributor

Will fix if AuthenticationService is staying.

@Ocramius

Ocramius Oct 2, 2012

Owner

Imo, AuthenticationService (your implementation) should be left out. Instead, we can just create an instance of Zend\Authentication\AuthenticationService

@Ocramius Ocramius commented on an outdated diff Oct 2, 2012

...trineModule/Service/Authentication/ServiceFactory.php
+class ServiceFactory extends AbstractFactory
+{
+ /**
+ *
+ * @param \Zend\ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @return \Zend\Authentication\AuthenticationService
+ */
+ public function createService(ServiceLocatorInterface $serviceLocator)
+ {
+ $options = $this->getOptions($serviceLocator, 'authentication');
+ if (is_string($options->getObjectManager())) {
+ $options->setObjectManager($serviceLocator->get($options->getObjectManager()));
+ }
+
+ if ($options->getStoreOnlyKeys()){
+ $storage = new Storage($options);
@Ocramius

Ocramius Oct 2, 2012

Owner

Reuse our storage factory?

@Ocramius Ocramius commented on an outdated diff Oct 2, 2012

...trineModule/Service/Authentication/ServiceFactory.php
+ * @return \Zend\Authentication\AuthenticationService
+ */
+ public function createService(ServiceLocatorInterface $serviceLocator)
+ {
+ $options = $this->getOptions($serviceLocator, 'authentication');
+ if (is_string($options->getObjectManager())) {
+ $options->setObjectManager($serviceLocator->get($options->getObjectManager()));
+ }
+
+ if ($options->getStoreOnlyKeys()){
+ $storage = new Storage($options);
+ } else {
+ $storage = $options->getStorage();
+ }
+
+ return new AuthenticationService($storage, new Adapter($options));
@Ocramius

Ocramius Oct 2, 2012

Owner

Reuse the adapter factory?

@Ocramius Ocramius and 1 other commented on an outdated diff Oct 2, 2012

tests/debug-tests.sh
@@ -0,0 +1,2 @@
+export XDEBUG_CONFIG="idekey=netbeans-xdebug";
@Ocramius

Ocramius Oct 2, 2012

Owner

Is this something coming from your IDE?

@superdweebie

superdweebie Oct 2, 2012

Contributor

Yep. Oops.

Owner

Ocramius commented Oct 2, 2012

Looks good @superdweebie, but I'd

  1. move the view helper and the controller plugin to ZF2 itself. I personally wouldn't want them in DoctrineModule, it's not what we should handle in here. Code is fine and imo would be accepted by @weierophinney
  2. avoid instantiating storage and adapter in the authentication service factory. Instead, the authentication factory can get the adapter and the storage from the service manager. This also removes the need to use storeOnlyKeys, since the end user has full power by just overriding the storage factory (example).
  3. I'd probably go explicit with testing. Given (1) and (2) are addressed, we have only 3 factories to test: authentication, storage and adapter. We can simply instantiate a ServiceManager, fill it with the required services each time and then verify that the correct instances are returned (happens in a base test case right now, but I personally find it difficult to follow, and since 2 of the factory tests that use it will be dropped...)
Member

bakura10 commented Oct 2, 2012

I'm on par with Oceanus about everything he said.

Contributor

superdweebie commented Oct 2, 2012

@Ocramius

  1. That would be cool. Is it significant enough to require a RFC?
  2. Ok.
  3. So you mean get rid of tests/DoctrineModuleTest/Service/Authentication/FactoryBaseTestCase.php and repeat the code in each test for clarity?
Contributor

superdweebie commented Oct 2, 2012

Also, I gather you guys want to keep the AuthenticationService and associated factory, but loose the login and logout methods?

Owner

Ocramius commented Oct 2, 2012

  1. @superdweebie you already got the code imo :) Just PR it to ZF2
  2. perfect
  3. Yes, exactly, since in two cases you need (different) configs set to $serviceManager->set('Config', array(...)), while the third just needs two mock services for auth and storage adapter.

About the AuthenticationService (class, not factory), I'd drop it entirely, keeping the factory and using ZF2's AuthenticationService as returned instance

Member

bakura10 commented Oct 2, 2012

I'll make a last review once everything has been changed.

Contributor

superdweebie commented Oct 3, 2012

View helper and controller plugin moved to zendframework/zf2#2650

Owner

Ocramius commented Oct 3, 2012

@superdweebie awesome :)

Owner

Ocramius commented Oct 3, 2012

Looks really good @superdweebie :)

Weird that github didn't reference doctrine/DoctrineORMModule#132 and doctrine/DoctrineMongoODMModule#41 automatically.

Can you take a look at the docs for it or shall I rebase this for you first? From the graph I'd say your branch doesn't have the docs

Contributor

superdweebie commented Oct 3, 2012

@Ocramius rebase done. Docs updated. (Note that the docs assume using zf2 develop branch, not master)

Owner

Ocramius commented Oct 4, 2012

Awesome :) Merging!

Ocramius added a commit that referenced this pull request Oct 4, 2012

Merge pull request #95 from superdweebie/storageFactory
Add authentication storage factory

@Ocramius Ocramius merged commit 185aa3b into doctrine:master Oct 4, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment