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

Improve Extensibility: Better custom support for conneg and authorized fetch #1148

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

pfefferle
Copy link
Member

This came up in a discussion with @Menrath to better support custom endpoints/uris with content negotiation and authorized fetch.

Proposed changes:

Other information:

  • Have you written new tests for your changes, if applicable?

Testing instructions:

  • Go to '..'

…d fetch

This came up in a discussion with @Menrath to better support custom endpoints/uris with content negotiation and authorized fetch.
@pfefferle
Copy link
Member Author

I would love to hear your feedback on that @Menrath !

@pfefferle pfefferle requested a review from a team January 10, 2025 22:32
@pfefferle pfefferle self-assigned this Jan 10, 2025
@Menrath Menrath mentioned this pull request Jan 11, 2025
1 task
@Menrath
Copy link
Contributor

Menrath commented Jan 11, 2025

Thanks for drafting this! For completeness and documentation, so that other interested persons can follow this discussion better I will write some more introduction lines, and review/propose improvements in a second step.

This review took me quite some hours. And it's not complete yet :/

Use cases:

  1. Redirect of cached remote ActivityPub objects. The internal use case is redirecting cached remote Notes [1] which are stored/used as WP_Comments.
  2. Dealing with ActivityPub objects that are stored within WordPress but may not be referenced by the it's post URL.
  3. Allow ActivityStreams representations of non-public post types (e.g., an Event plugin might have a custom post type for locations: these are non-public in the sense there is no public archive for humans, but I would like them to be fetchable by their ID via ActivityPub, and also be able to send updates of those, rather than having to send Updates off all Events using that location).
  4. Additional and new – I wanted to mention it here: Maybe also be able to negotiate to ActivityPub transformers for objects other than Posts, Comments or Users/Actors, e.g., Terms.

Goals:

  • Readability
  • Performance (e.g., not being forced to call the function is_activitypub_request again).

My concrete examples:

  1. Dealing with cached remote ActivityPub objects.
    The Event Bridge For ActivityPub is inserting remote ActivityPub objects as WP_Posts, they are treated as cache. Therefore they should not be federated by the current Dispatcher/Scheduler class.
    This is prevented by adding a filter to 'activitypub_is_post_disabled' which is applied in the ActivityPub\is_post_disabled function. These posts are displayed locally when queried without ActivityPub request headers. But if it is an ActivityPub requ

    Redirect of cached remote ActivityPubobjects. The internal use case is redirecting cached remote Notes [1] which are stored/used as WP_Comments.est one must be redirected to the origin. These can be achieved in the same manner as it happens with the comments.
    Maybe this template_redirect function could be more generic and cover all cases.

  2. This touched both use cases 2 and 3. I was writing an ActivityPub transformer for EventPrime events. I needed to Negotiate content that is referenced by a shortcode, the shortcode is referencing a post. The post has the custom post type where EventPrime stores its events. However, this post-type is non-public (which is kind of odd, but not up to my choice – and therefore not processed by the ActivityPub plugin, which is a sane default).

The filter activitypub_uri_supports_conneg only brings me one step further in my case.

Because I cannot get passed https://github.com/Automattic/wordpress-activitypub/blob/change/improve-conneg-extensibility/includes/functions.php#L390 as I have no guarantee that the post type page is "enabled" for ActivityPub, even though some page posts might contain the actual reference to what I want to show in the ActivityPub world.

Some general ideas:
I would propose separating concerns a bit more:

  • Having a function that checks if the request is an ActivityPub request, in the sense of it contains an activitystreams/json-ld header or the activitypub query parameter.
  • Doing the actual negotiation: What is the content being queried, does it have a ActivityPub representation enabled.

@pfefferle
Copy link
Member Author

pfefferle commented Jan 11, 2025

@Menrath I think you are right, especially because we already check for the allowed object types (is_author, is_post, ...) in several places. So I would also vote for simplifying the is_activitypub_request and have the specific checks in the template loader!

Thank you for taking the time to thoroughly review the code!!!!

@pfefferle
Copy link
Member Author

what about a queried_activitypub_object that is generated with the transformer and stored in the $wp_query ???

@@ -50,11 +50,10 @@ public static function init() {
}

\add_filter( 'activitypub_get_actor_extra_fields', array( Extra_Fields::class, 'default_actor_extra_fields' ), 10, 2 );
\add_filter( 'pre_render_activitypub_template', array( self::class, 'add_headers' ) );
Copy link
Contributor

@Menrath Menrath Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this actually the other way round?
Shouldn't it be, note the non, of course this action does not exist yet.

		\add_filter( 'pre_render_non_activitypub_template', array( self::class, 'add_headers' ) );

@Menrath
Copy link
Contributor

Menrath commented Jan 12, 2025

Actually I like the idea about getting the Transformer once and storing it "globally". This would make it possible to refactor and simplify a lot of functions and checks.

However, I wonder if the Transformer is the right place to pack the functions to determine if and which ActivityPub actor owns an object.

what about a queried_activitypub_object that is generated with the transformer and stored in the $wp_query ???

Also, I am unsure about adding stuff to WordPress globals. I have another idea in mind that might work: Create a new singleton class like ActivityPub_Request, ActivityPub_Negotiation or put it in the ActivityPub class. This would allow storing certain results for later use.

On construct (GET, non-REST requests) this would do some checks like:

  • is_activitypub_request
  • Negotiate the object the request is targeting (possibly remote or local)
  • The visibility of the object (there may be access restrictions)
  • which actor owns the object
  • Which transformer will take care of the transformation
  • Register other action hooks and filters

I haven't thought this through in detail, but maybe it will help.

@pfefferle
Copy link
Member Author

@Menrath I like the idea a lot and prepared something. That would simplify so much and would allow you to simply use the transformer!

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

Successfully merging this pull request may close these issues.

2 participants