WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

#25255 closed enhancement (invalid)

Extend admin-ajax to detect multiple user capability and admin-ajax.php does not return 0

Reported by: godhulii_1985 Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.6
Component: General Keywords: close dev-feedback
Focuses: Cc:

Description

Background
I was developing an ajax plugin and faced some problem. I wanted to write a code that will allow users to import some data as wordpress post.
However, I do not want every user to give this capability (say, only >=author can access this). So, I included the GUI as a submenu page and via POST request to the same page processed the imported data. Next, I wanted to extend the functionality and provided ajax post support so that users need not to refresh/wait their page to submit data, only a submit button beside each datarow will do the job.


Problem begins
I wrote my initial code:

add_action('wp_ajax_new_import', 'new_import_action_callback');

function new_import_action_callback() {
	new_import();
}

desired output: abc
constantly got: abc0

After lots of checking, I became sure that my code inside new_import() has no issue but the extra 0 is appearing after code flow leaves my code. So, I looked into admin-ajax.php and found this line at the end: die( '0' );

Things I understood from documentation
From execution flow, it is correct to kill/die/exit execution at this line since above this line the original wp_ajax_example action was called. However,

  • As a plugin+theme developer I never used die/exit in the codes written by me because I always transfer the control to wordpress to complete code flow (say, it has to close connection to database or other things that I don't know but necessary in core class destructors). I assumed the same for wp-ajax.
  • When I read the documentattion on http://codex.wordpress.org/AJAX_in_Plugins page, in Error Return Values section I read:
    • the response will be -1 or 0, depending on the reason for the failure => So, not clear what will happen if it succeed and I assumed nothing will be output by wp.
    • if the request succeeds, but the Ajax action does not match a Wordpress hook then admin-ajax.php will respond 0. => So, getting zero means request failed somehow.
    • One sample code mention that: // this is required to return a proper result . But this is in comment and do not reflect its vital significance. This die is necessary to end code forcefully (something unusual in plugin/theme development) but I didn't find anything (or maybe I missed) that mention that wp-ajax must be killed via ajax handling function.

Final problem: cannot restrict my function to subset of all user
It can be summarized as:

  • Submenu page or menu pages can be restricted to users via mentioning manage_example options. However, wp-ajax is common for all logged in user regardless of their capability.
  • In my original code, I do not want each user to import data so it is necessary for me to get a confirmation that logged in user has permission to import data. Currently I have modified my code as:
    add_action('wp_ajax_new_import', 'new_import_action_callback');
    
    function new_import_action_callback() {
    	if( current_user_can('manage_example') )
    		new_import();
    }
    

Suggestion: To remove confusion
The main reason for my hard time was that I didn't realize wp-ajax must be killed by me (code author). Moreover the return value was confusing and misleading also (same return value for 2 different condition which are mutually exclusive). Following things can be done:

  • Update codex to mention about the importance of killing/exiting code in wp-ajax function.
  • Update admin-ajax.php file's 77 line (wp-3.6) as: die(); . This die is unconditional and if code reached here then plugin execution is completed and wp should not echo something voluntarily that ajax-receiver end do not expect.
  • If an error happens then it is more precise to echo via http-404 header because almost every ajax plugin/libraray rely on success or error via http status code.

Enhancement: Wordpress-ajax registration function

In my opinion, some functionality should be provided for ajax functionality that give same level of control like menupage or submenu page. For example,

// restrict to only editors
add_ajax('handle_example', 'example_function', 'manage_categories');

function example_function(){
	// code goes here
	try{
	
	}catch($wp_error)
	{
		// WP_AJAX_HEADER = a function that takes a HTTP status code and print response lines from 2nd argument	
		WP_AJAX_HEADER(404, serialize($wp_error);
	}
}


Lastly, my thinking is that wordpress is like a blackbox to someone who uses this. So, access capability should be handled by wp (as done in page management) and wp should not output something extra (as it does in die('0'); even though the ajax function might have succeeded.

Change History (8)

comment:1 follow-up: markoheijnen8 months ago

The codex page http://codex.wordpress.org/AJAX_in_Plugins does describe the need of die(). I don't think we should change to just die() when the developer doesn't do it in their code. Main reason is that the code in general is used for WordPress core itself. I do like the idea of a 404 header when you don't handle die() correctly.

Also I can see why we need to have an header included to a function like wp_send_json_error() to a status code as 400.

I disagree on the part that WP should handle capabilities. Since basic capability checks can be done but you can't map meta capabilities. Since does do need to know more information like the 'edit_post' capability need to know the post ID.

comment:2 in reply to: ↑ 1 godhulii_19858 months ago

Replying to markoheijnen:

The codex page http://codex.wordpress.org/AJAX_in_Plugins does describe the need of die(). I don't think we should change to just die() when the developer doesn't do it in their code. Main reason is that the code in general is used for WordPress core itself. I do like the idea of a 404 header when you don't handle die() correctly.

Also I can see why we need to have an header included to a function like wp_send_json_error() to a status code as 400.

I disagree on the part that WP should handle capabilities. Since basic capability checks can be done but you can't map meta capabilities. Since does do need to know more information like the 'edit_post' capability need to know the post ID.

Hi Heijnen,

Thanks for your reply. I noticed in firebug console that wp-admin uses this, however if core needs that then definitely it should be there.

We both agree that wordpress should provide some feedback in case of a developer forgot to use die() properly.

Actually I mentioned basic/same capability like edit_categories, manage_options etc. My intention behind the sample code fragment was: New functionality to distribute and control ajax functions capability just like menu pages/submenu pages.


Thanks,
-Ahmed.

comment:3 follow-up: rmccue8 months ago

  • Keywords close added

I disagree that WordPress needs the built-in capability to handle this. Different people use different methods to encode the data that they pass back, so there's no way of standardising the handling here (e.g. JSON, URL encoding, HTML, etc).

comment:4 markoheijnen8 months ago

  • Keywords dev-feedback added

In general this ticket should be closed except that sending the correct status code would be something we could do.

comment:5 in reply to: ↑ 3 ; follow-up: godhulii_19858 months ago

Replying to rmccue:

I disagree that WordPress needs the built-in capability to handle this. Different people use different methods to encode the data that they pass back, so there's no way of standardising the handling here (e.g. JSON, URL encoding, HTML, etc).

What I wanted:

  • Do not let every user to call every ajax function I've written => I am using built-in capability check to distinguish users and it can be standardizing just like menu/submenu page handling. Currently wordpress distinguish logged in and logged out users, I wanted beyond that (separate in logged in users also).
  • Data handling => Sorry for the confusion, I did not ask about standardizing how developer should echo/print their code. Marko has mentioned the problem more precisely that if we have reached to line-77 in http://core.trac.wordpress.org/browser/trunk/src/wp-admin/admin-ajax.php then it does imply developer forgot to kill code and he is not aware about this. In my case, I got my expected result with HTTP-200 header followed by a zero that totally confused me.

comment:6 in reply to: ↑ 5 SergeyBiryukov8 months ago

Replying to godhulii_1985:

Do not let every user to call every ajax function I've written => I am using built-in capability check to distinguish users and it can be standardizing just like menu/submenu page handling. Currently wordpress distinguish logged in and logged out users, I wanted beyond that (separate in logged in users also).

Using current_user_can() in your AJAX handler sounds like the correct way to achieve that.

comment:7 azaozz8 months ago

Using current_user_can() in your AJAX handler sounds like the correct way to achieve that.

Exactly. Combining current_user_can() and a nonce check is the standard way core uses.

In general this ticket should be closed except that sending the correct status code would be something we could do.

Setting some HTTP status codes may behave unexpectedly with XHRs. We may be able to use some but don't see a big advantage in doing so. Thinking 'wontfix'.

comment:8 nacin8 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

The die( '0' ); isn't changing. You need to issue die() or wp_die() at the end of your callback. That's the way it has always been and there's no reason to change that.

Ajax responses are based on the same hooks API as most of core. Just because you have something hooked to an action, doesn't mean you actually need to take action. You could "fall through" to the next handler.

Note: See TracTickets for help on using tickets.