-
Notifications
You must be signed in to change notification settings - Fork 13
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
Switch to knpuniversity/oauth2-client-bundle #824
Conversation
use League\OAuth2\Client\Tool\BearerAuthorizationTrait; | ||
use Psr\Http\Message\ResponseInterface; | ||
|
||
final class ElifeProvider extends AbstractProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a library.
src/Security/User/OAuthUser.php
Outdated
|
||
use Symfony\Component\Security\Core\User\UserInterface; | ||
|
||
final class OAuthUser implements UserInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be useful upstream so will open a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
private function getClient() : OAuth2Client | ||
{ | ||
return $this->clientRegistry->getClient('elife'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for lazy loading of the service (supports()
is called on every request). Could use a proper lazy service instead.
|
||
$parameters = array_filter($parameters); | ||
|
||
ksort($parameters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all just makes the tests a bit simpler. However, it'd be better to compare URIs safely instead and not worry about order.
|
||
private function normalize(RequestInterface $request) : RequestInterface | ||
{ | ||
$headers = array_change_key_case($request->getHeaders()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to finish off csarrazi/CsaGuzzleBundle#202 as it's an upstream bug.
…t be unserialized from the session (thewilkybarkid) This PR was merged into the 2.7 branch. Discussion ---------- [Security] Fail gracefully if the security token cannot be unserialized from the session | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | If the security token in the session can't be unserialized, an `E_NOTICE` is issued. This prevents it (and provides a better log message if it's not even a `__PHP_Incomplete_Class`). This is similar to #24731, but I saw it triggered when changing OAuth library (elifesciences/journal#824), so the token class itself no longer exists. (I want to avoid having to manually invalidate all sessions, as not all sessions use that token class.) Commits ------- 053fa43 [Security] Fail gracefully if the security token cannot be unserialized from the session
$subRequest = $request->duplicate([], null, $path); | ||
if ($referer = trim($request->headers->get('Referer'))) { | ||
$firewall = $this->get('security.firewall.map')->getFirewallConfig($request); | ||
$this->saveTargetPath($request->getSession(), $firewall->getName(), $referer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any validation on the Referer
that checks the host matches for example? Is it coincevable for someone external to send a user with its own Referer
and getting it redirected there (and is it a problem?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, already present
journal/test/Controller/AuthenticationTest.php
Lines 86 to 119 in f2ccd2f
/** | |
* @test | |
* @dataProvider refererProvider | |
*/ | |
public function it_uses_the_referer_header_for_redirecting_after_logging_in(string $referer, string $expectedRedirect) | |
{ | |
$client = static::createClient(); | |
$client->followRedirects(false); | |
$client->request('GET', '/log-in?open-sesame', [], [], array_filter(['HTTP_REFERER' => $referer])); | |
$response = $client->getResponse(); | |
$this->assertTrue($response->isRedirect()); | |
$state = parse_query((new Uri($response->headers->get('Location')))->getQuery())['state']; | |
$this->readyToken(); | |
$client->request('GET', "/log-in/check?code=foo&state=$state"); | |
$response = $client->getResponse(); | |
$this->assertTrue($response->isRedirect()); | |
$this->assertSame($expectedRedirect, $response->headers->get('Location')); | |
} | |
public function refererProvider() : Traversable | |
{ | |
yield 'no header' => ['', 'http://localhost/']; | |
yield 'homepage' => ['http://localhost/', 'http://localhost/']; | |
yield 'local path' => ['http://localhost/foo', 'http://localhost/foo']; | |
yield 'local path with query string' => ['http://localhost/foo?bar', 'http://localhost/foo?bar']; | |
yield 'external site' => ['http://www.example.com/', 'http://localhost/']; | |
yield 'external site with path' => ['http://www.example.com/foo', 'http://localhost/']; | |
} |
|
||
public function onAuthenticationFailure(Request $request, AuthenticationException $exception) : Response | ||
{ | ||
$this->saveAuthenticationErrorToSession($request, $exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this go to a flash message or similar user-visible place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthenticationErrorSubscriber
turns it into a flash message.
{ | ||
$this->saveAuthenticationErrorToSession($request, $exception); | ||
|
||
return new RedirectResponse($this->urlGenerator->generate('home')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and would it be actually shown on the homepage or would the user just see a cached version identical for everyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a session so isn't cached.
use Psr\Http\Message\UriInterface; | ||
use function GuzzleHttp\Psr7\uri_for; | ||
|
||
trait Assertions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit like the Utils
class. Name it UriAssertions
for cohesion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do, had though any other assertions could be put here but a trait each is also fine.
A little more code (have to implement a custom Guard authenticator), but I'm happier with how this bundle works.
I think that currently-logged-in users will see a 500 page (as their session contains a serialized class that no longer exists), but will be logged out and subsequent pages will work (sessions currently expire after 24 hours too). Trying to think of a way of invalidating existing sessions.