Create better way to check if user owns a device

[dbudwin]

There is a strong use of the doesUserOwnDevice(...) function. While it works well, it's kind of bulky to repeat over and over. It would be nice to find another way handle this check, maybe as middleware or a function in a base class.

Also, I don't think I like the name of the function. It doesn't read super smoothly. Maybe simplify to just ownsDevice(...)

Example code in question:

$doesUserOwnDevice = $this->currentUser()->doesUserOwnDevice($id);

if (!$doesUserOwnDevice) {
   $request->session()->flash(FlashMessageLevels::DANGER, 'Error updating device!');

   return redirect()->route('devices');
}

[kyl0b1te]

Hello, @dbudwin. I've just analyze usage of the doesUserOwnDevice function. And as I see, you need different behavior in many places. I mean, different reaction on function result.

What I propose: Create middleware and store function result like Config::set('is_device_owner', doesUserOwnDevice(...)). And use this cached value in controllers.

What do you think about it ?

[thepassenger-hub]

Middleware should work nicely. However I'm not sure how you retrieve the current user. You made in both device controllers a private function called currentUser. I would make a helper function called getCurrentUser so you can call it in the middleware.

[dbudwin]

Hello @zhikiri and @thepassenger-hub

Thanks for your interest in this. I do believe middleware can be a great fix for this.

@zhikiri That's an interesting solution! If I understand correctly, the value for is_device_owner will not get changed if the user is only making changes/controller device "A" and will be updated when they start to modify device "B." I'd love to see how you implement this solution.

@thepassenger-hub Correct, there are 2 different getCurrentUser. This is something that I also recently noticed. I would say they are not equivalent to each other and the one in the API DevicesController is mainly taking a user ID and returning the equivalent user record from the database. The web version of the DevicesController doesn't take a parameter and looks up the user's ID from the session (set when they logged in) and then retieves the equivalent user record from the database. The difference being that the API is stateless so it doesn't make sense to store the user ID in a session for retrieval later. the API DevicesController at the very least should have that function renamed to clarify intent better. A little refactoring in there could be useful.

[kyl0b1te]

@dbudwin there is not perfect way to it right now. I've just tried solution with trait, that will be common for both controllers. But it's smells...

For now I see next roadmap for this refactoring:

• change the request parameter names (make it the same between requests); // I'm speaking about https://github.com/dbudwin/RoboHome-Web/blob/master/app/Http/Controllers/API/DevicesController.php#L63 and https://github.com/dbudwin/RoboHome-Web/blob/master/app/Http/Controllers/API/DevicesController.php#L78
• create middleware for api, that will call DeviceOwner service (new class that will handle device checking logic)
• call DeviceOwner service in the web controller

But this changes will break backward compatibility, if there are users of API, it should in next version

[dbudwin]

Good idea on changing the request parameter names. There is also a similar mismatch on the lines beneath it with regard to id and deviceId. I think deviceId would be preferable. Issue #81 is related to that.

What is the smell of the trait you're using?

I wouldn't be too concerned with changing the API. The API is not versioned yet as you may have noticed. Once I get a few more quirks (like this task you're working on) resolved, the API may be ready to be versioned.

[kyl0b1te]

@dbudwin I've create the trait

private function getCurrentUser(int $userId = null): ?User
{
    $userId = is_null($userId) ? session(env('SESSION_USER_ID')) : $userId;
    return $this->userModel->where('user_id', $userId)->first();
}

private function isDeviceOwner(User $user, int $deviceId): bool
{
    return $user->doesUserOwnDevice($deviceId);
}

And use it in API and WEB. I can create PR, if you want

[dbudwin]

Seems reasonable. I'll have a better idea if you PR it and I'll pull it down and play with the code locally later today.

[kyl0b1te]

PR request: https://github.com/dbudwin/RoboHome-Web/pull/104

[thepassenger-hub]

I would make a helper file autoloaded with composer with 2 functions. 1- getMiddlewareGroup: Check requests middleware group ('web' or 'api') 2- getCurrentUser: Call the above function and return the user according to requests middleware group.

Then you'd make a new middleware called ownsDevice that uses these 2 functions. So you would refactor the controller currentUser methods into 2 global functions that you may need to use in case you add other controllers and would only need a single middleware instead of one for each group. If you don't like the global helper file you can put those 2 functions into a trait or extend the base controller but then you'd have to repeat a bit of code into the middleware. Just my 2 cents. If you like it I can throw a pr tomorrow.

[dev-lav]

how about using middleware @dbudwin ? like this :

public function handle($request, Closure $next)
{
    $devices = $request->user('api')->devices->pluck('id')->toArray();
    $result = in_array($request->header('X-Device-Id'), $devices);

    return response()->json([
        'status_code' => ($result) ? 200 : 422,
        'mesage' => ($result) ? 'valid device' : 'invalid device',
    ], ($result) ? 200 : 422);
}

[dbudwin]

@thepassenger-hub Interesting approach. If you want to PR it, I can take a closer look at your proposed implementation.

[dbudwin]

@dev-lav If I understood your suggested implementation, it appears it would only word for the API. Also, I'm not sure what $request->user('api') would return. Feel free to make a PR to more closely demonstrate how you would fix this issue.

[thepassenger-hub]

Can't make the global helper pass the tests because it depends on the request object. I guess going trait or adding to base controller a function is the easiest way for now.

By the way, is it me or the middleware RedirectIfAuthenticated actually does the opposite of what the name suggests and redirects to index route (which is the homepage) if user is NOT authenticated? Maybe I'm missreading but seems to me it processes the request if the session has the env variable. (I'm assuming this is the way you check if user is logged?)

For the ownsDevice middleware we could chain the middleware with RedirectIf(Not?)Authenticated for web routes and apiAuthenticator for api routes.

[dbudwin]

Good catch, the RedirectIfAuthenticated class is named backwards. This is why I'm thankful to have other people look at my project :) I don't like 'not' in the name so maybe it can get renamed ContinueIfAuthenicated?

Im not 100% sure what you mean by "Can't make the global helper pass the tests because it depends on the request object?" What is the global helper? Also don't be afraid to change the tests. The tests should be as flexible as the actual code base.

[thepassenger-hub]

You should also change the route middleware shortcut in the kernel file. As it is now 'guest' => \App\Http\Middleware\RedirectIfAuthenticated::class the route is supposed to allow guests only when in reality you want to allow authenticated users only. I don't want to mess with the laravel provided auth route cause I don't want to break anything behind but you should choose something similar like 'authenticated' => \App\Http\Middleware\RedirectIfGuest::class seems easy to read to me.

About the global helper my idea was to make something similar to the Auth facade that we could use anywhere both in controllers and in the future middleware to retrieve the current logged in user but I can't figure out how. For now I guess you could just refactor the currentUser into trait or extend the common controller as a temporary fix.

The main problem for me is understanding how the authentication works here. I'll have to dive into the amazon authenticator and I have absolutely no experience with amazon services. Gonna take some time.

[dbudwin]

I like the 'authenticated' => \App\Http\Middleware\RedirectIfGuest::class idea.

The Amazon stuff is a little tricky, but I'm also considering the merits of moving to using the authentication facade built into Laravel like you mentioned since I also want to support smart home controllers made by companies other than Amazon (Issue #98). Maybe this issue will be enough to facilitate that change. It will also make setting this project up easier because then people don't need to deal with Amazon.

I removed a lot of the default Laravel stuff when I first set this project up that I wasn't using to make my codebase smaller, so it'll need to be readded.

If you think life will be easier to re-add Laravel's authentication and remove Amazon's authentication, I'm game. Or if you want me to discuss how the Amazon authentication works, I can chime in on that later today.

[thepassenger-hub]

I sent a pr with the authenticated change if you want to try it out. As the authentication I honestly don't have the experience to weigh in about using amazon or your own or the one from laravel. You will have to discuss it with people with more experience than me.

[dbudwin]

@thepassenger-hub I've merged your PR #110. I'm going to research more about how Laravel's built-in authentication works.