Error on user registration with file fields or ajaxified fields

@chrisgross Thanks for the report. I don't see the error message on my site. I notice that path of the error is "/media/...". So I think this error might only occur if the media module is enabled. Please could you check disabling media and see if you still see the problem?

I'm still interested in having a solution to the bug even if it only occurs with media. However I don't use media myself, so it would need more help from someone who actually uses the module and able to spend some time towards getting a solution.

OK, thanks for trying again - I can now reproduce. The key is your instruction "Then add an image field to the user profile entity."

Sorry first up I had assumed you meant to enable the setting on the accounts config page "Enable user pictures".

I now see you mean to use the tab "manage fields" and add an actual field of type image.

I'll have a look and see what I can figure out.

I've had a look but no luck so far. I'm not really that familiar with Ajax. I don't see any log in the dblog. I have no idea how to debug a problem like this. If someone else has the skills and can determine a line of code where the error occurs I can potentially solve.

Where I've got to so far:

file.module has file_ajax_upload which is the handler for 'file/ajax'. This calls drupal_process_form which I guess is the trouble.

Another idea - problems might come from the fact that this is an admin operation (i.e. not a self-registration), but $form['administer_users']['#value'] = FALSE. However when writing this module, I didn't set that to TRUE because there might be further unwanted consequences and it hasn't been necessary until now.

I expect the same warning will occur with any file field. However it only seems to occur on the register form - it's fine once the account has been created.

I'm inclined to bump this down to normal priority as it's only a specific case (field type = file, operation = create user) and it's only a warning - the file does get uploaded.

You are right. I thought I had tested that, but I guess I must have added the picture after.

Looks like there is a module https://www.drupal.org/project/reg_with_pic. Not sure if the bug would show up with that too.

I've a similar problem with an ajaxified field in the register form.

Users without the permission "administer users" can open the register form, because they have the permission "Create new users ". This works perfect.

Now, when the form is submitted by ajax (path 'system/ajax') to update a field, I get an permission error (looks like the error described in this issue).

The problem in my case is, that the module alters the menu "admin/people/create" and uses _administerusersbyrole_can_create_users as access callback. This callback does set the 'administer users' permission for the current request. This is not done on the ajax request and thats the reason why this error occurs.

I'm not sure it's the same problem as described with the file field, but the upload is also by ajax.

This patch solves the problem with ajaxified forms. The permission is now also set when the form is submitted by ajax. I use the hook_init, because I found no preprocess hook in the ajax_get_form() process.

I do not check the menu which is called (like "system/ajax"), because there are other submission paths like file upload.

I hope that AdamPS will do a review. ;-)

But I think it's save, because the same function is used as on the menu 'admin/people/create'.

@weri great work, thanks very much for figuring that out.

For testing - if it's not already been done, check sub-admin editing a user still works and doesn't elevate permissions (the create/edit) forms share some code. Check admin user still can create. Check non-sub-admin user still can't create. Check user self register. All the obvious stuff.

Same as @chrisgross, my main concern is security. As you say, the same function is used on the menu 'admin/people/create'. But I would encourage everyone to set that aside and think about all ways this might be exploited. (It's possible there are bugs in the code we are copying too - it was copied from another module, but I'm not enough of a security expert to say for sure it's safe.)

My ideas run along these lines. Suppose the attacker requests the form. Then they craft an exploit my posting something identical to the correct Ajax request except with admin role added to the new user.

Now that I start thinking about it, it seems that this new code is safer than the original. I would guess that the Ajax request isn't actually creating the user, just saving some fields (can easily test this of course). Maybe I should have another go at trying to break the original code!

Anyway I'm going to think about this for a few days (not much spare time today) if that's OK, then check it in some time next week if we haven't found any way to break it.

Actually roles #access is guarded by user_access('administer permissions') so that should be safe.

Testing on the lines I suggested in #18: if either of you have done it, or get a chance to do it, let me know as that will speed things along.

Sorry for the delay - just got back to this one. Code mostly looks good.

But how about performance? The hook_init code will run on every page request and if there is a form, call form_get_cache. It looks like this might result in extra DB queries from the calls to cache_get. On a website where forms are successfully retrieved from cache 99% of the time, doing the lookup twice might be a significant increase in database load.

As weri says, there are many possible menu paths (file/ajax, media/ajax etc), so I don't think we can use that.

Could try to detect Ajax? E.g. see http://drupal.stackexchange.com/questions/54296/how-can-i-detect-ajax-re.... Client can fake it, but all that will cause is for us to do form_get_cache, it won't grant them an extra access.

Or maybe could save something to $_SESSION on the original form request, in _administerusersbyrole_temp_administer_users? Then only bother to call form_get_cache etc. if the $_SESSION parameter is present?

Sorry I'm not really an expert to know if those are crazy ideas, just putting some suggestions out there.

So, this is kind of hacky, but we can add a callback into $form_state['wrapper_callback'] to elevate user permissions before the form gets rebuilt. It's not exactly what the wrapper_callback is for, but it seems to work and doesn't involve running code in hook_init() on every page request.

edit: actually, that doesn't work at all. I seem to have forgotten how to trigger the issue between finding the patch last night and deciding to look into another solution this morning. Oops.

It helps to alter the correct form...

@alieffring Although you say it's hacky I actually think it looks pretty neat. But I'm a bit out of my depth here. It would be great if @chrisgross or @weri could test and/or comment.

Workaround in #2477205: Not working propery with the Administer Users by Role module. Maybe can be used to fix this.

OK, I'm happy to commit it, that seems like enough votes in favour to me. But if anyone comes along later and has any concerns then please raise them even if I've already committed!

(Personally I don't like the look of the workaround in #2477205: Not working propery with the Administer Users by Role module - it looks like it might block access to system/ajax for ordinary users.)

#29 confuses me! I didn't port this fix to D8.

D8 status is currently unknown, as is the case for various other areas and recent fixes. Either it needs fixing in D8, or it might just work if the underlying difficultly in core has gone away. At some stage there will be some determined D8 testing to figure out what still needs fixing.