Feedback from validate_lola_access() review PR

[aaronjae22]

Currentlyvalidate_lola_access returns a two-key dict.

What happened	Current return value
access allowed	{"valid": True}
no scope on strict endpoint	{"valid": False, "error_response": <403 insufficient_scope>}
token bound to wrong actor	{"valid": False, "error_response": <403 actor_mismatch>}

There is no row where valid is True and an error_response exists, and none where valid is False without one.
We repeat this during the 7 call sites:

validation_result = validate_lola_access(request, required_scope=False)
if not validation_result["valid"]:
    return validation_result["error_response"]

---

With these changes the function just return that Response on failure, and None on success, which will indicates something like "nothing to send, move on".

validate_lola_access becomes lola_access_error later on during a new refactor.

Before

if required_scope and not has_scope:
	    return {"valid": False, "error_response": build_insufficient_scope_error(...)}
if not has_scope:
    return {"valid": True}
...
return {"valid": True}

After

# returns the error Response to send, or None if access is allowed
if required_scope and not has_scope:
    return build_insufficient_scope_error(...)
if not has_scope:
    return None
...
return None
**_check_actor_binding** **— same treatment (it returns the same dict today):**

---

Following the suggestions on PR #267 we would now add @actor_required and @validate_lola_access as decorators but keeping in mind that 404 should execute before 403 (a nonexisting actor gets 404 regardless of token because the existence check runs before validate_lola_access)

@api_view(["GET"])
@authentication_classes([OptionalOAuth2Authentication])
@activitypub_content
@actor_required                              # (D)
@validate_lola_access(required_scope=False)  # (E)
def actor_detail(request, pk, actor):
    auth_context = build_auth_context(request)
    return Response(build_actor_json_ld(actor, auth_context))

The sxecution order at request time would be like this:

DRF dispatch (A/B):       authenticate
activitypub_content (C):  calls inner, adds headers to final response (200/403/404)
actor_required (D):       Actor.objects.get(pk) → 404 if missing; else inject `actor`, call inner
validate_lola_access (E): compute error → 403 if scope/binding fails; else call the real view
actor_detail body:        build + return 200

---

Following refactor I changed validate_lola_access() to lola_access_error(). I decided to do this because I didn't want to add the parameter to @validate_lola_access such as @validate_lola_access(required_scope=True/False) and instead use two named decorators (@lola_scope_required / @lola_scope_optional).
I thought of naming it @lola_dual_mode since its established vocabulary in the Testbed but I thought that _required/_optional makes more sense.

This new def lola_access_error(request, required_scope, url_pk): already has pk in its kwargs so we pass url_pk in directly and drop the request.resolver_match (_get_url_pk goes away).

---

So pretty much our api views will look from this:

def actor_detail(request, pk):
    try:
        actor = Actor.objects.get (pk=pk)
    except Actor.DoesNotExist:
        return build_actor_not_found_error (pk, request)
    validation_result = validate_lola_access (request, required_scope=False)
    if not validation_result|"valid"]:
        return validation_result ["error_response"]
    auth_context = build_auth_context (request)
    return Response(build_actor_json_ld(actor, auth_context))

To:

@actor_required
@lola_scope_optional
def actor_detail (request, pk, actor):
    auth_context = build_auth_context (request)
    return Response(build_actor_json_ld(actor, auth_context)

This is a massive improvement.

---

One thing to mention is that portability_outbox_detail does not look up an Actor, it looks up PortabilityOutbox.objects.get(actor_id=pk). With @actor_required injecting the actor, it becomes

@actor_required
@lola_scope_optional
def portability_outbox_detail(request, pk, actor):
    outbox = actor.portability_outbox   # every actor gets one at creation
    ...

[lisad]

Very nice cleanup