dfunckt / django-rules

Awesome Django authorization, without the database
MIT License
1.86k stars 149 forks source link

Return rule denial reasons (and improved integration with rest framework) #63

Open nicksellen opened 7 years ago

nicksellen commented 7 years ago

Thanks for the great library! I love the philosophy to use rules written in simple python functions over database tables.

I ran into a few troubles trying to integrate it with a django rest framework project - we don't use django permissions, which means we can't use DjangoObjectPermissions easily.

I thought to connect django-rules directly to drf permissions - ends up similar-ish to dry-rest-permissions but using django-rules.

I also wanted to be able to return custom error messages explaining why a permission was denied.

I managed to implement this by subclassing stuff in django-rules so that you can use it like this:

# in rules.py

@predicate(messages=('Team is archived', 'Team is not archived'))
def is_team_archived(_, team):
  return team.status = 'archived'

can_archive_team = ~is_team_archived

# standalone use

result, message = can_archive_team.test_with_message(user, team)
if result:
  print('yes')
else:
  print('no:', message) # prints "no: Team is archived"

# use in a @detail_route

# creates a drf compatible permission class
CanArchiveTeam = create_permission_class('CanArchiveTeam', can_archive_team)

@detail_route(
    methods=['POST'],
    permission_classes=(IsAuthenticated, CanArchiveTeam)
)
def archive(self, request, pk=None):
  # do archiving

If the CanArchiveTeam does not pass, then it returns a permission denied error with the message Team is archived.

There is a bit more information in our project discussion.

I wanted to avoid magic naming conventions (the approach taken by dry-rest-permissions) or referring to permissions using string names - explicit imports are much clearer to me.

Actually, the only API change for rules is changing/adding a method that returns (result, message) instead of result, and accepting messages as predicate args.

My questions are:

The alternative is a fork, but I don't want to contribute to the ever fragmenting django permissions ecosystem. Thanks!

nicksellen commented 7 years ago

I'm not sure if the logic for returning the correct message is quite correct yet though, it works by holding onto the message for the last rule that returned false (that had messages set). This works for normal rules, and negated rules (as I override it to swap the messages over), but might fail for more complex combinations, I didn't spend much time on that aspect yet...

dfunckt commented 7 years ago

I think providing messages at predicate-level a bit too fine-grained and feels like overloading the predicates. I'm not strictly opposed to your suggestion, but I'd have to see a real nice interface proposed to be convinced.

BTW, would https://github.com/dfunckt/django-rules/issues/59 be enough for your use-case too? Rule-level names similar to Django permissions do feel more natural to be honest and should be easy/quick/clean to implement.

dfunckt commented 6 years ago

I think a good approach would be to allow a predicate to fail by raising a specific type of exception instead of returning false -- rules could catch that, turn the result into false, stash the exception as the denial reason and offer a way for the caller to retrieve it.

nicksellen commented 6 years ago

You mean there would then be two fails to fail a predicate check?

If so, it would seem confusing to me to have two ways to achieve a similar goal.

Also, the predicate itself doesn't always know which is the good or bad outcome (as you can combine them with a ~ operator) - and it makes sense for that part of it to be handled at a higher level than the predicate itself.

benwhalley commented 5 years ago

Perhaps a nicer way to avoid breaking backwards compatibility for predicates would be to return either True/False as now or True/Failed where Failed is a new object that is false-y but also contains a message string that is processed by rules?

j-osephlong commented 8 months ago

Any news regarding this functionality?

nicksellen commented 8 months ago

This is a very old issue, but as it happens I will be doing some work soon (as in sometime this year) around permissions in the project that I was writing the original issue about, so the topic will arise again for me, and I'll be looking how to solve it.

I wonder how the landscape looks for permissions in rules today and if anything notable changed about django rules since 2017!

j-osephlong commented 8 months ago

My vision of how this could work is instead of a predicate returning True or False, they could return True or str, with the str being the failure message. This would require some changes to how truthy values are handled though, as a string would be truthy even though it is meant to indicate failure in this case.

nicksellen commented 8 months ago

Implementing a __bool__ method can address that in many scenarios.

It probably comes down to whether there is an approach that @dfunckt would be interested in having in the codebase, and someone to implement it in that way.

For my scenario I will likely use a database-backed approach as the permissions will be customizable by users.

kevpio commented 3 months ago

I've just started using these rules. Nice idea and implementation!

However, I would also like to see some kind reason code or string as to why a permission wasn't granted returned or an exception thrown so I can inform the api user of his error.