flask-allows icon indicating copy to clipboard operation
flask-allows copied to clipboard

Stop passing request to requirements

Open justanr opened this issue 6 years ago • 6 comments

Tracking issue for following up on #20

justanr avatar Apr 15 '18 15:04 justanr

Just a question, how can the following be written if the request is not passed into the function?

def user_can_administer_org(ident, request):
    org_uuid = request.view_args['org_uuid']
    if (user_org_admin_p(ident.uuid, org_uuid) or ident.is_app_admin):
        return True
    else:
        return False
    return False

Not sure, but maybe it is possible to make the request an optional argument?

unuseless avatar Apr 17 '18 11:04 unuseless

It is optional as of 0.5. However, request in flask is a context local so you'll be able to access it via from flask import request.

This access looks weird but it's set per request and is thread safe. It's also the standard way of accessing the request rather than what I did, which was a concession to testing which unfortunately limits the ways Allows can be used.

justanr avatar Apr 17 '18 13:04 justanr

To give you an idea of the use case of the above snippet:

@main.route('/orgs/<org_uuid>/toggle-admin/<user_uuid>', methods=['POST'])
@login_required
@allows.requires(user_can_administer_org)
def org_toggle_admin(org_uuid, user_uuid):

The user_can_view_org function needs somehow access to the url parameter <org_uuid>. I fully understand that I can use the global request object to access the necessary data. And herein lies my problem: I'd like to avoid to rely on global state and pass the necessary information direct into the user_can_view_org function.

Any hint appreciated on how to achieve to pass multiple parameters into the decorator (new style without request).

unuseless avatar Apr 19 '18 19:04 unuseless

I can think of several ways, but really they're all variations on a theme: indirection.

My least favorite is, because it's overhead to remember:

lambda user: requirement(user, request) 

Alternatively, you could define a function that accepts the requirement and wraps it:

pass_request(f):
    @wraps(f)
    def wrapper(user):
        return f(user, request) 
    return wrapper

This I would actually consider including in a 0.5.1 release, potentially as a permanent member of the library. It eases the transition and would work with the helper in a predictable way.

One I like more, but can't be generalized into the library, given this Requirement:

def can_view_post(user, post_id):
    #...

Writing a wrapper like:

def passes_view_args(f, *view_args):
    @wraps(f)
    def wrapper(user):
        kwargs = {k: request.view_args.get(k) for k in view_args}
        return f(user, **kwargs) 
    return wrapper

But that only works for view args. You could generalize further and accept two arguments: one is the requirement and the other is a function that would pull data out of the request as needed and return a dictionary that would be splatted into the requirement.

justanr avatar Apr 19 '18 22:04 justanr

Adding

pass_request(f):
    @wraps(f)
    def wrapper(user):
        return f(user, request) 
    return wrapper

to the 0.5.1 release would be great, as this gives a clear, standard path on hoe to use the request in the requirements.

I think (just my opinion) that permissions should have three parts: what kind of permission is asked for, who is asking for permission, and for what object or route permission is asked.

  • what kind of permission is covered in the argument for @allows.requires
  • who is asking for permission is covered in the user object
  • what permission is asked for was covered in the request, and in the new version of flask-allows is now implicit in where it is used.

unuseless avatar Apr 23 '18 07:04 unuseless

https://flask-allows.readthedocs.io/en/latest/api.html#flask_allows.requirements.wants_request

You're in luck, I put out 0.5.1 with that change and transition docs yesterday. I forgot to update the changelog though. :facepalm:

I currently have wants_request marked as removed in 1.0 as well, but it might stick around longer.

justanr avatar Apr 23 '18 13:04 justanr