cakephp / authorization

PSR7 Middleware for authorization
MIT License
76 stars 47 forks source link

Feature request: Make Authorization service available via DI in the Middleware #265

Closed jamisonbryant closed 8 months ago

jamisonbryant commented 1 year ago

Per a recent community discussion it would be helpful for some apps to be able to obtain an instance of AuthorizationServiceInterface and/or the currently configured AuthorizationService from the service container.

Applications that rely heavily on the service container to build composable and reusable services often have to remain aware of Authorization concerns but do not want to rely on ServerRequest directly if avoidable. For example, a userland Searcher service class needs to scope a Query to members-only but cannot directly invoke the AuthorizationService without first obtaining it from a request attribute.

If the service were available via DI, that same Searcher class could list it as a dependency when configuring the application service container and then call it directly from within the service. This provides greater type safety and less (direct) reliance on the HTTP request object.

markstory commented 1 year ago

I agree that having the AuthorizationService & AuthenticationServices added to the applications DI container by the plugins makes sense.

jamisonbryant commented 10 months ago

One issue with the current implementation is that it assumes that the object providing the auth services is the same object providing the container, or at least that they have the same interfaces. This requires a bit of an awkward coupling/passing around of objects in our code as this is explicitly not the case for us.

The docs say to attach the AuthorizationServiceProviderInterface to the Application class and then to register the middleware by calling ->add(new AuthorizationMiddleware($this)). Well, we don't do that. Instead, we have a separate class entirely that implements the AuthorizationServiceProviderInterface and then we add the middleware by passing new CustomAuthorizationServiceProvider() instead of $this. We feel this is better separation of concerns.

But, since the AuthorizationMiddleware is checking for the ContainerApplicationInterface this means that we must implement this interface on our custom class in order to have the authz service available in DI. This further requires us to pass the container from the application through to the custom authz service provider just so the middleware can get it.

Here's what I mean:

/* file: src/Application.php */

// in the middleware() method
->add(new AuthenticationMiddleware(
  // pass the DI container to the custom service provider so middleware can get to it
  new CustomAuthenticationServiceProvider($this->getContainer())
))

/* file: src/Service/Provider/CustomAuthorizationServiceProvider.php */

public function __construct(ContainerInterface $container)
{
  // this property and constructor exist solely so the AuthzMiddleware can access the DI container
  $this->appContainer = $container;
}

public function services(ContainerInterface $container): void
{
  // this method is required by the ContainerApplicationInterface
  // it is completely useless because this class does not "provide" any services in the traditional DI sense
}

public function getContainer(): \Cake\Core\ContainerInterface
{
  // this method is required by the ContainerApplicationInterface
  // AuthzMiddleware is the only thing that calls this method
  return $this->appContainer;
}

I think the check for instanceof ContainerApplicationInterface is perhaps making too many assumptions about userland code, but I (sheepishly) cannot think of a more suitable alternative.

~Perhaps checking for instanceof ServiceProvider (the class that provides getContainer()) or even method_exists()? But that is definitely much less elegant than checking for the interface.~ (on second thought, none of these solve the problem, only mutate it)

Some more ideas: Could a parameter be added to AuthzMiddleware to allow you to pass in the container directly? Or could we use instance config? I think the latter might work, but would not be type-safe. Or is there an event we could hook into?

Basically, we currently have to pass the DI container object between two classes in order to get it in the right place for the middleware to be able to inject the services, and it feels like we should not have to do that. If we're doing things wrong on our end, advice is very welcome.

dereuromark commented 8 months ago

PR open: https://github.com/cakephp/authorization/pull/284 please approve.