#342 added admin_users to script configuration

bugy · bugy · commit 5accf8033598 · 2020-11-01T13:03:57.000+01:00
diff --git a/src/config/config_service.py b/src/config/config_service.py
@@ -49,6 +49,9 @@ def load_config(self, name, user):
         if config_object.get('name') is None:
             config_object['name'] = short_config.name
 
+        if not self._can_edit_script(user, short_config):
+            raise ConfigNotAllowedException(str(user) + ' has no admin access to ' + short_config.name)
+
         return {'config': config_object, 'filename': os.path.basename(path)}
 
     def create_config(self, user, config):
@@ -84,6 +87,9 @@ def update_config(self, user, config, filename):
         if (found_config_path is not None) and (os.path.basename(found_config_path) != filename):
             raise InvalidConfigException('Another script found with the same name: ' + name)
 
+        if (short_config is not None) and not self._can_edit_script(user, short_config):
+            raise ConfigNotAllowedException(str(user) + ' is not allowed to modify ' + short_config.name)
+
         LOGGER.info('Updating script config "' + name + '" in ' + original_file_path)
         self._save_config(config, original_file_path)
 
@@ -107,6 +113,9 @@ def load_script(path, content):
                 if short_config is None:
                     return None
 
+                if edit_mode and (not conf_service._can_edit_script(user, short_config)):
+                    return None
+
                 if (not edit_mode) and (not conf_service._can_access_script(user, short_config)):
                     return None
 
@@ -196,14 +205,17 @@ def _load_script_config(self, path, content_or_json_dict, user, parameter_values
     def _can_access_script(self, user, short_config):
         return self._authorizer.is_allowed(user.user_id, short_config.allowed_users)
 
+    def _can_edit_script(self, user, short_config):
+        return self._authorizer.is_allowed(user.user_id, short_config.admin_users)
+
     def _check_admin_access(self, user):
         if not self._authorizer.is_admin(user.user_id):
             raise AdminAccessRequiredException('Admin access to scripts is prohibited for ' + str(user))
 
 
 class ConfigNotAllowedException(Exception):
-    def __init__(self):
-        pass
+    def __init__(self, message=None):
+        super().__init__(message)
 
 
 class AdminAccessRequiredException(Exception):
diff --git a/src/model/script_config.py b/src/model/script_config.py
@@ -20,6 +20,7 @@ class ShortConfig(object):
     def __init__(self):
         self.name = None
         self.allowed_users = []
+        self.admin_users = []
         self.group = None
 
 
@@ -256,6 +257,7 @@ def read_short(file_path, json_object):
 
     config.name = _read_name(file_path, json_object)
     config.allowed_users = json_object.get('allowed_users')
+    config.admin_users = json_object.get('admin_users')
     config.group = read_str_from_config(json_object, 'group', blank_to_none=True)
 
     hidden = read_bool_from_config('hidden', json_object, default=False)
@@ -267,6 +269,11 @@ def read_short(file_path, json_object):
     elif (config.allowed_users == '*') or ('*' in config.allowed_users):
         config.allowed_users = ANY_USER
 
+    if config.admin_users is None:
+        config.admin_users = ANY_USER
+    elif (config.admin_users == '*') or ('*' in config.admin_users):
+        config.admin_users = ANY_USER
+
     return config
 
 
@@ -352,7 +359,15 @@ def get(self):
 
 
 def get_sorted_config(config):
-    key_order = ['name', 'script_path', 'working_directory', 'hidden', 'description', 'allowed_users', 'include',
+    key_order = ['name', 'script_path',
+                 'working_directory',
+                 'hidden',
+                 'description',
+                 'group',
+                 'allowed_users',
+                 'admin_users',
+                 'schedulable',
+                 'include',
                  'output_files', 'requires_terminal', 'bash_formatting', 'parameters']
 
     def get_order(key):
diff --git a/src/tests/config_service_test.py b/src/tests/config_service_test.py
@@ -138,6 +138,12 @@ def test_list_configs_when_edit_mode_and_admin_without_allowance(self):
 
         self.assert_list_config_names(self.admin_user, ['a1', 'c2'], mode='edit')
 
+    def test_list_configs_when_edit_mode_and_admin_not_in_admin_users(self):
+        _create_script_config_file('a1', admin_users=['user1'])
+        _create_script_config_file('c2', admin_users=['adm_user'])
+
+        self.assert_list_config_names(self.admin_user, ['c2'], mode='edit')
+
     def test_list_configs_when_edit_mode_and_non_admin(self):
         _create_script_config_file('a1', allowed_users=['user1'])
         _create_script_config_file('c2', allowed_users=['user1'])
@@ -266,6 +272,14 @@ def test_insert_sorted_values(self):
                                                            ('requires_terminal', False),
                                                            ('parameters', [{'name': 'param1'}])]))
 
+    def test_create_config_with_admin_users(self):
+        config = _prepare_script_config_object('conf1',
+                                               description='My wonderful test config',
+                                               admin_users=['another_user'])
+        self.config_service.create_config(self.admin_user, config)
+
+        _validate_config(self, 'conf1.json', config)
+
 
 class ConfigServiceUpdateConfigTest(unittest.TestCase):
 
@@ -375,6 +389,32 @@ def test_update_sorted_values(self):
                             ('parameters', [{'name': 'param1'}])])
         _validate_config(self, 'confX.json', body)
 
+    def test_update_config_allowed_admin_user(self):
+        config = _prepare_script_config_object('Conf X',
+                                               description='My wonderful test config',
+                                               admin_users=['admin_user'])
+        self.config_service.update_config(self.admin_user, config, 'confX.json')
+
+        new_config = _prepare_script_config_object('Conf X',
+                                                   description='New desc')
+        self.config_service.update_config(self.admin_user, new_config, 'confX.json')
+
+        _validate_config(self, 'confX.json', new_config)
+
+    def test_update_config_different_admin_user(self):
+        config = _prepare_script_config_object('Conf X',
+                                               description='My wonderful test config',
+                                               admin_users=['another_user'])
+        self.config_service.update_config(self.admin_user, config, 'confX.json')
+
+        new_config = _prepare_script_config_object('Conf X',
+                                                   description='New desc',
+                                                   admin_users=['admin_user'])
+        self.assertRaisesRegex(ConfigNotAllowedException, 'is not allowed to modify',
+                               self.config_service.update_config, self.admin_user, new_config, 'confX.json')
+
+        _validate_config(self, 'confX.json', config)
+
 
 class ConfigServiceLoadConfigForAdminTest(unittest.TestCase):
     def setUp(self):
@@ -413,6 +453,15 @@ def test_load_config_when_not_exists(self):
         config = self.config_service.load_config('ConfX', self.admin_user)
         self.assertIsNone(config)
 
+    def test_load_config_when_script_has_admin_users(self):
+        _create_script_config_file('ConfX', admin_users=['admin_user'])
+        config = self.config_service.load_config('ConfX', self.admin_user)
+        self.assertEqual(config['filename'], 'ConfX.json')
+
+    def test_load_config_when_script_has_different_admin_users(self):
+        _create_script_config_file('ConfX', admin_users=['admin_user2'])
+        self.assertRaises(ConfigNotAllowedException, self.config_service.load_config, 'ConfX', self.admin_user)
+
 
 def _create_script_config_file(filename, *, name=None, **kwargs):
     conf_folder = os.path.join(test_utils.temp_folder, 'runners')
diff --git a/src/web/server.py b/src/web/server.py
@@ -241,13 +241,22 @@ def put(self, user):
             self.application.config_service.update_config(user, config, filename)
         except (InvalidConfigException, InvalidFileException) as e:
             raise tornado.web.HTTPError(422, str(e))
+        except ConfigNotAllowedException:
+            LOGGER.warning('Admin access to the script "' + config['name'] + '" is denied for ' + user.get_audit_name())
+            respond_error(self, 403, 'Access to the script is denied')
+            return
 
 
 class AdminGetScriptEndpoint(BaseRequestHandler):
     @requires_admin_rights
     @inject_user
     def get(self, user, script_name):
-        config = self.application.config_service.load_config(script_name, user)
+        try:
+            config = self.application.config_service.load_config(script_name, user)
+        except ConfigNotAllowedException:
+            LOGGER.warning('Admin access to the script "' + script_name + '" is denied for ' + user.get_audit_name())
+            respond_error(self, 403, 'Access to the script is denied')
+            return
 
         if config is None:
             raise tornado.web.HTTPError(404, str('Failed to find config for name: ' + script_name))
diff --git a/web-src/src/admin/components/scripts-config/ScriptConfigForm.vue b/web-src/src/admin/components/scripts-config/ScriptConfigForm.vue
@@ -21,6 +21,16 @@
                       v-model="allowAllUsers"/>
         </div>
 
+        <div class="row">
+            <div v-if="allowAllAdmins" class="input-field col s9">
+                <input id="admin_users_disabled" disabled type="text" value="Any admin">
+                <label class="active" for="admin_users_disabled">Admin users</label>
+            </div>
+            <ChipsList v-else v-model="adminUsers" class="col s9" title="Admin users"/>
+            <CheckBox v-model="allowAllAdmins" :config="allowAllAdminsField"
+                      class="col s2 offset-s1 checkbox-field"/>
+        </div>
+
         <div class="row">
             <CheckBox :config="bashFormattingField" class="col s3 checkbox-field" v-model="bashFormatting"/>
             <CheckBox :config="requiresTerminalField" class="col s3 checkbox-field"
@@ -48,6 +58,7 @@
         scriptPathField,
         workDirField
     } from './script-fields';
+    import {allowAllAdminsField} from "@/admin/components/scripts-config/script-fields";
 
     export default {
         name: 'ScriptConfigForm',
@@ -73,12 +84,15 @@
                 bashFormatting: null,
                 allowedUsers: [],
                 allowAllUsers: true,
+                adminUsers: [],
+                allowAllAdmins: true,
                 nameField,
                 groupField,
                 scriptPathField,
                 workDirField,
                 descriptionField,
                 allowAllField,
+                allowAllAdminsField,
                 bashFormattingField,
                 requiresTerminalField,
                 includeScriptField
@@ -120,38 +134,64 @@
                     this.requiresTerminal = get(config, 'requires_terminal', true);
                     this.includeScript = config['include'];
                     this.bashFormatting = get(config, 'bash_formatting', true);
-                    let allowedUsers = get(config, 'allowed_users');
-                    if (isNull(allowedUsers)) {
-                        allowedUsers = [];
-                    }
-                    this.allowedUsers = allowedUsers.filter(u => u !== '*');
-                    this.allowAllUsers = isNull(config['allowed_users']) || allowedUsers.includes('*');
+                    this.updateAccessFieldInVm(config,
+                        'allowedUsers',
+                        'allowAllUsers',
+                        'allowed_users')
+
+                    this.updateAccessFieldInVm(config,
+                        'adminUsers',
+                        'allowAllAdmins',
+                        'admin_users')
                 }
             },
             allowAllUsers() {
                 this.updateAllowedUsers();
             },
             allowedUsers() {
                 this.updateAllowedUsers();
+            },
+            allowAllAdmins() {
+                this.updateAdminUsers();
+            },
+            adminUsers() {
+                this.updateAdminUsers();
             }
         },
 
         methods: {
             updateAllowedUsers() {
-                if (this.allowAllUsers) {
-                    if (isEmptyArray(this.allowedUsers)) {
-                        this.$delete(this.value, 'allowed_users');
-                    } else {
-                        if (this.allowedUsers.includes('*')) {
-                            this.value['allowed_users'] = this.allowedUsers;
+                this.updateAccessFieldInValue(this.allowAllUsers, 'allowedUsers', 'allowed_users');
+            },
+            updateAdminUsers() {
+                this.updateAccessFieldInValue(this.allowAllAdmins, 'adminUsers', 'admin_users');
+            },
+            updateAccessFieldInValue(allowAll, vmPropertyName, valuePropertyName) {
+                const newValue = this[vmPropertyName];
+
+                if (isEmptyArray(newValue)) {
+                    this.$delete(this.value, valuePropertyName);
+                } else {
+                    if (allowAll) {
+                        if (newValue.includes('*')) {
+                            this.value[valuePropertyName] = newValue;
                         } else {
-                            this.value['allowed_users'] = [...this.allowedUsers, '*'];
+                            this.value[valuePropertyName] = [...newValue, '*'];
                         }
+                    } else {
+                        this.value[valuePropertyName] = newValue;
                     }
-                } else {
-                    this.value['allowed_users'] = this.allowedUsers;
                 }
-            }
+            },
+            updateAccessFieldInVm(config, vmPropertyName, vmAllowAllPropertyName, valuePropertyName) {
+                let users = get(config, valuePropertyName);
+                if (isNull(users)) {
+                    users = [];
+                }
+                this[vmPropertyName] = users.filter(u => u !== '*');
+                this[vmAllowAllPropertyName] = isNull(config[valuePropertyName]) || users.includes('*');
+            },
+
         }
     }
 </script>
diff --git a/web-src/src/admin/components/scripts-config/script-fields.js b/web-src/src/admin/components/scripts-config/script-fields.js
@@ -19,6 +19,9 @@ export const workDirField = {
 export const allowAllField = {
     name: 'Allow all'
 };
+export const allowAllAdminsField = {
+    name: 'Any admin'
+};
 export const bashFormattingField = {
     name: 'Bash formatting',
     description: 'Enable ANSI escape sequences for text formatting and cursor moves'
diff --git a/web-src/tests/unit/admin/ScriptConfig_test.js b/web-src/tests/unit/admin/ScriptConfig_test.js
@@ -100,4 +100,28 @@ describe('Test ScriptConfig', function () {
         });
     });
 
+    describe('Test edit allowed_users', function () {
+        it('Test edit allowed_users manually', async function () {
+            await _setValueByUser('Allow all', false);
+            await _setValueByUser('Allowed users', ['user A', 'user B']);
+
+            expect(store.state.scriptConfig.scriptConfig.allowed_users).toEqual(['user A', 'user B'])
+        });
+    });
+
+    describe('Test edit admin_users', function () {
+        it('Test edit admin_users manually', async function () {
+            await _setValueByUser('Any admin', false);
+            await _setValueByUser('Admin users', ['user A', 'user B']);
+
+            expect(store.state.scriptConfig.scriptConfig.admin_users).toEqual(['user A', 'user B'])
+        });
+
+        it('Test set any admin = false without any user, manually', async function () {
+            await _setValueByUser('Any admin', false);
+
+            expect(store.state.scriptConfig.scriptConfig.admin_users).toBeNil()
+        });
+    });
+
 });
