Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

code to delete expired session entries from database #81

Closed
wants to merge 8 commits into from
Closed

code to delete expired session entries from database #81

wants to merge 8 commits into from

Conversation

mebjas
Copy link
Member

@mebjas mebjas commented Oct 30, 2013

as mentioned by Abbas the code checks for last time expired session entries were deleted and if its greater than a threshold time which I set as 60 minutes, and if it exceeds a single SQL query deletes all those entries in db which has expired.

this table stores the time when expired session sweep operation was last executed
getSweeptimeDifference(): returns time difference between present time and time when expired session entries were last deleted
clearExpiredSession(): deletes expired sessions from database and update the date in session_last_sweeped table
This query deletes associated data from SESSION_DATA when expired session entries are removed from SESSION table
@mebjas
Copy link
Member Author

mebjas commented Oct 31, 2013

@paulocmguerreiro have a look now, I have implemented code to delete data from session_data as well while deleting expired session entries

@paulocmguerreiro
Copy link
Contributor

@mebjas , it's perfect

Although, my opinion, you should call it from a LogOut Session instead of LogIn. At logout time the user can wait a little longer (when this script runs) without any hassle. I normally get more stressed when the login gets delayed that at the logout :)

Question:
Some of this problems could be solved with TRIGGERS, CONSTRANTS (as @rash805115 has in his RNJ eCommerce Project) OR SCHEDULLED EVENTS, should they be implemented in OWASP.sql!? What do you guys think?

@SvenRtbg
Copy link
Contributor

Does SQLite support all these database things? Or PostgreSQL in a compatible manner? I know the current tests only use MySQL, but I think using fancy DB stuff can pose an obstacle. I'd try to avoid it if possible.

Paulo Guerreiro notifications@github.com schrieb:

@mebjas , it's perfect

Although, my opinion, you should call it from a LogOut Session instead
of LogIn. At logout time the user can wait a little longer (when this
script runs) without any hassle. I normally get more stressed when the
login gets delayed that at the logout :)

Question:
Some of this problems could be solved with TRIGGERS, CONSTRANTS (as
@rash805115 has in his RNJ eCommerce Project) OR SCHEDULLED EVENTS,
should they be implemented in OWASP.sql!? What do you guys think?


Reply to this email directly or view it on GitHub:
#81 (comment)

Mit freundlichen Grüßen

Sven Rautenberg

@abiusx
Copy link
Contributor

abiusx commented Oct 31, 2013

+1 Sven,
Only use basic standard SQL functionality (CRUD) inside the app. Triggers are not only non portable, they are insanely hard to debug and track.
-A


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 31, 2013, at 1:08 PM, SvenRtbg notifications@github.com wrote:

Does SQLite support all these database things? Or PostgreSQL in a compatible manner? I know the current tests only use MySQL, but I think using fancy DB stuff can pose an obstacle. I'd try to avoid it if possible.

Paulo Guerreiro notifications@github.com schrieb:

@mebjas , it's perfect

Although, my opinion, you should call it from a LogOut Session instead
of LogIn. At logout time the user can wait a little longer (when this
script runs) without any hassle. I normally get more stressed when the
login gets delayed that at the logout :)

Question:
Some of this problems could be solved with TRIGGERS, CONSTRANTS (as
@rash805115 has in his RNJ eCommerce Project) OR SCHEDULLED EVENTS,
should they be implemented in OWASP.sql!? What do you guys think?


Reply to this email directly or view it on GitHub:
#81 (comment)

Mit freundlichen Grüßen

Sven Rautenberg

Reply to this email directly or view it on GitHub.

@paulocmguerreiro
Copy link
Contributor

@SvenRtbg you have a good argument there. :)

@mebjas
Copy link
Member Author

mebjas commented Oct 31, 2013

@paulocmguerreiro but users don't log out usually! what if user closes the browser without actually logging out? on the other hand if this functionality is called during creation of new session, it would be called for few users only and the query wont take a very considerable amount of time as expired sessions are being deleted time to time.

@abiusx
Copy link
Contributor

abiusx commented Oct 31, 2013

don’t worry much about the performance, usually session tables are stored on memory, and also deleting records off a table is the fastest operation.
-A


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 31, 2013, at 4:37 PM, minhaz notifications@github.com wrote:

@paulocmguerreiro but users don't log out usually! what if user closes the browser without actually logging out? on the other hand if this functionality is called during creation of new session, it would be called for few users only and the query wont take a very considerable amount of time as expired sessions are being deleted time to time.


Reply to this email directly or view it on GitHub.

@mebjas
Copy link
Member Author

mebjas commented Oct 31, 2013

@abiusx I think you are with deleting expired session while newSession() function is called?
Also I could not understand the probabilistic approach as mentioned by @rash805115

@abiusx
Copy link
Contributor

abiusx commented Oct 31, 2013

I haven’t done the code :D I’m not deleting them.


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 31, 2013, at 4:54 PM, minhaz notifications@github.com wrote:

@abiusx I think you are with deleting expired session while newSession() function is called?
Also I could not understand the probabilistic approach as mentioned by @rash805115


Reply to this email directly or view it on GitHub.

@rash805115
Copy link
Contributor

Couple of things as a summary in the above commits:

  1. Remove all the join and other queries from the code...they are
    incompatible.
  2. The travis built is failing..check that before I can pull

Now in this method, you created a new table which will at all time will
contain only one row...though not an issue...it is something that looks
ugly..
With the probability function (see jframework's session library) you do not
have to maintain the table...

Have a look on the requirement again...what we want is to sweep the
database from time to time. It would not hurt if the database is not
sweeped even in 1 month..its just containing some extra data...the thing
you proposed will sweep every 60 min...or whatever time you keep. But with
the probability approach, the sweep will be done in "non-deterministic"
time..
here is the code for reference:

function _Sweep($force=false)
{
//Removes timed out session
if (!$force) if (rand ( 0, 1000 ) / 1000.0 >
self::$SweepRatio) return; //10%
$Now = jf::time ();
jf::SQL ( "DELETE FROM {$this->TablePrefix()}session
WHERE LastAccess<? OR LoginDate<?", $Now- self::$NoAccessTimeout,
$Now- self::$ForcedTimeout);
}

If we put this code in creation of new session, we get what we desire.

On Thu, Oct 31, 2013 at 7:47 PM, AbiusX notifications@github.com wrote:

I haven’t done the code :D I’m not deleting them.


Notice: This message is digitally signed, its source and integrity are
verifiable.
If you mail client does not support S/MIME verification, it will display a
file (smime.p7s), which includes the X.509 certificate and the signature
body. Read more at Certified E-Mail with Comodo and Thunderbird in
AbiusX.com

On Oct 31, 2013, at 4:54 PM, minhaz notifications@github.com wrote:

@abiusx I think you are with deleting expired session while newSession()
function is called?
Also I could not understand the probabilistic approach as mentioned by
@rash805115


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/81#issuecomment-27538598
.

Regards,
Rahul Chaudhary
Ph - 412-519-9634

@mebjas
Copy link
Member Author

mebjas commented Nov 2, 2013

Can anyone help me with The Travis CI.... I don't get why its failing?

As mentioned by Rahul if we go for probabilistic approach we do not need to keep an extra table to store just one row.
As mentioned by Rahul I have replaced the time based sweeping of expired session by including a probability factor to the function.
Also the JOIN in sql query has been replaced by multiple queries to do the task separately.
I think I had mixed little of c++ with php, My bad!
@mebjas
Copy link
Member Author

mebjas commented Nov 2, 2013

The Travis CI build passed ! and now I get what it means ;)

@ghost ghost assigned rash805115 Nov 12, 2013
@mebjas mebjas closed this Dec 11, 2013
@abiusx
Copy link
Contributor

abiusx commented Dec 11, 2013

Create a new issue for this


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Dec 11, 2013, at 11:14 AM, minhaz notifications@github.com wrote:

Owasp cheat sheet for session management says we should bind session to ip address to make it more secure.
Why didnt we go for this in PHPSEC.?


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants