Skip to content

Commit

Permalink
[ENG-4316] Refactor Sessions: Project PR (CenterForOpenScience#10335)
Browse files Browse the repository at this point in the history
* Update Everything to Use Django SessionStore CenterForOpenScience#10337
* Use django-redis as Cache Backend for Session CenterForOpenScience#10342
* Periodically clear expired UserSessionMap objects CenterForOpenScience#10349
* Handle old but valid cookie CenterForOpenScience#10350
* Bug Fixes - realCAS related CenterForOpenScience#10356
* Fix status message CenterForOpenScience#10358
* Change session usage CenterForOpenScience#10359
* inal bug fixes (local) CenterForOpenScience#10365
* Override SessionMiddleware to use Django Session correctly CenterForOpenScience#10371
* Fix a bug where no cookie breaks API requests CenterForOpenScience#10372
* Only save session in update_counter when the session is not anonymous CenterForOpenScience#10373
* Fix a edge case where user session is not save in analytics CenterForOpenScience#10375
* Fix view only links CenterForOpenScience#10377
* Use request.session in has_admin_scope CenterForOpenScience#10378
* Increase maximum number of connections in connection pool CenterForOpenScience#10379
* Unit Tests - Part 1: Fix add-on tests CenterForOpenScience#10381
* Unit Tests - Part 2: Fix Everything CenterForOpenScience#10383
* Fix getting cookie from session for waterbutler and archiving CenterForOpenScience#10384
* Unit Tests - Part 3: New tests and more CenterForOpenScience#10393
* Unit Tests - Part 4: Fix tests after removing old Session Model CenterForOpenScience#10396
* Modify UnsignCookieSessionMiddleware to set empty SessionStore() on request.session CenterForOpenScience#10397

h/t @adlius for many of these
  • Loading branch information
cslzchen authored May 15, 2023
1 parent a21a604 commit 542fccb
Show file tree
Hide file tree
Showing 62 changed files with 1,301 additions and 624 deletions.
1 change: 1 addition & 0 deletions .docker-compose.env
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ ELASTIC_URI=192.168.168.167:9200
ELASTIC6_URI=192.168.168.167:9201
OSF_DB_HOST=192.168.168.167
DB_HOST=192.168.168.167
REDIS_HOST=redis://192.168.168.167:6379
WATERBUTLER_URL=http://localhost:7777
WATERBUTLER_INTERNAL_URL=http://192.168.168.167:7777
CAS_SERVER_URL=http://192.168.168.167:8080
Expand Down
15 changes: 9 additions & 6 deletions addons/osfstorage/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import pytz
from django.utils import timezone
from nose.tools import * # noqa
from importlib import import_module
from django.conf import settings as django_conf_settings

from framework.auth import Auth
from addons.osfstorage.models import OsfStorageFile, OsfStorageFileNode, OsfStorageFolder
Expand All @@ -27,6 +29,7 @@
from addons.osfstorage import settings
from website.files.exceptions import FileNodeCheckedOutError, FileNodeIsPrimaryFile

SessionStore = import_module(django_conf_settings.SESSION_ENGINE).SessionStore

@pytest.mark.django_db
class TestOsfstorageFileNode(StorageTestCase):
Expand Down Expand Up @@ -180,14 +183,14 @@ def test_download_count_file_defaults(self):
child = self.node_settings.get_root().append_file('Test')
assert_equals(child.get_download_count(), 0)

@mock.patch('framework.sessions.session')
def test_download_count_file(self, mock_session):
mock_session.data = {}
def test_download_count_file(self):
s = SessionStore()
s.create()
child = self.node_settings.get_root().append_file('Test')

utils.update_analytics(self.project, child, 0, mock_session)
utils.update_analytics(self.project, child, 1, mock_session)
utils.update_analytics(self.project, child, 2, mock_session)
utils.update_analytics(self.project, child, 0, s.session_key)
utils.update_analytics(self.project, child, 1, s.session_key)
utils.update_analytics(self.project, child, 2, s.session_key)

assert_equals(child.get_download_count(), 3)
assert_equals(child.get_download_count(0), 1)
Expand Down
27 changes: 13 additions & 14 deletions addons/osfstorage/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,17 @@
# encoding: utf-8
import pytest
from nose.tools import * # noqa
from importlib import import_module

from django.conf import settings as django_conf_settings

from framework import sessions
from framework.flask import request

from osf.models import Session
from addons.osfstorage.tests import factories
from addons.osfstorage import utils

from addons.osfstorage.tests.utils import StorageTestCase
from website.files.utils import attach_versions

SessionStore = import_module(django_conf_settings.SESSION_ENGINE).SessionStore

@pytest.mark.django_db
class TestSerializeRevision(StorageTestCase):
Expand All @@ -30,11 +29,11 @@ def setUp(self):
self.record.save()

def test_serialize_revision(self):
mock_session = Session()
sessions.sessions[request._get_current_object()] = mock_session
utils.update_analytics(self.project, self.record, 0, mock_session)
utils.update_analytics(self.project, self.record, 0, mock_session)
utils.update_analytics(self.project, self.record, 2, mock_session)
s = SessionStore()
s.create()
utils.update_analytics(self.project, self.record, 0, s.session_key)
utils.update_analytics(self.project, self.record, 0, s.session_key)
utils.update_analytics(self.project, self.record, 2, s.session_key)
expected = {
'index': 1,
'user': {
Expand All @@ -58,11 +57,11 @@ def test_serialize_revision(self):
assert_equal(self.record.get_download_count(version=0), 2)

def test_anon_revisions(self):
mock_session = Session()
sessions.sessions[request._get_current_object()] = mock_session
utils.update_analytics(self.project, self.record, 0, mock_session)
utils.update_analytics(self.project, self.record, 0, mock_session)
utils.update_analytics(self.project, self.record, 2, mock_session)
s = SessionStore()
s.create()
utils.update_analytics(self.project, self.record, 0, s.session_key)
utils.update_analytics(self.project, self.record, 0, s.session_key)
utils.update_analytics(self.project, self.record, 2, s.session_key)
expected = {
'index': 2,
'user': None,
Expand Down
21 changes: 8 additions & 13 deletions addons/osfstorage/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,24 @@
from framework.analytics import update_counter
from framework.celery_tasks import app
from framework.postcommit_tasks.handlers import enqueue_postcommit_task
from framework.sessions import session
from osf.models import BaseFileNode, Guid, Session
from framework.sessions import get_session
from osf.models import BaseFileNode, Guid

from addons.osfstorage import settings

logger = logging.getLogger(__name__)
LOCATION_KEYS = ['service', settings.WATERBUTLER_RESOURCE, 'object']

def enqueue_update_analytics(node, file, version_idx, action='download'):
enqueue_postcommit_task(update_analytics_async, (node._id, file._id, version_idx, session._id, session.data, action), {}, celery=True)
enqueue_postcommit_task(update_analytics_async, (node._id, file._id, version_idx, get_session().session_key, action), {}, celery=True)

@app.task(max_retries=5, default_retry_delay=60)
def update_analytics_async(node_id, file_id, version_idx, session_id=None, session_data=None, action='download'):
if not session_data:
session_data = {}
def update_analytics_async(node_id, file_id, version_idx, session_key=None, action='download'):
node = Guid.load(node_id).referent
file = BaseFileNode.load(file_id)
session_obj = Session.load(session_id)
if not session_obj:
session_obj = Session(data=session_data)
update_analytics(node, file, version_idx, session_obj, action)
update_analytics(node, file, version_idx, session_key, action)

def update_analytics(node, file, version_idx, session_obj, action='download'):
def update_analytics(node, file, version_idx, session_key, action='download'):
"""
:param Node node: Root node to update
:param str file_id: The _id field of a filenode
Expand All @@ -53,8 +48,8 @@ def update_analytics(node, file, version_idx, session_obj, action='download'):
}
resource = node.guids.first()

update_counter(resource, file, version=None, action=action, node_info=node_info, session_obj=session_obj)
update_counter(resource, file, version_idx, action, node_info=node_info, session_obj=session_obj)
update_counter(resource, file, version=None, action=action, node_info=node_info, session_key=session_key)
update_counter(resource, file, version_idx, action, node_info=node_info, session_key=session_key)


def serialize_revision(node, record, version, index, anon=False):
Expand Down
9 changes: 0 additions & 9 deletions addons/osfstorage/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from flask import request

from framework.auth import Auth
from framework.sessions import get_session
from framework.exceptions import HTTPError
from framework.auth.decorators import must_be_signed, must_be_logged_in

Expand Down Expand Up @@ -398,14 +397,6 @@ def osfstorage_delete(file_node, payload, target, **kwargs):
@must_be_signed
@decorators.autoload_filenode(must_be='file')
def osfstorage_download(file_node, payload, **kwargs):
# Set user ID in session data for checking if user is contributor
# to project.
user_id = payload.get('user')
if user_id:
current_session = get_session()
current_session.data['auth_user_id'] = user_id
current_session.save()

if not request.args.get('version'):
version_id = None
else:
Expand Down
45 changes: 22 additions & 23 deletions api/base/authentication/drf.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import itsdangerous
from importlib import import_module

from django.middleware.csrf import get_token
from django.utils.translation import ugettext_lazy as _
Expand All @@ -17,28 +18,29 @@
from framework.auth import cas
from framework.auth.core import get_user
from osf import features
from osf.models import OSFUser, Session
from osf.models import OSFUser
from osf.utils.fields import ensure_str
from website import settings

SessionStore = import_module(api_settings.SESSION_ENGINE).SessionStore

def get_session_from_cookie(cookie_val):

def drf_get_session_from_cookie(cookie_val):
"""
Given a cookie value, return the `Session` object or `None`.
Given a cookie value, return the Django native `Session` object or `None`, using the SessionStore.
When using DB backend, for expired sessions, SessionStore().exists(session_key=session_key) returns
true while SessionStore(session_key=session_key) doesn't load the session data. Thus, when using the
returned session object from this method, must check ``session.get('auth_user_id', None)``.
:param cookie_val: the cookie
:return: the `Session` object or None
:return: the Django native `Session` object or None
"""

try:
session_id = ensure_str(itsdangerous.Signer(settings.SECRET_KEY).unsign(cookie_val))
session_key = ensure_str(itsdangerous.Signer(settings.SECRET_KEY).unsign(cookie_val))
except itsdangerous.BadSignature:
return None
try:
session = Session.objects.get(_id=session_id)
return session
except Session.DoesNotExist:
return None
return SessionStore(session_key=session_key) if SessionStore().exists(session_key=session_key) else None


def check_user(user):
Expand Down Expand Up @@ -120,21 +122,18 @@ def authenticate(self, request):
:param request: the request
:return: the user
"""
cookie_val = request.COOKIES.get(settings.COOKIE_NAME)
if not cookie_val:
return None
session = get_session_from_cookie(cookie_val)
session = request.session
if not session:
return None
user_id = session.data.get('auth_user_id')
user_id = session.get('auth_user_id', None)
user = OSFUser.load(user_id)
if user:
if waffle.switch_is_active(features.ENFORCE_CSRF):
self.enforce_csrf(request)
# CSRF passed with authenticated user
check_user(user)
return user, None
return None
if not user:
return None
if waffle.switch_is_active(features.ENFORCE_CSRF):
self.enforce_csrf(request)
# CSRF passed with authenticated user
check_user(user)
return user, None

def enforce_csrf(self, request):
"""
Expand Down
18 changes: 18 additions & 0 deletions api/base/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
import cProfile
import pstats
import threading
from importlib import import_module

from django.conf import settings
from django.contrib.sessions.middleware import SessionMiddleware
from django.utils.deprecation import MiddlewareMixin
from raven.contrib.django.raven_compat.models import sentry_exception_handler
import corsheaders.middleware
Expand All @@ -20,7 +22,9 @@
)
from .api_globals import api_globals
from api.base import settings as api_settings
from api.base.authentication.drf import drf_get_session_from_cookie

SessionStore = import_module(settings.SESSION_ENGINE).SessionStore

class CeleryTaskMiddleware(MiddlewareMixin):
"""Celery Task middleware."""
Expand Down Expand Up @@ -139,3 +143,17 @@ def process_response(self, request, response):
response.content = s.getvalue()

return response


class UnsignCookieSessionMiddleware(SessionMiddleware):
"""
Overrides the process_request hook of SessionMiddleware
to retrieve the session key for finding the correct session
by unsigning the cookie value using server secret
"""
def process_request(self, request):
cookie = request.COOKIES.get(settings.SESSION_COOKIE_NAME)
if cookie:
request.session = drf_get_session_from_cookie(cookie)
else:
request.session = SessionStore()
17 changes: 15 additions & 2 deletions api/base/settings/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,13 @@
DEBUG_PROPAGATE_EXCEPTIONS = True

# session:
SESSION_COOKIE_NAME = 'api'
SESSION_COOKIE_NAME = osf_settings.COOKIE_NAME
SESSION_COOKIE_SECURE = osf_settings.SECURE_MODE
SESSION_COOKIE_HTTPONLY = osf_settings.SESSION_COOKIE_HTTPONLY
SESSION_COOKIE_SAMESITE = osf_settings.SESSION_COOKIE_SAMESITE
SESSION_COOKIE_AGE = 2592000 # 30 days in seconds
SESSION_ENGINE = 'django.contrib.sessions.backends.cache'
SESSION_CACHE_ALIAS = 'redis'

# csrf:
CSRF_COOKIE_NAME = 'api-csrf'
Expand Down Expand Up @@ -231,7 +234,7 @@
'api.base.middleware.CorsMiddleware',
'django.middleware.common.CommonMiddleware',
'django.middleware.csrf.CsrfViewMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',
'api.base.middleware.UnsignCookieSessionMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware',
'django.middleware.clickjacking.XFrameOptionsMiddleware',
Expand Down Expand Up @@ -338,6 +341,16 @@
'default': {
'BACKEND': 'django.core.cache.backends.locmem.LocMemCache',
},
'redis': {
'BACKEND': 'django_redis.cache.RedisCache',
'LOCATION': os.environ.get('REDIS_HOST', 'redis://192.168.168.167:6379'),
'OPTIONS': {
'CLIENT_CLASS': 'django_redis.client.DefaultClient',
'CONNECTION_POOL_KWARGS': {
'max_connections': 100,
},
},
},
STORAGE_USAGE_CACHE_NAME: {
'BACKEND': 'django.core.cache.backends.db.DatabaseCache',
'LOCATION': 'osf_cache_table',
Expand Down
5 changes: 4 additions & 1 deletion api/base/settings/local-dist.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
# django-silk
INSTALLED_APPS += ('silk',)
MIDDLEWARE += (
'django.contrib.sessions.middleware.SessionMiddleware',
'silk.middleware.SilkyMiddleware',
)

Expand All @@ -43,3 +42,7 @@
'files': '75/minute',
'files-burst': '3/second',
}

# Can switch between using Redis and using postgres as session storage
# SESSION_ENGINE = 'django.contrib.sessions.backends.db'
SESSION_ENGINE = 'django.contrib.sessions.backends.cache'
1 change: 1 addition & 0 deletions api/base/settings/local-travis.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

OSF_DB_PASSWORD = 'postgres'

SESSION_ENGINE = 'django.contrib.sessions.backends.db'

REST_FRAMEWORK['DEFAULT_THROTTLE_RATES'] = {
'user': '1000000/second',
Expand Down
3 changes: 1 addition & 2 deletions api/base/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
from rest_framework.exceptions import NotFound
from rest_framework.reverse import reverse

from api.base.authentication.drf import get_session_from_cookie
from api.base.exceptions import Gone, UserGone
from api.base.settings import HASHIDS_SALT
from framework.auth import Auth
Expand Down Expand Up @@ -184,7 +183,7 @@ def has_admin_scope(request):
"""
cookie = request.COOKIES.get(website_settings.COOKIE_NAME)
if cookie:
return bool(get_session_from_cookie(cookie))
return bool(request.session and request.session.get('auth_user_id', None))

token = request.auth
if token is None or not isinstance(token, CasResponse):
Expand Down
Loading

0 comments on commit 542fccb

Please sign in to comment.