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.

Sign up for GitHub

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

Feature/46 admin sessions #73

Merged
merged 4 commits into from
Oct 25, 2024
Merged

Feature/46 admin sessions #73

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19839d7
:wrench:[#46] add session cookie age env var
Coperh Sep 20, 2024
6a61597
:sparkles:[#46] add session profile
Coperh Sep 20, 2024
5565720
:white_check_mark:[#46] add session profile tests
Coperh Oct 11, 2024
b631bbf
:white_check_mark:[#46] add update session profile tests
Coperh Oct 18, 2024
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
12 changes: 5 additions & 7 deletions open_api_framework/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
from importlib import import_module

from django.conf import settings
from django.contrib import admin

from sessionprofile.models import SessionProfile

from open_api_framework.utils import get_session_store


@admin.register(SessionProfile)
class SessionProfileAdmin(admin.ModelAdmin):
Expand All @@ -13,7 +12,7 @@ class SessionProfileAdmin(admin.ModelAdmin):
@property
def SessionStore(self):

Copy link
Contributor Author

@Coperh Coperh Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I originally set this in the _init_ but that is not called every time I ran a test and the cached version always used the database backend.

Is there a way to fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this caused by assigning the setting here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because the pytest fixtures change the SESSION_ENGINE after the __init__ is done, I'm not sure if there is way around this tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can just use override_settings and it works as usual :/

return import_module(settings.SESSION_ENGINE).SessionStore
return get_session_store()

def get_queryset(self, request):
qs = super().get_queryset(request)
Expand All @@ -35,8 +34,7 @@ def delete_model(self, request, obj):

def delete_queryset(self, request, queryset):

session_keys = list(queryset.values_list("session_key", flat=True))
for session_key in session_keys:
self.SessionStore(session_key).flush()
for session_profile in queryset.iterator():
self.SessionStore(session_profile.session_key).flush()

super().delete_queryset(request, queryset)
2 changes: 2 additions & 0 deletions open_api_framework/conf/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,12 +231,14 @@
"mozilla_django_oidc_db",
"log_outgoing_requests",
"django_setup_configuration",
"sessionprofile",
"open_api_framework",
PROJECT_DIRNAME,
]

MIDDLEWARE = [
"django.middleware.security.SecurityMiddleware",
"sessionprofile.middleware.SessionProfileMiddleware",
"django.contrib.sessions.middleware.SessionMiddleware",
"corsheaders.middleware.CorsMiddleware",
"django.middleware.common.CommonMiddleware",
Expand Down
8 changes: 8 additions & 0 deletions open_api_framework/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from importlib import import_module

from django.conf import settings
from django.contrib.sessions.backends.base import SessionBase


def get_session_store() -> SessionBase:
return import_module(settings.SESSION_ENGINE).SessionStore
19 changes: 4 additions & 15 deletions tests/conftest.py
View file
Open in desktop
SonnyBA marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,17 +1,6 @@
from importlib import import_module
# import pytest

from django.conf import settings as django_settings

import pytest


@pytest.fixture
def cache_session_store(settings):
settings.SESSION_ENGINE = "django.contrib.sessions.backends.cache"
return import_module(django_settings.SESSION_ENGINE).SessionStore


@pytest.fixture
def db_session_store(settings):
settings.SESSION_ENGINE = "django.contrib.sessions.backends.db"
return import_module(django_settings.SESSION_ENGINE).SessionStore
# @pytest.fixture
# def some_fixture(request):
# return "foo"
19 changes: 6 additions & 13 deletions tests/factories.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
from importlib import import_module

from django.conf import settings

import factory
import factory.fuzzy
from sessionprofile.models import SessionProfile

from open_api_framework.utils import get_session_store


class SessionProfileFactory(factory.django.DjangoModelFactory):

Expand All @@ -14,11 +11,7 @@ class SessionProfileFactory(factory.django.DjangoModelFactory):
class Meta:
model = SessionProfile

@classmethod
def create(cls, **kwargs):
SessionStore = import_module(settings.SESSION_ENGINE).SessionStore

instance = super().create(**kwargs)
SessionStore(instance.session_key).save(True)

return instance
@factory.post_generation
def session(self, create, extracted, **kwargs):
SessionStore = get_session_store()
SessionStore(self.session_key).save(True)
75 changes: 55 additions & 20 deletions tests/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
from django.contrib.sessions.backends.cache import SessionStore as CachedSessionStore
from django.contrib.sessions.backends.db import SessionStore as DBSessionStore
from django.contrib.sessions.models import Session
from django.test import override_settings
from django.urls import reverse

import pytest
from pytest_django.fixtures import admin_user
from sessionprofile.models import SessionProfile

from open_api_framework.utils import get_session_store

from .factories import SessionProfileFactory


Expand All @@ -25,9 +29,6 @@ def test_session_profile_sanity(client, admin_user, session_changelist_url):
assert client.session.session_key == session.session_key


admin_user2 = admin_user


def test_only_session_profile_of_user_shown(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might still be possible to see the detail or delete view for another users 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 checked this and it is not possible to go to the detail view for other user's sessions, not sure about deleting though. Can you add tests for this?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that intended behavior? Either way would be nice to have tests that verify this (whether it is intended or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is intended on my end that you can only see you're own sessions. IDK if this is the correct message, do we want users to know other sessions exist?

I do not think it is in scope that a super admin user can see all sessions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the current message is fine, because it does not give users extra information about other user's session. Just adding tests to verify that other users cant access/delete other session is enough in my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs an assertion that deleting other users sessions is not possible, but might be nicer to move that to a separate test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now done

client, admin_user, django_user_model, session_changelist_url
):
Expand All @@ -54,18 +55,52 @@ def test_only_session_profile_of_user_shown(
other_user_session = SessionProfile.objects.get(user=other_admin)
assert other_user_session.session_key not in response.content.decode()

# should only be able to access own page
change_url = reverse(
"admin:sessionprofile_sessionprofile_change",
args=[admin_user_session.session_key],
)
response = client.get(change_url)
assert response.status_code == 200

def test_delete_with_session_db_backend(
client, admin_user, session_changelist_url, db_session_store
):
change_url = reverse(
"admin:sessionprofile_sessionprofile_change",
args=[other_user_session.session_key],
)
response = client.get(change_url)
assert response.status_code == 302
assert response.url == reverse("admin:index")


def test_cant_delete_other_users_session(client, admin_user, django_user_model):
client.force_login(admin_user)

other_admin = django_user_model.objects.create_superuser("garry")

other_user_session = SessionProfileFactory(user=other_admin)

delete_url = reverse(
"admin:sessionprofile_sessionprofile_delete",
args=[other_user_session.session_key],
)

response = client.post(delete_url, {"post": "yes"})
assert response.status_code == 302

SessionStore = get_session_store()

assert SessionStore().exists(other_user_session.session_key)


def test_delete_with_session_db_backend(client, admin_user, session_changelist_url):
client.force_login(admin_user)

session = SessionProfileFactory(user=admin_user)

assert SessionProfile.objects.count() == 1
# sesison created by login
assert Session.objects.count() == 2
assert db_session_store().exists(session.session_key)
assert DBSessionStore().exists(session.session_key)

url = reverse("admin:sessionprofile_sessionprofile_delete", args=[session.pk])

Expand All @@ -76,19 +111,18 @@ def test_delete_with_session_db_backend(
assert SessionProfile.objects.count() == 1
assert SessionProfile.objects.count() != session
assert Session.objects.count() == 1
assert not db_session_store().exists(session.session_key)
assert not DBSessionStore().exists(session.session_key)


def test_delete_with_session_cache_backend(
client, admin_user, session_changelist_url, cache_session_store
):
@override_settings(SESSION_ENGINE="django.contrib.sessions.backends.cache")
def test_delete_with_session_cache_backend(client, admin_user, session_changelist_url):
client.force_login(admin_user)

session = SessionProfileFactory(user=admin_user)

assert SessionProfile.objects.count() == 1
assert Session.objects.count() == 0
assert cache_session_store().exists(session.session_key)
assert CachedSessionStore().exists(session.session_key)

url = reverse("admin:sessionprofile_sessionprofile_delete", args=[session.pk])

Expand All @@ -99,11 +133,11 @@ def test_delete_with_session_cache_backend(
assert SessionProfile.objects.count() == 1
assert SessionProfile.objects.count() != session
assert Session.objects.count() == 0
assert not cache_session_store().exists(session.session_key)
assert not CachedSessionStore().exists(session.session_key)


def test_delete_action_with_session_db_backend(
client, admin_user, session_changelist_url, db_session_store
client, admin_user, session_changelist_url
):
client.force_login(admin_user)
sessions = SessionProfileFactory.create_batch(5, user=admin_user)
Expand All @@ -114,7 +148,7 @@ def test_delete_action_with_session_db_backend(

session_keys = [session.session_key for session in sessions]
for session_key in session_keys:
assert db_session_store().exists(session_key)
assert DBSessionStore().exists(session_key)

response = client.post(
session_changelist_url,
Expand All @@ -127,11 +161,12 @@ def test_delete_action_with_session_db_backend(
assert Session.objects.count() == 1

for session_key in session_keys:
assert not db_session_store().exists(session_key)
assert not DBSessionStore().exists(session_key)


@override_settings(SESSION_ENGINE="django.contrib.sessions.backends.cache")
def test_delete_action_with_session_cache_backend(
client, admin_user, session_changelist_url, cache_session_store
client, admin_user, session_changelist_url
):

client.force_login(admin_user)
Expand All @@ -145,7 +180,7 @@ def test_delete_action_with_session_cache_backend(

# sessions are created
for session_key in session_keys:
assert cache_session_store().exists(session_key)
assert CachedSessionStore().exists(session_key)

response = client.post(
session_changelist_url,
Expand All @@ -159,4 +194,4 @@ def test_delete_action_with_session_cache_backend(

# sessions should be deleted
for session_key in session_keys:
assert not cache_session_store().exists(session_key)
assert not CachedSessionStore().exists(session_key)
Loading