From: Nik Okuntseff <support@anuko.com>
Date: Thu, 6 Dec 2018 19:22:54 +0000 (+0000)
Subject: Improvements to group editor - access checks and usability.
X-Git-Tag: timetracker_1.19-1~467
X-Git-Url: http://wagnertech.de/git?a=commitdiff_plain;h=3b59dadb0e60b3379dc2230ffd06c49eaa2e5ef3;p=timetracker.git

Improvements to group editor - access checks and usability.
---

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 @@
       <br>
       <table cellspacing="0" cellpadding="4" width="100%" border="0">
         <tr>
-          <td align="center">&nbsp;Anuko Time Tracker 1.18.29.4597 | Copyright &copy; <a href="https://www.anuko.com/lp/tt_3.htm" target="_blank">Anuko</a> |
+          <td align="center">&nbsp;Anuko Time Tracker 1.18.29.4598 | Copyright &copy; <a href="https://www.anuko.com/lp/tt_3.htm" target="_blank">Anuko</a> |
             <a href="https://www.anuko.com/lp/tt_4.htm" target="_blank">{$i18n.footer.credits}</a> |
             <a href="https://www.anuko.com/lp/tt_5.htm" target="_blank">{$i18n.footer.license}</a> |
             <a href="https://www.anuko.com/lp/tt_7.htm" target="_blank">{$i18n.footer.improve}</a>
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() {
       <td>
         <table cellspacing="1" cellpadding="2" border="0">
 {if isTrue($smarty.const.SUBGROUP_DEBUG)}
-{if $user->can('manage_subgroups')}
+{if $user->can('manage_subgroups') && $group_dropdown}
           <tr>
             <td align="right" nowrap>{$i18n.label.group}:</td>
             <td>{$forms.groupForm.group.control}</td>
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