Feature access permissions

[rwrzesien]

Resolves #45
Resolves #182
Resolves #183

[kbeker]

@rwrzesien I've checked that this isn't working as we assupted. Can this work if this is still rest framework view? It even does not use get_queryset method.

[rwrzesien]

That's right, but this view is now refactored so it should work just fine.

[dybi]

Generally, seems like a piece of good work. Still, I would have some suggestions / questions.

[dybi]

the order of parent classes is different in ReportList and ReportDetail. Is it done on purpose? If yes. please explain why. If not, please keep it unified ;)

[rwrzesien]

My bad and good catch, should be like in ReportDetail, but still it won't work with APIView which doesn't use get_queryset method, but it is in progress of refactoring to django view by @Szymiks so it can wait for his pull request (or got merged, as it doesn't break anything).

[dybi]

sounds fine to me ;)

[dybi]

Maybe it is because my little experience with django, but this seems a bit complex.

1. In first decorator (login_required) we enforce user to be logged in
2. In second one (check_permissions) we check if user is of right type to be able to have access to the resource; if not, we redirect him to login page (?)
3. Then we modify the queryset of Employee, so only his reports are accessible.

Although the usage of mixin seems pretty clever, I am wondering if there's no easier / simpler way to achieve what we want. Don't get me wrong - I am really not sure, just asking :)

[rwrzesien]

Nothing I can think of. Consider this example:

There is a view class that has get_queryset() method with some complex filtering. If you would like to do this in decorator too, you would have to call and evaluate get_queryset() method of decorated class inside decorator (which sounds ugly already) to figure out if returned object match current user/role.

If this would be added in the view code in form of if, it would require overwriting method in each view in which we would like to add it. Sounds like a lot of work.

I think the mixin approach and limiting queryset is the cleanest and most optimized way to do this. At least of all those which I have thought of, but maybe there is a better way I haven't thought of.

The part I don't like with this solution is that it returns 404 instead of 403, but I think its fair trade-off.

if not, we redirect him to login page (?)

Yes, exactly this is done in this decorator.

[dybi]

@rwrzesien Ok, I'am convinced, thanks for the explanation :)

[dybi]

similarly, I would suggest adding check that we have 2 objects in the DB

[rwrzesien]

👍

[dybi]

[kbeker]

I like what you did in this PR. I tested this and this looks and works nice. Merging 👍