Report better errors on user CRUD operations #621

jaragunde · 2023-03-08T11:50:42Z

This affects the Postgres and LDAP DAOs. Notice there are many operations in the user management screen (assign users to groups and all the user history assignment) that aren't upgraded yet to OperationResult and they keep working as usual.

This warning has become an error in PHP 8. We also remove some unused code.

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.

The HybridUserDAO matches user data with the LDAP and keeps them in sync with the local database, to maintain the relations with other data tables. We modify the create operation in this DAO to return OperationResult, matching the new expectations of the upper layers. We change the Hybrid DAO to extend the Postgres DAO, so we can reuse the SQL operations. In this case, we call the create operation from the Postgres DAO after we do the corresponding LDAP checks.

jaragunde · 2023-03-08T11:54:47Z

model/dao/UserDAO/HybridUserDAO.php

@@ -417,35 +418,14 @@ public function getJourneyHistory($userId) {
    /** User updater for LDAP/PostgreSQL Hybrid.
     *
     * This function updates the data of a User by its {@link UserVO}.
+     * WARNING: this function allows to update to a name that does not exist in LDAP.
+     * Do we really want this behavior?


Do we do something about this? This could make sense as a transient state before we change the user in LDAP, and in this state the user would not be able to login. Alternatively, we could check that we rename to another login that exists in LDAP so we would have to modify LDAP first. In any case, this should be a very rarely used operation.

I agree that this is such a rare case that we can leave it like this for now.

Thanks, I think I'll rewrite the warning message to state the consequences clearly instead of asking this open question.

anarute

This is great, thank you! Sorry for the late review

anarute · 2023-03-15T18:26:40Z

model/dao/UserDAO/HybridUserDAO.php

+        if($ldapResult["count"] == 0) {
+            $result = new OperationResult(false);
+            $result->setResponseCode(400);
+            $result->setErrorNumber(1234);


Just double checking if 1234 is correct?

Same question here. I'm guessing you are picking an arbitrary error code here since we wouldn't have a php or sql error code in this instance.

Yes, sorry, I'm not really keeping track of what the error codes are and we aren't really using them. We'd have to follow up with a general review...

anarute · 2023-03-15T18:31:13Z

model/dao/UserDAO/HybridUserDAO.php

@@ -417,35 +418,14 @@ public function getJourneyHistory($userId) {
    /** User updater for LDAP/PostgreSQL Hybrid.
     *
     * This function updates the data of a User by its {@link UserVO}.
+     * WARNING: this function allows to update to a name that does not exist in LDAP.
+     * Do we really want this behavior?


I agree that this is such a rare case that we can leave it like this for now.

dmtrek14 · 2023-03-15T21:11:07Z

model/dao/UserDAO/PostgreSQLUserDAO.php

+
+            $result->setIsSuccessful(true);
+            $result->setMessage('User updated successfully.');
+            $result->setResponseCode(201);


Nit: If we are updating here and not creating, then we might want to use the 200 response code (for 'okay')

Thanks for catching this, this and the other nits are copy&paste issues I was blind to.

dmtrek14 · 2023-03-15T21:12:19Z

model/dao/UserDAO/PostgreSQLUserDAO.php

-            $affectedRows = pg_affected_rows($res);
+            $result->setIsSuccessful(true);
+            $result->setMessage('User updated successfully.');
+            $result->setResponseCode(201);


Nit: since we're deleting and not creating, a 200 response code might be better here. Also, if we're deleting, perhaps a slightly different message would be clearer? Something like "User deleted successfully".

dmtrek14 · 2023-03-15T21:18:38Z

model/dao/UserDAO/PostgreSQLUserDAO.php

+            $errorMessage = $ex->getMessage();
+            $resultMessage = "Error updating user:\n";
+            if(strpos($errorMessage, "Foreign key violation")) {
+                $result->setResponseCode(403);


Nit (again, sorry): My understanding of a 403 is that it means a user doesn't have permission to perform the attempted action. A FK violation feels a little more like a generic 500 to me, but I also recognize that some of these response codes don't always fit every situation.

Same feedback re the message: you might want to specify that that was an error when deleting the user instead of updating.

You are right about 403 doesn't really fit, but I don't want to use 500 because it's defined as a "catch-all" error for unexpected situations. It's possible to trigger this foreign key violation from the UI, if an admin attempts to delete a user and that user has already created tasks, so I don't consider it an unexpected situation. And I decided not to use 400 because I don't think it's a client error either...

Maybe 409 Conflict is a better match?

Picking the correct code is always the hardest part. I've never really used a 409 before, but it could fit here. I'm good with that unless @anarute has another suggestion.

I think 409 makes sense too!

dmtrek14

I had a few nits, but nothing to hold up the PR over.

jaragunde · 2023-03-16T11:53:27Z

I've pushed fixup patches to address your comments, and a new patch that allows us to report a message from the backend to the UI even in case of success. That's useful to show a warning when updating the login of an LDAP user.

anarute · 2023-03-20T13:22:25Z

The changes look good to me!

Modify the user update pipeline (service, facade, DAO) to use OperationResult. HybridUserDAO will be addressed in a subsequent patch.

This operation did not do anything related with LDAP, so we simply run the corresponding SQL operation. We don't modify the behavior of the operation, so we add a comment about a detected problem. In the web service layer, we add try/catch blocks for the LDAPInvalidOperationException like the ones in the create and delete user services, to prevent crashing on operations that aren't defined in the LDAP backend (namely, assigning users to groups).

Modify the user delete pipeline (service, facade, DAO) to use OperationResult. HybridUserDAO is changed to reuse the same operation from Postgres DAO, since it did not have any code related to LDAP.

When updating a login name in the LDAP backend there is a risk of side effects. With this change, we are able to report the success message from the backend through the UI instead of overwriting it with another string, so we can warn users when doing that operation.

jaragunde · 2023-03-20T15:34:09Z

Thanks! I've squashed the fixup patches, and will proceed to merge.

jaragunde added 3 commits March 8, 2023 10:16

[#431] Fix calling sizeof on non-countable.

4d307f2

This warning has become an error in PHP 8. We also remove some unused code.

jaragunde requested review from anarute and dmtrek14 March 8, 2023 11:50

jaragunde commented Mar 8, 2023

View reviewed changes

anarute approved these changes Mar 15, 2023

View reviewed changes

dmtrek14 reviewed Mar 15, 2023

View reviewed changes

dmtrek14 approved these changes Mar 15, 2023

View reviewed changes

jaragunde added 4 commits March 20, 2023 16:32

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

999ec3f

Modify the user update pipeline (service, facade, DAO) to use OperationResult. HybridUserDAO will be addressed in a subsequent patch.

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

73b21e5

Modify the user delete pipeline (service, facade, DAO) to use OperationResult. HybridUserDAO is changed to reuse the same operation from Postgres DAO, since it did not have any code related to LDAP.

jaragunde force-pushed the user-create-report-errors branch from 18773ce to e0fd789 Compare March 20, 2023 15:33

jaragunde merged commit 04cc1fd into main Mar 20, 2023

jaragunde deleted the user-create-report-errors branch March 20, 2023 15:34

This was referenced Mar 20, 2023

Fix PHP warnings #431

Open

Improve feedback on error in the application #173

Open

Do something meaningful with SQL exceptions #496

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report better errors on user CRUD operations #621

Report better errors on user CRUD operations #621

jaragunde commented Mar 8, 2023

jaragunde Mar 8, 2023

anarute Mar 15, 2023

jaragunde Mar 16, 2023

anarute left a comment

anarute Mar 15, 2023

dmtrek14 Mar 15, 2023

jaragunde Mar 16, 2023

anarute Mar 15, 2023

dmtrek14 Mar 15, 2023

jaragunde Mar 16, 2023

dmtrek14 Mar 15, 2023 •

edited

Loading

dmtrek14 Mar 15, 2023

jaragunde Mar 16, 2023

dmtrek14 Mar 16, 2023

anarute Mar 20, 2023

dmtrek14 left a comment

jaragunde commented Mar 16, 2023

anarute commented Mar 20, 2023

jaragunde commented Mar 20, 2023

Report better errors on user CRUD operations #621

Report better errors on user CRUD operations #621

Conversation

jaragunde commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarute left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrek14 Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrek14 left a comment

Choose a reason for hiding this comment

jaragunde commented Mar 16, 2023

anarute commented Mar 20, 2023

jaragunde commented Mar 20, 2023

dmtrek14 Mar 15, 2023 •

edited

Loading