From 47a77eabf2d85152f370d2daa15a70b5fc7d57d7 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Tue, 10 Oct 2023 15:06:08 -0700 Subject: [PATCH 1/7] Avoid logging error messages for webhook request failures --- app/Listeners/CheckoutableListener.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index d9680f82a1..13579ce6b6 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -18,6 +18,7 @@ use App\Notifications\CheckoutAccessoryNotification; use App\Notifications\CheckoutAssetNotification; use App\Notifications\CheckoutConsumableNotification; use App\Notifications\CheckoutLicenseSeatNotification; +use GuzzleHttp\Exception\ClientException; use Illuminate\Support\Facades\Notification; use Exception; use Log; @@ -61,6 +62,16 @@ class CheckoutableListener ); } } catch (Exception $e) { + if ($e instanceof ClientException){ + $statusCode = $e->getResponse()->getStatusCode(); + // If status code is in 400 range, we don't want to log it as an error + // @todo: 300 and 500 as well? + if ($statusCode >= 400 && $statusCode < 500) { + Log::debug("Exception caught during checkout notification: ".$e->getMessage()); + return; + } + } + Log::error("Exception caught during checkout notification: ".$e->getMessage()); } } From ab3a3de59b64cd837f6d824f086a5749cdd60bf3 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Tue, 10 Oct 2023 15:13:46 -0700 Subject: [PATCH 2/7] Fire webhook notification after sending emails --- app/Listeners/CheckoutableListener.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index 13579ce6b6..917a037703 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -45,11 +45,6 @@ class CheckoutableListener $acceptance = $this->getCheckoutAcceptance($event); try { - if ($this->shouldSendWebhookNotification()) { - Notification::route('slack', Setting::getSettings()->webhook_endpoint) - ->notify($this->getCheckoutNotification($event)); - } - if (! $event->checkedOutTo->locale) { Notification::locale(Setting::getSettings()->locale)->send( $this->getNotifiables($event), @@ -61,6 +56,11 @@ class CheckoutableListener $this->getCheckoutNotification($event, $acceptance) ); } + + if ($this->shouldSendWebhookNotification()) { + Notification::route('slack', Setting::getSettings()->webhook_endpoint) + ->notify($this->getCheckoutNotification($event)); + } } catch (Exception $e) { if ($e instanceof ClientException){ $statusCode = $e->getResponse()->getStatusCode(); From 2a29566458521bf7a56b2a8ab2845268ac4590da Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Tue, 10 Oct 2023 15:15:02 -0700 Subject: [PATCH 3/7] Catch all ClientExceptions on check out --- app/Listeners/CheckoutableListener.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index 917a037703..01e5041f67 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -63,13 +63,8 @@ class CheckoutableListener } } catch (Exception $e) { if ($e instanceof ClientException){ - $statusCode = $e->getResponse()->getStatusCode(); - // If status code is in 400 range, we don't want to log it as an error - // @todo: 300 and 500 as well? - if ($statusCode >= 400 && $statusCode < 500) { - Log::debug("Exception caught during checkout notification: ".$e->getMessage()); - return; - } + Log::debug("Exception caught during checkout notification: ".$e->getMessage()); + return; } Log::error("Exception caught during checkout notification: ".$e->getMessage()); From 9ef598d07bdaf6565ec79cf7078697e1634e56be Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Tue, 10 Oct 2023 15:16:12 -0700 Subject: [PATCH 4/7] Apply changes to exception handling for check outs to check ins --- app/Listeners/CheckoutableListener.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index 01e5041f67..34d93550ed 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -98,11 +98,6 @@ class CheckoutableListener } try { - if ($this->shouldSendWebhookNotification()) { - Notification::route('slack', Setting::getSettings()->webhook_endpoint) - ->notify($this->getCheckinNotification($event)); - } - // Use default locale if (! $event->checkedOutTo->locale) { Notification::locale(Setting::getSettings()->locale)->send( @@ -115,7 +110,17 @@ class CheckoutableListener $this->getCheckinNotification($event) ); } + + if ($this->shouldSendWebhookNotification()) { + Notification::route('slack', Setting::getSettings()->webhook_endpoint) + ->notify($this->getCheckinNotification($event)); + } } catch (Exception $e) { + if ($e instanceof ClientException){ + Log::debug("Exception caught during checkout notification: ".$e->getMessage()); + return; + } + Log::error("Exception caught during checkin notification: ".$e->getMessage()); } } From dae9e6d09659a811c1c1d93548f3b501ff1deaf1 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Tue, 10 Oct 2023 15:18:37 -0700 Subject: [PATCH 5/7] Improve try catch blocks --- app/Listeners/CheckoutableListener.php | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index 34d93550ed..ec51130b1a 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -42,7 +42,7 @@ class CheckoutableListener /** * Make a checkout acceptance and attach it in the notification */ - $acceptance = $this->getCheckoutAcceptance($event); + $acceptance = $this->getCheckoutAcceptance($event); try { if (! $event->checkedOutTo->locale) { @@ -61,12 +61,11 @@ class CheckoutableListener Notification::route('slack', Setting::getSettings()->webhook_endpoint) ->notify($this->getCheckoutNotification($event)); } - } catch (Exception $e) { - if ($e instanceof ClientException){ - Log::debug("Exception caught during checkout notification: ".$e->getMessage()); - return; - } - + } + catch (ClientException $e){ + Log::debug("Exception caught during checkout notification: ".$e->getMessage()); + } + catch (Exception $e) { Log::error("Exception caught during checkout notification: ".$e->getMessage()); } } @@ -115,12 +114,11 @@ class CheckoutableListener Notification::route('slack', Setting::getSettings()->webhook_endpoint) ->notify($this->getCheckinNotification($event)); } - } catch (Exception $e) { - if ($e instanceof ClientException){ - Log::debug("Exception caught during checkout notification: ".$e->getMessage()); - return; - } - + } + catch (ClientException $e){ + Log::debug("Exception caught during checkout notification: ".$e->getMessage()); + } + catch (Exception $e) { Log::error("Exception caught during checkin notification: ".$e->getMessage()); } } From 43b9e6401c62e8f99d46fe383b2aca1d45cbe08f Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Tue, 10 Oct 2023 15:18:55 -0700 Subject: [PATCH 6/7] Formatting --- app/Listeners/CheckoutableListener.php | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index ec51130b1a..17a8a6c1bf 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -61,12 +61,10 @@ class CheckoutableListener Notification::route('slack', Setting::getSettings()->webhook_endpoint) ->notify($this->getCheckoutNotification($event)); } - } - catch (ClientException $e){ - Log::debug("Exception caught during checkout notification: ".$e->getMessage()); - } - catch (Exception $e) { - Log::error("Exception caught during checkout notification: ".$e->getMessage()); + } catch (ClientException $e) { + Log::debug("Exception caught during checkout notification: " . $e->getMessage()); + } catch (Exception $e) { + Log::error("Exception caught during checkout notification: " . $e->getMessage()); } } @@ -114,12 +112,10 @@ class CheckoutableListener Notification::route('slack', Setting::getSettings()->webhook_endpoint) ->notify($this->getCheckinNotification($event)); } - } - catch (ClientException $e){ - Log::debug("Exception caught during checkout notification: ".$e->getMessage()); - } - catch (Exception $e) { - Log::error("Exception caught during checkin notification: ".$e->getMessage()); + } catch (ClientException $e) { + Log::debug("Exception caught during checkout notification: " . $e->getMessage()); + } catch (Exception $e) { + Log::error("Exception caught during checkin notification: " . $e->getMessage()); } } From 417f9c21e42a179cf23890c62fdc11008b24c50b Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Tue, 10 Oct 2023 17:51:29 -0700 Subject: [PATCH 7/7] Fix the storing of group permissions when creating via API --- app/Http/Controllers/Api/GroupsController.php | 2 +- tests/Feature/Api/Groups/GroupStoreTest.php | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/Api/Groups/GroupStoreTest.php diff --git a/app/Http/Controllers/Api/GroupsController.php b/app/Http/Controllers/Api/GroupsController.php index 7cc5d2d756..6dc7e83dd6 100644 --- a/app/Http/Controllers/Api/GroupsController.php +++ b/app/Http/Controllers/Api/GroupsController.php @@ -63,7 +63,7 @@ class GroupsController extends Controller $group = new Group; $group->name = $request->input('name'); - $group->permissions = $request->input('permissions'); // Todo - some JSON validation stuff here + $group->permissions = json_encode($request->input('permissions')); // Todo - some JSON validation stuff here if ($group->save()) { return response()->json(Helper::formatStandardApiResponse('success', $group, trans('admin/groups/message.create.success'))); diff --git a/tests/Feature/Api/Groups/GroupStoreTest.php b/tests/Feature/Api/Groups/GroupStoreTest.php new file mode 100644 index 0000000000..9ffba51913 --- /dev/null +++ b/tests/Feature/Api/Groups/GroupStoreTest.php @@ -0,0 +1,41 @@ +actingAsForApi(User::factory()->create()) + ->postJson(route('api.groups.store')) + ->assertForbidden(); + } + + public function testCanStoreGroup() + { + $this->actingAsForApi(User::factory()->superuser()->create()) + ->postJson(route('api.groups.store'), [ + 'name' => 'My Awesome Group', + 'permissions' => [ + 'admin' => '1', + 'import' => '1', + 'reports.view' => '0', + ], + ]) + ->assertOk(); + + $group = Group::where('name', 'My Awesome Group')->first(); + + $this->assertNotNull($group); + $this->assertEquals('1', $group->decodePermissions()['admin']); + $this->assertEquals('1', $group->decodePermissions()['import']); + $this->assertEquals('0', $group->decodePermissions()['reports.view']); + } +}