userprotect_form_alter uses arg(1) for uid [#752158]

This causes protection to fail when using http://drupal.org/project/me nodule for account edit aliases (user/me/edit instead of user//edit). The user account is not loaded in userprotect_form_alter. I see that in in hook_form_alter for user_profile_form there is $form['#uid'] which can be used instead. This would fix compatability with any form of user aliasing, not only the me module.

So instead of this:

account = user_load(array('uid' => arg(1)));

there would be this:

$account = user_load(array('uid' => $form['#uid']));

#uid is provided by core, see here:
http://api.drupal.org/api/function/user_edit_form/6

Other uses of arg(1) are:

openid_user_add
openid_user_delete_form
userprotect_user_edit_access
userprotect_user_delete_access

openid_user_add and openid_user_delete_form:

According to this page:
http://api.drupal.org/api/function/openid_user_delete_form/6

The uid will be provided in:

$form['uid']['#value']

But I don't know what to do with openid_user_add...

userprotect_user_edit_access and userprotect_user_delete_access:
I see that $account is already provided in the parameters to these functions, so why use arg(1) at all? I'm not sure how menu access callback work though. And perhaps this is not an issue because menu access callbacks are not called on the aliased but actual pages? That should work on hook_form_alter too, however...

Comment	File	Size	Author
#4	uid.patch	1.35 KB	jhl.verona
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

TripleEmcoder CreditAttribution: TripleEmcoder commented 24 March 2010 at 21:54

I also found a solution in issue #510866: Allow is_numeric(arg(1)) to function correctly with me.:

The content_profile maintainer could replace the is_numeric(arg(1)) check with something like ($user = menu_get_object('user')) which should serve the same purpose, and allow the module to work with me.

Comment #2

hunmonk CreditAttribution: hunmonk commented 25 March 2010 at 03:06

this is not a high priority issue for me, so it's doubtful i'll actively fix it any time soon. however, if you submit a quality patch which addresses all the issues you're describing, i'd be happy to commit it.

Comment #3

Anonymous (not verified) CreditAttribution: Anonymous commented 6 April 2010 at 20:39

Comment #4

jhl.verona CreditAttribution: jhl.verona commented 25 May 2010 at 08:23

Version:	6.x-1.x-dev	» 6.x-1.4
Status:	Active	» Needs review

File	Size
uid.patch	1.35 KB

I also would like to integrate the User Protect project into the Services project. Specifically the User Service. Though I have less stringent necessities than memfis - I'm only concerned with the userprotect_form_alter function's override of user_profile_form itself. In this particular situation, the form is used at an URL which does not have the user/%uid/edit format.

I have supplied a patch which avoids using the arg(1) value, where possible. The reasoning for this code is both logical and empirical.

Firstly, the core user_profile_form places the $account object in the form at $form['_account']['#value']. On the contrary the $form['#uid'] value that memfis mentions, is not present in the 'Profile' tab.

Secondly, using a sandbox with just the core user and profile modules enabled, I have empirically checked the user/%uid/edit page both for the 'Account' and 'Profile' tabs - in both cases the account object was present in the form.

Thirdly, just to be absolutely sure (belt and braces) I test that if value is empty, the account is loaded using the arg(1) value, as before.

As for the use of arg(1) in the two openid forms, I can make no comment, as I have no knowledge of either the forms or the code for OpenID.
Finally, the use of arg(1) in the two access functions only affects whether a message is displayed or not, it does not affect the protection logic or its functionality.

Best regards,

John

P.S. Very useful module by the way, thank you for the hard work that you've done.

Comment #5

hunmonk CreditAttribution: hunmonk commented 14 June 2010 at 23:42

Status:

Needs review

» Fixed

thanks to everyone for their insight into this issue. i've committed fixes to 5.x-1.x-dev, 6.x-1.x-dev, and HEAD for *some* of these issues -- others are not really solvable in a clean fashion given drupal's current limitations. some details on the fixes:

the use of arg() has been removed for a better solution in the profile edit form, and the openid forms
i decided to simplify the work in the patch in #4 -- i don't think we need to be that careful. we should be able to trust core and the fact that other contribs know how to play well with core, otherwise the code base would be bloated with paranoid safety checks ;)
the use of arg() in the access checks is unfortunately a much uglier issue. those checks are there to ensure that the user message is only displayed when we're on the actual user's edit page (the access check can be called at other times, for example when core is trying to decide whether to display the edit tab). it turns out that drupal still doesn't provide any easy way to determine within code what page the user is on. we could do better than the current arg() trick, but it's really a lot more work than i think it's worth. the penalty of the current solution is only that a user admin wouldn't see the message if the site had any module installed that sub'd out the uid for something else the way the 'me' module does. the access check itself still works just fine. i can live with this small UI imperfection until core provides a cleaner way to know the page context from within code.