User session deleted inappropriately by Services runtime

Where exactly are you looking to have to this new hook? Immediately before

if (drupal_save_session($user) === FALSE) {

The idea seems sensible on my part.

I don't understand why you are calling the authenication twice, your auth method should be dealing with what ever is required to perform authenication.

And then the deauthenticate_call simply needs to do

services_auth_invoke($auth_module, 'deauthenticate_call', $settings, $controller, $args);

The deauthenticate_call should only be reversing what the initail authenticate_call did

Ah adding caching for auth methods would make senese but that would be seperate patch in my opinion.

The check is because a custom auth method might be utilising a different user and could have reset drupal_save_session to true to be honest it shouldn't have $user being passed in if I'm honest..

I'm not really understanding the use case here?

Eg. Services OAuth sets the logged in user to that of the OAuth token which Services resets to the request's original user (by default just an anonymous) when the Services call is done.

Why is that cleanup not enough? Are your auth mechanism setting more global variables than just the user? If so - why?

Here is use case from user perspective:
1. I am happy new to services user set up rest endpoint with all enabled node resource
2. I create a node
3. I try open in my browser /node and see my newly created node
4. I go back to the site but I see that I am logged off. I start thinking what I have done wrong :(

Shall we take care about this user experience and take logged in user back?

I think yes.

Moving this issue to bug category.

Seems to be a bit of confusion here as to what Services is actually trying to do - I will try to describe what it's meant to do overall:

1. At the beginning of a Services call Services looks at what user Drupal thinks is logged in - it saves that user away along with its session.
2. It then deactivates sessions so that any changes to the global user object within Services can't leak out and resets the user object to be an anonymous user so that no external auth mechanisms leaks in.
3. If there's any authentication modules these gets executed - the new session auth module restores the global user object to what Drupal thought was logged in and eg. the Services OAuth module instead sets the user of the supplied user token as the global user object. (OAuth is relevant here as it's the only core auth module that actually sets the user to a non-session derived user)
4. The actual calls are executed - if you eg. call /user/login then a new session will be set, else sessions will still be deactivated.
5. The calls are done and if a new session hasn't been set then we restore the original state of the user and session.

Seems like @ygerasimov are suggesting that Services on purpose logs user out when an API is accessed - it does not. It's meant to restore the original session after each request - if that's not the case then that's a bug - we're only interested in enabling "sandboxing" of the user object during the duration of a Services call and there's good security reasons for doing that. Enabling the session auth module should make Services authentication behave like it did prior to the security fix - if that's not true then that's a abug as well.

@jumentous - that you get a new row in the session table seems like a bug. The only session we're touching is the one Drupal might be already using when the Services call is started. We deactivate it during the duration of the session call and then restores it. How that can cause your user 1 to be logged out seems very weird.

@jumentous - services.runtime is the right place as we want all API calls to be anonymous unless the endpoint has been specifically configured otherwise - this is to avoid similar CSRF-vulnerabilities that Drupal core solves through form tokens.

And lastly: Of course Services and the REST Server should be stateless - the REST Server was designed to be stateless - any case where Services isn't stateless is a bug (of course with the session auth exempt as sessions by nature isn't stateless).

@voxpelli thank you very very much for this detailed explanation. I think we should grab it to some wiki documentation page.

Then what I described in #7 is a bug that should be fixed.

@jumentous thanks for the patch above that code looks much tidier :)

That's the guide we followed as well when we did the initial code - I do however think that your change will break the login method on the user resource and thus break session authentication. Can someone test? If I'm wrong then please return to RTBC.

Re #9, would you be able to explain what changes I would need to make to work with the new Services runtime model?

- Flash application downloaded and invoked from Drupal site with session id.
- Flash application makes web services call with session id and other variables back to site via custom Services request, which returns custom auth token (can't use oauth model here).
- User can still use web site without being logged out.

As of now, as soon as that auth call is made the user is logged out, and thus any further requests are invalid because it's access denied.

How can I prevent the session from being destroyed?

Edit: sorry, the patch above will work in my case.

Bumping this to major as we need it for release.

Sorry gents, I got really confused. The test case I did in #7 is invalid, as browser received different cookies (for anonymous user) and then of course when it tries to open admin page, I got access denied. It has nothing to do with Services module.

I am not sure if I understand the question right on this issue. What user's session should/or should not be destroyed? Original user or user that we authenticate and act on behalf?

I have applied patch from #11 and it breaks tests on user. It happens because we destroy session after user has been logged in using drupal core auth mechanism with cookies.

@mradcliffe, @jumentous could you please come once online on IRC and explain this issue in details? Also I would like to write automatic test for this bug, so we need some guidance about your custom authentication you have problems with.

Changing status as we need more info about this issue to reproduce.

I finally created a test module that illustrates this (attached).

1. Login as an admin
2. Create a node
3. Create a service, add the testsvc blah resource (action).
4. Go to node
5. Click Submit
6. Click on View or refresh the node
7. You are now logged out.

If your curl call passes in the same session cookie, then that session is destroyed. I couldn't reproduce this until I passed in that session cookie. If the call to the web service does not include it, then it's fine. In my use case, however, a flash app. is doing this and is necessary to keep session as it will load up private filesystem assets.

Changing issue title to be a bit more informative on the issue queue.

These modified patches from jumentous *should* work. The original_user should NOT be anonymous in the test, but for some reason the conditional is failing.

As we can see in the test, we log in as one user so there is no way that we can be anonymous when we make user login request again. It doesn't make any sense. Debugging Services is so frustrating sometimes.

    $response = $this->servicesPost($this->endpoint->path . '/user/login', array('username' => $account->name, 'password' => $account->pass_raw));

    $response_data = $response['body'];

    $proper_answer = isset($response_data->sessid)
                  && isset($response_data->user)
                  && $response_data->user->name == $account->name;
    $this->assertTrue($proper_answer, t('User successfully logged in.'), 'UserResource: Login');

    // Save session details.
    $this->session_id = $response_data->sessid;
    $this->session_name = $response_data->session_name;
    $this->loggedInUser = $response_data->user;

    // Try to login with another user to get error.
    $account2 = $this->drupalCreateUser();
    $response = $this->servicesPost($this->endpoint->path . '/user/login', array('username' => $account2->name, 'password' => $account2->pass_raw));
    $this->assertTrue(strpos($response['status'], 'Already logged in as ' . $account->name) !== FALSE, t('Session is properly opened for logged in user.'), 'UserResource: Login');

    // Logout.
    $this->drupalLogout();

Is there something else going on? Is SimpleTest the problem here and not the actual code?

I really need help to get this committed because this bug is preventing me from updating Services at all.

Heres an updated patch, but as you can see, it needs work.

heres a new patch.

we've added a test, and rolled back a certain commit that we believed was causing this issue.

Please reopen if you experience this issue again.

If I am understanding this correctly, it seems like the bug is still in the latest dev release.

Following these steps results in the active user being logged out:

• Authenticate normally through Drupal.
• Make any services call (load a node for example) without any authentication calls.
• Refresh the page -- notice that you are now logged out.

This is without using any authentication services. Is this desired behavior? How would one go about ensuring that the session of the active user isn't destroyed? Maybe only calls to authentication-type methods should mess about with the active user?

Please feel free to change this back to fixed if I am misunderstanding something here.

Please provide a simple test to recreate this.

I'm just starting working with the testing module, and I'm not immediately sure how to demonstrate this problem through that interface at the moment -- but you can easily reproduce the problem by:

1. Log in to Drupal with a rest server and node resource enabled
2. Paste this into your js console:
   

   jQuery.ajax({
     url: location.protocol + '//' + location.host + Drupal.settings.basePath + 'rest/node/1.json',
     success: function(d,m){console.log(d);console.log(m); alert('Your Session is destroyed, and you are no longer logged in');},
     dataType: 'json'
   });

Note -- when manually applying the patch in #25 to services.runtime.inc -- the problem is resolved.

WEll, using the latest dev release of Services, I cannot reproduce this issue.

I copied and pasted your code for jQuery and was unable to see this as an issue.

I still think the patch in #25 should be committed though, but I haunt written a test to prove that is an issue.

We don't actually seem to have a bug so moving to a task.

I just checked out the latest version of 7.x-3.x and am still experiencing the issue with a fresh install and minimal configuration. That is: checked out services, added spyc, enabled a rest server with a node resource. I do not have any authentication modules enabled. I was still logged out by pasting the JavaScript into my console -- ie making a typical resource request. Perhaps I am missing a step in my configuration.

I was working on a simpletest to recreate this issue, but it seems like it doesn't occur when testing takes place within the sandbox of the testing class(es). When I replaced the typical means with a drupal_http_request however, my active user (not test the user) was logged out while running the test. I'm not confident that is the desired result for the test (the user running the test should ideally not be affected by such a thing), so more work would be required to figure out why this is happening (and I'm still becoming familiar with the simpletest framework).

Rypit if you enable session authentication does it fix the issue?

Like i said, i think #25 is still a valid task. Ill try and write a test tomorrow if I don't get any more real work tickets :P

kylebrowning -- enabling session authentication does seem to fix the issue. I've been trying to write a test but haven't had much luck... I'm not sure if servicesGet ever ends up running through services_controller_execute, but that's where the issue seems to be (as you've pointed out).

You need to make sure you include the same php session in the tes. Please see my testsvc module in #19 for an example. I have not been able to test this using that

I'm seeing this bug in 6.x-3.1:

- create a service that has the node resource, without session authentication
- go to myendpoint/node/xx
- reload another site page: I am logged out.

Please provide a simpletest that recreates this bug without a simple test this is not going anywhere because the implication of this change affect security massively, and to be honest the use case in #36 is not really a normal use case given that you are doing everything in the same context. The old broswer in 2.x used to do strange things as well in certain cases, so moving back to a task until we have a real world situation that is actually a 'bug'.

See #7.

This is a pain for a user using and exploring this module's capabilities.

Please do not change the category.....

have you tried the patch in #25?

I did, but it doesn't apply on 6.x-3.x.

> Please do not change the category.....

Sorry but how on earth is this not a bug?

Its not a bug because its slightly intended functionality of the anonymous usage of services wen you do not have session authentication enabled.

Enable sessions authentication and this shouldn't be an issue.

It is however rather annoying that thishapens when you are logged in, and I believe the patch in #25 fixes it.

Sounds like we disagree about the definition of what a bug is.

> It is however rather annoying

This makes it a bug.

Even if the *design* of an application is 'the application should smack the user in the face when they activate it', if the user doesn't *like* being smacked in the face and finds it unpleasant, then that is a *bug*.

I disagree, this ticket could technically be closed as, works as designed.

As someone who just spent a long time thinking I was doing something wrong before discovering this issue, I'm in the bug camp. I've read a good deal of documentation and didn't see any mention that I should expect to see my session deleted when making GET requests. So if this isn't a code bug, I'd suggest it's at least a documentation bug.

I'm still not sure how to avoid the problem. There's a few references in this thread to enabling session authentication. How exactly do I do that? I don't see any option to enable that on any of the services configuration screens I've found, nor any module I might enable that seems to match that description.

Found it! For anyone else looking: "session authentication" is a checkbox on the endpoint edit screen.

@kylebrowning's patch from #25 did work for me. I have re-rolled it.

I will try to re-roll for Drupal 6.

Here's a 6.x version of the same patch (untested).

I have real concerns about this line of code being removed

drupal_save_session(FALSE);

as it means that services calls are no longer running in a 'sandbox'

I think the one thing missing from kyle's approach was addressed in jumentous' patch with the saving and restoring of the old_state. Here's one that combines the two.

Either the tests are wrong, or the patch is bad :(

I think it's the same issue that was causing the past patches from jumentous to fail (my comments in #21). It has been so long since I had a chance to work on this.

I've taken the patch from #50 and created a patch that I can apply with git apply. It is a proper git patch.

I tried patch from #54 with the REST server -- it doesn't work with POST to user/login followed by resource GETs by an authenticated user (the authenticated session is not recognized; anonymous access is rejected). If I change the end of the patch as follows (leaving $user alone in case of a user login) it does work:

  if ($controller['callback'] == '_user_resource_login') {
    // Obliterate the old user session if we have a new login.
    drupal_save_session($user);
  }
  else {
    $user = $original_user;
  }
  drupal_save_session($old_state);

Also, why call drupal_save_session($user); and not drupal_save_session(TRUE); ?

Hi, I've been having this same issue (as in #7) with a site that uses Services for D6. I managed to backport the patch in #54 to Services=6.x-3.x-dev and resolve the problem. I haven't done that before, but I hope it helps someone.

@Klokie,

This patch isn't working for me. The user gets logged in successfully but if I send "Logout" right after login, I get below error:

"readyState":4,"responseText":"[\"User is not logged in.\"]","status":406,"statusText":"Not Acceptable: User is not logged in."}

am I missing something? I will appreciate any inputs on this.

FYI, I tried this patch on latest 6.3 dev version (18th Jul 2012), fresh install, clear cache.

I'd love to push this patch forward as this bug is the bane of when working with Services.

However, I don't follow what's being done:

+++ b/services.module
@@ -367,7 +367,11 @@ function services_services_authentication_info() {
-  $user = services_get_server_info('original_user');
+  $arg = func_get_args();
+  if ($arg[1]['callback'] <> '_user_resource_login') {
+    // The account should be restored to the session's user.
+    $user = services_get_server_info('original_user');
+  }
 }

What does that callback mean? (Also, should probably be != not <>, which isn't used anywhere in Drupal AFAIK.)

Nothing happens when the callback is that. Why?

+++ b/services.runtime.inc
@@ -101,8 +101,15 @@ function services_controller_execute($controller, $args = array(), $options = ar
+    // We do not care about the old session.
+    $old_state = drupal_save_session();

Code and comment don't seem to agree. We don't care, and yet we're saving a value?

+++ b/services.runtime.inc
@@ -161,10 +168,13 @@ function services_controller_execute($controller, $args = array(), $options = ar
+    drupal_save_session($user);

As pointed out above, it makes no sense to pass $user in here. This function takes a boolean parameter. If the intention is to pass TRUE in if there's a user account, that should be made explicit with (bool) $user, or (bool) $user->uid if what counts is whether it's an anon user or not.

Things may have changed in Drupal 7 when this patch was done but ill explain a bit about whats happening.

Basically when any method happens besides resource login, we dont want to change the user state, thats wy its checking that.

Your other comments on the code are correct.

This should be resolved in 7.x and 6.x

I'm not seeing this as fixed in the newest stable release.
I've got the server set up to only output nodes, and anonymous users have the permission to view content. When the session authentication is not checked and I make a request for the node via REST server, I get logged out if the request is made while being logged in.
Making the same request as a anonymous user works as expected and I get the content.

If I put the session authentication on, everything works as before, but I don't get logged out when making the request as a logged user.

Drupal version 7.19, exported server configuration attached, happy to provide any more debug information needed. Debug info shows that the logout is done during the services_controller_execute() (between watchdog('services', 'Passed arguments.... and watchdog('services', 'Called arguments...

I also find current state not quite right.

We should preserve session and global $user for every call beside login and logout.

Attached patch does some refactoring plus restoring user and session in case exception is caught during runtime.

Lets take a look at tests check.

#63: services-1240074-presave-session-refactoring.patch queued for re-testing.

#63: services-1240074-presave-session-refactoring.patch queued for re-testing.

It's trying to apply the ptach to tag 7.x-3.3, instead of branch 7.x-3.x. I was confused as well for a moment.

#63: services-1240074-presave-session-refactoring.patch queued for re-testing.

@mradcliffe well done! This is why bot was not able to apply the patch. I did it against 7.x-3.x-dev and not 3.3. Now it is green and I would like to know everyone's opinion about it.

I don't if my findings was beyond the scope of this issue, as I understood it you guys were trying to resolve this when using the resource of node
but I'm currently testing out Services 7.x-3.3 on a fresh install of Drupal
without any patch applied.

Using node resource with endpoint/node/[nid]
This works fine, I'm not logged out.

Changed resource. Using now user resource with endpoint/user/[uid]
I get ["Access denied for user anonymous"] and as I was user-1 I got logged out.
Was I approaching this the wrong way?

Sorry about my previous comment, guys
I totally configured my installation wrong.

What did you have to reconfigure to solve this?

To followup, when I check the "Session authentication" box at http://bestlocalreviews.com/admin/structure/services/list/ I am no longer forced to log out.

@ygerasimov I have read over this patch a few times and what is there makes sense to me. I wanted to give it a number of visits just because of what this code does.

Love it.

#63: services-1240074-presave-session-refactoring.patch queued for re-testing.

Re-roll with new services.runtime.inc location.

As its just a re-roll :)

As a follow-up, there's at least one place where this module's tests have had to work around this bug:

    // After accessing node resource we got logged out. So we login again.
    $this->drupalLogin($this->privilegedUser);

Thank you very much for reroll. I have committed patch #82.