Allow the auth error message to be customized (it was almost impossible before)

Author: Gaetano Giunta

Adapter/ClientInterface.php

@@ -2,6 +2,7 @@
 
 namespace Kaliop\IdentityManagementBundle\Adapter;
 
+use Symfony\Component\Security\Core\Exception\AuthenticationException;
 use Kaliop\IdentityManagementBundle\Security\User\RemoteUser;
 
 interface ClientInterface
@@ -10,7 +11,7 @@ interface ClientInterface
      * @param string $login
      * @param string $password
      * @return RemoteUser
-     * @throws BadCredentialsException|AuthenticationServiceException
+     * @throws AuthenticationException
      */
     public function authenticateUser($login, $password);
-}
+}

Resources/config/services.yml

@@ -1,5 +1,4 @@
 parameters:
-    #kaliop_identity.interactive_event_listener.class: Kaliop\IdentityManagementBundle\EventListener\InteractiveLoginListener
     kaliop_identity.security.ip_to_user_mapper.class: Kaliop\IdentityManagementBundle\Security\Helper\IpToUserMapper
     kaliop_identity.security.authentication.provider.ip: Kaliop\IdentityManagementBundle\Security\Authentication\Provider\IPAuthenticationProvider
     kaliop_identity.security.authentication.listener.ip.class: Kaliop\IdentityManagementBundle\Security\Firewall\IPListener
@@ -14,6 +13,9 @@ parameters:
     # Kaliop\IdentityManagementBundle\Adapter\AMS\RemoteUser: kaliop_identity.security.remoteuser_handler.ams
     kaliop_identity.remoteuser_service_map: {}
 
+    # When set to true, authentication errors of type UsernameNotFoundException, BadCredentialsException and unknown
+    # exceptions will be masked with a standard error message
+    kaliop_identity.mask_user_exceptions: true
 
 services:
 
@@ -49,7 +51,7 @@ services:
             - "" # A client service. Will be injected from the config of the firewall
             - "" # A user provider service. Will be injected from the config of the firewall
             - "" # Firewall id. Will be injected from the config of the firewall
-            - true
+            - %kaliop_identity.mask_user_exceptions%
         abstract: true
     # the security listener is just a plain form-based one
     kaliop_identity.security.authentication.listener.remoteuser:

Security/Authentication/Provider/RemoteUserAuthenticationProvider.php

@@ -7,6 +7,9 @@
 use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
 use Symfony\Component\Security\Core\Exception\BadCredentialsException;
+use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Kaliop\IdentityManagementBundle\Adapter\ClientInterface;
 use Psr\Log\LoggerInterface;
@@ -19,11 +22,18 @@
  */
 class RemoteUserAuthenticationProvider implements AuthenticationProviderInterface
 {
+    /**
+     * @var bool $hideUserNotFoundExceptions when true, auth exceptions of type UsernameNotFoundException,
+     * BadCredentialsException and unkown (non-AuthenticationException) will be masked with a standard error message
+     */
     protected $hideUserNotFoundExceptions;
     //protected $userChecker;
     protected $providerKey;
+    /** @var ClientInterface $client */
     protected $client;
+    /** @var UserProviderInterface $userProvider */
     protected $userProvider;
+    /** @var LoggerInterface|null $logger */
     protected $logger;
 
     public function __construct(ClientInterface $client, UserProviderInterface $userProvider, $providerKey, $hideUserNotFoundExceptions = true)
@@ -52,10 +62,8 @@ public function supports(TokenInterface $token)
      * fetch 1st, then check pwd, we do fetch-while-checking-pwd
      *
      * @param TokenInterface $token
-     * @return UsernamePasswordToken|void
-     * @throws AuthenticationServiceException
-     * @throws UsernameNotFoundException
-     * @throws \Exception
+     * @return UsernamePasswordToken
+     * @throws AuthenticationException
      *
      * @see DaoAuthenticationProvider
      */
@@ -65,14 +73,14 @@ public function authenticate(TokenInterface $token)
             return;
         }
 
+        /* we can not fetch the user 1st based on his login
         /// @todo throw a BadCredentialsException instead ?
         $username = $token->getUsername();
         if ('' === $username || null === $username) {
             $username = 'NONE_PROVIDED';
         }
 
-        // we can not fetch the user 1st based on his login
-        /* try {
+        try {
             $user = $this->retrieveUser($username, $token);
         } catch (UsernameNotFoundException $e) {
             if ($this->hideUserNotFoundExceptions) {
@@ -92,6 +100,12 @@ public function authenticate(TokenInterface $token)
             $user = $this->retrieveUserAndCheckAuthentication($token);
             /// @todo !important reintroduce this check?
             //$this->userChecker->checkPostAuth($user);
+        } catch (UsernameNotFoundException $e) {
+            if ($this->hideUserNotFoundExceptions) {
+                throw new BadCredentialsException('Bad credentials.', 0, $e);
+            }
+
+            throw $e;
         } catch (BadCredentialsException $e) {
             if ($this->hideUserNotFoundExceptions) {
                 throw new BadCredentialsException('Bad credentials.', 0, $e);
@@ -109,6 +123,7 @@ public function authenticate(TokenInterface $token)
     /**
      * @param UsernamePasswordToken $token
      * @return mixed|UserInterface
+     * @throws BadCredentialsException|AuthenticationException
      */
     protected function retrieveUserAndCheckAuthentication(UsernamePasswordToken $token)
     {
@@ -119,6 +134,7 @@ protected function retrieveUserAndCheckAuthentication(UsernamePasswordToken $tok
             if ($currentUser->getPassword() !== $token->getCredentials()) {
                 throw new BadCredentialsException('The credentials were changed from another session.');
             }
+
             return $currentUser;
 
         } else {
@@ -140,8 +156,14 @@ protected function retrieveUserAndCheckAuthentication(UsernamePasswordToken $tok
                 //$user = $this->userProvider->loadUserByUsername($username);
                 return $user;
 
+            } catch(AuthenticationException $e) {
+                // let through any exception of the expected authentication type
+                throw $e;
             } catch(\Exception $e) {
-                throw new BadCredentialsException('The presented username or password is invalid.');
+                // we mask any internal, unexpected error from the Client
+                /// @todo we should log a message here: the Client used an unexpected exception type...
+                /// @tood we should really be using an AuthenticationServiceException here
+                throw new BadCredentialsException('The presented username or password is invalid.', 0, $e);
             }
 
             // no need to check the password after loading the user: the remote ws does that

ezpublish_legacy/identitymanagementextension/login_handler/ezremoteuserloginuser.php

@@ -61,12 +61,12 @@ protected static function _loginUser( $login, $password, $authenticationMatch =
             return self::fetch($user->id);
 
         } catch(\Exception $e) {
+            /// @todo make it easier to tell apart system error from user errors such as bad password...
+
             eZDebug::writeError($e->getMessage(), __METHOD__ );
 
             return false;
         }
-
-        $checker = $container->get('security.authorization_checker');
     }
 
 }

todo.md

@@ -27,7 +27,7 @@ Kaliop Identity Management Bundle
 - to do: check if it is a good idea to remove the 'remoteuser' provider in app/security.yml. Remoteusers after all are
          not meant to be used as actual logged in users anyway
 - to do: add support for forgotpassword
-- to do: add more comprehensive logging support
+- to do: add more comprehensive logging
 - to do: add an interface for RemoteUserHandler classes
 
 out of scope (but could be done):