Form _submit and _validate callbacks should not call user_access() checks. Doing so interferes with form processing and can break hook_form_alter().

Looks like the form itself needs to be rewritten to pass appropriate values to trigger certain conditions.

CommentFileSizeAuthor
#18 687586-18.patch1.08 KBMike Wacker
#11 access_check_submit_remove-68786-11.patch1.06 KBoriol_e9g
#8 access_check_submit_remove-687586-8.patch1.05 KBoriol_e9g
#6 access_check_submit_remove-687586-6.patch1.03 KBoriol_e9g

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Version: 7.x-dev » 8.x-dev
Issue tags: -D7DX +Novice, +Needs backport to D7

Moving.

starsinmypockets’s picture

starsinmypockets commented

Hmm.. this function doesn't seem to be in user.module.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Title: Remove access check from user_register_form_submit() » Remove access check from user_register_submit()

Looks like it was renamed to user_register_submit().

starsinmypockets’s picture

starsinmypockets commented

Can the user_access functions simply be removed or do they need to be replaced by some other form of access validation?

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

We should set a #value in the calling form and respond to that.

  $form['access_whatever'] = array(
     '#type' => 'value',
     '#value' => user_access('access whatever'),
  );

Doing so allows form_alter without needing to override the submit handler.

There are related issues in the queue for other submit handlers that need this pattern.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
Status: Active » Needs review
StatusFileSize
new access_check_submit_remove-687586-6.patch1.03 KB

Maybe with something like this?

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
Assigned: Unassigned » oriol_e9g
oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
StatusFileSize
new access_check_submit_remove-687586-8.patch1.05 KB

spaces

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Yes, please. The comments need some proofreading for English grammar, though.

Try.

+  // Pass access information to the submit handler. Running an access check
+  // inside the submit function interferes with form processing and breaks hook_form_alter().
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Needs work

Overlooked the actual error in the patch. The submit handler should be:

  $admin = $form_state['values']['administer_users'];

It is not proper to check $form at this stage. And since 'permissions' get stripped, we might want to change the variable name. Perhaps just 'permission'?

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
Status: Needs work » Needs review
StatusFileSize
new access_check_submit_remove-68786-11.patch1.06 KB

@agentrickard sorry if you are wasting your time :S Tests are needed/possible?

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Not a waste at all. You saved me from writing the patch.

Does this functionality already have a test? If so, this doesn't break it. If the functionality has no test, then, yes, we probably need one.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented

subscribing

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
Assigned: oriol_e9g » Unassigned
moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

This does not look like right fix to me. The form should set #redirect to where it wants to go and then no special code is needed in submit handler. An alternative is to set destination=foo in the querystring when you get to this form in the case when you want a land on a non-standard page after submission.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

@moshe

That comment seems entirely off-topic here.

tsphethean’s picture

tsphethean commented
Status: Needs work » Needs review

Agree with @agentrickard - the patch here seems to fix the issue. I've applied the same approach to a patch in http://drupal.org/node/687588#comment-5756978 for a similar issue. Setting back to needs review.

Mike Wacker’s picture

Mike Wacker commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 687586-18.patch1.08 KB

The patch is the exact same as in #11, the only difference is the path changed from modules/... to core/modules/...

The patch doesn't introduce a new feature or change anything from a functional perspective, so as long as the unit tests pass, we shouldn't need an additional test for this patch. I did however, insert a drupal_set_message() into user_register_submit() to do some one-off manual testing of the patch, and I confirmed that it works.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Hm. Isn't the correct way to do this with #access on the form..?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Oh, ok. I get it. The form can be submitted by multiple people, one option of which is an admin. And by moving the access check into the form, it can be altered to some other permission by a module.

Seems reasonable to me, and shouldn't break anything. Committed and pushed to 8.x and 7.x. Thanks!

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Actually, the problem is that you can't form_alter out the admin behavior without rewriting the submit handler from scratch. :-)

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -forms api, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Novice, +forms api, +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.