How to improve the architecture and remove duplication?

Question

symfony

In short, there are users, they have roles and they are tied to some kind of workspace. Based on these two conditions, certain actions must be carried out. The first version of the controller

<?php

namespace App\Controller;

use App\Entity\User;
use App\Service\UserService;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Security\Core\Security;
use Twig\Environment;

class UserController
{
    private $userService;

    /**
     * @var Environment
     */
    private $twig;

    /**
     * @var AuthorizationCheckerInterface
     */
    private $authorizationChecker;
    /**
     * @var Security
     */
    private $security;

    public function __construct(
        UserService $userService,
        Environment $twig,
        AuthorizationCheckerInterface $authorizationChecker,
        Security $security
    ) {
        $this->userService = $userService;
        $this->twig = $twig;
        $this->authorizationChecker = $authorizationChecker;
        $this->security = $security;
    }

    public function index(): Response
    {
        if (
            $this->authorizationChecker
                ->isGranted(User::ROLE_SUPER_ADMIN)
        ) {
            $users = $this->userService->getAll();

            $content = $this->twig->render('admin/users.html.twig', [
                'users' => $users
            ]);

            return new Response($content);
        }

        if (
            $this->authorizationChecker
                ->isGranted(User::ROLE_CUSTOMER_ADMIN)
        ) {
            /** @var User $user */
            $user = $this->security->getUser();

            $users = $this
                ->userService
                ->findByRoleAndWorkspaceId(
                    User::ROLE_TEAM_MEMBER,
                    $user->getWorkspace()->getId()
                );

            $content = $this->twig->render('user/users.html.twig', [
                'users' => $users
            ]);

            return new Response($content);
        }

        throw new AccessDeniedException();
    }

    public function read(string $id): Response
    {
        if (
            $this->authorizationChecker
                ->isGranted(User::ROLE_SUPER_ADMIN)
        ) {
            $user = $this->userService->getById($id);

            $content = $this->twig->render('admin/user.html.twig', [
                'user' => $user
            ]);

            return new Response($content);
        }

        if (
            $this->authorizationChecker
                ->isGranted(User::ROLE_CUSTOMER_ADMIN)
        ) {
            $user = $this->userService->getByIdAndWorkspaceId($id);

            $content = $this->twig->render('user/user.html.twig', [
                'user' => $user
            ]);

            return new Response($content);
        }

        throw new AccessDeniedException();
    }
}

Other crud methods will be added and they will also have a check similar to this one and calls to different methods from the service.
I'm not sure if this solution is correct.
I'm considering moving the user role check to a service and calling the appropriate method, and the controller methods will look something like this

public function index(): Response
    {
        $users = $this->userService->getAll();

        if (
            $this->authorizationChecker
                ->isGranted(User::ROLE_SUPER_ADMIN)
        ) {
            $content = $this->twig->render('admin/users.html.twig', [
                'users' => $users
            ]);

            return new Response($content);
        }

        if (
            $this->authorizationChecker
                ->isGranted(User::ROLE_CUSTOMER_ADMIN)
        ) {
            $content = $this->twig->render('user/users.html.twig', [
                'users' => $users
            ]);

            return new Response($content);
        }

        throw new AccessDeniedException();
    }

Which option is preferable and what problems might arise?

Answer 1

I would do so. Moreover, if more methods are added there, then you will drown in ifs with your approach

<?php

namespace App\Controller;

use App\Entity\User;
use App\Service\UserService;
use Symfony\Component\DependencyInjection\ServiceLocator;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Security\Core\Security;
use Twig\Environment;

interface AdminControllerInterface
{
    public function index(): Response;

    public function read(string $id): Response;
}

class UserController implements AdminControllerInterface
{
    /**
     * Security - объект-helper, он содержит в себе AuthorizationCheckerInterface
     *
     * @var Security
     */
    private $security;

    /**
     * @var ServiceLocator
     */
    private $controllerLocators;

    public function __construct(Security $security, ServiceLocator $controllerLocators)
    {
        $this->security = $security;
        $this->controllerLocators = $controllerLocators;
    }

    public function index(): Response
    {
        $role = $this->getRole();

        if (!$this->controllerLocators->has($role)) {
            throw new AccessDeniedException();
        }

        /** @var AdminControllerInterface $controller */
        $controller = $this->controllerLocators->get($role);

        return $controller->index();
    }

    public function read(string $id): Response
    {
        $role = $this->getRole();

        if (!$this->controllerLocators->has($role)) {
            throw new AccessDeniedException();
        }

        /** @var AdminControllerInterface $controller */
        $controller = $this->controllerLocators->get($role);

        return $controller->read($id);
    }

    private function getRole(): string
    {
        // Для простоты представим, что роль у пользователя одна. Иначе искать среди ролей
        // ROLE_SUPER_ADMIN и ROLE_CUSTOMER_ADMIN и возвращаем с соответствующим приоритетом
        return current($this->security->getToken()->getRoles());
    }
}

class SuperAdminController implements AdminControllerInterface
{
    private $userService;

    /**
     * @var Environment
     */
    private $twig;

    public function __construct(
        UserService $userService,
        Environment $twig
    ) {
        $this->userService = $userService;
        $this->twig = $twig;
    }

    public function index(): Response
    {
        $users = $this->userService->getAll();

        return new Response($this->twig->render('admin/users.html.twig', [
            'users' => $users
        ]));
    }

    public function read(string $id): Response
    {
        $user = $this->userService->getById($id);

        return new Response($this->twig->render('admin/user.html.twig', [
            'user' => $user
        ]));
    }
}

class CustomerAdminController implements AdminControllerInterface
{
    private $userService;

    /**
     * @var Security
     */
    private $security;

    /**
     * @var Environment
     */
    private $twig;

    public function __construct(
        UserService $userService,
        Environment $twig
    ) {
        $this->userService = $userService;
        $this->twig = $twig;
    }

    public function index(): Response
    {
        /** @var User $user */
        $user = $this->security->getUser();

        $users = $this->userService->findByRoleAndWorkspaceId(
            User::ROLE_TEAM_MEMBER,
            $user->getWorkspace()->getId()
        );

        return new Response($this->twig->render('user/users.html.twig', [
            'users' => $users
        ]));
    }

    public function read(string $id): Response
    {
        $user = $this->userService->getByIdAndWorkspaceId($id);

        return new Response($this->twig->render('user/user.html.twig', [
            'user' => $user
        ]));
    }
}

app.admin_controller_locator:
    class: Symfony\Component\DependencyInjection\ServiceLocator
    arguments:
        -
            ROLE_SUPER_ADMIN: '@App\Controller\SuperAdminController'
            ROLE_CUSTOMER_ADMIN: '@App\Controller\CustomerAdminController'
    tags: ['container.service_locator']