3
\$\begingroup\$

I want to make my API unavailable to every client who doesn't have the token to access. This means the Android app will send a client as Android and token as token string in the header with keys client and token.

Now in middleware, I am checking it with my table fields to pass through authorization. If both match, then I will authorize and if don't then it will send a 403 response.

I am aware of Passport but it is not what I am looking for. In fact, consider it as a first layer of security and then use Passport as a second layer of security to authorize the API.

Is this code correct?

As I am not so familiar with Laravel - Middleware I just want to get some feedback from experts whether the code I have written is accurate and up to the standard. If not, I would appreciate your suggestion and help to make it better.

Middleware

namespace App\Http\Middleware;

use App\ApiToken;
use Closure;
use function response;

class ApiAccess
{
    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request $request
     * @param  \Closure                 $next
     *
     * @return mixed
     */
    public function handle( $request, Closure $next ) {

        if ( $this->checkToken( $request ) ) {
            return $next( $request );
        }

        return response()->json( [ 'error' => 'Unauthorized' ], 403 );


    }

    public function checkToken( $request ) {

        $client = $request->header( 'client' );
        $token  = $request->header( 'token' );

        $checkToken = ApiToken::where( 'client', $client )
                              ->where( 'token', $token )->first();

        return $checkToken;
    }
}

API Route

I am fetching results from the ApiToken table just to check:

Route::get('/', function(Request $request) {
    return ApiToken::all();
})->middleware('apiAccess');

Optimized with Muhammad Nauman's answer here

public function checkToken( $request ) {

    $client = $request->header( 'client' );
    $token  = $request->header( 'token' );

    return ApiToken::where( 'client', $client )
                          ->where( 'token', $token )->exists();
// Nicer, and it will return true and false based on the existence of the token and client.
}
\$\endgroup\$
1
\$\begingroup\$

My take on this is the following:

  1. rename the middleware to something meaningful (I took example from VerifyCsrfToken middleware)
  2. throw exception if token is mismatched
  3. rename checkToken to verify, reader of the code knows that code checks/verifies token because thats the point of the middleware
  4. simplify even more verify function and add docblock
  5. optimize the query to the following
    select exists(select `id` from `tokens` where (`client` = 'Android' and `token` = 'OuK0ELzYkN3Ss9Zf')) as `exists`
    

So ApiAccess becomes following:

<?php

namespace App\Http\Middleware;

use App\Exceptions\TokenMismatchException;
use App\ApiToken;
use Closure;

class VerifyApiToken
{

    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request $request
     * @param  \Closure                 $next
     *
     * @return mixed
     * @throws \App\Exceptions\TokenMismatchException
     */
    public function handle(Request $request, Closure $next)
    {
        if ($this->verify($request)) {
            return $next($request);
        }

        throw new TokenMismatchException;
    }


    /**
     * Verify token by querying database for existence of the client:token pair specified in headers.
     *
     * @param \Illuminate\Http\Request $request
     *
     * @return bool
     */
    public function verify($request): bool //optional return types
    {
        return ApiToken::select('id')->where([    // add select so Eloquent does not query for all fields
            'client' => $request->header('client'), // remove variable that is used only once
            'token'  => $request->header('token'),  // remove variable that is used only once
        ])->exists();
    }
}

Create new Exception, php artisan make:exception TokenMismatchException - yes kind of same as Laravel's stock one used when CSRF token is mismatched.

With body:

<?php

namespace App\Exceptions;

use Exception;

class TokenMismatchException extends Exception
{

    /**
     * Report the exception.
     *
     * @return void
     */
    public function report()
    {
        //
    }


    /**
     * Render the exception into an HTTP response.
     *
     * @param  \Illuminate\Http\Request $request
     *
     * @return \Illuminate\Http\Response|null
     */
    public function render($request)
    {
        if ($request->wantsJson()) { // if request has `Accept: application/json` header present
            return response()->json(['error' => 'Unauthorized'], 403);
        }

        return abort(403);
    }
}

Note: remove use function response; statements because helpers are auto-loaded from Laravel's helpers

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.