permissions: Allow permissions to be composed by xordoquy · Pull Request #5753 · encode/django-rest-framework (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

xordoquy

Implement a system to compose permissions with and / or.
This is performed by returning an OperationHolder instance that keeps the
permission classes and type of composition (and / or).
When called it will return a AND/OR instance that will then delegate the
permission check to the operands.

This will allow permissions to look such as:

permission_classes = [IsAuthenticated & (ReadOnly | IsAdmin)]

MattBlack85, auvipy, rpkilby, bluesurfer, rsinger86, stunaz, eavictor, hongweipeng, kltdwrds, willstott101, and 9 more reacted with hooray emoji auvipy, jerinpetergeorge, klement97, toransahu, Chamane, and donaldte reacted with heart emoji

@carltongibson

Ooh. This looks quite good fun. 🙂

@xordoquy xordoquy changed the titlepermissions: Allow permissions to be composed [WIP] permissions: Allow permissions to be composed

Jan 20, 2018

carltongibson

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eavictor

Great feature, this will make project code more simple than before.

I'm wondering if it is a good idea to also include ! (NOT operation) in.

If we have an API for account registration from mobile device.

example:
permission_classes = [! IsAuthenticated]

@xordoquy

I'm wondering if it is a good idea to also include ! (NOT operation) in.

I though about it but think using an explicit not is better although I haven't tested it yet.

@hongweipeng

This looks wonderful. It means that permission_classes does not need to be a list, an OperationHolder can be competent.

example:

permission_classes = IsAuthenticated & (ReadOnly | IsAdmin)

@stunaz

yeah, wonderfull, what's the hold up?

@xordoquy

lack of time to update the documentation.

@carltongibson

Hey @xordoquy. I don't know if you have capacity to add a small section to the permissions docs for this? If not I'll try and have a look at it. Thanks. 👍

@tomchristie

I apprciate the work here, but if I'm being honest I'd have to say that I'm lukewarm on us allowing users to add further indirections to the flow of determining permissions. I'd probably rather force users to write explicit custom permission classes.

@xordoquy

I'd probably rather force users to write explicit custom permission classes.

I'd sort of agree here but also consider that it raises the bar higher because you have to get a more in-depth understanding of the rest framework permissions. Anyone that don't spent a lot of time in APIs might not be able to spend a lot of time in DRF itself.
What about a warning saying that we highly prefer custom permissions over composition but still offer composition ?

@carltongibson

I also ❤️ this. 🙂 (I think it's nice API.)

One thing I'd half-pondered was including a decent example of writing a test case to make sure you'd covered the truth-table correctly when composing permissions. What's the danger here? Users getting and when they meant or...? — it seems a bit mean to not include it for that.

(Of course there's always the third-party package but...)

@xordoquy

Yup, coverage should be good enough but the truth table should be better.
Will try to find some time for documentation first, then I'll refactor the tests (custom true/false permission classes instead of IsAdmin / AllowAny and py.test parametrization)

@xordoquy

Implement a system to compose permissions with and / or. This is performed by returning an OperationHolder instance that keeps the permission classes and type of composition (and / or). When called it will return a AND/OR instance that will then delegate the permission check to the operands.

@xordoquy

PR rebased & documentation updated.

@xordoquy xordoquy changed the title[WIP] permissions: Allow permissions to be composed permissions: Allow permissions to be composed

Oct 2, 2018

@xordoquy

let me know if you think the documentation is not enough.

@carltongibson

@xordoquy I can't see the documentation changes. There's just the code + test. (Did you push? 🙂)

@xordoquy

sure, I did push. Looks like I forgot to git add the file though...

@xordoquy

@xordoquy

carltongibson

return request.method in SAFE_METHODS
class ExampleView(APIView):
permission_classes = (IsAuthenticated|ReadOnly)

This comment was marked as resolved.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch.

@xordoquy

@xordoquy

Let me know if this requires other changes

@carltongibson

I think it looks good to me. (Thanks for the effort @xordoquy!)

@tomchristie

Ace work @xordoquy. Lovely headline feature, that.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request

Nov 17, 2020

@xordoquy

Implement a system to compose permissions with and / or. This is performed by returning an OperationHolder instance that keeps the permission classes and type of composition (and / or). When called it will return a AND/OR instance that will then delegate the permission check to the operands.