Update access_view_set_mixin.py

[helderlgoliveira]

Included get_queryset method to return the scope_queryset if it has been explicity declared in the access_policy.
It calls super().get_queryset() instead of directly self.queryset to allow work with anothers mixins.
That way, won't be necessary to call the scope_queryset everytime in the view.

[micurbanski]

At first I thought it's a good idea, but after consideration there is a downside to that, if I'm correct. Let's say we have

# policies.py
class CustomAccessViewSetMixin(AccessViewSetMixin):
    def get_queryset(self):
        queryset = super().get_queryset()
        scope_queryset = self.access_policy.scope_queryset(self.request, queryset)
        if scope_queryset.exists():
            return scope_queryset
        return queryset

class SomeAccessPolicy(AccessPolicy):
    @classmethod
    def scope_queryset(cls, request, qs):
        role = request.user.role
        if role == "author":
            return qs.filter(author=request.user)
        elif role == "reviewer":
            return qs.filter(reviewer=request.user)
        else:
            return qs.none()

# views.py
class SomeView(CustomAccessViewSetMixin, viewsets.ModelViewSet):
    queryset = SomeObject.objects.all()
    access_policy = SomeAccessPolicy

Then if scoped_queryset turns out empty (users role is outside of if-clauses or user is neither author nor reviewer of any requested object) , mixin will return the view's queryset which would be quite unfortunate.

I believe that here the default approach for overwriting get_queryset each time we are using scoped_queryset is safer.

[helderlgoliveira]

Then if scoped_queryset turns out empty (users role is outside of if-clauses or user is neither author nor reviewer of any requested object) , mixin will return the view's queryset which would be quite unfortunate.

Good point, thanks! What you think about:

    def get_queryset(self):
        queryset = super().get_queryset()
        scope_queryset = self.access_policy.scope_queryset
        if scope_queryset.__name__ not in self.access_policy.__dict__:
            return queryset
        return scope_queryset(self.request, queryset)

That way, it always will return the scope_queryset if it has been overrided in the view's access_policy class.

[micurbanski]

I think that it will always evaluates to false by default:

In [20]: from rest_access_policy import AccessPolicy
In [21]: access_policy = AccessPolicy
In [22]: scope_queryset = access_policy.scope_queryset

In [23]: scope_queryset.__name__ not in access_policy.__dict__
Out[23]: False

The most stratightforward solution I see right now is to go with something like:

class AccessPolicy(permissions.BasePermission):
    "..."

    @classmethod
    def scope_queryset(cls, request, qs):
        raise NotImplementedError  # was: return qs.none()

class CustomAccessViewSetMixin(AccessViewSetMixin):
    def get_queryset(self):
        queryset = super().get_queryset()
        try:
            scope_queryset = self.access_policy.scope_queryset(self.request, queryset)
            return scope_queryset
        except NotImplementedError:
            return queryset

Or something in similar fashion. Raising NotImplementedError may not be the best solution for signalling that you're using the library default, but that's how I would go about it.

[helderlgoliveira]

    def get_queryset(self):
        queryset = super().get_queryset()
        scope_queryset = self.access_policy.scope_queryset
        if scope_queryset.__name__ not in self.access_policy.__dict__:
            return queryset
        return scope_queryset(self.request, queryset)

I think that it will always evaluates to false by default

Yes, because of the not in. The idea is to only return the scope_queryset automatically if it has been explicity declared(overriden) in the Access Policy class:

from app.views_access_policies import ScopedQuerysetAccessPolicy, NotScopedQuerysetAccessPolicy

# AccessPolicy without scope_queryset:
NotScopedQuerysetAccessPolicy.scope_queryset.__name__ not in NotScopedQuerysetAccessPolicy.__dict__
# Out: True (view's queryset will be returned)

# AccessPolicy with scope_queryset:
ScopedQuerysetAccessPolicy.scope_queryset.__name__ not in ScopedQuerysetAccessPolicy.__dict__
# Out: False (scope_queryset will be returned)

That way, if I'm correct, I think it will remains optional to implement or not the scope_queryset in the Access Policy class. Once implemented, it'll be called.

Sorry if I didn't understand what you said.

[rsinger86]

It didn't occur to me that the mixin could call this automatically for the user. Thanks for the input.

scope_queryset will always be defined because the version in the base class returns an empty queryset:
https://github.com/rsinger86/drf-access-policy/blob/master/rest_access_policy/access_policy.py#L69

I guess I'd lean toward the viewset mixin always calling scope_queryset, which seems like the safest default because it will return zero rows, unless the user defines it.

@helderlgoliveira @micurbanski what do you think?

[micurbanski]

I'm fine with that, but just to be sure- it would got something like this:

class AccessPolicy(permissions.BasePermission):
    "..."

    @classmethod
    def scope_queryset(cls, request, qs):
        return qs.none()

###
class AccessViewSetMixin:
    "..."

    def get_queryset(self):
        queryset = super().get_queryset()
        return self.access_policy.scope_queryset(self.request, queryset)

And the worst case scenario it will return qs.none() if scope_queryset is not defined by user, right? That's what I'm fine with :)

[rsinger86]

yep, exactly :)

[helderlgoliveira]

I agree, but it'll be a breaking change. Apps that didn't override scope_queryset in Access Policies classes will return the empty queryset instead of the ViewSet's queryset.

If I'm not wrong, maybe this could maintain backwards compability:

    def get_queryset(self):
        queryset = super().get_queryset()
        scope_queryset = self.access_policy.scope_queryset
        if scope_queryset.__name__ not in self.access_policy.__dict__:
            return queryset
        return scope_queryset(self.request, queryset)

Or changing the default scope_queryset to return the ViewSet's queryset, that way the method override won't be mandatory:

@classmethod
    def scope_queryset(cls, request, qs):
        return qs  # before: qs.none()

Regards

[rsinger86]

oh, good point about the breaking change.

What do you think about introducing a new mixin called StrictAccessViewSetMixin that includes the change and leave the existing mixin as-is?

[helderlgoliveira]

I agree, good idea

[ihucos]

I did a PR for that: #105