Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

permissions: Allow permissions to be composed #5753

Merged
merged 3 commits into from Oct 3, 2018

Conversation

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 19, 2018

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)]
@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Jan 20, 2018

Ooh. This looks quite good fun. 🙂

@xordoquy xordoquy changed the title permissions: Allow permissions to be composed [WIP] permissions: Allow permissions to be composed Jan 20, 2018
@carltongibson carltongibson added this to the 3.8 Release milestone Jan 25, 2018
Copy link
Collaborator

carltongibson left a comment

Hey @xordoquy. This looks great.

On a standalone basis the main thing now is docs.

The tests seem about right: https://codecov.io/gh/encode/django-rest-framework/pull/5753/diff

I'm going to start pulling together the v3.8 release. This would make a great headline feature. 🎉

@eavictor

This comment has been minimized.

Copy link

eavictor commented Mar 2, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Mar 2, 2018

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.

@carltongibson carltongibson removed this from the 3.8 Release milestone Apr 3, 2018
@hongweipeng

This comment has been minimized.

Copy link

hongweipeng commented Apr 19, 2018

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

This comment has been minimized.

Copy link

stunaz commented Apr 25, 2018

yeah, wonderfull, what's the hold up?

@xordoquy

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Apr 25, 2018

lack of time to update the documentation.

@carltongibson carltongibson added this to the 3.9 Release milestone Jul 6, 2018
@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Jul 6, 2018

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

This comment has been minimized.

Copy link
Member

tomchristie commented Jul 13, 2018

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 xordoquy force-pushed the xordoquy/composed_permissions branch from 59fb01e to cae5507 Sep 12, 2018
@xordoquy

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Sep 12, 2018

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

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Sep 12, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Sep 12, 2018

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)

@encode encode deleted a comment from codecov-io Sep 12, 2018
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 xordoquy force-pushed the xordoquy/composed_permissions branch from cae5507 to daea006 Oct 2, 2018
@xordoquy

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Oct 2, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Oct 2, 2018

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

@encode encode deleted a comment from codecov-io Oct 2, 2018
@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Oct 2, 2018

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

@xordoquy

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Oct 2, 2018

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

@xordoquy

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Oct 2, 2018

There you are

return request.method in SAFE_METHODS

class ExampleView(APIView):
permission_classes = (IsAuthenticated|ReadOnly)

This comment was marked as resolved.

Copy link
@carltongibson

carltongibson Oct 2, 2018

Collaborator

This example doesn't match the description. You've got IsAdminOrReadOnly vs (IsAuthenticated|ReadOnly).

This comment has been minimized.

Copy link
@xordoquy

xordoquy Oct 2, 2018

Author Collaborator

good catch.

@xordoquy

This comment has been minimized.

Copy link
Collaborator Author

xordoquy commented Oct 3, 2018

Let me know if this requires other changes

@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Oct 3, 2018

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

@encode encode deleted a comment from codecov-io Oct 3, 2018
@tomchristie tomchristie merged commit b41a6cf into master Oct 3, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@tomchristie tomchristie deleted the xordoquy/composed_permissions branch Oct 3, 2018
@tomchristie

This comment has been minimized.

Copy link
Member

tomchristie commented Oct 3, 2018

Ace work @xordoquy. Lovely headline feature, that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.