Skip to content

Commit ff9acab

Browse files
authored
improve permissison handling in invoice screen (#2965)
1 parent 76e0944 commit ff9acab

15 files changed

+183
-124
lines changed

Diff for: assets/js/plugins/KimaiForm.js

+12-36
Original file line numberDiff line numberDiff line change
@@ -33,46 +33,22 @@ export default class KimaiForm extends KimaiPlugin {
3333
this.getContainer().getPlugin('date-range-picker').destroyDateRangePicker(formSelector);
3434
}
3535

36-
getFormData(form) {
36+
/**
37+
* @param {HTMLFormElement} form
38+
* @param {Object} overwrites
39+
* @returns {string}
40+
*/
41+
convertFormDataToQueryString(form, overwrites = {})
42+
{
3743
let serialized = [];
44+
let data = new FormData(form);
3845

39-
// Loop through each field in the form
40-
for (let i = 0; i < form.elements.length; i++) {
41-
42-
let field = form.elements[i];
43-
44-
// Don't serialize a couple of field types (button and submit are important to exclude, eg. invoice preview would fail otherwise)
45-
if (!field.name || field.disabled || field.type === 'file' || field.type === 'reset' || field.type === 'submit' || field.type === 'button') {
46-
continue;
47-
}
48-
49-
// If a multi-select, get all selections
50-
if (field.type === 'select-multiple') {
51-
for (var n = 0; n < field.options.length; n++) {
52-
if (!field.options[n].selected) {
53-
continue;
54-
}
55-
serialized.push({
56-
name: field.name,
57-
value: field.options[n].value
58-
});
59-
}
60-
} else if ((field.type !== 'checkbox' && field.type !== 'radio') || field.checked) {
61-
serialized.push({
62-
name: field.name,
63-
value: field.value
64-
});
65-
}
46+
for (const key in overwrites) {
47+
data.set(key, overwrites[key]);
6648
}
6749

68-
return serialized;
69-
}
70-
71-
convertFormDataToQueryString(formData) {
72-
let serialized = [];
73-
74-
for (let row of formData) {
75-
serialized.push(encodeURIComponent(row.name) + "=" + encodeURIComponent(row.value));
50+
for (let row of data) {
51+
serialized.push(encodeURIComponent(row[0]) + "=" + encodeURIComponent(row[1]));
7652
}
7753

7854
return serialized.join('&');

Diff for: public/build/app.8609d161.js

+2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
File renamed without changes.

Diff for: public/build/app.920ba43e.js

-2
This file was deleted.

Diff for: public/build/entrypoints.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"app": {
44
"js": [
55
"build/runtime.b8e7bb04.js",
6-
"build/app.920ba43e.js"
6+
"build/app.8609d161.js"
77
],
88
"css": [
99
"build/app.3bc2b4d9.css"

Diff for: public/build/manifest.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"build/app.css": "build/app.3bc2b4d9.css",
3-
"build/app.js": "build/app.920ba43e.js",
3+
"build/app.js": "build/app.8609d161.js",
44
"build/invoice.css": "build/invoice.ff32661a.css",
55
"build/invoice.js": "build/invoice.19f36eca.js",
66
"build/invoice-pdf.css": "build/invoice-pdf.9a7468ef.css",

Diff for: src/Controller/InvoiceController.php

+73-63
Original file line numberDiff line numberDiff line change
@@ -48,21 +48,9 @@
4848
*/
4949
final class InvoiceController extends AbstractController
5050
{
51-
/**
52-
* @var ServiceInvoice
53-
*/
5451
private $service;
55-
/**
56-
* @var InvoiceTemplateRepository
57-
*/
5852
private $templateRepository;
59-
/**
60-
* @var InvoiceRepository
61-
*/
6253
private $invoiceRepository;
63-
/**
64-
* @var EventDispatcherInterface
65-
*/
6654
private $dispatcher;
6755

6856
public function __construct(ServiceInvoice $service, InvoiceTemplateRepository $templateRepository, InvoiceRepository $invoiceRepository, EventDispatcherInterface $dispatcher)
@@ -130,10 +118,11 @@ public function indexAction(Request $request, SystemConfiguration $configuration
130118
}
131119

132120
/**
133-
* @Route(path="/preview/{customer}/{template}", name="invoice_preview", methods={"GET"})
134-
* @Security("is_granted('view_invoice')")
121+
* @Route(path="/preview/{customer}", name="invoice_preview", methods={"GET"})
122+
* @Security("is_granted('access', customer)")
123+
* @Security("is_granted('create_invoice')")
135124
*/
136-
public function previewAction(Customer $customer, InvoiceTemplate $template, Request $request, SystemConfiguration $configuration): Response
125+
public function previewAction(Customer $customer, Request $request, SystemConfiguration $configuration): Response
137126
{
138127
if (!$this->templateRepository->hasTemplate()) {
139128
return $this->redirectToRoute('invoice');
@@ -143,10 +132,8 @@ public function previewAction(Customer $customer, InvoiceTemplate $template, Req
143132
$form = $this->getToolbarForm($query, $configuration->find('invoice.simple_form'));
144133
$form->submit($request->query->all(), false);
145134

146-
if ($form->isValid() && $this->isGranted('create_invoice')) {
135+
if ($form->isValid()) {
147136
try {
148-
$query->setTemplate($template);
149-
$query->setCustomers([$customer]);
150137
$model = $this->service->createModel($query);
151138

152139
return $this->service->renderInvoiceWithModel($model, $this->dispatcher);
@@ -156,12 +143,15 @@ public function previewAction(Customer $customer, InvoiceTemplate $template, Req
156143
}
157144
}
158145

146+
$this->flashFormError($form);
147+
159148
return $this->redirectToRoute('invoice');
160149
}
161150

162151
/**
163152
* @Route(path="/save-invoice/{customer}/{template}", name="invoice_create", methods={"GET"})
164-
* @Security("is_granted('view_invoice')")
153+
* @Security("is_granted('access', customer)")
154+
* @Security("is_granted('create_invoice')")
165155
*/
166156
public function createInvoiceAction(Customer $customer, InvoiceTemplate $template, Request $request, SystemConfiguration $configuration): Response
167157
{
@@ -173,62 +163,22 @@ public function createInvoiceAction(Customer $customer, InvoiceTemplate $templat
173163
$form = $this->getToolbarForm($query, $configuration->find('invoice.simple_form'));
174164
$form->submit($request->query->all(), false);
175165

176-
if ($form->isValid() && $this->isGranted('create_invoice')) {
166+
if ($form->isValid()) {
177167
$query->setTemplate($template);
178168
$query->setCustomers([$customer]);
179169

180170
return $this->renderInvoice($query, $request);
181171
}
182172

183-
return $this->redirectToRoute('invoice');
184-
}
185-
186-
private function getDefaultQuery(): InvoiceQuery
187-
{
188-
$factory = $this->getDateTimeFactory();
189-
$begin = $factory->getStartOfMonth();
190-
$end = $factory->getEndOfMonth();
191-
192-
$query = new InvoiceQuery();
193-
$query->setBegin($begin);
194-
$query->setEnd($end);
195-
// limit access to data from teams
196-
$query->setCurrentUser($this->getUser());
197-
198-
if (!$this->isGranted('view_other_timesheet')) {
199-
// limit access to own data
200-
$query->setUser($this->getUser());
201-
}
202-
203-
return $query;
204-
}
205-
206-
private function renderInvoice(InvoiceQuery $query, Request $request)
207-
{
208-
// use the current request locale as fallback, if no translation was configured
209-
if (null !== $query->getTemplate() && null === $query->getTemplate()->getLanguage()) {
210-
$query->getTemplate()->setLanguage($request->getLocale());
211-
}
212-
213-
try {
214-
$invoices = $this->service->createInvoices($query, $this->dispatcher);
215-
216-
$this->flashSuccess('action.update.success');
217-
218-
if (\count($invoices) === 1) {
219-
return $this->redirectToRoute('admin_invoice_list', ['id' => $invoices[0]->getId()]);
220-
}
221-
222-
return $this->redirectToRoute('admin_invoice_list');
223-
} catch (Exception $ex) {
224-
$this->flashUpdateException($ex);
225-
}
173+
$this->flashFormError($form);
226174

227175
return $this->redirectToRoute('invoice');
228176
}
229177

230178
/**
231179
* @Route(path="/change-status/{id}/{status}", name="admin_invoice_status", methods={"GET", "POST"})
180+
* @Security("is_granted('access', invoice.getCustomer())")
181+
* @Security("is_granted('create_invoice')")
232182
*/
233183
public function changeStatusAction(Invoice $invoice, string $status, Request $request): Response
234184
{
@@ -256,6 +206,8 @@ public function changeStatusAction(Invoice $invoice, string $status, Request $re
256206

257207
/**
258208
* @Route(path="/delete/{id}/{token}", name="admin_invoice_delete", methods={"GET"})
209+
* @Security("is_granted('access', invoice.getCustomer())")
210+
* @Security("is_granted('delete_invoice')")
259211
*/
260212
public function deleteInvoiceAction(Invoice $invoice, string $token, CsrfTokenManagerInterface $csrfTokenManager): Response
261213
{
@@ -279,6 +231,8 @@ public function deleteInvoiceAction(Invoice $invoice, string $token, CsrfTokenMa
279231

280232
/**
281233
* @Route(path="/download/{id}", name="admin_invoice_download", methods={"GET"})
234+
* @Security("is_granted('access', invoice.getCustomer())")
235+
* @Security("is_granted('create_invoice')")
282236
*/
283237
public function downloadAction(Invoice $invoice): Response
284238
{
@@ -295,6 +249,7 @@ public function downloadAction(Invoice $invoice): Response
295249

296250
/**
297251
* @Route(path="/show/{page}", defaults={"page": 1}, requirements={"page": "[1-9]\d*"}, name="admin_invoice_list", methods={"GET"})
252+
* @Security("is_granted('view_invoice')")
298253
*/
299254
public function showInvoicesAction(Request $request, int $page): Response
300255
{
@@ -325,6 +280,7 @@ public function showInvoicesAction(Request $request, int $page): Response
325280

326281
/**
327282
* @Route(path="/export", name="invoice_export", methods={"GET"})
283+
* @Security("is_granted('view_invoice')")
328284
*/
329285
public function exportAction(Request $request, AnnotatedObjectExporter $exporter)
330286
{
@@ -474,6 +430,60 @@ public function deleteTemplate(InvoiceTemplate $template, string $token, CsrfTok
474430
return $this->redirectToRoute('admin_invoice_template');
475431
}
476432

433+
private function getDefaultQuery(): InvoiceQuery
434+
{
435+
$factory = $this->getDateTimeFactory();
436+
$begin = $factory->getStartOfMonth();
437+
$end = $factory->getEndOfMonth();
438+
439+
$query = new InvoiceQuery();
440+
$query->setBegin($begin);
441+
$query->setEnd($end);
442+
// limit access to data from teams
443+
$query->setCurrentUser($this->getUser());
444+
445+
if (!$this->isGranted('view_other_timesheet')) {
446+
// limit access to own data
447+
$query->setUser($this->getUser());
448+
}
449+
450+
return $query;
451+
}
452+
453+
private function renderInvoice(InvoiceQuery $query, Request $request)
454+
{
455+
// use the current request locale as fallback, if no translation was configured
456+
if (null !== $query->getTemplate() && null === $query->getTemplate()->getLanguage()) {
457+
$query->getTemplate()->setLanguage($request->getLocale());
458+
}
459+
460+
try {
461+
$invoices = $this->service->createInvoices($query, $this->dispatcher);
462+
463+
$this->flashSuccess('action.update.success');
464+
465+
if (\count($invoices) === 1) {
466+
return $this->redirectToRoute('admin_invoice_list', ['id' => $invoices[0]->getId()]);
467+
}
468+
469+
return $this->redirectToRoute('admin_invoice_list');
470+
} catch (Exception $ex) {
471+
$this->flashUpdateException($ex);
472+
}
473+
474+
return $this->redirectToRoute('invoice');
475+
}
476+
477+
private function flashFormError(FormInterface $form): void
478+
{
479+
$err = '';
480+
foreach ($form->getErrors(true, true) as $error) {
481+
$err .= PHP_EOL . '[' . $error->getOrigin()->getName() . '] ' . $error->getMessage();
482+
}
483+
484+
$this->flashError('action.update.error', ['%reason%' => $err]);
485+
}
486+
477487
private function renderTemplateForm(InvoiceTemplate $template, Request $request): Response
478488
{
479489
$editForm = $this->createEditForm($template);

Diff for: src/Repository/CustomerRepository.php

+2-1
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,8 @@ public function getQueryBuilderForFormType(CustomerFormTypeQuery $query): QueryB
241241

242242
$outerQuery = $qb->expr()->orX();
243243

244-
if ($query->hasCustomers()) {
244+
// this is a risk, as a user can manipulate the query and inject IDs that would be hidden otherwise
245+
if ($query->isAllowCustomerPreselect() && $query->hasCustomers()) {
245246
$outerQuery->add($qb->expr()->in('c.id', ':customer'));
246247
$qb->setParameter('customer', $query->getCustomers());
247248
}

Diff for: src/Repository/Query/CustomerFormTypeQuery.php

+11
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ final class CustomerFormTypeQuery extends BaseFormTypeQuery
2020
* @var Customer|null
2121
*/
2222
private $customerToIgnore;
23+
private $allowCustomerPreselect = false;
2324

2425
/**
2526
* @param Customer|int|null $customer
@@ -34,6 +35,16 @@ public function __construct($customer = null)
3435
}
3536
}
3637

38+
public function isAllowCustomerPreselect(): bool
39+
{
40+
return $this->allowCustomerPreselect;
41+
}
42+
43+
public function setAllowCustomerPreselect(bool $allowCustomerPreselect): void
44+
{
45+
$this->allowCustomerPreselect = $allowCustomerPreselect;
46+
}
47+
3748
/**
3849
* @return Customer|null
3950
*/

Diff for: src/Voter/CustomerVoter.php

+17
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ final class CustomerVoter extends Voter
3434
'comments',
3535
'comments_create',
3636
'details',
37+
'access',
3738
];
3839

3940
private $permissionManager;
@@ -75,6 +76,22 @@ protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
7576
return false;
7677
}
7778

79+
// this is a virtual permission, only meant to be used by developer
80+
// it checks if access to the given customer is potentially possible
81+
if ($attribute === 'access') {
82+
if ($subject->getTeams()->count() === 0) {
83+
return true;
84+
}
85+
86+
foreach ($subject->getTeams() as $team) {
87+
if ($user->isInTeam($team)) {
88+
return true;
89+
}
90+
}
91+
92+
return false;
93+
}
94+
7895
if ($this->permissionManager->hasRolePermission($user, $attribute . '_customer')) {
7996
return true;
8097
}

Diff for: src/Voter/UserVoter.php

-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ final class UserVoter extends Voter
2929
'preferences',
3030
'api-token',
3131
'hourly-rate',
32-
// teams_own_profile could be merged with view_team_member
3332
'view_team_member',
3433
];
3534

0 commit comments

Comments
 (0)