User gets logged out without calling hook_user_logout(), even if "CSRF validation failed" error occurred.

In _services_controller_execute_preserve_user_switch_anonymous($controller) function we switch user to anonymous user so that other module can perform authentication. we restore user to original user at the end of function call. If we do not restore user to original user, its session may get destroyed (see drupal_session_commit()).

If we return before calling logout callback due to CSRF validation error, hooks on user logout don't get called. But user gets logged out. This seems a bug.

CommentFileSizeAuthor
#17 bug_in_user_logout-2804641-17.patch716 bytestyler.frankenstein
#8 bug_in_user_logout-2804641-8.patch1.79 KBamar.deokar
#4 bug_in_user_logout-2804641-4.patch1.3 KBamar.deokar

Comments

amar.deokar created an issue. See original summary.

rasikap’s picture

rasikap commented
Assigned: Unassigned » rasikap
rasikap’s picture

rasikap commented

I debugged the issue, the function services_error in _services_authenticate_user in services.runtime.inc returns the error CSRF error and deletes the session without actually going into the user_logout.

amar.deokar’s picture

amar.deokar commented
StatusFileSize
new bug_in_user_logout-2804641-4.patch1.3 KB

@rasikap thanks for debugging issue.
I have tried to create patch for it.

amar.deokar’s picture

amar.deokar commented

Above patch also resolves issue 2797885 .

tyler.frankenstein’s picture

tyler.frankenstein commented
Priority: Major » Normal
Status: Active » Needs review
rasikap’s picture

rasikap commented

I tried the applying the patch, when the logout is successful, the session gets deleted and an entry for uid 0 is made in the sessions table, which gets replaced on next user login

Thanks,
Rasika P

amar.deokar’s picture

amar.deokar commented
StatusFileSize
new bug_in_user_logout-2804641-8.patch1.79 KB

@rasikap thanks for reviewing the patch. I have updated the patch for issue which you have mentioned on #7 .

rasikap’s picture

rasikap commented
Assigned: rasikap » Unassigned
Status: Needs review » Reviewed & tested by the community

Patch in #8 works for me !

Thanks,
Rasika P

amar.deokar’s picture

amar.deokar commented

@tyler.frankenstein is patch #8 good to commit ? Do we need test cases for it ?

kylebrowning’s picture

kylebrowning commented

This looks fin to me.

tyler.frankenstein’s picture

tyler.frankenstein commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

Sherif Darwish’s picture

Sherif Darwish commented

Hi,

I see a problem in this issue & the patch as when i do login to the services, i get error message "[CSRF validation failed"]", this was not working the same way in earlier version of service. I am not sure if this is intended that even login should have X-CSRF-Token header.

I traced this down and end up with this line in function _services_controller_execute_preserve_user_switch_anonymous

 $preserve_session = !is_string($controller['callback']) || (strpos($controller['callback'], 'login') === FALSE && strpos($controller['callback'], 'logout') === FALSE);

Changed to

 $preserve_session = !user_is_anonymous();

After the patch, the original session will be saved only if the user is authenticated, this means that user who tries to login will have his original session saved and then will get this error "["CSRF validation failed"]" on login.

I just need some one to confirm this as well, as i assume this change will break the login scenario in any existing application that uses drupal services and do not provide token in login.

amar.deokar’s picture

amar.deokar commented

Hi @Sherif Darwish ,
Before this patch, logout api can be called without CSRF token which returns "CSRF validation failed" error but user still gets logged out.
This patch enforces logout api to be called with valid 'CSRF token' without that user will not be logged out.

In your case it is might be possible that you were trying to login without getting logged out properly.

tyler.frankenstein’s picture

tyler.frankenstein commented
Version: 7.x-3.17 » 7.x-3.x-dev
Status: Closed (fixed) » Needs work
Related issues: +#2858216: Fix bug in user logout API 1.1
StatusFileSize
new bug_in_user_logout-2804641-17.patch716 bytes

Disclaimer - this may only effect sites running Services + (Login Toboggan and/or Email Registration modules)

I can confirm @Sherif Darwish's problem as well, except it's also effecting the "auto login" of a user immediately after registering an account by throwing the "CSRF validation failed". All subsequent login attempts for this new user also receive the "CSRF validation failed" message.

Here's a patch which reverts the one line of code causing the problem. It appears this does not effect Services out of the box, but since utilizing Services in conjunction with Login Toboggan and/or Email Registration isn't uncommon, I'd like to arrive at a solution that makes all camps happy.

@rasikap & amar.deokar, does the one line revert still provide the desired behavior for you?

Possibly related: #2858216: Fix bug in user logout API 1.1

tyler.frankenstein’s picture

tyler.frankenstein commented
Status: Needs work » Needs review
RAWDESK’s picture

RAWDESK commented

Patch #8 seems to have been committed to 7.x-3.20 in march 2017.
Why was this not documented ?