feature #15141 [DX] [Security] Renamed Token#getKey() to getSecret() (WouterJ)

This PR was squashed before being merged into the 2.8 branch (closes #15141).

Discussion
----------

[DX] [Security] Renamed Token#getKey() to getSecret()

There are 2 very vague parameter names in the authentication process: `$providerKey` and `$key`. Some tokens/providers have the first one, some tokens/providers the second one and some both. An overview:

| Token | `providerKey` | `key`
| --- | --- | ---
| `AnonymousToken` | - | yes
| `PreAuth...Token` | yes | -
| `RememberMeToken` | yes | yes
| `UsernamePasswordToken` | yes | -

Both names are extremely general and their PHPdocs contains pure no-shit-sherlock-descriptions :squirrel: (like "The key."). This made me and @iltar think it's just an inconsistency and they have the same meaning.
...until we dived deeper into the code and came to the conclusion that `$key` has a Security task (while `$providerKey` doesn't really). If it takes people connected to Symfony internals 30+ minutes to find this out, it should be considered for an improvement imo.

So here is our suggestion: **Rename `$key` to `$secret`**. This explains much better what the value of the string has to be (for instance, it's important that the string is not easily guessable and cannot be found out, according to the Spring docs). It also explains the usage better (it's used as a replacement for credentials and to hash the RememberMeToken).

**Tl;dr**: `$key` and `$providerKey` are too general names, let's improve DX by renaming them. This PR tackles `$key` by renaming it to `$secret`.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

*My excuse for the completely unrelated branch name*

Commits
-------

24e0eb6 [DX] [Security] Renamed Token#getKey() to getSecret()

Author: fabpot

CHANGELOG.md

@@ -1,6 +1,12 @@
 CHANGELOG
 =========
 
+2.8.0
+-----
+
+ * deprecated `getKey()` of the `AnonymousToken`, `RememberMeToken` and `AbstractRememberMeServices` classes
+   in favor of `getSecret()`.
+
 2.7.0
 -----
 

Core/Authentication/Provider/AnonymousAuthenticationProvider.php

@@ -22,16 +22,22 @@
  */
 class AnonymousAuthenticationProvider implements AuthenticationProviderInterface
 {
-    private $key;
+    /**
+     * Used to determine if the token is created by the application
+     * instead of a malicious client.
+     *
+     * @var string
+     */
+    private $secret;
 
     /**
      * Constructor.
      *
-     * @param string $key The key shared with the authentication token
+     * @param string $secret The secret shared with the AnonymousToken
      */
-    public function __construct($key)
+    public function __construct($secret)
     {
-        $this->key = $key;
+        $this->secret = $secret;
     }
 
     /**
@@ -43,7 +49,7 @@ public function authenticate(TokenInterface $token)
             return;
         }
 
-        if ($this->key !== $token->getKey()) {
+        if ($this->secret !== $token->getSecret()) {
             throw new BadCredentialsException('The Token does not contain the expected key.');
         }
 

Core/Authentication/Provider/RememberMeAuthenticationProvider.php

@@ -19,20 +19,20 @@
 class RememberMeAuthenticationProvider implements AuthenticationProviderInterface
 {
     private $userChecker;
-    private $key;
+    private $secret;
     private $providerKey;
 
     /**
      * Constructor.
      *
      * @param UserCheckerInterface $userChecker An UserCheckerInterface interface
-     * @param string               $key         A key
-     * @param string               $providerKey A provider key
+     * @param string               $secret      A secret
+     * @param string               $providerKey A provider secret
      */
-    public function __construct(UserCheckerInterface $userChecker, $key, $providerKey)
+    public function __construct(UserCheckerInterface $userChecker, $secret, $providerKey)
     {
         $this->userChecker = $userChecker;
-        $this->key = $key;
+        $this->secret = $secret;
         $this->providerKey = $providerKey;
     }
 
@@ -45,14 +45,14 @@ public function authenticate(TokenInterface $token)
             return;
         }
 
-        if ($this->key !== $token->getKey()) {
-            throw new BadCredentialsException('The presented key does not match.');
+        if ($this->secret !== $token->getSecret()) {
+            throw new BadCredentialsException('The presented secret does not match.');
         }
 
         $user = $token->getUser();
         $this->userChecker->checkPreAuth($user);
 
-        $authenticatedToken = new RememberMeToken($user, $this->providerKey, $this->key);
+        $authenticatedToken = new RememberMeToken($user, $this->providerKey, $this->secret);
         $authenticatedToken->setAttributes($token->getAttributes());
 
         return $authenticatedToken;

Core/Authentication/Token/AnonymousToken.php

@@ -20,20 +20,20 @@
  */
 class AnonymousToken extends AbstractToken
 {
-    private $key;
+    private $secret;
 
     /**
      * Constructor.
      *
-     * @param string          $key   The key shared with the authentication provider
-     * @param string          $user  The user
-     * @param RoleInterface[] $roles An array of roles
+     * @param string          $secret A secret used to make sure the token is created by the app and not by a malicious client
+     * @param string          $user   The user
+     * @param RoleInterface[] $roles  An array of roles
      */
-    public function __construct($key, $user, array $roles = array())
+    public function __construct($secret, $user, array $roles = array())
     {
         parent::__construct($roles);
 
-        $this->key = $key;
+        $this->secret = $secret;
         $this->setUser($user);
         $this->setAuthenticated(true);
     }
@@ -47,29 +47,39 @@ public function getCredentials()
     }
 
     /**
-     * Returns the key.
-     *
-     * @return string The Key
+     * @deprecated Since version 2.8, to be removed in 3.0. Use getSecret() instead.
      */
     public function getKey()
     {
-        return $this->key;
+        @trigger_error(__method__.'() is deprecated since version 2.8 and will be removed in 3.0. Use getSecret() instead.', E_USER_DEPRECATED);
+
+        return $this->getSecret();
+    }
+
+    /**
+     * Returns the secret.
+     *
+     * @return string
+     */
+    public function getSecret()
+    {
+        return $this->secret;
     }
 
     /**
      * {@inheritdoc}
      */
     public function serialize()
     {
-        return serialize(array($this->key, parent::serialize()));
+        return serialize(array($this->secret, parent::serialize()));
     }
 
     /**
      * {@inheritdoc}
      */
     public function unserialize($serialized)
     {
-        list($this->key, $parentStr) = unserialize($serialized);
+        list($this->secret, $parentStr) = unserialize($serialized);
         parent::unserialize($parentStr);
     }
 }

Core/Authentication/Token/RememberMeToken.php

@@ -20,32 +20,32 @@
  */
 class RememberMeToken extends AbstractToken
 {
-    private $key;
+    private $secret;
     private $providerKey;
 
     /**
      * Constructor.
      *
      * @param UserInterface $user
      * @param string        $providerKey
-     * @param string        $key
+     * @param string        $secret      A secret used to make sure the token is created by the app and not by a malicious client
      *
      * @throws \InvalidArgumentException
      */
-    public function __construct(UserInterface $user, $providerKey, $key)
+    public function __construct(UserInterface $user, $providerKey, $secret)
     {
         parent::__construct($user->getRoles());
 
-        if (empty($key)) {
-            throw new \InvalidArgumentException('$key must not be empty.');
+        if (empty($secret)) {
+            throw new \InvalidArgumentException('$secret must not be empty.');
         }
 
         if (empty($providerKey)) {
             throw new \InvalidArgumentException('$providerKey must not be empty.');
         }
 
         $this->providerKey = $providerKey;
-        $this->key = $key;
+        $this->secret = $secret;
 
         $this->setUser($user);
         parent::setAuthenticated(true);
@@ -64,23 +64,33 @@ public function setAuthenticated($authenticated)
     }
 
     /**
-     * Returns the provider key.
+     * Returns the provider secret.
      *
-     * @return string The provider key
+     * @return string The provider secret
      */
     public function getProviderKey()
     {
         return $this->providerKey;
     }
 
     /**
-     * Returns the key.
-     *
-     * @return string The Key
+     * @deprecated Since version 2.8, to be removed in 3.0. Use getSecret() instead.
      */
     public function getKey()
     {
-        return $this->key;
+        @trigger_error(__method__.'() is deprecated since version 2.8 and will be removed in 3.0. Use getSecret() instead.', E_USER_DEPRECATED);
+
+        return $this->getSecret();
+    }
+
+    /**
+     * Returns the secret.
+     *
+     * @return string
+     */
+    public function getSecret()
+    {
+        return $this->secret;
     }
 
     /**
@@ -97,7 +107,7 @@ public function getCredentials()
     public function serialize()
     {
         return serialize(array(
-            $this->key,
+            $this->secret,
             $this->providerKey,
             parent::serialize(),
         ));
@@ -108,7 +118,7 @@ public function serialize()
      */
     public function unserialize($serialized)
     {
-        list($this->key, $this->providerKey, $parentStr) = unserialize($serialized);
+        list($this->secret, $this->providerKey, $parentStr) = unserialize($serialized);
         parent::unserialize($parentStr);
     }
 }

Core/Tests/Authentication/Provider/AnonymousAuthenticationProviderTest.php

@@ -37,7 +37,7 @@ public function testAuthenticateWhenKeyIsNotValid()
     {
         $provider = $this->getProvider('foo');
 
-        $this->assertNull($provider->authenticate($this->getSupportedToken('bar')));
+        $provider->authenticate($this->getSupportedToken('bar'));
     }
 
     public function testAuthenticate()
@@ -50,9 +50,9 @@ public function testAuthenticate()
 
     protected function getSupportedToken($key)
     {
-        $token = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\AnonymousToken', array('getKey'), array(), '', false);
+        $token = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\AnonymousToken', array('getSecret'), array(), '', false);
         $token->expects($this->any())
-              ->method('getKey')
+              ->method('getSecret')
               ->will($this->returnValue($key))
         ;
 

Core/Tests/Authentication/Provider/RememberMeAuthenticationProviderTest.php

@@ -36,10 +36,10 @@ public function testAuthenticateWhenTokenIsNotSupported()
     /**
      * @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException
      */
-    public function testAuthenticateWhenKeysDoNotMatch()
+    public function testAuthenticateWhenSecretsDoNotMatch()
     {
-        $provider = $this->getProvider(null, 'key1');
-        $token = $this->getSupportedToken(null, 'key2');
+        $provider = $this->getProvider(null, 'secret1');
+        $token = $this->getSupportedToken(null, 'secret2');
 
         $provider->authenticate($token);
     }
@@ -77,7 +77,7 @@ public function testAuthenticate()
         $this->assertEquals('', $authToken->getCredentials());
     }
 
-    protected function getSupportedToken($user = null, $key = 'test')
+    protected function getSupportedToken($user = null, $secret = 'test')
     {
         if (null === $user) {
             $user = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
@@ -87,7 +87,7 @@ protected function getSupportedToken($user = null, $key = 'test')
                 ->will($this->returnValue(array()));
         }
 
-        $token = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\RememberMeToken', array('getProviderKey'), array($user, 'foo', $key));
+        $token = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\RememberMeToken', array('getProviderKey'), array($user, 'foo', $secret));
         $token
             ->expects($this->once())
             ->method('getProviderKey')

Core/Tests/Authentication/Token/AnonymousTokenTest.php

@@ -28,7 +28,7 @@ public function testConstructor()
     public function testGetKey()
     {
         $token = new AnonymousToken('foo', 'bar');
-        $this->assertEquals('foo', $token->getKey());
+        $this->assertEquals('foo', $token->getSecret());
     }
 
     public function testGetCredentials()

Core/Tests/Authentication/Token/RememberMeTokenTest.php

@@ -22,7 +22,7 @@ public function testConstructor()
         $token = new RememberMeToken($user, 'fookey', 'foo');
 
         $this->assertEquals('fookey', $token->getProviderKey());
-        $this->assertEquals('foo', $token->getKey());
+        $this->assertEquals('foo', $token->getSecret());
         $this->assertEquals(array(new Role('ROLE_FOO')), $token->getRoles());
         $this->assertSame($user, $token->getUser());
         $this->assertTrue($token->isAuthenticated());
@@ -31,7 +31,7 @@ public function testConstructor()
     /**
      * @expectedException \InvalidArgumentException
      */
-    public function testConstructorKeyCannotBeNull()
+    public function testConstructorSecretCannotBeNull()
     {
         new RememberMeToken(
             $this->getUser(),
@@ -43,7 +43,7 @@ public function testConstructorKeyCannotBeNull()
     /**
      * @expectedException \InvalidArgumentException
      */
-    public function testConstructorKeyCannotBeEmptyString()
+    public function testConstructorSecretCannotBeEmptyString()
     {
         new RememberMeToken(
             $this->getUser(),