zoopcommerce / commerce-user-module

A user module to handle access control, roles and user data
0 stars 0 forks source link

Auth #2

Closed joshystuart closed 10 years ago

joshystuart commented 10 years ago

See my recent commit: https://github.com/zoopcommerce/commerce-user-module/commit/3f52f950fdb97284dd8a8c217059460f6df3a56b

I'm not sure if this is the right way to go about it. I'm still a little confused as to the best way to do auth.

Currently:

What are your thoughts? Messy?

Also, for some reason, I can't return any documents in this test: http://goo.gl/6R0isd . It creates and authorizes the user fine, but it doesn't return any documents even though I can see 1 in the DB. Could this be another case of creating and fetching documents in the same request? I am sure I fixed all of that. Would appreciate some help if you get a chance.

The test might be a bit naff over all. Any suggestions on doing it better?

joshystuart commented 10 years ago

tagged: @superdweebie

superdweebie commented 10 years ago

Point 1 - that's odd. shard-module should register it here: https://github.com/zoopcommerce/shard-module/blob/master/config/module.config.php#L29. BTW, that abstract factory config is incorrect. It shouldn't have the 'user' =>. Abstract factories don't register what they return, that's what makes them abstract. Perhaps have a look at the manifest service manager when debugging, and see if the user abstract factory is registered. It seems to be working in the tests.

Point 2 - So I gather, by asking for a user, you'll cause an error if one isn't found, and halt the request? The cleaner way to do it would be like this (a bit more code, but I think clearer, and you don't have to pass through shard-module stuff when there is no need):

if ($serviceLocator->has('Zend\Authentication\AuthenticationService')) {
  $authenticationService = $serviceLocator->get('Zend\Authentication\AuthenticationService');
  if (!$authenticationService->hasIdentity() || !$authenticationService->authenticate()->isValid()) {
    //throw No valid user
  }
} else {
  //throw No AuthenticationService defined
}

Point 3 - looks fine, although I have tried to resist creating a guest user in the past, you might have good reasons.

Not sure about the problems with the test at the moment. I think I'll need to run the code locally.

joshystuart commented 10 years ago

Thanks for the feedback.

Point 1: Yeah I knew it was in the shard-model config, but for some reason if I don't explicitly define it in my config, it fails. I'll have to dig into it to see where it's being overridden.

Point 2/3: I'm not sure about throwing an error, which is why I'm returning Guest. I'm just trying to think of how to use the API for every purpose eg. add to cart, get all merchant users, get all partner stores etc. Maybe I'm confused with this?

Thanks

superdweebie commented 10 years ago

If there is a guest user, then I don't think auth should be done every request. Basically, having a guest is the same as having no authenticated user, so why have it at all? Instead only check auth when a request asks for restricted data.

joshystuart commented 10 years ago

Fair point.

Thanks

joshystuart commented 10 years ago

@superdweebie so things are moving on this now! However I have 2 questions:

  1. The reason why I wasn't seeing documents returned is that I hadn't explicitly defined the manifest here: https://github.com/zoopcommerce/commerce-user-module/blob/master/config/module.config.php#L37. Is there a way to set it "globally"?
  2. I've ran into somewhat of a circular dependency problem: I need an authorized user to get users, but I need to be able to get a user first. So I need to be able to disable access control in here: https://github.com/zoopcommerce/commerce-user-module/blob/master/src/Zoop/User/HttpBasicResolver.php#L56. I couldn't see how to do that though.

Thanks!

joshystuart commented 10 years ago

Just noticed I can do:

protected function getUserFromHttpAuth($username, $realm, $password = null)
    {
        $dm = $this->getDocumentManager();
        $filterCollection = $dm->getFilterCollection();
        $accessController = $filterCollection->getFilter('odmfilter');
         //disable shard acl
        $filterCollection->disable('odmfilter');

        $qb = $dm->createQueryBuilder(self::USER_COLLECTION);

        $qb->field('apiCredentials')->elemMatch(
            $qb->expr()->field('key')->equals($username)
                ->field('secret')->equals($password)
        );

        $user = $qb->getQuery()
            ->getSingleResult();

        $filterCollection->enable('odmfilter');

        return $user;
    }

But I don't think I'm enabling it properly again because I get an error

Argument 2 passed to Zoop\Shard\Core\ReadEventArgs::__construct() must be an instance of Doctrine\Common\EventManager, null given, called in /mnt/www/commerce-user-module/vendor/zoopcommerce/shard/lib/Zoop/Shard/ODMCore/Filter.php on line 58 and defined , class : PHPUnit_Framework_Error , file : /mnt/www/commerce-user-module/vendor/zoopcommerce/shard/lib/Zoop/Shard/Core/ReadEventArgs.php , line :48}
joshystuart commented 10 years ago

Hi @superdweebie, so I've revisited this module with the recent updates to gateway-module, however I'm still running into a few problems:

  1. I do not want the @Shard\Permission\Basic(roles="*", allow={"read", "create"}), permissions on any user documents as this will allow anyone to create and view them. note; this was the way in which gomi-module has implemented it.
  2. As a "work around" I tried to set a sys::auth user that has elevated permissions on the authenticate() call. However it does not replace the sys::auth in the ServiceManager once the correct user is found.
  3. Since I am using an AbstractUser class, I have to set the permissions on this document, however that prevents me from filtering out users based on their permissions. eg.

I have basically solved 1 and 2 by creating a new odm connection that does not have accesscontrol enabled. This allows me to do the authentication on the user collection, while still maintaining permissions on the user collection.

  1. Is this an ok approach?
  2. Is there a way we can introduce something to gateway-module to get around this issue? We could extend the DoctrineModule\Authentication\Adapter\ObjectRepositoryAdapter and turn off accesscontrol on the authenticate() function?

For point 3, what is the best way to resolve this? Can it be applied using zone filters? (note; I'm not hugely familiar with those, so it'd be great if you could point me in the right direction).

Thanks in advance!

superdweebie commented 10 years ago

@crimsonronin all good points. On a side note, do you think there are fundamental problems with the way gomi-module does things?

On to the individual issues:

     * @ODM\Collection
     * @Shard\Zones

Then add the zones extension to the shard manifest. Then when a user is authenticated, add this line of code to activate the zone filter on all subsequent db requests:

$manifest->getServiceManager()->get('extension.zone')->setReadFilterInclude(['partner', 'company']);
joshystuart commented 10 years ago

Thanks for the reply.

gomi seems fine to me, although I haven't delved too deeply.

gomi was actually the initial inspiration for the sys::auth approach. I believe that is how you do the password reset by defining a system level user that can update the password of a user.

The problem is that in order to take any action on the user collection I need to have an authorized user, which in turn needs access to the user collection. So you can see the issue there (ie circular dependency). I think we might have to disable shard's access control when retrieving the user identity. As far as I can tell though, you cannot disable access control mid request? Do you think this is the way to go?

Thanks for the zones example. I think I will need to have a chat with you regarding how much we implement for MVP. Essentially I'm trying to get all the pieces together so we have a good base to work from. But I'm trying to prevent myself from going to far down the rabbit hole before we have proven the idea with partners first.

Thoughts?

superdweebie commented 10 years ago

@crimsonronin I've thought some more about it. A sys::auth user might be good to add to the shard-module authentication service. Seems like the simplest solution. We give sys::auth read access to the user collection only, and then after authentication swap in the authenticated user (or nothing for a guest). I can make those changes in the next day or so.

superdweebie commented 10 years ago

Actually, correct that. I think the logic needs to be added to gateway-module, not shard-module

joshystuart commented 10 years ago

@superdweebie Sounds good to me. I'm not in a super rush because I have a work around (ie. extra connection), but it is much better than what I have (the reason being is that there's a lot of dependency added to the shard default connection which needs to be included in the authuser connection).

superdweebie commented 10 years ago

All done gateway-module 2.1.0.

joshystuart commented 10 years ago

w00t!

Thanks