-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[WSSE] - Using a PSR6 cache instead of file cache #6617
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,7 +127,7 @@ set an authenticated token in the token storage if successful. | |
|
||
public function __construct(TokenStorageInterface $tokenStorage, AuthenticationManagerInterface $authenticationManager) | ||
{ | ||
$this->tokenStorage = $tokenStorage; | ||
$this->tokenStorage = $tokenStorage; | ||
$this->authenticationManager = $authenticationManager; | ||
} | ||
|
||
|
@@ -208,6 +208,7 @@ the ``PasswordDigest`` header value matches with the user's password. | |
// src/AppBundle/Security/Authentication/Provider/WsseProvider.php | ||
namespace AppBundle\Security\Authentication\Provider; | ||
|
||
use Psr\Cache\CacheItemPoolInterface; | ||
use Symfony\Component\Security\Core\Authentication\Provider\AuthenticationProviderInterface; | ||
use Symfony\Component\Security\Core\User\UserProviderInterface; | ||
use Symfony\Component\Security\Core\Exception\AuthenticationException; | ||
|
@@ -218,12 +219,12 @@ the ``PasswordDigest`` header value matches with the user's password. | |
class WsseProvider implements AuthenticationProviderInterface | ||
{ | ||
private $userProvider; | ||
private $cacheDir; | ||
private $cachePool; | ||
|
||
public function __construct(UserProviderInterface $userProvider, $cacheDir) | ||
public function __construct(UserProviderInterface $userProvider, CacheItemPoolInterface $cachePool) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imo we should type hint against the AdapterInterface from the Cache component. This way we can inject cache adapters defined through the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. We should not type hint against AdapterInterface. We want to use the PSR. Becasue we do not want to force the user to use Symfonys cache component. Let's use the strength of the PSRs. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AdapterInterface extends the PSR ones, so no problem here and indeed: Let's be as generic as possible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. There's no need to require the more specialised interface here. |
||
{ | ||
$this->userProvider = $userProvider; | ||
$this->cacheDir = $cacheDir; | ||
$this->cachePool = $cachePool; | ||
} | ||
|
||
public function authenticate(TokenInterface $token) | ||
|
@@ -258,19 +259,18 @@ the ``PasswordDigest`` header value matches with the user's password. | |
return false; | ||
} | ||
|
||
// Validate that the nonce is *not* used in the last 5 minutes | ||
// if it has, this could be a replay attack | ||
if ( | ||
file_exists($this->cacheDir.'/'.md5($nonce)) | ||
&& file_get_contents($this->cacheDir.'/'.md5($nonce)) + 300 > time() | ||
) { | ||
// Try to fetch the cache item from pool | ||
$cacheItem = $this->cachePool->getItem(md5($nonce)); | ||
|
||
// Validate that the nonce is *not* in cache | ||
// if it is, this could be a replay attack | ||
if ($cacheItem->isHit()) { | ||
throw new NonceExpiredException('Previously used nonce detected'); | ||
} | ||
// If cache directory does not exist we create it | ||
if (!is_dir($this->cacheDir)) { | ||
mkdir($this->cacheDir, 0777, true); | ||
} | ||
file_put_contents($this->cacheDir.'/'.md5($nonce), time()); | ||
|
||
// Store the item in cache for 5 minutes | ||
$cacheItem->set(null)->expiresAfter(300); | ||
$this->cachePool->save($cacheItem); | ||
|
||
// Validate Secret | ||
$expected = base64_encode(sha1(base64_decode($nonce).$created.$secret, true)); | ||
|
@@ -411,7 +411,7 @@ to service ids that do not exist yet: ``wsse.security.authentication.provider`` | |
class: AppBundle\Security\Authentication\Provider\WsseProvider | ||
arguments: | ||
- '' # User Provider | ||
- '%kernel.cache_dir%/security/nonces' | ||
- '@cache' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should inject the |
||
public: false | ||
|
||
wsse.security.authentication.listener: | ||
|
@@ -433,7 +433,7 @@ to service ids that do not exist yet: ``wsse.security.authentication.provider`` | |
public="false" | ||
> | ||
<argument /> <!-- User Provider --> | ||
<argument>%kernel.cache_dir%/security/nonces</argument> | ||
<argument type="service" id="cache"></argument> | ||
</service> | ||
|
||
<service id="wsse.security.authentication.listener" | ||
|
@@ -456,7 +456,7 @@ to service ids that do not exist yet: ``wsse.security.authentication.provider`` | |
'AppBundle\Security\Authentication\Provider\WsseProvider', | ||
array( | ||
'', // User Provider | ||
'%kernel.cache_dir%/security/nonces', | ||
new Reference('cache'), | ||
) | ||
); | ||
$definition->setPublic(false); | ||
|
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.
Please revert this change (we do not align assignments in Symfony).
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.
Sorry. I looked at the other constructor on this page where you do align it. I'll revert it. Thank you.