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

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

Open
nicksellen opened this issue Sep 2, 2017 · 10 comments

Comments

@nicksellen
Copy link

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:

  • would you be interested in having something this inside django-rules?
  • if not, would you be interested in making django-rules officially extendable so I don't need to use private methods/variables which might break in future versions?

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

@nicksellen
Copy link
Author

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
Copy link
Owner

dfunckt commented Sep 12, 2017

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 #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
Copy link
Owner

dfunckt commented Jul 22, 2018

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
Copy link
Author

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

  • return false
  • raise a certain exception

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
Copy link

benwhalley commented Oct 17, 2019

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
Copy link

Any news regarding this functionality?

@nicksellen
Copy link
Author

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
Copy link

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
Copy link
Author

nicksellen commented Mar 12, 2024

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
Copy link

kevpio commented Jul 23, 2024

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.

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

No branches or pull requests

5 participants