Repository: hwi/HWIOAuthBundle (php)

Issue:
User roles do not refresh
My `refreshUser` method of UserProvider reloads user from database, so user roles can be updated. For roles to be updated I use setting in security.yml:

```
always_authenticate_before_granting: true
```

If it is set, AuthenticationManager tries to run `authenticate` method on token before each `isGranted`. If we take a look into `RememberMeAuthenticationProvider` for example it re-creates token with current user roles: 

```
$authenticatedToken = new RememberMeToken($user, $this->providerKey, $this->secret);
$authenticatedToken->setAttributes($token->getAttributes());

return $authenticatedToken;
```

But `OAuthProvider` does nothing since https://github.com/hwi/HWIOAuthBundle/issues/847 and `always_authenticate_before_granting` setting affects nothing.

Is there any possible workaround to get roles refreshed for users?

Pull request:
Rework security OAuthProvider to properly refresh data inside tokens
Fixes #1703.
Replaces #1704.

Patch:
diff --git a/Security/Core/Authentication/Provider/OAuthProvider.php b/Security/Core/Authentication/Provider/OAuthProvider.php
index 14f73f5..6468206 100644
--- a/Security/Core/Authentication/Provider/OAuthProvider.php
+++ b/Security/Core/Authentication/Provider/OAuthProvider.php
@@ -24,8 +24,6 @@ use Symfony\Component\Security\Core\User\UserCheckerInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
- * OAuthProvider.
- *
  * @author Geoffrey Bachelet <geoffrey.bachelet@gmail.com>
  * @author Alexander <iam.asm89@gmail.com>
  */
@@ -51,11 +49,6 @@ class OAuthProvider implements AuthenticationProviderInterface
      */
     private $tokenStorage;
 
-    /**
-     * @param OAuthAwareUserProviderInterface $userProvider     User provider
-     * @param ResourceOwnerMapInterface       $resourceOwnerMap Resource owner map
-     * @param UserCheckerInterface            $userChecker      User checker
-     */
     public function __construct(OAuthAwareUserProviderInterface $userProvider, ResourceOwnerMapInterface $resourceOwnerMap, UserCheckerInterface $userChecker, TokenStorageInterface $tokenStorage)
     {
         $this->userProvider = $userProvider;
@@ -67,29 +60,34 @@ class OAuthProvider implements AuthenticationProviderInterface
     /**
      * {@inheritdoc}
      */
-    public function supports(TokenInterface $token)
+    public function supports(TokenInterface $token): bool
     {
-        return
-            $token instanceof OAuthToken
-            && $this->resourceOwnerMap->hasResourceOwnerByName($token->getResourceOwnerName())
-        ;
+        if (!$token instanceof OAuthToken) {
+            return false;
+        }
+
+        return $this->resourceOwnerMap->hasResourceOwnerByName($token->getResourceOwnerName());
     }
 
     /**
      * {@inheritdoc}
      */
-    public function authenticate(TokenInterface $token)
+    public function authenticate(TokenInterface $token): ?TokenInterface
     {
         if (!$this->supports($token)) {
-            return;
+            return null;
         }
 
-        // fix connect to external social very time
+        // If token is authenticated, re-create it to reload user details
+        /** @var OAuthToken $token */
         if ($token->isAuthenticated()) {
-            return $token;
+            /** @var UserInterface $user */
+            $user = $token->getUser();
+
+            return $this->createOAuthToken($token->getRawToken(), $token, $user);
         }
 
-        /* @var OAuthToken $token */
+        /** @var ResourceOwnerInterface $resourceOwner */
         $resourceOwner = $this->resourceOwnerMap->getResourceOwnerByName($token->getResourceOwnerName());
 
         $oldToken = $token->isExpired() ? $this->refreshToken($token, $resourceOwner) : $token;
@@ -111,29 +109,51 @@ class OAuthProvider implements AuthenticationProviderInterface
         $this->userChecker->checkPreAuth($user);
         $this->userChecker->checkPostAuth($user);
 
-        $token = new OAuthToken($oldToken->getRawToken(), $user->getRoles());
-        $token->setResourceOwnerName($resourceOwner->getName());
-        $token->setUser($user);
-        $token->setAuthenticated(true);
-        $token->setRefreshToken($oldToken->getRefreshToken());
-        $token->setCreatedAt($oldToken->getCreatedAt());
-
-        return $token;
+        return $this->createOAuthToken($oldToken->getRawToken(), $oldToken, $user);
     }
 
     /**
-     * @return OAuthToken|TokenInterface
+     * @param OAuthToken $expiredToken
      */
-    protected function refreshToken(TokenInterface $expiredToken, ResourceOwnerInterface $resourceOwner)
+    private function refreshToken(TokenInterface $expiredToken, ResourceOwnerInterface $resourceOwner): OAuthToken
     {
         if (!$expiredToken->getRefreshToken()) {
             return $expiredToken;
         }
 
-        $token = new OAuthToken($resourceOwner->refreshAccessToken($expiredToken->getRefreshToken()));
-        $token->setRefreshToken($expiredToken->getRefreshToken());
+        /** @var UserInterface $user */
+        $user = $expiredToken->getUser();
+
+        $token = $this->createOAuthToken(
+            $resourceOwner->refreshAccessToken($expiredToken->getRefreshToken()),
+            $expiredToken,
+            $user
+        );
+
         $this->tokenStorage->setToken($token);
 
         return $token;
     }
+
+    /**
+     * @param string|array $data
+     */
+    private function createOAuthToken(
+        $data,
+        OAuthToken $oldToken,
+        UserInterface $user
+    ): OAuthToken {
+        $token = new OAuthToken($data, $user->getRoles());
+        $token->setResourceOwnerName($oldToken->getResourceOwnerName());
+        $token->setUser($user);
+        $token->setAuthenticated(true);
+        $token->setCreatedAt($oldToken->getCreatedAt());
+
+        // Don't use old data if newer was already set
+        if (!$token->getRefreshToken()) {
+            $token->setRefreshToken($oldToken->getRefreshToken());
+        }
+
+        return $token;
+    }
 }


Test patch:
diff --git a/Tests/Security/Core/Authentication/Provider/OAuthProviderTest.php b/Tests/Security/Core/Authentication/Provider/OAuthProviderTest.php
index 85a90dc..c8d61e2 100644
--- a/Tests/Security/Core/Authentication/Provider/OAuthProviderTest.php
+++ b/Tests/Security/Core/Authentication/Provider/OAuthProviderTest.php
@@ -51,25 +51,14 @@ class OAuthProviderTest extends TestCase
             'oauth_token_secret' => 'secret',
         ];
 
-        $oauthTokenMock = $this->getOAuthTokenMock();
-        $oauthTokenMock->expects($this->exactly(2))
-            ->method('getResourceOwnerName')
-            ->willReturn('github');
-        $oauthTokenMock->expects($this->exactly(2))
-            ->method('getRawToken')
-            ->willReturn($expectedToken);
-        $oauthTokenMock->expects($this->once())
-            ->method('getRefreshToken')
-            ->willReturn($expectedToken['refresh_token']);
+        $oauthToken = new OAuthToken($expectedToken);
+        $oauthToken->setResourceOwnerName('github');
 
         $resourceOwnerMock = $this->getResourceOwnerMock();
         $resourceOwnerMock->expects($this->once())
             ->method('getUserInformation')
             ->with($expectedToken)
             ->willReturn($this->getUserResponseMock());
-        $resourceOwnerMock->expects($this->once())
-            ->method('getName')
-            ->willReturn('github');
 
         $resourceOwnerMapMock = $this->getResourceOwnerMapMock();
         $resourceOwnerMapMock->expects($this->once())
@@ -100,7 +89,7 @@ class OAuthProviderTest extends TestCase
 
         $oauthProvider = new OAuthProvider($userProviderMock, $resourceOwnerMapMock, $userCheckerMock, $tokenStorageMock);
 
-        $token = $oauthProvider->authenticate($oauthTokenMock);
+        $token = $oauthProvider->authenticate($oauthToken);
         $this->assertTrue($token->isAuthenticated());
         $this->assertInstanceOf(OAuthToken::class, $token);
 
@@ -126,25 +115,8 @@ class OAuthProviderTest extends TestCase
             'oauth_token_secret' => 'secret',
         ];
 
-        $oauthTokenMock = $this->getOAuthTokenMock();
-        $oauthTokenMock->expects($this->exactly(3))
-            ->method('getResourceOwnerName')
-            ->willReturn('github');
-        $oauthTokenMock->expects($this->exactly(2))
-            ->method('getRawToken')
-            ->willReturn($expectedToken);
-        $oauthTokenMock->expects($this->once())
-            ->method('getAccessToken')
-            ->willReturn($expectedToken['access_token']);
-        $oauthTokenMock->expects($this->once())
-            ->method('getRefreshToken')
-            ->willReturn($expectedToken['refresh_token']);
-        $oauthTokenMock->expects($this->once())
-            ->method('getExpiresIn')
-            ->willReturn($expectedToken['expires_in']);
-        $oauthTokenMock->expects($this->once())
-            ->method('getTokenSecret')
-            ->willReturn($expectedToken['oauth_token_secret']);
+        $oauthToken = new OAuthToken($expectedToken);
+        $oauthToken->setResourceOwnerName('github');
 
         $resourceOwnerMock = $this->getResourceOwnerMock();
         $resourceOwnerMock->expects($this->once())
@@ -174,7 +146,7 @@ class OAuthProviderTest extends TestCase
         $oauthProvider = new OAuthProvider($userProviderMock, $resourceOwnerMapMock, $userCheckerMock, $tokenStorageMock);
 
         try {
-            $oauthProvider->authenticate($oauthTokenMock);
+            $oauthProvider->authenticate($oauthToken);
 
             $this->assertTrue(false, 'Exception was not thrown.');
         } catch (OAuthAwareException $e) {
@@ -205,29 +177,15 @@ class OAuthProviderTest extends TestCase
             'oauth_token_secret' => 'secret_new',
         ];
 
-        $oauthTokenMock = $this->getOAuthTokenMock();
-        $oauthTokenMock->expects($this->once())
-            ->method('isExpired')
-            ->willReturn(true);
-        $oauthTokenMock->expects($this->exactly(2))
-            ->method('getResourceOwnerName')
-            ->willReturn('github');
-        $oauthTokenMock->expects($this->exactly(3))
-            ->method('getRefreshToken')
-            ->willReturn($expiredToken['refresh_token']);
-
         $resourceOwnerMock = $this->getResourceOwnerMock();
         $resourceOwnerMock->expects($this->once())
             ->method('refreshAccessToken')
             ->with($expiredToken['refresh_token'])
             ->willReturn($refreshedToken);
-        $resourceOwnerMock->expects($this->once())
+        $resourceOwnerMock->expects($this->atLeastOnce())
             ->method('getUserInformation')
             ->with($refreshedToken)
             ->willReturn($this->getUserResponseMock());
-        $resourceOwnerMock->expects($this->once())
-            ->method('getName')
-            ->willReturn('github');
 
         $resourceOwnerMapMock = $this->getResourceOwnerMapMock();
         $resourceOwnerMapMock->expects($this->once())
@@ -240,7 +198,7 @@ class OAuthProviderTest extends TestCase
             ->willReturn(true);
 
         $userMock = $this->getUserMock();
-        $userMock->expects($this->once())
+        $userMock->expects($this->exactly(2))
             ->method('getRoles')
             ->willReturn(['ROLE_TEST']);
 
@@ -258,7 +216,14 @@ class OAuthProviderTest extends TestCase
 
         $oauthProvider = new OAuthProvider($userProviderMock, $resourceOwnerMapMock, $userCheckerMock, $tokenStorageMock);
 
-        $token = $oauthProvider->authenticate($oauthTokenMock);
+        $oauthToken = new OAuthToken($refreshedToken);
+        $oauthToken->setResourceOwnerName('github');
+        $oauthToken->setRefreshToken($expiredToken['refresh_token']);
+        $oauthToken->setExpiresIn(30);
+        $oauthToken->setCreatedAt(time() - 3600);
+        $oauthToken->setUser($userMock);
+
+        $token = $oauthProvider->authenticate($oauthToken);
         $this->assertTrue($token->isAuthenticated());
         $this->assertInstanceOf(OAuthToken::class, $token);