Changeset View
Standalone View
src/app/Http/Controllers/API/V4/CompanionAppsController.php
- This file was added.
<?php | |||||
namespace App\Http\Controllers\API\V4; | |||||
use App\Http\Controllers\Controller; | |||||
use Illuminate\Support\Facades\Cache; | |||||
use Illuminate\Http\Request; | |||||
use Illuminate\Support\Facades\Auth; | |||||
class CompanionAppsController extends Controller | |||||
{ | |||||
public function register(Request $request) | |||||
{ | |||||
$user = Auth::guard()->user(); | |||||
if (!$user) { | |||||
throw new \Exception("Authentication required."); | |||||
} | |||||
$notificationToken = $request->notificationToken; | |||||
$deviceId = $request->deviceId; | |||||
\Log::debug("Registering app. Notification token: {$notificationToken} Device id: {$deviceId}"); | |||||
machniak: I prefer to use $this->guard(), one `use` clause less. | |||||
$app = \App\CompanionApp::where('device_id', $deviceId)->first(); | |||||
Done Inline ActionsHere you can just use $this->errorResponse(401); And anyway, is that check needed. I think it's controlled by the middleware. BTW, you could test that. machniak: Here you can just use `$this->errorResponse(401);` And anyway, is that check needed. I think… | |||||
if (!$app) { | |||||
$app = new \App\CompanionApp(); | |||||
Done Inline ActionsDon't we need input validation for these? machniak: Don't we need input validation for these? | |||||
$app->user_id = $user->id; | |||||
Done Inline Actionsdevice_id column size is 100 in the database. The notification_token column has no limit set, which it should because otherwise it uses the default length, which value I never remember. machniak: device_id column size is 100 in the database. The notification_token column has no limit set… | |||||
$app->device_id = $deviceId; | |||||
$app->mfa_enabled = true; | |||||
} | |||||
Done Inline ActionsTwo problems here. There's no index on device_id column. Second, we should check if the existing entry belongs to the same user, to be safe. machniak: Two problems here. There's no index on `device_id` column. Second, we should check if the… | |||||
$app->notification_token = $notificationToken; | |||||
$app->save(); | |||||
Done Inline ActionsThere's a max too, isn't it? machniak: There's a max too, isn't it? | |||||
$result['status'] = 'success'; | |||||
return response()->json($result); | |||||
} | |||||
/* //Require TOTP? */ | |||||
/* //We could also use a 2fa auth capability, that we could then use to guard the clientscontroller */ | |||||
/* public function enable2FA($token) */ | |||||
/* { */ | |||||
/* \Log::debug("Confirm on {$token}"); */ | |||||
/* /1* if ($connectionId = Cache::get("confirm-{$token}")) { *1/ */ | |||||
/* /1* if ($connection = \App\AuthAttempt::where('id', $connectionId)->first()) { *1/ */ | |||||
/* /1* $connection->accept(); *1/ */ | |||||
/* /1* $connection->save(); *1/ */ | |||||
/* /1* } else { *1/ */ | |||||
/* /1* \Log::warning("Couldn't find the connection: {$connectionId}"); *1/ */ | |||||
/* /1* } *1/ */ | |||||
Done Inline ActionsI think 403 will be more appropriate. We don't use 401 outside of the AuthController. machniak: I think 403 will be more appropriate. We don't use 401 outside of the AuthController. | |||||
/* /1* Cache::forget("confirm-{$token}"); *1/ */ | |||||
/* /1* } else { *1/ */ | |||||
/* /1* \Log::warning("Couldn't find the token: {$token}"); *1/ */ | |||||
/* /1* } *1/ */ | |||||
/* return response("", 200); */ | |||||
/* } */ | |||||
Done Inline ActionsI don't think we should return/display the other user ID to the requesting user. This should be just $this->errorResponse(401), I guess. machniak: I don't think we should return/display the other user ID to the requesting user. This should be… | |||||
} |
I prefer to use $this->guard(), one use clause less.