From a0ac9df5fd87da1d418f6498b58c96db004f65cc Mon Sep 17 00:00:00 2001 From: David Bomba Date: Wed, 25 Jan 2023 11:28:23 +1100 Subject: [PATCH] Improve rate limiting when using send with gmail --- app/Helpers/Mail/GmailTransport.php | 5 ++-- app/Http/Controllers/BaseController.php | 40 +++++-------------------- app/Jobs/Mail/NinjaMailerJob.php | 2 +- app/Models/User.php | 37 +++++++++++++++++++++++ tests/Unit/PermissionsTest.php | 35 ++++++++++++++++++++++ 5 files changed, 84 insertions(+), 35 deletions(-) diff --git a/app/Helpers/Mail/GmailTransport.php b/app/Helpers/Mail/GmailTransport.php index 479fbdbdeb59..a8bf46e0ba83 100644 --- a/app/Helpers/Mail/GmailTransport.php +++ b/app/Helpers/Mail/GmailTransport.php @@ -68,12 +68,13 @@ class GmailTransport extends AbstractTransport try{ $service->users_messages->send('me', $body, []); } - catch(Google\Service\Exception $e) { + catch(\Google\Service\Exception $e) { /* Need to slow down */ if($e->getCode() == '429') { - sleep(5); + sleep(rand(5,10)); + nlog("429 google - retrying "); $service->users_messages->send('me', $body, []); } diff --git a/app/Http/Controllers/BaseController.php b/app/Http/Controllers/BaseController.php index bbd2c7b8d04b..289b78724add 100644 --- a/app/Http/Controllers/BaseController.php +++ b/app/Http/Controllers/BaseController.php @@ -449,11 +449,13 @@ class BaseController extends Controller 'company.bank_integrations'=> function ($query) use ($updated_at, $user) { $query->whereNotNull('updated_at'); + //scopes down permissions for users with no permissions if (! $user->hasPermission('view_bank_transaction')) { $query->where('bank_integrations.user_id', $user->id); } - if(!$user->isAdmin() && !$user->isOwner() && $user->can('create', BankTransaction::class)) { + //allows us to return integrations for users who can create bank transactions + if(!$user->isSuperUser() && $user->hasIntersectPermissions(['create_bank_transaction','edit_bank_transaction','view_bank_transaction'])) { $query->exclude(["balance"]); } @@ -553,7 +555,7 @@ class BaseController extends Controller $query->where('bank_integrations.user_id', $user->id); } - if(!$user->isAdmin() && !$user->isOwner() && $user->can('create', BankTransaction::class)) { + if(!$user->isSuperUser() && $user->hasIntersectPermissions(['create_bank_transaction','edit_bank_transaction','view_bank_transaction'])) { $query->exclude(["balance"]); } @@ -809,7 +811,7 @@ class BaseController extends Controller $query->where('bank_integrations.user_id', $user->id); } - if(!$user->isAdmin() && !$user->isOwner() && $user->can('create', BankTransaction::class)) { + if(!$user->isSuperUser() && $user->hasIntersectPermissions(['create_bank_transaction','edit_bank_transaction','view_bank_transaction'])) { $query->exclude(["balance"]); } @@ -857,39 +859,15 @@ class BaseController extends Controller $query->with($includes); - - - /*Restore here if refactor produces unexpected edge cases*/ -/* - if (auth()->user() && ! auth()->user()->hasPermission('view'.lcfirst(class_basename(Str::snake($this->entity_type))))) { - //06-10-2022 - some entities do not have assigned_user_id - this becomes an issue when we have a large company and low permission users - if(lcfirst(class_basename(Str::snake($this->entity_type))) == 'user') - $query->where('id', auth()->user()->id); - elseif($this->entity_type == BankTransaction::class){ //table without assigned_user_id - $query->where('user_id', '=', auth()->user()->id); - } - elseif(in_array(lcfirst(class_basename(Str::snake($this->entity_type))),['design','group_setting','payment_term'])){ - //need to pass these back regardless - nlog($this->entity_type); - } - else - $query->where('user_id', '=', auth()->user()->id)->orWhere('assigned_user_id', auth()->user()->id); - - } -*/ - - /*21-01-2023*/ -/**/ - // 10-01-2022 need to ensure we snake case properly here to ensure permissions work as expected - // 28-03-2022 this is definitely correct here, do not append _ to the view, it resolved correctly when snake cased if (auth()->user() && ! auth()->user()->hasPermission('view_'.Str::snake(class_basename($this->entity_type)))) { //06-10-2022 - some entities do not have assigned_user_id - this becomes an issue when we have a large company and low permission users if(in_array($this->entity_type, [User::class])){ $query->where('id', auth()->user()->id); } elseif(in_array($this->entity_type, [BankTransactionRule::class,CompanyGateway::class, TaxRate::class, BankIntegration::class, Scheduler::class, BankTransaction::class, Webhook::class, ExpenseCategory::class])){ //table without assigned_user_id - if($this->entity_type == BankIntegration::class && !auth()->user()->isAdmin() && !auth()->user()->isOwner() && auth()->user()->can('create', BankTransaction::class)) - $query->exclude(["balance"]); + + if($this->entity_type == BankIntegration::class && !auth()->user()->isSuperUser() && auth()->user()->hasIntersectPermissions(['create_bank_transaction','edit_bank_transaction','view_bank_transaction'])) + $query->exclude(["balance"]); //allows us to selective display bank integrations back to the user if they can view / create bank transactions but without the bank balance being present in the response else $query->where('user_id', '=', auth()->user()->id); } @@ -900,8 +878,6 @@ class BaseController extends Controller $query->where('user_id', '=', auth()->user()->id)->orWhere('assigned_user_id', auth()->user()->id); } -/**/ - // $query->exclude(['balance','credit_balance','paid_to_date']); if (request()->has('updated_at') && request()->input('updated_at') > 0) { diff --git a/app/Jobs/Mail/NinjaMailerJob.php b/app/Jobs/Mail/NinjaMailerJob.php index 5302a41d6397..d3915f2e40e7 100644 --- a/app/Jobs/Mail/NinjaMailerJob.php +++ b/app/Jobs/Mail/NinjaMailerJob.php @@ -463,7 +463,7 @@ class NinjaMailerJob implements ShouldQueue $google->getClient()->setAccessToken(json_encode($user->oauth_user_token)); - sleep(rand(2,4)); + sleep(rand(1,6)); } catch(\Exception $e) { $this->logMailError('Gmail Token Invalid', $this->company->clients()->first()); diff --git a/app/Models/User.php b/app/Models/User.php index d10104914f3d..6978dc019520 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -317,6 +317,16 @@ class User extends Authenticatable implements MustVerifyEmail // return $this->company_user->is_owner; } + /** + * Returns true is user is an admin _or_ owner + * + * @return boolean + */ + public function isSuperUser() :bool + { + return $this->token()->cu->is_owner || $this->token()->cu->is_admin; + } + /** * Returns all user created contacts. * @@ -401,6 +411,33 @@ class User extends Authenticatable implements MustVerifyEmail } + /** + * Used when we need to match a range of permissions + * the user + * + * This method is used when we need to scope down the query + * and display a limited subset. + * + * @param array $permissions + * @return boolean + */ + public function hasIntersectPermissions(array $permissions = []): bool + { + + foreach($permissions as $permission) + { + + if($this->hasExactPermission($permission)) + return true; + + } + + return false; + + } + + + public function documents() { return $this->morphMany(Document::class, 'documentable'); diff --git a/tests/Unit/PermissionsTest.php b/tests/Unit/PermissionsTest.php index 484a65dc1d04..0c950cb17f85 100644 --- a/tests/Unit/PermissionsTest.php +++ b/tests/Unit/PermissionsTest.php @@ -79,6 +79,41 @@ class PermissionsTest extends TestCase } + public function testIntersectPermissions() + { + + $low_cu = CompanyUser::where(['company_id' => $this->company->id, 'user_id' => $this->user->id])->first(); + $low_cu->permissions = '["view_client"]'; + $low_cu->save(); + + $this->assertFalse($this->user->hasIntersectPermissions(["viewclient"])); + $this->assertTrue($this->user->hasIntersectPermissions(["view_client"])); + + + $low_cu = CompanyUser::where(['company_id' => $this->company->id, 'user_id' => $this->user->id])->first(); + $low_cu->permissions = '["view_all"]'; + $low_cu->save(); + + $this->assertFalse($this->user->hasIntersectPermissions(["viewclient"])); + $this->assertTrue($this->user->hasIntersectPermissions(["view_client"])); + + $this->assertFalse($this->user->hasIntersectPermissions(["viewbank_transaction"])); + $this->assertTrue($this->user->hasIntersectPermissions(["view_bank_transaction"])); + + $low_cu = CompanyUser::where(['company_id' => $this->company->id, 'user_id' => $this->user->id])->first(); + $low_cu->permissions = '["create_all"]'; + $low_cu->save(); + + $this->assertFalse($this->user->hasIntersectPermissions(["createclient"])); + $this->assertTrue($this->user->hasIntersectPermissions(["create_client"])); + + $this->assertFalse($this->user->hasIntersectPermissions(["createbank_transaction"])); + $this->assertTrue($this->user->hasIntersectPermissions(["create_bank_transaction"])); + $this->assertTrue($this->user->hasIntersectPermissions(['create_bank_transaction','edit_bank_transaction','view_bank_transaction'])); + + + } + public function testViewClientPermission() {