Test suite/cleanup #53

Merged
merged 9 commits into from Jan 10, 2013

Conversation

Projects
None yet
3 participants
Collaborator

Ocramius commented Dec 19, 2012

Since I'm working with ACLs for the rest of the week, I took myself some time to help in cleaning up BjyAuthorize :)

I'm still working on this, but for now I'll open the PR to allow any reviews.

Planned:

  1. Support for generic ObjectManagers/ObjectRepositories (doctrine) instead of usage of a raw connection
  2. detailed integration tests on various configurations
  3. Dummy auth-based role provider (a simple 'guest' vs 'user' switcher, which is a fairly common use case)
  4. Eventually add integration with ZDT
Owner

bjyoungblood commented Dec 20, 2012

Wow. Looks excellent so far. :)

Collaborator

Ocramius commented Dec 20, 2012

@bjyoungblood glad you like it =)

Collaborator

Ocramius commented Jan 3, 2013

@bjyoungblood ok, this is ready for merge I guess. I won't have any more time to get back to this soon, so I'll ask for it now.

Thing to check here is just one, which is not really clear to me. Commenting on code to ask for feedback

- $actionResource = sprintf('controller/%s:%s', $controller, $action);
+ $authorized = $service->isAllowed($this->getResourceName($controller))
+ || $service->isAllowed($this->getResourceName($controller, $action))
+ || ($method && $service->isAllowed($this->getResourceName($controller, $method)));
@Ocramius

Ocramius Jan 3, 2013

Collaborator

As you can see, I use the HTTP method and the action in the same location here. Shall we define different resource names for those? This was basically because I was unable to limit certain interactions with my rest controllers otherwise...

Collaborator

Ocramius commented Jan 3, 2013

Oh, also... please release 2 stable tags for this lib... one before merging this one and one after... That would be GREAT! =)

Collaborator

Ocramius commented Jan 9, 2013

bjyoungblood added a commit that referenced this pull request Jan 10, 2013

@bjyoungblood bjyoungblood merged commit d79845e into bjyoungblood:master Jan 10, 2013

Owner

bjyoungblood commented Jan 10, 2013

Sorry for the delay -- been busy the past couple weeks. Everything looks great :)

Collaborator

Ocramius commented Jan 10, 2013

@bjyoungblood awesome! I'll keep adding tests if I get back at the ACLs of my app ;)

You mean

return new Provider\Role\Doctrine(array(), $serviceLocator->get('doctrine.entitymanager.orm_default'));

I guess?

Collaborator

Ocramius replied Jan 15, 2013

Ouch! Will fix

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