Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Throttle set_session_expires. #2041

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions gittip/models/participant.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
from aspen.utils import typecheck
from postgres.orm import Model
from psycopg2 import IntegrityError
import pytz

import gittip
from gittip import NotSane
Expand Down Expand Up @@ -141,17 +140,26 @@ def _update_session_token(self, new_token):
def set_session_expires(self, expires):
"""Set session_expires in the database.

:param float expires: A UNIX timestamp, which XXX we assume is UTC?
:param datetime expires:
:database: One UPDATE, one row

"""
session_expires = datetime.datetime.fromtimestamp(expires) \
.replace(tzinfo=pytz.utc)
self.db.run( "UPDATE participants SET session_expires=%s "
"WHERE id=%s AND is_suspicious IS NOT true"
, (session_expires, self.id,)
)
self.set_attributes(session_expires=session_expires)
if not self.session_expires + datetime.timedelta(hours=12) < expires:
return
with self.db.get_cursor() as c:
done = c.one("""
UPDATE participants SET session_expires=%(expires)s
WHERE id=%(id)s
AND is_suspicious IS NOT true
AND session_expires + INTERVAL '12 hours' < %(expires)s
RETURNING id
""", dict(expires=expires, id=self.id))
if done is None:
return
add_event(c, 'participant',
dict(action='set', id=self.id, values=dict(session_expires=unicode(expires))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there really a need to record this event? Seems like that's going to bloat the database without adding value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The final goal here is to be able to recreate the whole db the events table. So all changes should be logged here. See #1549 (comment) for the general plan.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we ever want to recreate session_expires? Why are we storing this in the database in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like a signed / encrypted cookie would be a much better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 If only you had asked me 17 days ago 😀. I have been trying to find with 🔦 where it is used for some time now. It seems.... surprise surprise... that nowhere ‼️. Thank you!

Just dumping the column session_expires and all code related to it would not change a bit about the site behavior. The participant object is just created from session_token gittip/security/authentication.py#L49. Grepping for expires brings up only code related to writing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

35% of our participants table has an expired session (session_token is not null and session_expires < now()).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(P.S. 1.3% have a non-expired session.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just killed old sessions with this:

update participants set session_token=null where session_expires < now();

Looks like we don't reset session_expires in Participant.end_session, which is also a bug. But we should probably clean up such things in a separate PR, eh?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zwn @seanlinsley So what's the next step here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. update this PR so we don't store an event
  2. update the code so we don't respect expired sessions
  3. (optional, after this PR) instead of storing in the database, sign the cookie with something like tokenlib, ref: move caching out of memory #715

self.set_attributes(session_expires=expires)



# Claimed-ness
Expand Down
11 changes: 7 additions & 4 deletions gittip/security/authentication.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
"""Defines website authentication helpers.
"""
import rfc822
import time
from datetime import datetime, timedelta

import pytz
import gittip
from aspen import Response
from gittip.security import csrf
from gittip.security.user import User


BEGINNING_OF_EPOCH = rfc822.formatdate(0)
TIMEOUT = 60 * 60 * 24 * 7 # one week
TIMEOUT = timedelta(weeks=1)
EPOCH = datetime(1970, 1, 1, tzinfo=pytz.utc)
ROLES = ['anonymous', 'authenticated', 'owner', 'admin']
ROLES_SHOULD_BE = "It should be one of: {}.".format(', '.join(ROLES))

Expand Down Expand Up @@ -92,14 +94,15 @@ def outbound(request, response):
else:
# expired cookie in the request, instruct browser to delete it
response.headers.cookie['session'] = ''
expires = 0
expires = EPOCH
else: # user is authenticated
response.headers['Expires'] = BEGINNING_OF_EPOCH # don't cache
response.headers.cookie['session'] = user.participant.session_token
expires = time.time() + TIMEOUT
expires = datetime.now(tz=pytz.utc) + TIMEOUT
user.keep_signed_in_until(expires)

cookie = response.headers.cookie['session']
expires = (expires - EPOCH).total_seconds()
# I am not setting domain, because it is supposed to default to what we
# want: the domain of the object requested.
#cookie['domain']
Expand Down
2 changes: 1 addition & 1 deletion gittip/security/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def sign_in(self):
def keep_signed_in_until(self, expires):
"""Extend the user's current session.

:param float expires: A UNIX timestamp (XXX timezone?)
:param datetime expires:

"""
self.participant.set_session_expires(expires)
Expand Down
10 changes: 7 additions & 3 deletions tests/py/test_goal_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ def test_all_goals_are_stored_in_events_table(self):
self.change_goal("custom", "300", alice)
self.change_goal("null", "", alice)
self.change_goal("custom", "400", alice)
actual = self.db.all("SELECT (payload->'values'->>'goal')::int AS goal "
"FROM events ORDER BY ts DESC")
assert actual == [400, None, 300, 200, 100, None]
actual = self.db.all("""
SELECT (payload->'values'->>'goal')::int AS goal
FROM events
WHERE 'goal' IN (SELECT * FROM json_object_keys(payload->'values'))
ORDER BY ts DESC
""")
assert actual == [400, None, 300, 200, 100]