Differential D2674 Diff 7990 src/app/Http/Controllers/API/V4/CompanionAppsController.php

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(
				machniakUnsubmitted Done Inline Actions I prefer to use $this->guard(), one `use` clause less. machniak: I prefer to use $this->guard(), one `use` clause less.
				$request->all(),
				[
				machniakUnsubmitted Done Inline Actions Here 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',
				machniakUnsubmitted Done Inline Actions Don't we need input validation for these? machniak: Don't we need input validation for these?
				]
				machniakUnsubmitted Done Inline Actions device_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()) {
				machniakUnsubmitted Done Inline Actions Two 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;
				machniakUnsubmitted Done Inline Actions There'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);
				}
				machniakUnsubmitted Done Inline Actions I 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']);
				}
				machniakUnsubmitted Done Inline Actions I 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…
				}