The attached patch unsets the session data, and removes the cookie from the browser when someone logs off.

CommentFileSizeAuthor
session-destroy.patch770 byteskbahey

Comments

lilou’s picture

lilou commented
Status: Active » Needs review
maartenvg’s picture

maartenvg commented
Category: task » feature
Status: Needs review » Needs work

I don't think this is a good idea. The function sess_destroy_sid($sid) should be used to destroy the session of the user with the session_id $sid, not (also) the session of the current user. If a session_id of a specific user X (not being the current user) is fed into this function, both the current user and user X will be logged out, which is not the desired behavior.

The only way I can think of how this might become functional, is creating a sess_destroy_current(), which calls session_destroy_uid() with the current UID, and then destroys the cookies and session.

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Closed (won't fix)

sess_destroy_sid() is registered to the PHP session handler using session_set_save_handler(), as described in the PHP manual [1]:

The destroy handler, this is executed when a session is destroyed with session_destroy() and takes the session id as its only parameter.

The cookie management is handled by the PHP session handler. We don't have anything to do here.

[1] http://www.php.net/manual/en/function.session-set-save-handler.php

kbahey’s picture

kbahey commented
Status: Closed (won't fix) » Needs review

Damien

The handler we have just deletes the sid from the database, and is passed the sid as an argument. It does not do anything else.

If you look at session_destroy()'s documentation, the example does exactly what this patch does: a) unsets the session data, and b) destroys the cookie. We don't need to do anything to the sid since the sess_destroy_sid() function takes care of that.

Perhaps we can change the $_SESSION = array() to session_unset() too, but the main point here is not to leave the cookie in the browser.

moshe weitzman’s picture

moshe weitzman commented

I do not think cookie destroy is handled automatically by php so this is a useful patch. Code looks good to me.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

The last submitted patch failed testing.

valthebald’s picture

valthebald
Location Sofia
commented
Status: Needs work » Closed (fixed)

Both 8.x-dev and 7.x-dev session.inc contain the code from the patch.
Given the age of the issue, it was probably fixed by other patch