improve permissison handling in invoice screen (#2965)

assets/js/plugins/KimaiForm.js

@@ -33,46 +33,22 @@ export default class KimaiForm extends KimaiPlugin {
         this.getContainer().getPlugin('date-range-picker').destroyDateRangePicker(formSelector);
     }
 
-    getFormData(form) {
+    /**
+     * @param {HTMLFormElement} form
+     * @param {Object} overwrites
+     * @returns {string}
+     */
+    convertFormDataToQueryString(form, overwrites = {})
+    {
         let serialized = [];
+        let data = new FormData(form);
 
-        // Loop through each field in the form
+        for (const key in overwrites) {
-        for (let i = 0; i < form.elements.length; i++) {
+            data.set(key, overwrites[key]);
-
-            let field = form.elements[i];
-
-            // Don't serialize a couple of field types (button and submit are important to exclude, eg. invoice preview would fail otherwise)
-            if (!field.name || field.disabled || field.type === 'file' || field.type === 'reset' || field.type === 'submit' || field.type === 'button') {
-                continue;
-            }
-
-            // If a multi-select, get all selections
-            if (field.type === 'select-multiple') {
-                for (var n = 0; n < field.options.length; n++) {
-                    if (!field.options[n].selected) {
-                        continue;
-                    }
-                    serialized.push({
-                        name: field.name,
-                        value: field.options[n].value
-                    });
-                }
-            } else if ((field.type !== 'checkbox' && field.type !== 'radio') || field.checked) {
-                serialized.push({
-                    name: field.name,
-                    value: field.value
-                });
-            }
         }
 
-        return serialized;
+        for (let row of data) {
-    }
+            serialized.push(encodeURIComponent(row[0]) + "=" + encodeURIComponent(row[1]));
-
-    convertFormDataToQueryString(formData) {
-        let serialized = [];
-
-        for (let row of formData) {
-            serialized.push(encodeURIComponent(row.name) + "=" + encodeURIComponent(row.value));
         }
 
         return serialized.join('&');

public/build/entrypoints.json

@@ -3,7 +3,7 @@
     "app": {
       "js": [
         "build/runtime.b8e7bb04.js",
-        "build/app.920ba43e.js"
+        "build/app.8609d161.js"
       ],
       "css": [
         "build/app.3bc2b4d9.css"

public/build/manifest.json

@@ -1,6 +1,6 @@
 {
   "build/app.css": "build/app.3bc2b4d9.css",
-  "build/app.js": "build/app.920ba43e.js",
+  "build/app.js": "build/app.8609d161.js",
   "build/invoice.css": "build/invoice.ff32661a.css",
   "build/invoice.js": "build/invoice.19f36eca.js",
   "build/invoice-pdf.css": "build/invoice-pdf.9a7468ef.css",

src/Controller/InvoiceController.php

@@ -48,21 +48,9 @@ use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
  */
 final class InvoiceController extends AbstractController
 {
-    /**
-     * @var ServiceInvoice
-     */
     private $service;
-    /**
-     * @var InvoiceTemplateRepository
-     */
     private $templateRepository;
-    /**
-     * @var InvoiceRepository
-     */
     private $invoiceRepository;
-    /**
-     * @var EventDispatcherInterface
-     */
     private $dispatcher;
 
     public function __construct(ServiceInvoice $service, InvoiceTemplateRepository $templateRepository, InvoiceRepository $invoiceRepository, EventDispatcherInterface $dispatcher)
@@ -130,10 +118,11 @@ final class InvoiceController extends AbstractController
     }
 
     /**
-     * @Route(path="/preview/{customer}/{template}", name="invoice_preview", methods={"GET"})
+     * @Route(path="/preview/{customer}", name="invoice_preview", methods={"GET"})
-     * @Security("is_granted('view_invoice')")
+     * @Security("is_granted('access', customer)")
+     * @Security("is_granted('create_invoice')")
      */
-    public function previewAction(Customer $customer, InvoiceTemplate $template, Request $request, SystemConfiguration $configuration): Response
+    public function previewAction(Customer $customer, Request $request, SystemConfiguration $configuration): Response
     {
         if (!$this->templateRepository->hasTemplate()) {
             return $this->redirectToRoute('invoice');
@@ -143,10 +132,8 @@ final class InvoiceController extends AbstractController
         $form = $this->getToolbarForm($query, $configuration->find('invoice.simple_form'));
         $form->submit($request->query->all(), false);
 
-        if ($form->isValid() && $this->isGranted('create_invoice')) {
+        if ($form->isValid()) {
             try {
-                $query->setTemplate($template);
-                $query->setCustomers([$customer]);
                 $model = $this->service->createModel($query);
 
                 return $this->service->renderInvoiceWithModel($model, $this->dispatcher);
@@ -156,12 +143,15 @@ final class InvoiceController extends AbstractController
             }
         }
 
+        $this->flashFormError($form);
+
         return $this->redirectToRoute('invoice');
     }
 
     /**
      * @Route(path="/save-invoice/{customer}/{template}", name="invoice_create", methods={"GET"})
-     * @Security("is_granted('view_invoice')")
+     * @Security("is_granted('access', customer)")
+     * @Security("is_granted('create_invoice')")
      */
     public function createInvoiceAction(Customer $customer, InvoiceTemplate $template, Request $request, SystemConfiguration $configuration): Response
     {
@@ -173,62 +163,22 @@ final class InvoiceController extends AbstractController
         $form = $this->getToolbarForm($query, $configuration->find('invoice.simple_form'));
         $form->submit($request->query->all(), false);
 
-        if ($form->isValid() && $this->isGranted('create_invoice')) {
+        if ($form->isValid()) {
             $query->setTemplate($template);
             $query->setCustomers([$customer]);
 
             return $this->renderInvoice($query, $request);
         }
 
-        return $this->redirectToRoute('invoice');
+        $this->flashFormError($form);
-    }
-
-    private function getDefaultQuery(): InvoiceQuery
-    {
-        $factory = $this->getDateTimeFactory();
-        $begin = $factory->getStartOfMonth();
-        $end = $factory->getEndOfMonth();
-
-        $query = new InvoiceQuery();
-        $query->setBegin($begin);
-        $query->setEnd($end);
-        // limit access to data from teams
-        $query->setCurrentUser($this->getUser());
-
-        if (!$this->isGranted('view_other_timesheet')) {
-            // limit access to own data
-            $query->setUser($this->getUser());
-        }
-
-        return $query;
-    }
-
-    private function renderInvoice(InvoiceQuery $query, Request $request)
-    {
-        // use the current request locale as fallback, if no translation was configured
-        if (null !== $query->getTemplate() && null === $query->getTemplate()->getLanguage()) {
-            $query->getTemplate()->setLanguage($request->getLocale());
-        }
-
-        try {
-            $invoices = $this->service->createInvoices($query, $this->dispatcher);
-
-            $this->flashSuccess('action.update.success');
-
-            if (\count($invoices) === 1) {
-                return $this->redirectToRoute('admin_invoice_list', ['id' => $invoices[0]->getId()]);
-            }
-
-            return $this->redirectToRoute('admin_invoice_list');
-        } catch (Exception $ex) {
-            $this->flashUpdateException($ex);
-        }
 
         return $this->redirectToRoute('invoice');
     }
 
     /**
      * @Route(path="/change-status/{id}/{status}", name="admin_invoice_status", methods={"GET", "POST"})
+     * @Security("is_granted('access', invoice.getCustomer())")
+     * @Security("is_granted('create_invoice')")
      */
     public function changeStatusAction(Invoice $invoice, string $status, Request $request): Response
     {
@@ -256,6 +206,8 @@ final class InvoiceController extends AbstractController
 
     /**
      * @Route(path="/delete/{id}/{token}", name="admin_invoice_delete", methods={"GET"})
+     * @Security("is_granted('access', invoice.getCustomer())")
+     * @Security("is_granted('delete_invoice')")
      */
     public function deleteInvoiceAction(Invoice $invoice, string $token, CsrfTokenManagerInterface $csrfTokenManager): Response
     {
@@ -279,6 +231,8 @@ final class InvoiceController extends AbstractController
 
     /**
      * @Route(path="/download/{id}", name="admin_invoice_download", methods={"GET"})
+     * @Security("is_granted('access', invoice.getCustomer())")
+     * @Security("is_granted('create_invoice')")
      */
     public function downloadAction(Invoice $invoice): Response
     {
@@ -295,6 +249,7 @@ final class InvoiceController extends AbstractController
 
     /**
      * @Route(path="/show/{page}", defaults={"page": 1}, requirements={"page": "[1-9]\d*"}, name="admin_invoice_list", methods={"GET"})
+     * @Security("is_granted('view_invoice')")
      */
     public function showInvoicesAction(Request $request, int $page): Response
     {
@@ -325,6 +280,7 @@ final class InvoiceController extends AbstractController
 
     /**
      * @Route(path="/export", name="invoice_export", methods={"GET"})
+     * @Security("is_granted('view_invoice')")
      */
     public function exportAction(Request $request, AnnotatedObjectExporter $exporter)
     {
@@ -474,6 +430,60 @@ final class InvoiceController extends AbstractController
         return $this->redirectToRoute('admin_invoice_template');
     }
 
+    private function getDefaultQuery(): InvoiceQuery
+    {
+        $factory = $this->getDateTimeFactory();
+        $begin = $factory->getStartOfMonth();
+        $end = $factory->getEndOfMonth();
+
+        $query = new InvoiceQuery();
+        $query->setBegin($begin);
+        $query->setEnd($end);
+        // limit access to data from teams
+        $query->setCurrentUser($this->getUser());
+
+        if (!$this->isGranted('view_other_timesheet')) {
+            // limit access to own data
+            $query->setUser($this->getUser());
+        }
+
+        return $query;
+    }
+
+    private function renderInvoice(InvoiceQuery $query, Request $request)
+    {
+        // use the current request locale as fallback, if no translation was configured
+        if (null !== $query->getTemplate() && null === $query->getTemplate()->getLanguage()) {
+            $query->getTemplate()->setLanguage($request->getLocale());
+        }
+
+        try {
+            $invoices = $this->service->createInvoices($query, $this->dispatcher);
+
+            $this->flashSuccess('action.update.success');
+
+            if (\count($invoices) === 1) {
+                return $this->redirectToRoute('admin_invoice_list', ['id' => $invoices[0]->getId()]);
+            }
+
+            return $this->redirectToRoute('admin_invoice_list');
+        } catch (Exception $ex) {
+            $this->flashUpdateException($ex);
+        }
+
+        return $this->redirectToRoute('invoice');
+    }
+
+    private function flashFormError(FormInterface $form): void
+    {
+        $err = '';
+        foreach ($form->getErrors(true, true) as $error) {
+            $err .= PHP_EOL . '[' . $error->getOrigin()->getName() . '] ' . $error->getMessage();
+        }
+
+        $this->flashError('action.update.error', ['%reason%' => $err]);
+    }
+
     private function renderTemplateForm(InvoiceTemplate $template, Request $request): Response
     {
         $editForm = $this->createEditForm($template);

src/Repository/CustomerRepository.php

@@ -241,7 +241,8 @@ class CustomerRepository extends EntityRepository
 
         $outerQuery = $qb->expr()->orX();
 
-        if ($query->hasCustomers()) {
+        // this is a risk, as a user can manipulate the query and inject IDs that would be hidden otherwise
+        if ($query->isAllowCustomerPreselect() && $query->hasCustomers()) {
             $outerQuery->add($qb->expr()->in('c.id', ':customer'));
             $qb->setParameter('customer', $query->getCustomers());
         }

src/Repository/Query/CustomerFormTypeQuery.php

@@ -20,6 +20,7 @@ final class CustomerFormTypeQuery extends BaseFormTypeQuery
      * @var Customer|null
      */
     private $customerToIgnore;
+    private $allowCustomerPreselect = false;
 
     /**
      * @param Customer|int|null $customer
@@ -34,6 +35,16 @@ final class CustomerFormTypeQuery extends BaseFormTypeQuery
         }
     }
 
+    public function isAllowCustomerPreselect(): bool
+    {
+        return $this->allowCustomerPreselect;
+    }
+
+    public function setAllowCustomerPreselect(bool $allowCustomerPreselect): void
+    {
+        $this->allowCustomerPreselect = $allowCustomerPreselect;
+    }
+
     /**
      * @return Customer|null
      */

src/Voter/CustomerVoter.php

@@ -34,6 +34,7 @@ final class CustomerVoter extends Voter
         'comments',
         'comments_create',
         'details',
+        'access',
     ];
 
     private $permissionManager;
@@ -75,6 +76,22 @@ final class CustomerVoter extends Voter
             return false;
         }
 
+        // this is a virtual permission, only meant to be used by developer
+        // it checks if access to the given customer is potentially possible
+        if ($attribute === 'access') {
+            if ($subject->getTeams()->count() === 0) {
+                return true;
+            }
+
+            foreach ($subject->getTeams() as $team) {
+                if ($user->isInTeam($team)) {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
         if ($this->permissionManager->hasRolePermission($user, $attribute . '_customer')) {
             return true;
         }

src/Voter/UserVoter.php

@@ -29,7 +29,6 @@ final class UserVoter extends Voter
         'preferences',
         'api-token',
         'hourly-rate',
-        // teams_own_profile could be merged with view_team_member
         'view_team_member',
     ];
 

templates/invoice/index.html.twig

@@ -109,8 +109,8 @@
                                 </td>
                                 <td class="w-min text-center">
                                     {{ widgets.action_button('show', {'url': '#invoice_preview_details_' ~ model.customer.id, 'title': 'timesheet.all'|trans, 'class': 'btn btn-sm hidden-xs hidden-sm'}, 'link') }}
-                                    {{ widgets.action_button('print', {'url': '#', 'onclick': 'return singleInvoice(this)', 'title': 'button.preview'|trans, 'target': '_blank', 'class': 'btn btn-sm', 'attr': {'data-href': path('invoice_preview', {'customer': model.customer.id, 'template': model.template.id})}}) }}
+                                    {{ widgets.action_button('print', {'url': '#', 'onclick': 'return singleInvoice(this)', 'title': 'button.preview'|trans, 'target': '_blank', 'class': 'btn btn-sm', 'attr': {'data-customer': model.customer.id, 'data-template': model.template.id, 'data-href': path('invoice_preview', {'customer': model.customer.id})}}) }}
-                                    {{ widgets.action_button('save', {'url': '#', 'onclick': 'return singleInvoice(this)', 'title': 'action.save'|trans, 'class': 'btn btn-sm', 'attr': {'data-href': path('invoice_create', {'customer': model.customer.id, 'template': model.template.id})}}, 'success') }}
+                                    {{ widgets.action_button('save', {'url': '#', 'onclick': 'return singleInvoice(this)', 'title': 'action.save'|trans, 'class': 'btn btn-sm', 'attr': {'data-customer': model.customer.id, 'data-template': model.template.id, 'data-href': path('invoice_create', {'customer': model.customer.id, 'template': model.template.id})}}, 'success') }}
                                 </td>
                                 <td class="w-min text-right hidden-xs">
                                     {{ model.calculator.timeWorked|duration(isDecimal) }}
@@ -214,20 +214,20 @@
 
         function singleInvoice(link)
         {
-            let formPlugin = kimai.getPlugin('form');
+            const formPlugin = kimai.getPlugin('form');
-            let data = formPlugin.getFormData(document.getElementById('{{ formId }}'));
+            const overwrites = {'customers[]': link.dataset['customer'], 'template': link.dataset['template']};
-            let uri = formPlugin.convertFormDataToQueryString(data);
+            const uri = formPlugin.convertFormDataToQueryString(document.getElementById('{{ formId }}'), overwrites);
-            let baseUrl = link.getAttribute('data-href');
+
-            link.href = baseUrl + '?' + uri;
+            link.href = link.dataset['href'] + '?' + uri;
 
             return true;
         }
 
         function saveAllInvoices(link)
         {
-            let formPlugin = kimai.getPlugin('form');
+            const formPlugin = kimai.getPlugin('form');
-            let data = formPlugin.getFormData(document.getElementById('{{ formId }}'));
+            const uri = formPlugin.convertFormDataToQueryString(document.getElementById('{{ formId }}'));
-            let uri = formPlugin.convertFormDataToQueryString(data);
+
             link.href = '{{ path('invoice') }}?createInvoice=true&' + uri;
 
             return true;

tests/Controller/InvoiceControllerTest.php

@@ -207,7 +207,7 @@ class InvoiceControllerTest extends ControllerBaseTest
         }
     }
 
-    public function testPrintAction()
+    public function testPreviewAction()
     {
         $client = $this->getClientForAuthenticatedUser(User::ROLE_TEAMLEAD);
 
@@ -233,9 +233,11 @@ class InvoiceControllerTest extends ControllerBaseTest
         $params = [
             'daterange' => $dateRange,
             'projects' => [1],
+            'template' => $id,
+            'customers[]' => 1
         ];
 
-        $action = '/invoice/preview/1/' . $id . '?' . http_build_query($params);
+        $action = '/invoice/preview/1?' . http_build_query($params);
         $this->request($client, $action);
         $this->assertTrue($client->getResponse()->isSuccessful());
         $node = $client->getCrawler()->filter('body');
@@ -243,7 +245,7 @@ class InvoiceControllerTest extends ControllerBaseTest
         $this->assertEquals('invoice_print', $node->getIterator()[0]->getAttribute('class'));
     }
 
-    public function testCreateActionAsAdminWithDownloadAndStatusChangeAndDelete()
+    public function testCreateActionAsAdminWithDownloadAndStatusChange()
     {
         $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN);
 
@@ -352,12 +354,6 @@ class InvoiceControllerTest extends ControllerBaseTest
         $this->assertIsRedirect($client, '/invoice/show');
         $client->followRedirect();
         $this->assertTrue($client->getResponse()->isSuccessful());
-
-        // this does not delete the invoice, because the token is wrong
-        $this->request($client, '/invoice/delete/' . $id . '/fghfkjhgkjhg');
-        $this->assertIsRedirect($client, '/invoice/show');
-        $client->followRedirect();
-        $this->assertTrue($client->getResponse()->isSuccessful());
     }
 
     public function testEditTemplateAction()

tests/Repository/Query/CustomerFormTypeQueryTest.php

@@ -25,6 +25,9 @@ class CustomerFormTypeQueryTest extends BaseFormTypeQueryTest
         $this->assertBaseQuery($sut);
 
         $customer = new Customer();
+        self::assertFalse($sut->isAllowCustomerPreselect());
+        $sut->setAllowCustomerPreselect(true);
+        self::assertTrue($sut->isAllowCustomerPreselect());
         self::assertNull($sut->getCustomerToIgnore());
         self::assertInstanceOf(CustomerFormTypeQuery::class, $sut->setCustomerToIgnore($customer));
         self::assertSame($customer, $sut->getCustomerToIgnore());

tests/Voter/CustomerVoterTest.php

@@ -118,4 +118,50 @@ class CustomerVoterTest extends AbstractVoterTest
 
         $this->assertVote($user, $customer, 'edit', VoterInterface::ACCESS_GRANTED);
     }
+
+    public function testAccess()
+    {
+        // ALLOW: customer has no teams
+        $this->assertVote(new User(), new Customer(), 'access', VoterInterface::ACCESS_GRANTED);
+
+        // ALLOW: customer has no teams
+        $user = new User();
+        $user->addTeam(new Team());
+        $this->assertVote($user, new Customer(), 'access', VoterInterface::ACCESS_GRANTED);
+
+        // ALLOW: user and customer are in the same team (as teamlead)
+        $team = new Team();
+        $user = new User();
+        $team->addTeamlead($user);
+
+        $customer = new Customer();
+        $customer->addTeam($team);
+
+        $this->assertVote($user, $customer, 'access', VoterInterface::ACCESS_GRANTED);
+
+        // ALLOW: user and customer are in the same team (as member)
+        $team = new Team();
+        $user = new User();
+        $user->addTeam(new Team());
+        $user->addTeam($team);
+
+        $customer = new Customer();
+        $customer->addTeam($team);
+
+        $this->assertVote($user, $customer, 'access', VoterInterface::ACCESS_GRANTED);
+
+        // DENY: customer has a team, user not
+        $customer = new Customer();
+        $customer->addTeam(new Team());
+
+        $this->assertVote(new User(), $customer, 'access', VoterInterface::ACCESS_DENIED);
+
+        // DENY: user and customer has a team are not in the same team
+        $user = new User();
+        $user->addTeam(new Team());
+        $customer = new Customer();
+        $customer->addTeam(new Team());
+
+        $this->assertVote($user, $customer, 'access', VoterInterface::ACCESS_DENIED);
+    }
 }