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\Http\Request; | |||||
use Illuminate\Support\Facades\Validator; | |||||
class CompanionAppsController extends Controller | |||||
{ | |||||
/** | |||||
* Register a companion app. | |||||
* | |||||
* @param \Illuminate\Http\Request $request The API request. | |||||
* | |||||
* @return \Illuminate\Http\JsonResponse The response | |||||
*/ | |||||
public function register(Request $request) | |||||
{ | |||||
$user = $this->guard()->user(); | |||||
$v = Validator::make( | |||||
machniak: I prefer to use $this->guard(), one `use` clause less. | |||||
$request->all(), | |||||
[ | |||||
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… | |||||
'notificationToken' => 'required|min:4|max:512', | |||||
'deviceId' => 'required|min:4|max:64', | |||||
Done Inline ActionsDon't we need input validation for these? machniak: Don't we need input validation for these? | |||||
] | |||||
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… | |||||
); | |||||
if ($v->fails()) { | |||||
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… | |||||
return response()->json(['status' => 'error', 'errors' => $v->errors()], 422); | |||||
} | |||||
$notificationToken = $request->notificationToken; | |||||
Done Inline ActionsThere's a max too, isn't it? machniak: There's a max too, isn't it? | |||||
$deviceId = $request->deviceId; | |||||
\Log::info("Registering app. Notification token: {$notificationToken} Device id: {$deviceId}"); | |||||
$app = \App\CompanionApp::where('device_id', $deviceId)->first(); | |||||
if (!$app) { | |||||
$app = new \App\CompanionApp(); | |||||
$app->user_id = $user->id; | |||||
$app->device_id = $deviceId; | |||||
$app->mfa_enabled = true; | |||||
} else { | |||||
//FIXME this allows a user to probe for another users deviceId | |||||
if ($app->user_id != $user->id) { | |||||
\Log::warning("User mismatch on device registration. Expected {$user->id} but found {$app->user_id}"); | |||||
return $this->errorResponse(403); | |||||
} | |||||
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. | |||||
} | |||||
$app->notification_token = $notificationToken; | |||||
$app->save(); | |||||
return response()->json(['status' => 'success']); | |||||
} | |||||
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.