[irods#7455] Return better error for non-member user removal from group

korydraughn · Mar 27, 2024 · 06ed239 · 06ed239
1 parent b098e1e
commit 06ed239
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 8 deletions.
diff --git a/lib/core/include/irods/rodsErrorTable.h b/lib/core/include/irods/rodsErrorTable.h
@@ -809,6 +809,7 @@ NEW_ERROR(INVALID_KVP_STRING,                   -1826000)
 NEW_ERROR(PLUGIN_ERROR_MISSING_SHARED_OBJECT,   -1827000)
 NEW_ERROR(RULE_ENGINE_ERROR,                    -1828000)
 NEW_ERROR(REBALANCE_ALREADY_ACTIVE_ON_RESOURCE, -1829000)
+NEW_ERROR(USER_NOT_IN_GROUP,                    -1830000)
 /** @} */
 
 

diff --git a/plugins/database/src/db_plugin.cpp b/plugins/database/src/db_plugin.cpp
@@ -7903,6 +7903,12 @@ irods::error db_mod_group_op(
                       "delete from R_USER_GROUP where group_user_id = ? and user_id = ?",
                       &icss );
         if ( status != 0 ) {
+            if (CAT_SUCCESS_BUT_WITH_NO_INFO == status) {
+                // If the removal resulted in nothing happening, the target user is not a member of the target group.
+                // Some clients and REPs do not acknowledge CAT_SUCCESS_BUT_WITH_NO_INFO so we need to return something
+                // a little more specific to indicate the state of affairs.
+                status = USER_NOT_IN_GROUP;
+            }
             log_db::info("chlModGroup cmlExecuteNoAnswerSql delete failure {}", status);
             _rollback( "chlModGroup" );
             return ERROR( status, "delete failure" );

diff --git a/scripts/irods/test/test_all_rules.py b/scripts/irods/test/test_all_rules.py
@@ -1715,23 +1715,41 @@ def test_msiRemoveUserFromGroup_returns_error_when_attempting_to_remove_user_fro
 
         # This map contains rule code key'd off of the the REP type.
         rule_map = {
-            'irods_rule_engine_plugin-irods_rule_language': textwrap.dedent(f'''
-                msiRemoveUserFromGroup('{group}', '{self.user0.username}', '')
+            'irods_rule_engine_plugin-irods_rule_language': textwrap.dedent('''
+                issue_7165 {{
+                    msiRemoveUserFromGroup('{0}', '{1}', '{2}')
+                }}
+
+                INPUT null
+                OUTPUT ruleExecOut
             '''),
-            'irods_rule_engine_plugin-python': textwrap.dedent(f'''
-                def issue_7165(_, callback, _):
-                    callback.msiRemoveUserFromGroup('{group}', '{self.user0.username}', '')
+            'irods_rule_engine_plugin-python': textwrap.dedent('''
+                def main(rule_args, callback, rei):
+                    try:
+                        callback.msiRemoveUserFromGroup('{0}', '{1}', '{2}')
+                    except Exception as e:
+                        callback.writeLine('stderr', str(e))
+
+                INPUT null
+                OUTPUT ruleExecOut
             '''),
         }
 
+        plugin = IrodsConfig().default_rule_engine_plugin
+        rule_file_path = os.path.join(
+            self.admin.local_session_dir,
+            'test_msiRemoveUserFromGroup_returns_error_when_attempting_to_remove_user_from_group_which_they_are_not_a_member_of__issue_7165.r')
+
         try:
             # Create a new group.
             self.admin.assert_icommand(['iadmin', 'mkgroup', group])
             self.admin.assert_icommand(['iadmin', 'lg', group], 'STDOUT', ['No rows found'])
 
             # Remove the user from the group using the microservice.
             rep_instance = plugin_name + '-instance'
-            self.admin.assert_icommand(['irule', '-r', rep_instance, rule_map[plugin_name], 'null', 'ruleExecOut'], 'STDERR', ['-819000 CAT_SUCCESS_BUT_WITH_NO_INFO'])
+            with open(rule_file_path, 'w') as rule_file:
+                rule_file.write(rule_map[plugin].format(group, self.user0.username, ''))
+            self.admin.assert_icommand(['irule', '-r', rep_instance, '-F', rule_file_path], 'STDERR', ['-1830000', 'USER_NOT_IN_GROUP'])
 
         finally:
             self.admin.run_icommand(['iadmin', 'rmgroup', group])

diff --git a/scripts/irods/test/test_iadmin.py b/scripts/irods/test/test_iadmin.py
@@ -1457,11 +1457,11 @@ def test_group_membership(self):
         self.admin.assert_icommand(["iadmin", "mkgroup", "g1"])
         self.admin.assert_icommand(["iadmin", "atg", "g1", self.user0.username])
         self.admin.assert_icommand(["iadmin", "atg", "g1a", self.user0.username], 'STDERR_SINGLELINE', 'CAT_INVALID_GROUP')
-        self.admin.assert_icommand(["iadmin", "rfg", "g1", self.user1.username])
+        self.admin.assert_icommand(["iadmin", "rfg", "g1", self.user1.username], 'STDERR', '-1830000 USER_NOT_IN_GROUP')
         self.admin.assert_icommand(["iadmin", "atg", "g1", self.user1.username])
         self.admin.assert_icommand(["iadmin", "rfg", "g1", self.user0.username])
         self.admin.assert_icommand(["iadmin", "rfg", "g1", self.user1.username])
-        self.admin.assert_icommand(["iadmin", "rfg", "g1", self.user1.username])
+        self.admin.assert_icommand(["iadmin", "rfg", "g1", self.user1.username], 'STDERR', '-1830000 USER_NOT_IN_GROUP')
         self.admin.assert_icommand(["iadmin", "rmgroup", "g1"])
 
     def test_idempotent_aua__issue_3104(self):