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

Fix csrf token with ensure_logout #677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
85 changes: 81 additions & 4 deletions Controller/AuthorizeController.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Security\Core\Exception\InvalidCsrfTokenException;
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Csrf\CsrfToken;
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
use Twig\Environment as TwigEnvironment;

/**
Expand Down Expand Up @@ -95,6 +98,11 @@ class AuthorizeController
*/
private $eventDispatcher;

/**
* @var CsrfTokenManagerInterface
*/
private $csrfTokenManager;

/**
* This controller had been made as a service due to support symfony 4 where all* services are private by default.
* Thus, this is considered a bad practice to fetch services directly from container.
Expand All @@ -113,6 +121,7 @@ public function __construct(
ClientManagerInterface $clientManager,
EventDispatcherInterface $eventDispatcher,
TwigEnvironment $twig,
CsrfTokenManagerInterface $csrfTokenManager,
SessionInterface $session = null
) {
$this->requestStack = $requestStack;
Expand All @@ -124,6 +133,7 @@ public function __construct(
$this->router = $router;
$this->clientManager = $clientManager;
$this->eventDispatcher = $eventDispatcher;
$this->csrfTokenManager = $csrfTokenManager;
$this->twig = $twig;
}

Expand All @@ -134,17 +144,21 @@ public function authorizeAction(Request $request)
{
$user = $this->tokenStorage->getToken()->getUser();

$form = $this->authorizeForm;
$formHandler = $this->authorizeFormHandler;

if (!$user instanceof UserInterface) {
throw new AccessDeniedException('This user does not have access to this section.');
}

if ($this->session && true === $this->session->get('_fos_oauth_server.ensure_logout')) {
$this->checkCsrfTokenBeforeInvalidingTheSession($form, $request);

$this->session->invalidate(600);
$this->session->set('_fos_oauth_server.ensure_logout', true);
}

$form = $this->authorizeForm;
$formHandler = $this->authorizeFormHandler;
$this->regenerateTokenForInvalidatedSession($form, $request);
}

/** @var PreAuthorizationEvent $event */
$event = $this->eventDispatcher->dispatch(new PreAuthorizationEvent($user, $this->getClient()));
Expand Down Expand Up @@ -216,7 +230,7 @@ protected function getClient()

if (null === $clientId = $request->get('client_id')) {
$formData = $request->get($this->authorizeForm->getName(), []);
$clientId = isset($formData['client_id']) ? $formData['client_id'] : null;
$clientId = $formData['client_id'] ?? null;
}

$this->client = $this->clientManager->findClientByPublicId($clientId);
Expand Down Expand Up @@ -247,4 +261,67 @@ private function getCurrentRequest()

return $request;
}

/**
* Validate if the current POST CSRF token is valid.
* We need to do this now as the session will be regenerated due to the `ensure_logout` parameter.
*/
private function checkCsrfTokenBeforeInvalidingTheSession(Form $form, Request $request): void
{
if (!$request->isMethod('POST')) {
// no need to check the CSRF token if we are not on a POST request (ie. submitting the form)
return;
}

if (!$form->getConfig()->getOption('csrf_protection')) {
// no csrf security, no need to validate token
return;
}

$tokenFieldName = $form->getConfig()->getOption('csrf_field_name');
$tokenId = $form->getConfig()->getOption('csrf_token_id') ?? $form->getName();

$formData = $request->request->get($form->getName());
$tokenValue = $formData[$tokenFieldName] ?? null;

$token = new CsrfToken($tokenId, $tokenValue);

if (!$this->csrfTokenManager->isTokenValid($token)) {
throw new InvalidCsrfTokenException();
}
}

/**
* This method will inject a newly regenerated CSRF token into the actual form
* as Symfony's form manager will check this token upon the current session.
*
* As we have regenerate a session, we need to inject the newly generated token into
* the form data.
*
* It does bypass Symfony form CSRF protection, but the CSRF token is validated
* in the `checkCsrfTokenBeforeInvalidingTheSession` method
*/
private function regenerateTokenForInvalidatedSession(Form $form, Request $request): void
{
if (!$request->isMethod('POST')) {
// no need to check the CSRF token if we are not on a POST request (ie. submitting the form)
return;
}

if (!$form->getConfig()->getOption('csrf_protection')) {
// no csrf security, no need to regenerate a valid token
return;
}

$tokenFieldName = $form->getConfig()->getOption('csrf_field_name');
$tokenId = $form->getConfig()->getOption('csrf_token_id') ?? $form->getName();

// regenerate a new token and replace the form data as Symfony's form manager will check this token.
// the request token has already been checked.
$newToken = $this->csrfTokenManager->refreshToken($tokenId);

$formData = $request->request->get($form->getName());
$formData[$tokenFieldName] = $newToken->getValue();
$request->request->set($form->getName(), $formData);
}
}
1 change: 1 addition & 0 deletions Resources/config/authorize.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
<argument type="service" id="fos_oauth_server.client_manager" />
<argument type="service" id="event_dispatcher" />
<argument type="service" id="twig" />
<argument type="service" id="security.csrf.token_manager" />
<argument type="service" id="session" on-invalid="null" />
</service>
</services>
Expand Down
11 changes: 11 additions & 0 deletions Tests/Controller/AuthorizeControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
use Twig\Environment as TwigEnvironment;

class AuthorizeControllerTest extends \PHPUnit\Framework\TestCase
Expand Down Expand Up @@ -132,6 +133,11 @@ class AuthorizeControllerTest extends \PHPUnit\Framework\TestCase
*/
protected $formView;

/**
* @var \Symfony\Component\Security\Csrf\CsrfTokenManagerInterface
*/
protected $csrfTokenManager;

public function setUp(): void
{
$this->requestStack = $this->getMockBuilder(RequestStack::class)
Expand Down Expand Up @@ -170,6 +176,10 @@ public function setUp(): void
->disableOriginalConstructor()
->getMock()
;
$this->csrfTokenManager = $this->getMockBuilder(CsrfTokenManagerInterface::class)
->disableOriginalConstructor()
->getMock()
;
$this->session = $this->getMockBuilder(SessionInterface::class)
->disableOriginalConstructor()
->getMock()
Expand All @@ -185,6 +195,7 @@ public function setUp(): void
$this->clientManager,
$this->eventDispatcher,
$this->twig,
$this->csrfTokenManager,
$this->session
);

Expand Down