Incorrect access check for system.connect method

Just giving a quick eyeball and I need to speak to heyrocker about this and yes I can see the issue, we either need to add a new permission for system connect in the system services module because at the moment we are calling user_access with no data hence get a FALSE returned or set the access callback is simply TRUE. The immediate fix I can see would be to add line 36 to system_service.module

'#access callback' => TRUE,

I think we will need a solution for user.login and user.logout as well. Those should also be forced always open.

I was looking through the code this morning and while forcing these callbacks to return TRUE (or 'services_acess_menu') is an option, I want to make sure it is the best one, and do some thorough testing before rolling it out. Last thing I want is to create another SA.

This will require rolling another release though.

whytewolf I assume the above temp solution worked for you?

Yes, when I call system.connect via the browser, I get that error too.

Then I updated from beta1 to beta2 of AMFPHP and now when I call system.connect via the browser, I get the error:

warning: Missing argument 1 for user_access() in /home/xxxx/public_html/modules/user/user.module on line 482.

I re-installed beta1 of amfphp and then set

'#access callback' => 'services_access_menu',

instead of to TRUE since this parameter should be a callable function.

The error then disappears.

My problem with beta2 of amfphp remains, though. Any suggestions?

Hmm...However, although system.connect and a lot of other functions work, even though I am back to using beta1 of amfphp, I still get

warning: Missing argument 1 for user_access() in /home/xxx/public_html/modules/user/user.module on line 482.

for some functions. user.logout for example.

Yes you will need to do the same thing for user.login and user.logout I believe.

After thinking about this for the evening doing the check via a real access callback is better than just setting the callback to TRUE, just keeps the code tidier and will allow for documentation of that function to explain why it just returns true. Eg Use this if you need to provide open access to services method.

I think the list we have is all need to have open access - connect, login, logout.

So did you

1. Put in

'#access callback' => 'services_access_menu',

for user.login as well? (NOTE: In the user_service module, not in user.module)

2. Re-install user service on the Modules page?

That worked for me.

You don't actually have to reinstall the module but you do have to clear cache.

I take that back. I do experience the same problem with user.login. Sorry about that.

OK attached is a patch for review. Here is how I tested it. I'm trying to make these instructions extremely detailed so that a) people can reproduce the testing environment and b) if there's a problem in my methodology someone can point it out.

I setup a fresh D6 install with nothing but stock Drupal and Services 2.1. The Services modules enabled were

Services
XMLRPC Server
Key Auth
System, Node and User Services.

I configured Services to use Sessions without API Keys. I disabled all permissions for the anonymous user, and made a simple Story node for testing. I used the following script in another sandbox to test.

// Get anon session. system.connect is the only service that does
// not require a sessionid even when you have 'Use Sess ID' checked.
$xmlrpc_url = 'http://mydomain.com/services/xmlrpc';
$anon_session = xmlrpc($xmlrpc_url, 'system.connect');

// Use anon session id to login with authentication
$user = 'user';
$password = 'password';
$authenticated_session = xmlrpc($xmlrpc_url, 'user.login', $anon_session['sessid'], $user, $password);  

// Now we have an authenticated session, and when this ID is passed to services, it will run under that user's permissions
$auth_node = xmlrpc($xmlrpc_url, 'node.get', $authenticated_session['sessid'], '1');

// If this succeeded print the node, otherwise print the error.
if ($auth_node === FALSE) {
  print '<pre>' . print_r(xmlrpc_error(), TRUE) . '<pre>';
}
else {
  print_r($auth_node);
}

// Now try the same thing with the anonymous user.
$anon_node = xmlrpc($xmlrpc_url, 'node.get', $anon_session['sessid'], '1');

// If this succeeded print the node, otherwise print the error.
if ($anon_node === FALSE) {
  print '<pre>' . print_r(xmlrpc_error(), TRUE) . '<pre>';
}
else {
  print_r($anon_node);
}

This is my standard testing script for session authentication, based on the same one I wrote up for the handbook.

So in 2.1 this all fails, because system.connect fails and without that you're nowhere. I then applied the attached patch to 2.1 to the remote server and cleared the remote server's cache. This is important!

Now when I run the above test I get

Array
(
    [nid] => 1
    [type] => story
    [language] => 
    [uid] => 1
    [status] => 1
    [created] => 1273805511
    [changed] => 1273805511
    [comment] => 2
    [promote] => 1
    [moderate] => 0
    [sticky] => 0
    [tnid] => 0
    [translate] => 0
    [vid] => 1
    [revision_uid] => 1
    [title] => Test node for Services
    [body] => Test node for Services
    [teaser] => Test node for Services
    [log] => 
    [revision_timestamp] => 1273805511
    [format] => 1
    [name] => admin
    [picture] => 
    [data] => a:0:{}
    [last_comment_timestamp] => 1273805511
    [last_comment_name] => 
    [comment_count] => 0
    [taxonomy] => Array
        (
        )
 
)
<pre>stdClass Object
(
    [is_error] => 1
    [code] => 1
    [message] => Access denied
)
<pre>

So it all works.

Would love to see some testing of this patch so we can get this fixed.

Thanks all.

Just to be through I also ran the above code against user.get, since it is an example of a service that uses the default access callback rather than specifying a custom one. This also ran properly (auth user got the user info, anon user did not.)

From a code angle this looks fine - maybe we need some docs on the services_access_menu callback only because in 6 months time we are going to have a why is it doing that moment.

So the patch inserts

'#access callback' => 'services_access_menu', for system.connect, user.login and user.logout as suggested.

This is the change both I and whytewolf implemented and still user.login won't work (the exact error described by whytewolf above). Could you please explain why? Is it because we use API keys for user.login?? I'd really like to know what the problem is since I cannot get user.login up and running on my site.

i installed the patch and system.connect works ok.
I still have an issue with menu.get. From the services admin page, when calling menu.get without any menu_id i get the result but with a warning: "Missing argument 1 for user_access() in .../modules/user/user.module on line 482.". (with menu_id it's ok, using sessid and no key auth.).
Is there a similar patch to apply to menu.get?

PWG are you using API Keys AND Sessions both together? Or just API Keys?

Both.

I just realised user.logout -via the service browser - generates the error

warning: Missing argument 1 for user_access() in /home/xxxx/public_html/modules/user/user.module on line 482.

The result reported from the function is "1"

Of all the stupid, stupid things to do! I was continuously uploading the wrong versions of the module files.
I'll do some more testing, but I think my problems with user.login/logout just went away.

I am also!!!!

Today,I update the Services module to version"6.x-2.1".

I test the "system.connect" , the Return Value is :
["#error"]=> bool(true) ["#message"]=> string(13) "Access denied"

I used API Keys AND Sessions both together.
But "system.connect" Does not require authentication....

WHO CAN TELL ME WHY ??????????????????????????????????????????????????????

I am also!!!!

Today,I update the Services module to version"6.x-2.1".

I test the "system.connect" , the Return Value is :
["#error"]=> bool(true) ["#message"]=> string(13) "Access denied"

I used API Keys AND Sessions both together.
But "system.connect" Does not require authentication....

WHO CAN TELL ME WHY ??????????????????????????????????????????????????????

I am also!!!!

Today,I update the Services module to version"6.x-2.1".

I test the "system.connect" , the Return Value is :
["#error"]=> bool(true) ["#message"]=> string(13) "Access denied"

I used API Keys AND Sessions both together.
But "system.connect" Does not require authentication....

WHO CAN TELL ME WHY ??????????????????????????????????????????????????????

@wangzhaocheng please stop spamming this issue. If you want to help, test the patch.

@PWG I test sessions + api keys this morning and it worked fine, let me know if you're good to go

Patch from #15 clears up the issue. Just make sure to clear the cache after patching.

This has been committed and should appear in the next dev release. I am going to wait a couple days to make sure no new issues crop up, then I will roll a new release by Monday.

Thanks all for your help.

@heyrocker I'm so sorry it.

Please forgive me for stupid behavior. ^_^

I am a rookie,I am very anxious.

My trouble is:
【Regardless of any method calls are returned the same value：“Access denied”】

I would also like to know，what's the patch to test ???

Thank you !!!

Looking forward to reply

Please use the patch in comment #15 or download the current -dev version of the module for testing.

heyrocker: Yes, I'm good to go!

Just reporting that I experienced the same problem and that the patch of #15 works for me. Thanks!

#15: 797448.patch queued for re-testing.

Please do not change status when a maintainer has marked stuff as fixed.

Doesn't the menu_service.module also need '#access callback' => 'services_access_menu' ? I applied the patch, and tried menu.get which gave me an Access denied error. Adding the access callback line to menu_service.module removed the access denied error, but doesn't give me any results. The result is empty..

Anyone else having troubles with loading menus with as3?

I think a better option here would be to add an access permission for this service. The base function menu_tree_all_data() already checks user_access() so I don't think any protected data can be retrieved, I still think that having a permission here is prudent.

Can you test the attached patch? You will have to clear cache and give the appropriate user permission to 'get menu information'.

menu patch working for me. Thanks!
Another question: i use amfphp with sessid and no auth. key, is it normal that i can specify any string as sessid ("cool" would work as well as a valid sessid). I think it's odd that i *have* to specify a String but it can be anything.

I found two taxonomy services in the same situation and added access checks for them as well. Attached is the final patch which I will commit today.

@sitron2 I don't know if it is "normal" but it is the expected behavior. These sessions are assigned to the anonymous user just like any other unknown or new session so I've never considered it a problem worth fixing.

ok, thanks for the explanation.

Thanks for the quick patch heyrocker!
menu.get works now with ordinary menus, but I still have problems with one menu where I've added links to views on the first menu level. ie attached image. Using AS3 it returns nothing (no error, no object, no nothing), but testing the same menu in drupal (services/browse/menu.get) I get the whole menu-tree as intended.

Hello,

I installed the #15 patch, but with some "system" method I continue to have warnings :

warning: call_user_func_array() expects parameter 2 to be array, null given in /sites/all/modules/services/services.module on line 434.

when I call system.connect, system.getServices and system.cacheClearAll...

It seems the warning is because theses methods doesn't have arguments for the call_user_func_array to test access callback

If someone have an idea on how to fix this problem...

Please upgrade to 2.2 and clear cache.

heyrocker, it's done... 2.2 installed, cache cleared in /admin/settings/performance, /admin/settings/performance/boost and even in database cache table (all entries) but problem persist :-(

this bug apparently depends on platform / php version, we fixed it like this (in services.module)

line 434: if (call_user_func_array($method['access callback'], (array)$access_arguments) != TRUE) {

line 445: $result = call_user_func_array($method['callback'], (array)$args);

Could you tell me wabout your hosting information and what versions of things you're using? it would be nice to get that sorted out if it really is a platform/version problem.

I think that "warning: call_user_func_array() expects parameter 2 to be array, null given" happens due to incompatibility with PHP 5.3. Do you run PHP 5.3?

#52, you're right, the one that wasn't working is running php 5.3. The other one 5.2

Im also running PHP Version 5.3.2-1ubuntu4.2 and getting warning: call_user_func_array() expects parameter 2 to be array, null given in /var/www/sites/all/modules/services/services.module on line 434. when I go to system.connect and click on call method button..

So far this happens to:

system.getServices
system.cacheClearAll

using Services 6.x-2.2

This is a PHP 5.3 issue, and I'd like to move all discussion over to #809226: PHP 5.3 and Services Issues with call_user_func_array

Thanks

same issue for services 3.x

i had to disable the following code part (line 139, services.runtime.inc)

  /** Call default or custom access callback
  if (call_user_func_array($controller['access callback'], $access_arguments) != TRUE) {
    global $user;
    return services_error(t('Access denied for user !uid "@user"', array(
      '!uid'  => $user->uid,
      '@user' => isset($user->name) ? $user->name : 'anonymous',
    )), 401);
  }
  **/

no i can get a session id via path/to/endpoint/system/connect

is there any possibilty to fix this issue?

#50 Solved it for me! Strong cast that punk. Thanks attics!