Browse Source

Fix RBAC permission edge cases

Alexander Musikhin 1 week ago
parent
commit
a6ab13a666

+ 42 - 5
app/Http/Controllers/Admin/RoleController.php

@@ -71,11 +71,7 @@ class RoleController extends Controller
             $slug = 'role_' . $user->id;
             $slug = 'role_' . $user->id;
         }
         }
 
 
-        $baseSlug = $slug;
-        $counter = 2;
-        while (Role::query()->where('slug', $slug)->exists()) {
-            $slug = $baseSlug . '_' . $counter++;
-        }
+        $slug = $this->uniqueRoleSlug($slug);
 
 
         $role = DB::transaction(function () use ($validated, $slug, $user, $accessService) {
         $role = DB::transaction(function () use ($validated, $slug, $user, $accessService) {
             $role = Role::query()->create([
             $role = Role::query()->create([
@@ -103,6 +99,36 @@ class RoleController extends Controller
             ->with('success', 'Роль создана из прав пользователя.');
             ->with('success', 'Роль создана из прав пользователя.');
     }
     }
 
 
+    public function copy(Role $role, AccessService $accessService): RedirectResponse
+    {
+        $copy = DB::transaction(function () use ($role, $accessService) {
+            $role->load('permissions');
+
+            $copy = Role::query()->create([
+                'slug' => $this->uniqueRoleSlug($role->slug . '_copy'),
+                'name' => 'Копия ' . $role->name,
+                'description' => $role->description,
+                'is_system' => false,
+                'is_active' => true,
+                'sort' => (int) ($role->sort ?? 100),
+            ]);
+
+            $sync = [];
+            foreach ($role->permissions as $permission) {
+                $sync[$permission->id] = ['effect' => $permission->pivot->effect];
+            }
+
+            $copy->permissions()->sync($sync);
+            $accessService->bumpCacheVersion();
+
+            return $copy;
+        });
+
+        return redirect()
+            ->route('admin.roles.edit', $copy)
+            ->with('success', 'Роль скопирована.');
+    }
+
     public function edit(Role $role): View
     public function edit(Role $role): View
     {
     {
         $effects = $role->permissions()
         $effects = $role->permissions()
@@ -217,4 +243,15 @@ class RoleController extends Controller
 
 
         $role->permissions()->sync($sync);
         $role->permissions()->sync($sync);
     }
     }
+
+    private function uniqueRoleSlug(string $slug): string
+    {
+        $baseSlug = $slug;
+        $counter = 2;
+        while (Role::query()->where('slug', $slug)->exists()) {
+            $slug = $baseSlug . '_' . $counter++;
+        }
+
+        return $slug;
+    }
 }
 }

+ 8 - 1
app/Http/Controllers/ImportController.php

@@ -4,6 +4,7 @@ namespace App\Http\Controllers;
 
 
 use App\Jobs\Import\ImportJob;
 use App\Jobs\Import\ImportJob;
 use App\Models\Import;
 use App\Models\Import;
+use App\Services\Access\AccessService;
 use Illuminate\Http\Request;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Storage;
 use Illuminate\Support\Facades\Storage;
@@ -56,7 +57,7 @@ class ImportController extends Controller
         return view('import.index', $this->data);
         return view('import.index', $this->data);
     }
     }
 
 
-    public function store(Request $request)
+    public function store(Request $request, AccessService $accessService)
     {
     {
         // validate data
         // validate data
         $request->validate([
         $request->validate([
@@ -64,6 +65,12 @@ class ImportController extends Controller
             'import_file' => 'required|file',
             'import_file' => 'required|file',
         ]);
         ]);
 
 
+        if ($request->type === 'catalog') {
+            $accessService->assertCan($request->user(), 'catalog.import');
+        } else {
+            $accessService->assertCan($request->user(), 'import.create');
+        }
+
         // load and save file
         // load and save file
         $path = Str::random(2) . '/' . Str::uuid() . '.' .$request->file('import_file')->getClientOriginalExtension();
         $path = Str::random(2) . '/' . Str::uuid() . '.' .$request->file('import_file')->getClientOriginalExtension();
         Storage::disk('upload')->put($path, $request->file('import_file')->getContent());
         Storage::disk('upload')->put($path, $request->file('import_file')->getContent());

+ 11 - 2
app/Http/Middleware/EnsureRoutePermission.php

@@ -18,7 +18,11 @@ class EnsureRoutePermission
         $user = $request->user();
         $user = $request->user();
         $routeName = $request->route()?->getName();
         $routeName = $request->route()?->getName();
 
 
-        if (!$user || !$routeName || !$this->accessService->routePermission($routeName)) {
+        $routePermission = $routeName === 'import.create' && $request->input('type') === 'catalog'
+            ? 'catalog.import'
+            : $this->accessService->routePermission($routeName);
+
+        if (!$user || !$routeName || !$routePermission) {
             return $next($request);
             return $next($request);
         }
         }
 
 
@@ -27,7 +31,12 @@ class EnsureRoutePermission
             return $next($request);
             return $next($request);
         }
         }
 
 
-        abort_unless($this->accessService->canAccessRoute($user, $routeName), 403);
+        abort_unless(
+            is_array($routePermission)
+                ? $this->accessService->canAny($user, $routePermission)
+                : $this->accessService->can($user, $routePermission),
+            403
+        );
 
 
         return $next($request);
         return $next($request);
     }
     }

+ 6 - 1
app/Http/Middleware/EnsureUserHasRole.php

@@ -25,7 +25,12 @@ class EnsureUserHasRole
     {
     {
         $user = $request->user();
         $user = $request->user();
 
 
-        if ($user?->hasRole($roles) || ($user && $this->accessService->canAccessRoute($user, $request->route()?->getName()))) {
+        $routeName = $request->route()?->getName();
+        $hasRoutePermission = $user && $routeName === 'import.create' && $request->input('type') === 'catalog'
+            ? $this->accessService->can($user, 'catalog.import')
+            : ($user && $this->accessService->canAccessRoute($user, $routeName));
+
+        if ($user?->hasRole($roles) || $hasRoutePermission) {
             return $next($request);
             return $next($request);
         }
         }
 
 

+ 22 - 3
app/Services/Access/AccessService.php

@@ -74,7 +74,7 @@ class AccessService
             return null;
             return null;
         }
         }
 
 
-        $exact = config('access_routes.exact.' . $routeName);
+        $exact = config('access_routes.exact', [])[$routeName] ?? null;
         if ($exact) {
         if ($exact) {
             return $exact;
             return $exact;
         }
         }
@@ -147,11 +147,21 @@ class AccessService
         }
         }
 
 
         $effects = collect();
         $effects = collect();
+        $denied = collect();
 
 
         $role = $user->roleModel()->with('permissions')->first();
         $role = $user->roleModel()->with('permissions')->first();
         if ($role) {
         if ($role) {
             foreach ($role->permissions as $permission) {
             foreach ($role->permissions as $permission) {
-                $effects->put($permission->slug, $permission->pivot->effect);
+                $effect = $permission->pivot->effect;
+                if ($effect === 'deny') {
+                    $denied->put($permission->slug, true);
+                    $effects->put($permission->slug, 'deny');
+                    continue;
+                }
+
+                if (!$denied->has($permission->slug)) {
+                    $effects->put($permission->slug, $effect);
+                }
             }
             }
         }
         }
 
 
@@ -163,7 +173,16 @@ class AccessService
             ->get();
             ->get();
 
 
         foreach ($userPermissions as $permission) {
         foreach ($userPermissions as $permission) {
-            $effects->put($permission->slug, $permission->pivot->effect);
+            $effect = $permission->pivot->effect;
+            if ($effect === 'deny') {
+                $denied->put($permission->slug, true);
+                $effects->put($permission->slug, 'deny');
+                continue;
+            }
+
+            if (!$denied->has($permission->slug)) {
+                $effects->put($permission->slug, $effect);
+            }
         }
         }
 
 
         return $effects;
         return $effects;

+ 1 - 0
config/access.php

@@ -63,6 +63,7 @@ return [
             'create' => 'Создание',
             'create' => 'Создание',
             'update' => 'Редактирование',
             'update' => 'Редактирование',
             'delete' => 'Удаление',
             'delete' => 'Удаление',
+            'import' => 'Импорт',
             'export' => 'Экспорт',
             'export' => 'Экспорт',
             'certificates.upload' => 'Загрузка сертификата',
             'certificates.upload' => 'Загрузка сертификата',
             'certificates.delete' => 'Удаление сертификата',
             'certificates.delete' => 'Удаление сертификата',

+ 12 - 0
database/seeders/RbacSeeder.php

@@ -143,6 +143,7 @@ class RbacSeeder extends Seeder
             'reclamations.act.delete',
             'reclamations.act.delete',
             'reclamations.spare_parts.manage',
             'reclamations.spare_parts.manage',
             'catalog.view',
             'catalog.view',
+            'catalog.import',
             'maf.view',
             'maf.view',
             'maf.update',
             'maf.update',
             'maf.passports.upload',
             'maf.passports.upload',
@@ -172,6 +173,8 @@ class RbacSeeder extends Seeder
             'chat_messages.notify',
             'chat_messages.notify',
         ]);
         ]);
 
 
+        $manager = array_merge($manager, $this->fieldPermissions('catalog', ['view'], $permissions));
+
         $brigadier = array_merge($auth, [
         $brigadier = array_merge($auth, [
             'reclamations.act.upload',
             'reclamations.act.upload',
         ]);
         ]);
@@ -204,4 +207,13 @@ class RbacSeeder extends Seeder
             $role->permissions()->sync($sync);
             $role->permissions()->sync($sync);
         }
         }
     }
     }
+
+    private function fieldPermissions(string $module, array $actions, array $permissions): array
+    {
+        return array_values(array_filter(
+            array_keys($permissions),
+            fn (string $slug): bool => str_starts_with($slug, "{$module}.fields.")
+                && in_array(substr($slug, strrpos($slug, '.') + 1), $actions, true)
+        ));
+    }
 }
 }

+ 4 - 0
resources/views/admin/roles/index.blade.php

@@ -37,6 +37,10 @@
                     <td>{{ $role->users_count }}</td>
                     <td>{{ $role->users_count }}</td>
                     <td class="text-end">
                     <td class="text-end">
                         <a href="{{ route('admin.roles.edit', $role) }}" class="btn btn-sm btn-outline-primary">Открыть</a>
                         <a href="{{ route('admin.roles.edit', $role) }}" class="btn btn-sm btn-outline-primary">Открыть</a>
+                        <form action="{{ route('admin.roles.copy', $role) }}" method="post" class="d-inline">
+                            @csrf
+                            <button type="submit" class="btn btn-sm btn-outline-secondary">Копировать</button>
+                        </form>
                     </td>
                     </td>
                 </tr>
                 </tr>
             @endforeach
             @endforeach

+ 1 - 1
resources/views/catalog/index.blade.php

@@ -10,7 +10,7 @@
             @include('partials.year-switcher')
             @include('partials.year-switcher')
         </div>
         </div>
         <div class="col-auto col-md-4 text-md-end page-header-actions">
         <div class="col-auto col-md-4 text-md-end page-header-actions">
-            @if(hasAccess('import.create', 'admin'))
+            @if(hasAccess('catalog.import', 'admin'))
                 <button type="button" class="btn btn-sm mb-1 btn-primary page-action-btn" data-bs-toggle="modal" data-bs-target="#importModal" aria-label="Импорт">
                 <button type="button" class="btn btn-sm mb-1 btn-primary page-action-btn" data-bs-toggle="modal" data-bs-target="#importModal" aria-label="Импорт">
                     <i class="bi bi-upload page-action-btn__icon"></i>
                     <i class="bi bi-upload page-action-btn__icon"></i>
                     <span class="page-action-btn__label">Импорт</span>
                     <span class="page-action-btn__label">Импорт</span>

+ 1 - 0
routes/web.php

@@ -86,6 +86,7 @@ Route::middleware(['auth:web', 'route.permission'])->group(function () {
             Route::get('create', [RoleController::class, 'create'])->name('create');
             Route::get('create', [RoleController::class, 'create'])->name('create');
             Route::post('', [RoleController::class, 'store'])->name('store');
             Route::post('', [RoleController::class, 'store'])->name('store');
             Route::post('from-user/{user}', [RoleController::class, 'storeFromUser'])->name('store-from-user');
             Route::post('from-user/{user}', [RoleController::class, 'storeFromUser'])->name('store-from-user');
+            Route::post('{role}/copy', [RoleController::class, 'copy'])->name('copy');
             Route::get('{role}', [RoleController::class, 'edit'])->name('edit');
             Route::get('{role}', [RoleController::class, 'edit'])->name('edit');
             Route::put('{role}', [RoleController::class, 'update'])->name('update');
             Route::put('{role}', [RoleController::class, 'update'])->name('update');
             Route::delete('{role}', [RoleController::class, 'destroy'])->name('destroy');
             Route::delete('{role}', [RoleController::class, 'destroy'])->name('destroy');

+ 21 - 0
tests/Feature/AdminRoleControllerTest.php

@@ -154,4 +154,25 @@ class AdminRoleControllerTest extends TestCase
             ->assertOk()
             ->assertOk()
             ->assertSee('Роли и права');
             ->assertSee('Роли и права');
     }
     }
+
+    public function test_role_can_be_copied_with_permissions(): void
+    {
+        $managerRole = Role::query()->where('slug', Role::MANAGER)->firstOrFail();
+        $permission = Permission::query()->where('slug', 'catalog.view')->firstOrFail();
+
+        $this->actingAs($this->adminUser)
+            ->post(route('admin.roles.copy', $managerRole))
+            ->assertRedirect();
+
+        $copy = Role::query()
+            ->where('slug', 'manager_copy')
+            ->firstOrFail();
+
+        $this->assertFalse($copy->is_system);
+        $this->assertDatabaseHas('role_permissions', [
+            'role_id' => $copy->id,
+            'permission_id' => $permission->id,
+            'effect' => 'allow',
+        ]);
+    }
 }
 }

+ 50 - 0
tests/Feature/RoutePermissionMiddlewareTest.php

@@ -7,6 +7,8 @@ use App\Models\Role;
 use App\Models\User;
 use App\Models\User;
 use Database\Seeders\RbacSeeder;
 use Database\Seeders\RbacSeeder;
 use Illuminate\Foundation\Testing\RefreshDatabase;
 use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Http\UploadedFile;
+use Illuminate\Support\Facades\Bus;
 use Tests\TestCase;
 use Tests\TestCase;
 
 
 class RoutePermissionMiddlewareTest extends TestCase
 class RoutePermissionMiddlewareTest extends TestCase
@@ -53,4 +55,52 @@ class RoutePermissionMiddlewareTest extends TestCase
             ->get(route('order.index'))
             ->get(route('order.index'))
             ->assertOk();
             ->assertOk();
     }
     }
+
+    public function test_catalog_import_requires_catalog_import_permission(): void
+    {
+        Bus::fake();
+
+        $importPermission = Permission::query()->where('slug', 'import.create')->firstOrFail();
+        $role = Role::query()->create([
+            'slug' => 'generic_importer',
+            'name' => 'Generic importer',
+            'is_system' => false,
+            'is_active' => true,
+        ]);
+        $role->permissions()->sync([
+            $importPermission->id => ['effect' => 'allow'],
+        ]);
+        $user = User::factory()->create(['role' => $role->slug, 'role_id' => $role->id]);
+
+        $this->actingAs($user)
+            ->post(route('import.create'), [
+                'type' => 'catalog',
+                'import_file' => UploadedFile::fake()->create('catalog.xlsx', 10),
+            ])
+            ->assertForbidden();
+    }
+
+    public function test_catalog_import_allows_catalog_import_permission(): void
+    {
+        Bus::fake();
+
+        $permission = Permission::query()->where('slug', 'catalog.import')->firstOrFail();
+        $role = Role::query()->create([
+            'slug' => 'catalog_importer',
+            'name' => 'Catalog importer',
+            'is_system' => false,
+            'is_active' => true,
+        ]);
+        $role->permissions()->sync([
+            $permission->id => ['effect' => 'allow'],
+        ]);
+        $user = User::factory()->create(['role' => $role->slug, 'role_id' => $role->id]);
+
+        $this->actingAs($user)
+            ->post(route('import.create'), [
+                'type' => 'catalog',
+                'import_file' => UploadedFile::fake()->create('catalog.xlsx', 10),
+            ])
+            ->assertRedirect(route('import.index'));
+    }
 }
 }

+ 32 - 0
tests/Unit/Services/AccessServiceTest.php

@@ -45,9 +45,41 @@ class AccessServiceTest extends TestCase
         $manager->refresh();
         $manager->refresh();
 
 
         $this->assertTrue(app(AccessService::class)->can($manager, 'catalog.view'));
         $this->assertTrue(app(AccessService::class)->can($manager, 'catalog.view'));
+        $this->assertTrue(app(AccessService::class)->can($manager, 'catalog.fields.product_price.view'));
         $this->assertFalse(app(AccessService::class)->can($manager, 'catalog.update'));
         $this->assertFalse(app(AccessService::class)->can($manager, 'catalog.update'));
     }
     }
 
 
+    public function test_role_deny_overrides_user_allow(): void
+    {
+        $this->seed(RbacSeeder::class);
+
+        $permission = Permission::query()->where('slug', 'catalog.view')->firstOrFail();
+        $role = Role::query()->create([
+            'slug' => 'deny_catalog',
+            'name' => 'Deny catalog',
+            'is_system' => false,
+            'is_active' => true,
+        ]);
+        $role->permissions()->sync([
+            $permission->id => ['effect' => 'deny'],
+        ]);
+        $user = User::factory()->create(['role' => $role->slug, 'role_id' => $role->id]);
+        $user->permissions()->sync([
+            $permission->id => ['effect' => 'allow'],
+        ]);
+        app(AccessService::class)->bumpCacheVersion();
+
+        $this->assertFalse(app(AccessService::class)->can($user, 'catalog.view'));
+    }
+
+    public function test_exact_route_permission_lookup_supports_dotted_route_names(): void
+    {
+        $this->assertSame(
+            'catalog.search',
+            app(AccessService::class)->routePermission('product.search')
+        );
+    }
+
     public function test_user_deny_overrides_role_allow(): void
     public function test_user_deny_overrides_role_allow(): void
     {
     {
         $manager = User::factory()->create(['role' => Role::MANAGER]);
         $manager = User::factory()->create(['role' => Role::MANAGER]);