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

Prevents Ajax upload from non-logged in users #1020

Merged
merged 2 commits into from Jun 9, 2017

Conversation

Projects
None yet
4 participants
@jom
Copy link
Member

commented Jun 9, 2017

See: p6r3EZ-my-p2

@jom jom force-pushed the enh/file-upload-checks branch from 40cb39e to 7162396 Jun 9, 2017

@pgk

pgk approved these changes Jun 9, 2017

Copy link
Contributor

left a comment

LRFGTM! I love what you did there with the tests!

@jom jom force-pushed the enh/file-upload-checks branch from 7162396 to 987151e Jun 9, 2017

@@ -4,7 +4,7 @@
$field_name = isset( $field['name'] ) ? $field['name'] : $key;
$field_name .= ! empty( $field['multiple'] ) ? '[]' : '';
if ( ! empty( $field['ajax'] ) ) {
if ( ! empty( $field['ajax'] ) && job_manager_user_can_upload_file_via_ajax() ) {

This comment has been minimized.

Copy link
@tripflex

tripflex Jun 9, 2017

Collaborator

Only comment I have is that if theme devs and myself update our template overrides to include this, if a user installs or is using an older version of WPJM it will throw fatal error, so we will have to check if function exists first before calling, besides that looks good to me

This comment has been minimized.

Copy link
@tripflex

tripflex Jun 9, 2017

Collaborator

So my solution was to just add this function and call it from my template overrides:

if( ! function_exists( 'field_editor_user_can_upload_file_via_ajax' ) ){

	/**
	 * Checks if the user can upload a file via the Ajax endpoint.
	 *
	 * This is a function added for compatibility with older versions of WP Job Manager, as this function
	 * has to be called in the file-field.php template file to prevent unauthorized ajax uploads.
	 *
	 * @since @@since
	 * @return bool
	 */
	function field_editor_user_can_upload_file_via_ajax() {

		// Added in core @since 1.26.2
		if( function_exists( 'job_manager_user_can_upload_file_via_ajax' ) ){
			return job_manager_user_can_upload_file_via_ajax();
		}

		$can_upload = is_user_logged_in() && job_manager_user_can_post_job();

		/**
		 * Override ability of a user to upload a file via Ajax.
		 *
		 * @since @@since
		 *
		 * @param bool $can_upload True if they can upload files from Ajax endpoint.
		 */
		return apply_filters( 'job_manager_user_can_upload_file_via_ajax', $can_upload );
	}

}

This comment has been minimized.

Copy link
@jom

jom Jun 9, 2017

Author Member

That works. Since Ajax uploads would technically be allowed in previous versions, you could also just do a WPJM version check. Either works, though!

@tripflex

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2017

👍 LGTM

@jom jom merged commit 94c5a84 into master Jun 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jom jom deleted the enh/file-upload-checks branch Jun 9, 2017

@jom jom added this to the 1.26.2 milestone Jun 12, 2017

@razwan

This comment has been minimized.

Copy link

commented on wp-job-manager-functions.php in c4e124f Jun 15, 2017

hello @jom

I'm trying to get my head around this but something seems wrong to me. Users should be allowed to upload images based on the job_manager_user_requires_account option. However, $can_upload will always be false if the user is not logged in. Am I right?

I added a filter to job_manager_user_can_upload_file_via_ajax that always returns true to fix my issue. But am I missing something?

Best regards
Razvan

This comment has been minimized.

Copy link
Member Author

replied Jun 15, 2017

@razwan Hi there! They shouldn't be prevented from uploading files, just through the use of the Ajax endpoint. The forms should fallback to normal file uploads and work just fine.

@tripflex

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2017

@razwan --- going off @jom comment, that is true, but ONLY if you're using the core template file, and not using a theme with a template override, your own template override, or any plugin that overrides that file-field.php file

see #1043

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.