Route protection with custom middleware in Laravel 8

Question

I am working on a blogging application in Laravel 8.

The application assigns users roles and permissions. There is a many-to-many relationship between roles and permissions.

I have created a custom middleware to give users access to routes based on their permissions:

namespace App\Http\Middleware;
use Closure;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Auth;

class CheckUserPermissions
{
    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Closure(\Illuminate\Http\Request): (\Illuminate\Http\Response|\Illuminate\Http\RedirectResponse)  $next
     * @return \Illuminate\Http\Response|\Illuminate\Http\RedirectResponse
     */

    // Permissions checker
    public function hasPermissionTo($permission) {
        return in_array($permission, Auth::user()->role->permissions->pluck('slug')->toArray());
    }

    public function handle(Request $request, Closure $next, ...$permissions)
    {
      // Check user permissions
        foreach ($permissions as $permission) {
          if (!$this->hasPermissionTo($permission)) { 
            $permission_label = join(' ',  explode('-', $permission));
            return redirect()->back()->with('error', 'You do not have permission to ' . $permission_label);
          }
        }

        return $next($request);
    }
}

In routes\web.php I have:

// Dashboard routes
Route::group(['prefix' => 'dashboard', 'middleware' => ['auth']], function() {
  Route::get('/', [DashboardController::class, 'index'])->name('dashboard');

    // Settings routes
  Route::group(['prefix' => 'settings', 'middleware' => ['checkUserPermissions:edit-settings']], function() {
        Route::get('/', [SettingsController::class, 'index'])->name('dashboard.settings');
        Route::post('/update', [SettingsController::class, 'update'])->name('dashboard.settings.update');
    });

    // Pages routes
    Route::group(['prefix' => 'pages'], function() {
        Route::get('/', [PageController::class, 'index'])->name('dashboard.pages')->middleware('checkUserPermissions:view-pages');
        Route::get('/new', [PageController::class, 'create'])->name('dashboard.pages.new')->middleware('checkUserPermissions:add-pages');
        Route::post('/add', [PageController::class, 'save'])->name('dashboard.pages.add');
        Route::get('/edit/{id}', [PageController::class, 'edit'])->name('dashboard.pages.edit')->middleware('checkUserPermissions:edit-pages');
        Route::post('/update/{id}', [PageController::class, 'update'])->name('dashboard.pages.update');
        Route::get('/delete/{id}', [PageController::class, 'delete'])->name('dashboard.pages.delete')->middleware('checkUserPermissions:delete-pages');
    });

    // Category routes
    Route::group(['prefix' => 'categories'], function() {
        Route::get('/', [ArticleCategoryController::class, 'index'])->name('dashboard.categories')->middleware('checkUserPermissions:view-categories');
        Route::get('/new', [ArticleCategoryController::class, 'create'])->name('dashboard.categories.new')->middleware('checkUserPermissions:add-categories');
        Route::post('/add', [ArticleCategoryController::class, 'save'])->name('dashboard.categories.add');
        Route::get('/edit/{id}', [ArticleCategoryController::class, 'edit'])->name('dashboard.categories.edit')->middleware('checkUserPermissions:edit-categories');
        Route::post('/update/{id}', [ArticleCategoryController::class, 'update'])->name('dashboard.categories.update');
        Route::get('/delete/{id}', [ArticleCategoryController::class, 'delete'])->name('dashboard.categories.delete')->middleware('checkUserPermissions:delete-categories');
    });

    // Article routes
    Route::group(['prefix' => 'articles'], function() {
        Route::get('/', [ArticleController::class, 'index'])->name('dashboard.articles')->middleware('checkUserPermissions:view-articles');
        Route::get('/new', [ArticleController::class, 'create'])->name('dashboard.articles.new')->middleware('checkUserPermissions:add-articles');
        Route::post('/add', [ArticleController::class, 'save'])->name('dashboard.articles.add');
        Route::get('/edit/{id}', [ArticleController::class, 'edit'])->name('dashboard.articles.edit')->middleware('checkUserPermissions:edit-articles');
        Route::post('/update/{id}', [ArticleController::class, 'update'])->name('dashboard.articles.update');
        Route::get('/delete/{id}', [ArticleController::class, 'delete'])->name('dashboard.articles.delete')->middleware('checkUserPermissions:delete-articles');
    });

    // Comments routes
    Route::group(['prefix' => 'comments'], function() {
        Route::get('/', [CommentController::class, 'index'])->name('dashboard.comments')->middleware('checkUserPermissions:view-comments');
        Route::get('/delete/{id}', [CommentController::class, 'delete'])->name('dashboard.comments.delete')->middleware('checkUserPermissions:delete-comments');
        Route::get('/approve/{id}', [CommentController::class, 'approve'])->name('dashboard.comments.approve')->middleware('checkUserPermissions:approve-comments');
        Route::get('/unapprove/{id}', [CommentController::class, 'unapprove'])->name('dashboard.comments.unapprove')->middleware('checkUserPermissions:unapprove-comments');
    });

    // User management routes
    Route::group(['prefix' => 'users'], function() {
        Route::get('/rights', [UserRightsController::class, 'index'])->name('user-rights')->middleware('checkUserPermissions:manage-user-rights');;
        Route::get('/rights/change-role/{id}', [UserRightsController::class, 'change_role'])->name('change-role')->middleware('checkUserPermissions:assign-user-roles');
        Route::post('/rights/update-role/{id}', [UserRightsController::class, 'update_role'])->name('update-role');
        Route::get('/rights/ban/{id}', [UserRightsController::class, 'ban_user'])->name('ban-user')->middleware('checkUserPermissions:ban-users');
        Route::get('/rights/activate/{id}', [UserRightsController::class, 'activate_user'])->name('activate-user')->middleware('checkUserPermissions:activate-users');
    });

});

The Super-admin has several abilities related to user management:

use App\Models\User;
use App\Models\Role;

class UserRightsController extends Controller
{
  public function roles() {
    return Role::all();
  }
  
  public function index() {
    $users = User::paginate(10);
    return view('dashboard/user-rights', ['users' => $users]);
  }

  public function change_role($id) {
    $user = User::find($id);
    return view('dashboard/change-role',['user' => $user, 'roles' => $this->roles()]);
  }

  public function update_role(Request $request, $id) {
    $user = User::find($id);
    $user->role_id = $request->get('role_id');
    $user->save();
    return redirect()->route('user-rights')->with('success', 'The role for ' . $user->first_name . ' ' . $user->last_name . ' was updated');
  }

  public function ban_user($id){
    User::find($id)->update(['active' => 0]);
    return redirect()->back()->with('success', 'The user is now banned');
  }

  public function activate_user($id){
    User::find($id)->update(['active' => 1]);
    return redirect()->back()->with('success', 'The user is now active');
  }
}

Questions:

1. Do you see any security issues in the code above?
2. Are there ways to make it more DRY?
3. Do you see any architecture flaw?

Answer 1

1. I see an avoidable single-use variable. And explode-join can be replaced with str_replace.

   $permission_label = join(' ',  explode('-', $permission));
   return redirect()->back()->with('error', 'You do not have permission to ' . $permission_label);
   

   Can be reduced to:

   return redirect()->back()->with(
       'error',
       'You do not have permission to ' . str_replace('-', ' ', $permission)
   );
   

2. I'd probably not declare $users in index() nor $user in change_role().

3. I recommend adding type declarations to all of the $id arguments in the UserRightsController() class as well as $permission in the hasPermissionTo() method. Even handle() can have string ...$permissions.

I do not have any insights regarding security; I have no experience with Laravel.

Answer 2

I would add a lot of line breaks for readability

For example here:

public function hasPermissionTo($permission) {
    return in_array($permission, 
        Auth::user()
            ->role
            ->permissions
            ->pluck('slug')
            ->toArray()
    );
}

or here

public function update_role(Request $request, $id) {
    $user = User::find($id);
    $user->role_id = $request->get('role_id');
    $user->save();
    return redirect()
        ->route('user-rights')
        ->with('success', 'The role for ' 
            . $user->first_name . ' ' 
            . $user->last_name . ' was updated');
}

and in the web.php

Route::get('/unapprove/{id}', [CommentController::class, 'unapprove'])
    ->name('dashboard.comments.unapprove')
    ->middleware('checkUserPermissions:unapprove-comments');