deliciousbrains / wp-background-processing

WordPress background processing class
GNU General Public License v2.0
1.06k stars 163 forks source link

Unable to get it up and running with WP REST api calls. #81

Closed noel123007 closed 7 months ago

noel123007 commented 3 years ago

I tried to trigger the WP background processing using rest api. I used the register_rest_route callback to instantiate the WP background processing class and upon api request, I tried to add tasks and dispatch them. But it doesn't work.

dgwyer commented 3 years ago

I'm also interested in triggering bg processes via the REST API, so would be interested in a fix for this too.

tserofcj commented 2 years ago

same problem here

atticus7 commented 2 years ago

Same problem here too. Works as expected when triggered from the browser. but running dispatch on the same class via the REST API doesn't work....

atticus7 commented 2 years ago

This appears to be a nonce not validating responding to the wp_remote_post. In my case I'm using an application password to call the Rest API, and that user_id is used by wp_create_nonce. But that user is no longer the current user on the other side of the wp_remote_post - hence the nonce failing check_ajax_referer.

It turns out that I don't need this REST API call to be asynchronous anyway, so it's non-issue for me now, thankfully.

ianmjones commented 1 year ago

At present the library uses AJAX for dispatching the background process and uses its security features, making it difficult to use WP-REST-API for handling the request.

There's also potential plugin load timing issues with instantiating a background process via a REST-API call which at the very least should be confirmed and documented if not fixed.

We'll keep this issue as the feature request for enabling general compatibility with the WP-REST-API.

matgargano commented 1 year ago

@ianmjones, I added a way to address this. I am not quite sure what you are getting at with There's also potential plugin load timing issues but this suffices for my use case. Doubly so in my case as my API calls require authentication.

Edit Edit: https://github.com/deliciousbrains/wp-background-processing/pull/105

matgargano commented 1 year ago

Besides doing it at the library level, as I demonstrated in #105, the only straightforward alternative is a patch to core (or plugging as they are pluggable), updating wp_create_nonce and wp_verify_nonce to include the new filter I (hypothetically) added 'nonce_user_logged_in' as demonstrated below:

if ( ! function_exists( 'wp_create_nonce' ) ) :
    /**
     * Creates a cryptographic token tied to a specific action, user, user session,
     * and window of time.
     *
     * @since 2.0.3
     * @since 4.0.0 Session tokens were integrated with nonce creation.
     *
     * @param string|int $action Scalar value to add context to the nonce.
     * @return string The token.
     */
    function wp_create_nonce( $action = -1 ) {
        $user = wp_get_current_user();
        $uid  = (int) $user->ID;
        if ( ! $uid ) {
            /** This filter is documented in wp-includes/pluggable.php */
            $uid = apply_filters( 'nonce_user_logged_out', $uid, $action );
        }

              $uid = apply_filters( 'nonce_user_logged_in', $uid, $action, 'wp_create_nonce' );

        $token = wp_get_session_token( $action );
        $i     = wp_nonce_tick( $action );

        return substr( wp_hash( $i . '|' . $action . '|' . $uid . '|' . $token, 'nonce' ), -12, 10 );
    }
endif;

and

function wp_verify_nonce( $nonce, $action = -1 ) {
        $nonce = (string) $nonce;
        $user  = wp_get_current_user();
        $uid   = (int) $user->ID;
        if ( ! $uid ) {
            /**
             * Filters whether the user who generated the nonce is logged out.
             *
             * @since 3.5.0
             *
             * @param int        $uid    ID of the nonce-owning user.
             * @param string|int $action The nonce action, or -1 if none was provided.
             */
            $uid = apply_filters( 'nonce_user_logged_out', $uid, $action );
        }

        $uid = apply_filters( 'nonce_user_logged_in', $uid, $action, 'wp_verify_nonce' );

        if ( empty( $nonce ) ) {
            return false;
        }

        $token = wp_get_session_token();
        $i     = wp_nonce_tick( $action );

        // Nonce generated 0-12 hours ago.
        $expected = substr( wp_hash( $i . '|' . $action . '|' . $uid . '|' . $token, 'nonce' ), -12, 10 );
        if ( hash_equals( $expected, $nonce ) ) {
            return 1;
        }

        // Nonce generated 12-24 hours ago.
        $expected = substr( wp_hash( ( $i - 1 ) . '|' . $action . '|' . $uid . '|' . $token, 'nonce' ), -12, 10 );
        if ( hash_equals( $expected, $nonce ) ) {
            return 2;
        }

        /**
         * Fires when nonce verification fails.
         *
         * @since 4.4.0
         *
         * @param string     $nonce  The invalid nonce.
         * @param string|int $action The nonce action.
         * @param WP_User    $user   The current user object.
         * @param string     $token  The user's session token.
         */
        do_action( 'wp_verify_nonce_failed', $nonce, $action, $user, $token );

        // Invalid nonce.
        return false;
    }

Then a filter such as:


add_filter('nonce_user_logged_in', function($uid, $action){
    if(str_contains($action, 'swpr-ingest-generator')) {
        return 0;
    }
    return $uid;
},10,2);

The problem is I am sure that WP would never merge that in (security, I'm sure). If you want your code to be used on other sites you would have to plug those two functions, which is theoretically possible, but... yet another thing to keep track of.

johnkraczek commented 1 year ago

I was struggling to get this to work ( still looking to get it working ) I tried https://github.com/deliciousbrains/wp-background-processing/pull/105 But that didn't seem to help either.

Aside from patching the core and keeping things here at the library level, has anyone figured out how to make this work?

johnkraczek commented 1 year ago

Ok, Just wanted to provide my findings, It seems this is more of a timing issue than a permissions issue.

In my plugin, each class implements a provider. This provider requires a single function to be defined: register()

Then in the root of my plugin I list all the classes and call the register function for each of them. This gives each class an opportunity to setup any hooks or actions they desire.

class AsyncJobAPI implements Provider{

private AsyncJob $job;

    public function register()
    {
        $this->job = new AsyncJob();

        add_action('rest_api_init', function () {
            register_rest_route('pt-server/v1', '/pk', [
                'methods' => 'POST',
                'callback' => array( $this, 'handleRouteCallback' )
            ]);
        });

        ... Other Routes Registered here & other class setup functions ...
    }

  ... Other Class Functions ...
}

.

In this instance, AsyncJob is extended from WP_Async_Request. If I instantiate this here in the register/setup function everything works perfectly no changes to the WP_Async_Request or anything. I was curious so I set up a test and edited the WP_Async_Request to provide a bad/invalid nonce token to the dispatch function, and things stopped working, replaced it and we are golden. In that test, I confirmed that maybe_handle was called and ran until the nonce_check

For the sake of completeness, Here is my handleRouteCallback function.

public function handleRouteCallback(\WP_REST_Request $request){

    ... other things your route will do ...

    $this->job->data( array( 'value1' => $value1, 'value2' => $value2 ) );

    $this->job->dispatch();

}

On the other hand, the way I assumed that it would work and I assume everyone else is setting this up is instantiating the AsyncJob from within the route callback like this:

public function handleRouteCallback(\WP_REST_Request $request){

    $job = new AsyncJob();

    $job->data( array( 'value1' => $value1, 'value2' => $value2 ) );

    $job->dispatch();

}

That does not work and after thinking about it overnight, its obvious why: constructing the plugin adds two actions, actions that need to be setup at the beginning of the lifecycle.

    add_action( 'wp_ajax_' . $this->identifier, array( $this, 'maybe_handle' ) );
    add_action( 'wp_ajax_nopriv_' . $this->identifier, array( $this, 'maybe_handle' ) );

these cant be setup inside a callback that is already passed that point in the lifecycle process.

Anyway, I got it working and hope that others can use this info to set up their async functions and use them with rest routes.

johnkraczek commented 1 year ago

Also, at the cost of possibly being too obvious: your code doesn't need to have the register/provider function like mine, the only real requirement is that of setting up the job early enough in the WordPress lifecycle process.

matgargano commented 1 year ago

You may be right. I could swear it worked for me so I may just put together a use case to test again— after this I realized that I can just throw my request in a background job and not have to worry about nonce validation bc - it’s backend to backend as opposed to frontend (admin user) to backend (public) and nonces not syncing. For prudency- now I just send the request from server a to server b in a background job it uses a rest call to server b which throws what I was initially doing in an async job in the background and it works great. Sent from my mobile phoneOn Jul 29, 2023, at 5:32 PM, John @.***> wrote: Also, at the cost of possibly being too obvious: your code doesn't need to have the register/provider function like mine, the only real requirement is that of setting up the job early enough in the WordPress lifecycle process.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

ianmjones commented 8 months ago

When I first read this issue I must have got the wrong idea and thought people were struggling to get WP Background Processing to use WP-REST-API for its background dispatch chain, I did not realise people were just struggling to start a background process.

We use WP-REST-API to kick off a background process all the time in various plugins, even though it uses admin-ajax.php for its background dispatch chain, and nonces to ensure that the received background request is valid.

The key is as @johnkraczek pointed out in his examples, you must have your subclass of either WP_Async_Request or WP_Background_Process instantiated as a property somewhere in the plugin nice and early, usually as part of the init hook flow.

When you then later handle a REST request to assemble (and save) job data and dispatch it to run in the background, the nonce created and passed along with the dispatched request will be accepted and checked by the admin-ajax.php process that has an instance of your class set up during init by calling maybe_handle() and then hopefully handle() if all is well.

@matgargano I take it this info makes your PR redundant?

ken-cdit commented 1 month ago

When I first read this issue I must have got the wrong idea and thought people were struggling to get WP Background Processing to use WP-REST-API for its background dispatch chain, I did not realise people were just struggling to start a background process.

We use WP-REST-API to kick off a background process all the time in various plugins, even though it uses admin-ajax.php for its background dispatch chain, and nonces to ensure that the received background request is valid.

The key is as @johnkraczek pointed out in his examples, you must have your subclass of either WP_Async_Request or WP_Background_Process instantiated as a property somewhere in the plugin nice and early, usually as part of the init hook flow.

When you then later handle a REST request to assemble (and save) job data and dispatch it to run in the background, the nonce created and passed along with the dispatched request will be accepted and checked by the admin-ajax.php process that has an instance of your class set up during init by calling maybe_handle() and then hopefully handle() if all is well.

@matgargano I take it this info makes your PR redundant?

Great info. It should be at the top of the README instead of a closed issue.

In my case I was injecting my WP_Background_Process subclass via DI container as a factory method, so it was not created early enough. Wasted a day troubleshooting until I noticed ajax requests with 400 status in access log, realized it was nonce-related, and then found this issue.

I think I will fork and customize this since I'm already getting problems from other plugins that include older versions of these classes.