Allow requirement runner and on_fail to be aware of which requirement failed.

[justanr]

Right now allows.fulfill doesn't know which requirement failed and consequently the on_fail callback doesn't either. We noticed this in flaskbb when adding on_fail callbacks got noisy to say the least.

For example:

    decorators = [allows.requires(
        CanAccessForum(),
        on_fail=FlashAndRedirect(
            message=_("You are not allowed to access that forum"),
            level="warning",
            endpoint=lambda *a, **k: current_category.url
        )
    )]

While this one example isn't that bad, it's repeated over and over throughout the views file.

To ease this up, it seems beneficial to let on_fail know which requirement failed which involves a few changes:

Allows.run's check no longer uses all (this is only import internally):

for req in all_requirements:
    result = _call_requirement(req, identity, request)
    if not result:
        return result
return True  # this isn't great, I'll get to that

Next, Requirement.fulfill changes its return value from a boolean to a result type. Just free styling something, it could look like:

class Result(object):
    __slots__ = ('passed', 'context')

    def __init__(self, passed, context=None):
        self.passed = passed
        self.context = context  # just a blob, but we should standardize it

    def __bool__(self):
        return self.passed

    __nonzero__ = __bool__  # py2

    @classmethod
    def as_passed(cls, context=None):
        return cls(True, context)

    @classmethod
    def as_failed(cls, context=None):
        return cls(False, context)

    # helper method to ease transition from raw bools
    @classmethod
    def from_bool(cls, value, context=None):
        return cls.as_passed(context) if value else cls.as_failed(context)

Now the runner is aware of not only which requirement failed but has context about the failure.

An alternative thought is that fulfill would return the requirement that failed. This would require putting a __bool__ method on the requirement itself that could somehow know if it failed or not, it also complicates function based requirements (functions are always truthy).

Finally, on_fail needs to change -- this would be a breaking change no matter how it's done so I'd rather bite the bullet and make the big change rather than a small change we'll need to adjust in the future. Since I learned my lesson with user-only requirements, I'll provide a helper for the transition so it's not a completely breaking change, could actually act as a toggle -- anyways.

Right now, on_fail is called with whatever arguments the original function was called with -- in the case of Permission its called with nothing and in the case of the new guard_blueprint whatever the view_args for the request are. However, in order to make on_fail aware of the failure context, we need to supplement those arguments. In my opinion, it's not nice to assume that a particular keyword will never be passed to a function and commandeer it's usage (unless you're the Python interpreter, in which we're all in the same boat). In that case, I propose passing two arguments to the on_fail. It's new interface looks like: on_fail(result, call_context).

• result would be the result information returned from the requirement fun.
• call_context would be the original arguments that would be passed down (empty for Permission, loaded with request.view_args for guard_blueprint, the original *args and **kwargs for decorated functions) -- the arguments would be exposed as .args and .kwargs on this object.

Back to why returning True from a successful run isn't great. These changes occur because we're relying on primitive types. In the future, if we decide that a on_success callback is needed -- for example, run a before_request check and see if we trigger something, though in general I consider whatever code executes after the requirements check to be the success handling I can this feature being requested in the future. So in general, requirements should always return whatever Result object we devise and the for loop will break on the first false and return the last result it obtained:

for req in all_requirements:
    result = _call_requirement(r, identity, request)
    if not result:
        break

return result

This will cause an exception if no requirements are run, so a default result will need to be pre-generated:

result = Result.as_passed()

for req in all_requirements:
    result = _call_requirement(r, identity, request)
    if not result:
        break

return result

The final consideration is ConditionalRequirement. Since this is a combination of many requirements that are reduced via an arbitrary operator and could be negated, it doesn't necessarily make sense to get the result object from the last failed requirement. Instead, a result object could be passed into the constructor and a success result constructed inside its fulfill method:

class ConditionalRequirement(Requirement):
    def __init__(self, *requirements, **kwargs):
        # snip
        self.failure_result = kwargs.get('failure_result', Result.as_failed())

    def fulfill(self, user, request):
        # snip
        if reduced is not None:
            reduced = not reduced if self.negated else reduced
            return self.failure_result if not reduced else Result.as_passed()

        return Result.as_passed()

If a success callback is ever needed, we'd need to add a success_result to the constructor. The downside here is that whatever is passed into the ConditionalRequirement's failure_result needs to be immutable.

Alternatively, we just keep the result from the latest requirement and return that. However, that could complicated Not and Or (as well as user defined combiners).

Finally, an adapter helper for the transition:

# this decorator would be added implicitly at this feature add
# but only if `__allows_new_style__` doesn't exist
# in the future, it will be required explicitly as the default assumption would
# be that everything is new style
def old_style_on_fail(f):  # needs a better name
    @wraps(f)
    def wrapper(result, call_context):
        return f(*call_context.args, **call_context.kwargs)
    wrapper.__allows_new_style__ = False
    return wrapper

# this decorator would need to be added explicitly by the user
# and removed in the future
def new_style_on_fail(f): # needs a better name
    f.__allows_new_style__ = True
    return f

Both would be supplied but it would be assumed that anything lacking __allows_new_style__ is an old style and continue receiving the original *args, **kwargs until some version (1.0.0??) and then opposite would be assumed, in the future the only way to trigger the old behavior would be the old_style_on_fail decorator and the new style decorator would be removed.

cc: @sh4nks, @unuseless and @jkelley05 (you two have been burned by changes like this previously).

[justanr]

Part of the result object should be the reason the check failed, otherwise this doesn't really address the main issue. This complicates things. 🤔

At first blush, reason should probably be a collection to support conditionals. But we'd need to support conditionals adding to it. Result is starting to seem like a bad abstraction or maybe I looking at it from the wrong angle.