Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix | Fixed bundle controllers to not depend on container if possible #1689

Merged
merged 2 commits into from
Jul 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions Controller/ConnectController.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,36 @@ final class ConnectController extends AbstractController
*/
private $requestStack;

private $enableConnect;
private $grantRule;
private $failedUseReferer;
private $failedAuthPath;
private $enableConnectConfirmation;
private $firewallNames;
stloyd marked this conversation as resolved.
Show resolved Hide resolved

/**
* @param string[] $firewallNames
*/
public function __construct(
OAuthUtils $oauthUtils,
ResourceOwnerMapLocator $resourceOwnerMapLocator,
RequestStack $requestStack
RequestStack $requestStack,
bool $enableConnect,
string $grantRule,
bool $failedUseReferer,
string $failedAuthPath,
bool $enableConnectConfirmation,
array $firewallNames
) {
$this->oauthUtils = $oauthUtils;
$this->resourceOwnerMapLocator = $resourceOwnerMapLocator;
$this->requestStack = $requestStack;
$this->enableConnect = $enableConnect;
$this->grantRule = $grantRule;
$this->failedUseReferer = $failedUseReferer;
$this->failedAuthPath = $failedAuthPath;
$this->enableConnectConfirmation = $enableConnectConfirmation;
$this->firewallNames = $firewallNames;
}

/**
Expand All @@ -77,12 +99,11 @@ public function __construct(
*/
public function registrationAction(Request $request, string $key): Response
{
$connect = $this->getParameter('hwi_oauth.connect');
if (!$connect) {
if (!$this->enableConnect) {
throw new NotFoundHttpException();
}

$hasUser = $this->isGranted($this->getParameter('hwi_oauth.grant_rule'));
$hasUser = $this->isGranted($this->grantRule);
if ($hasUser) {
throw new AccessDeniedException('Cannot connect already registered account.');
}
Expand Down Expand Up @@ -165,12 +186,11 @@ public function registrationAction(Request $request, string $key): Response
*/
public function connectServiceAction(Request $request, string $service): Response
{
$connect = $this->getParameter('hwi_oauth.connect');
if (!$connect) {
if (!$this->enableConnect) {
throw new NotFoundHttpException();
}

$hasUser = $this->isGranted($this->getParameter('hwi_oauth.grant_rule'));
$hasUser = $this->isGranted($this->grantRule);
if (!$hasUser) {
throw new AccessDeniedException('Cannot connect an account.');
}
Expand Down Expand Up @@ -202,15 +222,15 @@ public function connectServiceAction(Request $request, string $service): Respons

// Redirect to the login path if the token is empty (Eg. User cancelled auth)
if (null === $accessToken) {
if ($this->getParameter('hwi_oauth.failed_use_referer') && $targetPath = $this->getTargetPath($session)) {
if ($this->failedUseReferer && $targetPath = $this->getTargetPath($session)) {
return $this->redirect($targetPath);
}

return $this->redirectToRoute($this->getParameter('hwi_oauth.failed_auth_path'));
return $this->redirectToRoute($this->failedAuthPath);
}

// Show confirmation page?
if (!$this->getParameter('hwi_oauth.connect.confirmation')) {
if (!$this->enableConnectConfirmation) {
return $this->getConfirmationResponse($request, $accessToken, $service);
}

Expand Down Expand Up @@ -247,7 +267,7 @@ public function connectServiceAction(Request $request, string $service): Respons
*/
private function getResourceOwnerByName(string $name): ResourceOwnerInterface
{
foreach ($this->getParameter('hwi_oauth.firewall_names') as $firewall) {
foreach ($this->firewallNames as $firewall) {
if (!$this->resourceOwnerMapLocator->has($firewall)) {
continue;
}
Expand Down Expand Up @@ -299,7 +319,7 @@ private function getTargetPath(?SessionInterface $session): ?string
return null;
}

foreach ($this->getParameter('hwi_oauth.firewall_names') as $providerKey) {
foreach ($this->firewallNames as $providerKey) {
$sessionKey = '_security.'.$providerKey.'.target_path';
if ($session->has($sessionKey)) {
return $session->get($sessionKey);
Expand Down
17 changes: 12 additions & 5 deletions Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
namespace HWI\Bundle\OAuthBundle\Controller;

use HWI\Bundle\OAuthBundle\Security\Core\Exception\AccountNotLinkedException;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
Expand All @@ -23,11 +22,12 @@
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
use Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException;
use Symfony\Component\Security\Http\Authentication\AuthenticationUtils;
use Twig\Environment;

/**
* @author Alexander <iam.asm89@gmail.com>
*/
final class LoginController extends AbstractController
final class LoginController
{
/**
* @var bool
Expand Down Expand Up @@ -59,18 +59,25 @@ final class LoginController extends AbstractController
*/
private $requestStack;

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

public function __construct(
AuthenticationUtils $authenticationUtils,
RouterInterface $router,
AuthorizationCheckerInterface $authorizationChecker,
RequestStack $requestStack,
Environment $twig,
bool $connect,
string $grantRule
) {
$this->authenticationUtils = $authenticationUtils;
$this->router = $router;
$this->authorizationChecker = $authorizationChecker;
$this->requestStack = $requestStack;
$this->twig = $twig;
$this->connect = $connect;
$this->grantRule = $grantRule;
}
Expand Down Expand Up @@ -110,9 +117,9 @@ public function connectAction(Request $request): Response
$error = $error->getMessageKey();
}

return $this->render('@HWIOAuth/Connect/login.html.twig', [
'error' => $error,
]);
return new Response(
$this->twig->render('@HWIOAuth/Connect/login.html.twig', ['error' => $error])
);
}

private function getSession(): ?SessionInterface
Expand Down
16 changes: 8 additions & 8 deletions Resources/config/controller.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,28 @@
<argument type="service" id="hwi_oauth.security.oauth_utils" />
<argument type="service" id="hwi_oauth.resource_ownermap_locator" />
<argument type="service" id="request_stack" />
<argument key="$enableConnect">%hwi_oauth.connect%</argument>
<argument key="$grantRule">%hwi_oauth.grant_rule%</argument>
<argument key="$failedUseReferer">%hwi_oauth.failed_use_referer%</argument>
<argument key="$failedAuthPath">%hwi_oauth.failed_auth_path%</argument>
<argument key="$enableConnectConfirmation">%hwi_oauth.connect.confirmation%</argument>
<argument key="$firewallNames">%hwi_oauth.firewall_names%</argument>

<call method="setContainer">
<argument type="service" id="service_container" />
</call>

<tag name="controller.service_arguments" />
<tag name="container.service_subscriber" />
</service>

<service id="HWI\Bundle\OAuthBundle\Controller\LoginController" public="true">
<argument type="service" id="security.authentication_utils" />
<argument type="service" id="router" />
<argument type="service" id="security.authorization_checker" />
<argument type="service" id="request_stack" />
<argument type="service" id="twig" />
<argument>%hwi_oauth.connect%</argument>
<argument>%hwi_oauth.grant_rule%</argument>

<call method="setContainer" />

<tag name="container.service_subscriber" />
<tag name="controller.service_arguments" />
</service>

<service id="HWI\Bundle\OAuthBundle\Controller\RedirectToServiceController" public="true">
Expand All @@ -38,8 +40,6 @@
<argument>%hwi_oauth.target_path_parameter%</argument>
<argument>%hwi_oauth.failed_use_referer%</argument>
<argument>%hwi_oauth.use_referer%</argument>

<tag name="controller.service_arguments" />
</service>
</services>
</container>
36 changes: 23 additions & 13 deletions Tests/Controller/AbstractConnectControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,6 @@

abstract class AbstractConnectControllerTest extends TestCase
{
/**
* @var ConnectController
*/
protected $controller;

/**
* @var MockObject&AuthorizationCheckerInterface
*/
Expand Down Expand Up @@ -182,15 +177,9 @@ protected function setUp(): void
$this->session = $this->createMock(SessionInterface::class);
$this->request = Request::create('/');
$this->request->setSession($this->session);

$this->controller = new ConnectController($this->oAuthUtils, $this->resourceOwnerMapLocator, $this->createMock(RequestStack::class));
$this->controller->setContainer($this->container);
}

/**
* @return AccountNotLinkedException
*/
protected function createAccountNotLinkedException()
protected function createAccountNotLinkedException(): AccountNotLinkedException
{
$accountNotLinked = new AccountNotLinkedException();
$accountNotLinked->setResourceOwnerName('facebook');
Expand All @@ -199,12 +188,33 @@ protected function createAccountNotLinkedException()
return $accountNotLinked;
}

protected function mockAuthorizationCheck($granted = true)
protected function mockAuthorizationCheck(bool $granted = true): void
{
$this->authorizationChecker->expects($this->once())
->method('isGranted')
->with('IS_AUTHENTICATED_REMEMBERED')
->willReturn($granted)
;
}

protected function createConnectController(
bool $connectEnabled = true,
bool $confirmConnect = true,
array $firewallNames = ['default']
): ConnectController {
$controller = new ConnectController(
$this->oAuthUtils,
$this->resourceOwnerMapLocator,
$this->createMock(RequestStack::class),
$connectEnabled,
'IS_AUTHENTICATED_REMEMBERED',
true,
'fake_route',
$confirmConnect,
$firewallNames
);
$controller->setContainer($this->container);

return $controller;
}
}
26 changes: 14 additions & 12 deletions Tests/Controller/ConnectControllerConnectServiceActionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ public function testNotEnabled()
{
$this->expectException(NotFoundHttpException::class);

$this->container->setParameter('hwi_oauth.connect', false);

$this->controller->connectServiceAction($this->request, 'facebook');
$controller = $this->createConnectController(false);
$controller->connectServiceAction($this->request, 'facebook');
}

public function testAlreadyConnected()
Expand All @@ -37,18 +36,18 @@ public function testAlreadyConnected()

$this->mockAuthorizationCheck(false);

$this->controller->connectServiceAction($this->request, 'facebook');
$controller = $this->createConnectController();
$controller->connectServiceAction($this->request, 'facebook');
}

public function testUnknownResourceOwner()
{
$this->expectException(NotFoundHttpException::class);

$this->container->setParameter('hwi_oauth.firewall_names', []);

$this->mockAuthorizationCheck();

$this->controller->connectServiceAction($this->request, 'unknown');
$controller = $this->createConnectController(true, true, []);
$controller->connectServiceAction($this->request, 'unknown');
}

public function testConnectConfirm()
Expand Down Expand Up @@ -85,7 +84,8 @@ public function testConnectConfirm()
->with('@HWIOAuth/Connect/connect_confirm.html.twig')
;

$this->controller->connectServiceAction($this->request, 'facebook');
$controller = $this->createConnectController();
$controller->connectServiceAction($this->request, 'facebook');
}

public function testConnectSuccess()
Expand Down Expand Up @@ -129,15 +129,15 @@ public function testConnectSuccess()
->with('@HWIOAuth/Connect/connect_success.html.twig')
;

$this->controller->connectServiceAction($this->request, 'facebook');
$controller = $this->createConnectController();
$controller->connectServiceAction($this->request, 'facebook');
}

public function testConnectNoConfirmation()
{
$key = time();

$this->request->query->set('key', $key);
$this->container->setParameter('hwi_oauth.connect.confirmation', false);

$this->mockAuthorizationCheck();

Expand All @@ -164,7 +164,8 @@ public function testConnectNoConfirmation()
->with('@HWIOAuth/Connect/connect_success.html.twig')
;

$this->controller->connectServiceAction($this->request, 'facebook');
$controller = $this->createConnectController(true, false);
$controller->connectServiceAction($this->request, 'facebook');
}

public function testResourceOwnerHandle()
Expand Down Expand Up @@ -201,6 +202,7 @@ public function testResourceOwnerHandle()
->with('@HWIOAuth/Connect/connect_confirm.html.twig')
;

$this->controller->connectServiceAction($this->request, 'facebook');
$controller = $this->createConnectController();
$controller->connectServiceAction($this->request, 'facebook');
}
}
Loading