[#173,#496] Report detailed errors on user creation.

Modify the user creation pipeline (service, facade, DAO) to use OperationResult and return better errors. This affects only the postgres implementation but there is another DAO for LDAP+DB that will be addressed in a subsequent patch.
Igalia · Mar 8, 2023 · 2ea7e95 · 2ea7e95
1 parent 4d307f2
commit 2ea7e95
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 60 deletions.
diff --git a/model/dao/UserDAO/PostgreSQLUserDAO.php b/model/dao/UserDAO/PostgreSQLUserDAO.php
@@ -441,32 +441,51 @@ public function update(UserVO $userVO) {
      * The internal id of <var>$userVO</var> will be set after its creation.
      *projectId
      * @param UserVO $userVO the {@link UserVO} with the data we want to insert on database.
-     * @return int the number of rows that have been affected (it should be 1).
-     * @throws {@link SQLQueryErrorException}, {@link SQLUniqueViolationException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     public function create(UserVO $userVO) {
+        $result = new OperationResult(false);
 
-        $affectedRows = 0;
-
-        $sql = "INSERT INTO usr (login, password) VALUES(" . DBPostgres::checkStringNull($userVO->getLogin()) . ", ";
-        if (DBPostgres::checkStringNull($userVO->getPassword()) == "NULL")
-            $sql = $sql . "NULL)";
-        else
-            $sql = $sql . "md5(" . DBPostgres::checkStringNull($userVO->getPassword()) . "))";
-
-        $res = pg_query($this->connect, $sql);
-
-        if ($res == NULL)
-            if (strpos(pg_last_error(), "unique_usr_login"))
-                throw new SQLUniqueViolationException(pg_last_error());
-            else throw new SQLQueryErrorException(pg_last_error());
+        $sql = "INSERT INTO usr (login, password) ";
+        $emptyPassword = false;
+        if ($userVO->getPassword() !== '') {
+            $sql = $sql . "VALUES(:login, md5(:password))";
+        }
+        else {
+            $emptyPassword = true;
+            $sql = $sql . "VALUES(:login, NULL)";
+        }
 
-        $userVO->setID(DBPostgres::getId($this->connect, "usr_id_seq"));
+        try {
+            $statement = $this->pdo->prepare($sql);
+            $statement->bindValue(":login", $userVO->getLogin(), PDO::PARAM_STR);
+            if (!$emptyPassword)
+                $statement->bindValue(":password", $userVO->getPassword(), PDO::PARAM_STR);
+            $statement->execute();
 
-        $affectedRows = pg_affected_rows($res);
+            $userVO->setID($this->pdo->lastInsertId("usr_id_seq"));
 
-        return $affectedRows;
+            $result->setIsSuccessful(true);
+            $result->setMessage('User created successfully.');
+            $result->setResponseCode(201);
+        }
+        catch (PDOException $ex) {
+            $errorMessage = $ex->getMessage();
+            $resultMessage = "Error creating user:\n";
+            if(strpos($errorMessage, "unique_usr_login")) {
+                $resultMessage .= "Login is already used.";
+                $result->setResponseCode(400);
+            }
+            else {
+                $resultMessage .= $errorMessage;
+                $result->setResponseCode(500);
+            }
+            $result->setErrorNumber($ex->getCode());
+            $result->setMessage($resultMessage);
+            $result->setIsSuccessful(false);
+        }
 
+        return $result;
     }
 
     /** User deleter for PostgreSQL.

diff --git a/model/dao/UserDAO/UserDAO.php b/model/dao/UserDAO/UserDAO.php
@@ -31,6 +31,7 @@
 
 include_once(PHPREPORT_ROOT . '/model/vo/UserVO.php');
 include_once(PHPREPORT_ROOT . '/model/dao/BaseDAO.php');
+include_once(PHPREPORT_ROOT . '/model/OperationResult.php');
 include_once(PHPREPORT_ROOT . '/util/IncorrectLoginException.php');
 
 /** DAO for Users
@@ -277,8 +278,7 @@ public abstract function update(UserVO $userVO);
      * The internal id of <var>$userVO</var> will be set after its creation.
      *
      * @param UserVO $userVO the {@link UserVO} with the data we want to insert on database.
-     * @return int the number of rows that have been affected (it should be 1).
-     * @throws {@link OperationErrorException}, {@link InvalidOperationException}, {@link SQLUniqueViolationException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     public abstract function create(UserVO $userVO);
 

diff --git a/model/facade/UsersFacade.php b/model/facade/UsersFacade.php
@@ -168,15 +168,11 @@ static function GetAllActiveUsers($filterEmployees = false) {
      *  This function is used for creating a new User.
      *
      * @param UserVO $user the User value object we want to create.
-     * @return int it just indicates if there was any error (<i>-1</i>) or not (<i>0</i>).
-     * @throws {@link SQLQueryErrorException}, {@link SQLUniqueViolationException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     static function CreateUser(UserVO $user) {
-
-    $action = new CreateUserAction($user);
-
-    return $action->execute();
-
+        $action = new CreateUserAction($user);
+        return $action->execute();
     }
 
     /** Delete User Function

diff --git a/model/facade/action/CreateUserAction.php b/model/facade/action/CreateUserAction.php
@@ -68,17 +68,11 @@ public function __construct(UserVO $user) {
      *
      * This is the function that contains the code that creates the new User, storing it persistently.
      *
-     * @return int it just indicates if there was any error (<i>-1</i>) or not (<i>0</i>).
-     * @throws {@link SQLQueryErrorException}, {@link SQLUniqueViolationException}
+     * @return OperationResult the result {@link OperationResult} with information about operation status
      */
     protected function doExecute() {
-
-    $dao = DAOFactory::getUserDAO();
-        if ($dao->create($this->user)!=1) {
-            return -1;
-        }
-
-        return 0;
+        $dao = DAOFactory::getUserDAO();
+        return $dao->create($this->user);
     }
 
 }

diff --git a/web/services/createUsersService.php b/web/services/createUsersService.php
@@ -160,34 +160,33 @@
 
         } while ($parser->read());
 
-        //var_dump($createUsers);
 
+        foreach((array)$createUsers as $createUser)
+        {
+            $result = UsersFacade::CreateUser($createUser);
+            if (!$result->getIsSuccessful()) {
+                http_response_code($result->getResponseCode());
+                $string = "<return service='createUsers'><error id='" . $result->getErrorNumber() . "'>" .
+                        $result->getMessage() . "</error></return>";
+                break;
+            }
 
-        if (count($createUsers) >= 1)
-            foreach((array)$createUsers as $createUser)
-            {
-                if (UsersFacade::CreateUser($createUser) == -1)
+            try {
+                foreach((array) $createUser->getGroups() as $group)
                 {
-                    $string = "<return service='createUsers'><error id='1'>There was some error while updating the users</error></return>";
-                    break;
-                }
-
-                try {
-                    foreach((array) $createUser->getGroups() as $group)
+                    $group = UsersFacade::GetUserGroupByName($group->getName());
+                    if (UsersFacade::AssignUserToUserGroup($createUser->getId(), $group->getId()) == -1)
                     {
-                        $group = UsersFacade::GetUserGroupByName($group->getName());
-                        if (UsersFacade::AssignUserToUserGroup($createUser->getId(), $group->getId()) == -1)
-                        {
-                            $string = "<return service='createUsers'><error id='1'>There was some error while updating the user groups new assignements</error></return>";
-                            break;
-                        }
+                        $string = "<return service='createUsers'><error id='1'>There was some error while updating the user groups new assignements</error></return>";
+                        break;
                     }
                 }
-                catch (LDAPInvalidOperationException $e) {
-                    // the LDAP backend is enabled so UserGroup operations are forbidden
-                    // we can ignore this error
-                }
             }
+            catch (LDAPInvalidOperationException $e) {
+                // the LDAP backend is enabled so UserGroup operations are forbidden
+                // we can ignore this error
+            }
+        }
 
 
 

diff --git a/web/viewUsers.php b/web/viewUsers.php
@@ -293,8 +293,15 @@ function areas(val){
             'write': function() {
                 App.setAlert(true, "Users Changes Saved");
             },
-            'exception': function(){
-                App.setAlert(false, "Some Error Occurred While Saving The Changes");
+            'exception': function(proxy, type, action, eOpts, res) {
+                let parser = new DOMParser();
+                let errorDoc = parser.parseFromString(res.responseText, "text/xml");
+                let errorMessage = "";
+                for (error of errorDoc.getElementsByTagName("error")) {
+                    errorMessage += error.childNodes[0].nodeValue + "\n";
+                }
+                App.setAlert(false, errorMessage);
+                tasksStore.error = true;
             },
             'update': function() {
                 this.save();