From 3b59dadb0e60b3379dc2230ffd06c49eaa2e5ef3 Mon Sep 17 00:00:00 2001 From: Nik Okuntseff Date: Thu, 6 Dec 2018 19:22:54 +0000 Subject: [PATCH] Improvements to group editor - access checks and usability. --- WEB-INF/lib/ttGroupHelper.class.php | 2 +- WEB-INF/templates/footer.tpl | 2 +- WEB-INF/templates/group_edit.tpl | 2 +- group_delete.php | 18 ++-- group_edit.php | 132 +++++++++++++++------------- 5 files changed, 86 insertions(+), 70 deletions(-) diff --git a/WEB-INF/lib/ttGroupHelper.class.php b/WEB-INF/lib/ttGroupHelper.class.php index e5288a09..1a32124c 100644 --- a/WEB-INF/lib/ttGroupHelper.class.php +++ b/WEB-INF/lib/ttGroupHelper.class.php @@ -91,7 +91,7 @@ class ttGroupHelper { $name = $fields['name']; $description = $fields['description']; - // We need to inherit other attributes from the parent group. + // We need to inherit attributes from the parent group. $attrs = ttGroupHelper::getGroupAttrs($parent_id); $columns = '(parent_id, org_id, name, description, currency, decimal_mark, lang, date_format, time_format'. diff --git a/WEB-INF/templates/footer.tpl b/WEB-INF/templates/footer.tpl index 9d8323c4..e57d10e1 100644 --- a/WEB-INF/templates/footer.tpl +++ b/WEB-INF/templates/footer.tpl @@ -12,7 +12,7 @@
-
 Anuko Time Tracker 1.18.29.4597 | Copyright © Anuko | +  Anuko Time Tracker 1.18.29.4598 | Copyright © Anuko | {$i18n.footer.credits} | {$i18n.footer.license} | {$i18n.footer.improve} diff --git a/WEB-INF/templates/group_edit.tpl b/WEB-INF/templates/group_edit.tpl index f839d7e9..bb83d3a3 100644 --- a/WEB-INF/templates/group_edit.tpl +++ b/WEB-INF/templates/group_edit.tpl @@ -27,7 +27,7 @@ function handleTaskRequiredCheckbox() { {if isTrue($smarty.const.SUBGROUP_DEBUG)} -{if $user->can('manage_subgroups')} +{if $user->can('manage_subgroups') && $group_dropdown} diff --git a/group_delete.php b/group_delete.php index db24a448..2865351a 100644 --- a/group_delete.php +++ b/group_delete.php @@ -31,17 +31,21 @@ import('form.Form'); import('ttGroupHelper'); // Access checks. -if (!ttAccessAllowed('delete_group')) { +if (!(ttAccessAllowed('delete_group') || ttAccessAllowed('manage_subgroups'))) { header('Location: access_denied.php'); // No rights. exit(); } -if (!$user->isGroupValid($request->getParameter('id'))) { +$group_id = (int)$request->getParameter('id'); +if (!$user->isGroupValid($group_id)) { header('Location: access_denied.php'); // Wrong group id. exit(); } +if ($group_id == $user->group_id && !$user->can('delete_group')) { + header('Location: access_denied.php'); // Trying to delete home group without right. + exit(); +} // End of access checks. -$group_id = (int)$request->getParameter('id'); $group_name = ttGroupHelper::getGroupName($group_id); $form = new Form('groupForm'); @@ -53,7 +57,6 @@ if ($request->isPost()) { if ($request->getParameter('btn_delete')) { $markedDeleted = ttGroupHelper::markGroupDeleted($group_id); if ($markedDeleted) { - // TODO: conditional redirects don't look nice. Any better ideas? if ($group_id == $user->group_id) { // We marked deleted our own group. Logout and redirect to login page. $auth->doLogout(); @@ -61,8 +64,10 @@ if ($request->isPost()) { header('Location: login.php'); exit(); } else { - // We marked deleted a subgroup. Redirect to groups.pgp. - header('Location: groups.php'); + // We marked deleted a subgroup. + if ($user->behalfGroup && $user->behalfGroup->id == $group_id) + $user->setOnBehalfGroup($user->group_id); // Remove on behalf group from session. + header('Location: success.php'); exit(); } } else @@ -70,7 +75,6 @@ if ($request->isPost()) { } if ($request->getParameter('btn_cancel')) { - // TODO: conditional redirects don't look nice. Any better ideas? if ($group_id == $user->group_id) { header('Location: group_edit.php'); exit(); diff --git a/group_edit.php b/group_edit.php index 44c5d7f9..29bfd801 100644 --- a/group_edit.php +++ b/group_edit.php @@ -33,32 +33,49 @@ import('ttRoleHelper'); import('ttConfigHelper'); // Access checks. -if (!(ttAccessAllowed('manage_basic_settings') || ttAccessAllowed('manage_advanced_settings'))) { - header('Location: access_denied.php'); - exit(); +// There are 4 distinct situations: +// 1) Editing home group in get or post. +// 2) Editing a subgroup in get or post. +// We'll check access separately as it is about different right checks. +if ($request->isGet()) { + $group_id = $request->getParameter('id') ? $request->getParameter('id') : $user->getGroup(); +} else { + $group_id = $request->getParameter('group'); } -$group_id = (int)$request->getParameter('id'); -if ($group_id && !$user->isGroupValid($group_id)) { - header('Location: access_denied.php'); - exit(); +$home_group = $user->group_id == $group_id; +if ($home_group) { + // Editing home group. + if (!(ttAccessAllowed('manage_basic_settings') || ttAccessAllowed('manage_advanced_settings'))) { + header('Location: access_denied.php'); // Not allowed to edit home group settings. + exit(); + } +} else { + // Editing a subgroup. + if (!ttAccessAllowed('manage_subgroups')) { + header('Location: access_denied.php'); // No right to manage subgroups. + exit(); + } + if (!$user->isSubgroupValid($group_id)) { + header('Location: access_denied.php'); // Wrong subgroup. + exit(); + } } // End of access checks. -if ($group_id) { - // We are passed a valid group_id. - // Set on behalf group accordingly. - $user->setOnBehalfGroup($group_id); +// Set on behalf group accordingly. +$groupChanged = $request->getParameter('group_changed'); +if ($request->isPost() && $groupChanged) { + $user->setOnBehalfGroup($group_id); } -if (!$group_id) $group_id = $user->getGroup(); $groups = $user->getGroupsForDropdown(); $group = ttGroupHelper::getGroupAttrs($group_id); $config = new ttConfigHelper($group['config']); -$advanced_settings = $user->can('manage_advanced_settings'); +$advanced_settings = $home_group ? $user->can('manage_advanced_settings') : true; if (!defined('CURRENCY_DEFAULT')) define('CURRENCY_DEFAULT', '$'); -if ($request->isPost()) { +if ($request->isPost() && !$groupChanged) { $cl_group = trim($request->getParameter('group_name')); $cl_description = trim($request->getParameter('description')); $cl_currency = trim($request->getParameter('currency')); @@ -208,13 +225,6 @@ if ($user->can('delete_group')) $form->addInput(array('type'=>'submit','name'=>' $form->setValueByElement('group_changed',''); if ($request->isPost()) { - if ($request->getParameter('group_changed')) { - // User changed the group in dropdown. - $new_group_id = $request->getParameter('group'); - // Redirect to self. - header('Location: group_edit.php?id='.$new_group_id); - exit(); - } if ($request->getParameter('btn_delete')) { // Delete button pressed, redirect. @@ -222,47 +232,49 @@ if ($request->isPost()) { exit(); } - // Validate user input. - if (!ttValidString($cl_group)) $err->add($i18n->get('error.field'), $i18n->get('label.group_name')); - if (!ttValidString($cl_description, true)) $err->add($i18n->get('error.field'), $i18n->get('label.description')); - if (!ttValidString($cl_currency, true)) $err->add($i18n->get('error.field'), $i18n->get('label.currency')); - if ($advanced_settings) { - if (!ttValidEmail($cl_bcc_email, true)) $err->add($i18n->get('error.field'), $i18n->get('label.bcc')); - if (!ttValidIP($cl_allow_ip, true)) $err->add($i18n->get('error.field'), $i18n->get('form.group_edit.allow_ip')); - } - // Finished validating user input. + if ($request->getParameter('btn_save')) { + // Validate user input. + if (!ttValidString($cl_group)) $err->add($i18n->get('error.field'), $i18n->get('label.group_name')); + if (!ttValidString($cl_description, true)) $err->add($i18n->get('error.field'), $i18n->get('label.description')); + if (!ttValidString($cl_currency, true)) $err->add($i18n->get('error.field'), $i18n->get('label.currency')); + if ($advanced_settings) { + if (!ttValidEmail($cl_bcc_email, true)) $err->add($i18n->get('error.field'), $i18n->get('label.bcc')); + if (!ttValidIP($cl_allow_ip, true)) $err->add($i18n->get('error.field'), $i18n->get('form.group_edit.allow_ip')); + } + // Finished validating user input. - if ($err->no()) { - // Update config. - $config->setDefinedValue('show_holidays', $cl_show_holidays); - $config->setDefinedValue('punch_mode', $cl_punch_mode); - $config->setDefinedValue('allow_overlap', $cl_allow_overlap); - $config->setDefinedValue('future_entries', $cl_future_entries); - $config->setDefinedValue('uncompleted_indicators', $cl_uncompleted_indicators); - $config->setDefinedValue('confirm_save', $cl_confirm_save); + if ($err->no()) { + // Update config. + $config->setDefinedValue('show_holidays', $cl_show_holidays); + $config->setDefinedValue('punch_mode', $cl_punch_mode); + $config->setDefinedValue('allow_overlap', $cl_allow_overlap); + $config->setDefinedValue('future_entries', $cl_future_entries); + $config->setDefinedValue('uncompleted_indicators', $cl_uncompleted_indicators); + $config->setDefinedValue('confirm_save', $cl_confirm_save); - if ($user->updateGroup(array( - 'group_id' => $group_id, - 'name' => $cl_group, - 'description' => $cl_description, - 'currency' => $cl_currency, - 'lang' => $cl_lang, - 'decimal_mark' => $cl_decimal_mark, - 'date_format' => $cl_date_format, - 'time_format' => $cl_time_format, - 'week_start' => $cl_start_week, - 'tracking_mode' => $cl_tracking_mode, - 'project_required' => $cl_project_required, - 'task_required' => $cl_task_required, - 'record_type' => $cl_record_type, - 'uncompleted_indicators' => $cl_uncompleted_indicators, - 'bcc_email' => $cl_bcc_email, - 'allow_ip' => $cl_allow_ip, - 'config' => $config->getConfig()))) { - header('Location: success.php'); - exit(); - } else - $err->add($i18n->get('error.db')); + if ($user->updateGroup(array( + 'group_id' => $group_id, + 'name' => $cl_group, + 'description' => $cl_description, + 'currency' => $cl_currency, + 'lang' => $cl_lang, + 'decimal_mark' => $cl_decimal_mark, + 'date_format' => $cl_date_format, + 'time_format' => $cl_time_format, + 'week_start' => $cl_start_week, + 'tracking_mode' => $cl_tracking_mode, + 'project_required' => $cl_project_required, + 'task_required' => $cl_task_required, + 'record_type' => $cl_record_type, + 'uncompleted_indicators' => $cl_uncompleted_indicators, + 'bcc_email' => $cl_bcc_email, + 'allow_ip' => $cl_allow_ip, + 'config' => $config->getConfig()))) { + header('Location: success.php'); + exit(); + } else + $err->add($i18n->get('error.db')); + } } } // isPost -- 2.20.1
{$i18n.label.group}: {$forms.groupForm.group.control}