Update validate_lola_access() to enforce actor binding consistently across dual-mode endpoints

[aaronjae22]

Closes #266

This PR updates the existing validate_lola_access() endpoint to continue enforcing LOLA §5 token-to-actor binding consistently across every actor-scoped endpoint.

Currently, a portability token bound to actor A could reactor actor B's LOLA augmented dual-mode responses, including B's private outbox activities. This doesn't respect LOLA §5.

Every actor-scoped LOLA endpoint falls into one of two behavioral categories:

Strict (LOLA-gated) endpoints — these serve only private/privacy-sensitive data and have no meaningful public version:

• followers, content, liked, blocked

They each call validate_lola_access(request, required_scope=True). No portability scope → 403 insufficient_scope. They already enforce token-to-actor binding.

Dual-mode endpoints — these serve a public ActivityPub response to anonymous callers, and an augmented response when a portability token is present:

• actor_detail — public: basic Actor; with scope: adds the migration object + outbox/following/followers/liked/blocked URLs.
• portability_outbox_detail — public: public activities only; with scope: all activities including private.
• following_collection— public: following list; with scope: nested actor objects gain migration augmentation.

These three do not call validate_lola_access at all. They look at request.has_portability_scope directly (inside the JSON-LD builders) and augment the response — but they never check which actor the token was issued for.

[lisad]

I have some suggestions you might enjoy, but overall this is readable and sane

[lisad]

this is a useful test util. At some point it makes sense to organize test utilities some other way than putting them in conftest.py, as a matter of taste/readability

[aaronjae22]

I noticed that myself too, that work is plan for an upcoming PR

[lisad]

Is there any time when "valid" would be True and error response would be the actor mismatch error? ISTM these could be combined.

I like the utilities you've made for building error responses. I think of them as simple templates. If I have a lot of response templates for an API or protocol, I might make a file of response_templates. They don't all have to be called "build_X_error", they can be "actor_mismatch_response". The whole response is the response template then, including "valid": False.

[aaronjae22]

valid: True never carries an error. I was kind of aware about the “valid”: False/True situation and the way I am constructing the errors and I thought of doing some work on them.

I was planning on double checking where I would need to change the responses to make them less redundant. To be honest, I added valid to have a quick way to scan through the codebase (it worked for me) but I was aware that I would need to make some adjustment to them.

In the current codebase we're using the following

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}

And that could become:

if required_scope and not has_scope:
    return build_insufficient_scope_error(...)
  if not has_scope:
    return None
  
  ...
  
  return None

[lisad]

If validate_lola_access is a decorator, why is this not applied as a decorator? It could be a decorator that returns its own result if validate lola access fails. Is it because the method takes a "required_scope"? that could be solved with two decorators, "validate_access_scope_true" and "validate_access_no_scope" or something like that

[aaronjae22]

Makes sense, I agree

[lisad]

If validate_lola_access is going to be treated as a decorator and declared on the view, then this also needs to be - but this would also make a nice clean "actor_required" decorator.

[lisad]

Since you need the actor after applying the decorator, just pass the actor into "actor_detail" from the decorator.

[aaronjae22]

You’re completely right about it. Let me check how I would implement those but since the order is 404 first, then 403 (a nonexisting actor gets 404 regardless of token because the existence check runs before validate_lola_access) should be something like

@api_view(["GET"])
  @authentication_classes([OptionalOAuth2Authentication])
  @activitypub_content
  @actor_required # runs first → 404 if missing, injects `actor`
  @validate_lola_access(required_scope=False) # runs second → 403 on bad binding
  def actor_detail(request, pk, actor):
    auth_context = build_auth_context(request)
    return Response(build_actor_json_ld(actor, auth_context))

[aaronjae22]

I iterated over these suggestions and I incorporated them but since I am working on stacked PRs I’ll submit them in another PR next just with these changes.

To be fair, I had to reorganize a couple of things in order to include these changes first but I agree with all of them and I saw them as kind of necessary for upcoming work. They are a great refactor to the codebase and it also helped me to unblock a few more things that could be done/refactor.