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

Feature/46 admin sessions #73

Merged
merged 4 commits into from
Oct 25, 2024
Merged
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
40 changes: 40 additions & 0 deletions open_api_framework/admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
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):
list_display = ["session_key", "user", "exists"]

@property
def SessionStore(self):
Copy link
Contributor Author

@Coperh Coperh Oct 15, 2024

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 get_session_store()

def get_queryset(self, request):
qs = super().get_queryset(request)
return qs.filter(user=request.user)

def has_add_permission(self, request, obj=None):
return False

def has_change_permission(self, request, obj=None):
return False

@admin.display(boolean=True)
def exists(self, obj):
return self.SessionStore().exists(obj.session_key)

def delete_model(self, request, obj):
self.SessionStore(obj.session_key).flush()
stevenbal marked this conversation as resolved.
Show resolved Hide resolved
super().delete_model(request, obj)

def delete_queryset(self, request, queryset):

for session_profile in queryset.iterator():
self.SessionStore(session_profile.session_key).flush()

super().delete_queryset(request, queryset)
9 changes: 8 additions & 1 deletion open_api_framework/conf/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,19 +231,20 @@
"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",
"django.middleware.csrf.CsrfViewMiddleware",
"django.contrib.auth.middleware.AuthenticationMiddleware",
"maykin_2fa.middleware.OTPMiddleware",
"mozilla_django_oidc_db.middleware.SessionRefresh",
"django.contrib.messages.middleware.MessageMiddleware",
"django.middleware.clickjacking.XFrameOptionsMiddleware",
"axes.middleware.AxesMiddleware",
Expand Down Expand Up @@ -537,6 +538,12 @@

SESSION_COOKIE_NAME = f"{PROJECT_DIRNAME}_sessionid"
SESSION_ENGINE = "django.contrib.sessions.backends.cache"
SESSION_COOKIE_AGE = config(
"SESSION_COOKIE_AGE",
default=1209600,
help_text="For how long, in seconds, the session cookie will be valid.",
)


LOGIN_URL = reverse_lazy("admin:login")
LOGIN_REDIRECT_URL = reverse_lazy("admin:index")
Expand Down
8 changes: 8 additions & 0 deletions open_api_framework/utils.py
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
2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ dependencies = [
"flower>=2.0.1",
"maykin-2fa>=1.0.1",
"django-setup-configuration>=0.1.0",
"django-sessionprofile>=3.0.0",
]

[project.urls]
Expand All @@ -72,6 +73,7 @@ tests = [
"isort",
"black",
"flake8",
"factory-boy",
]
coverage = [
"pytest-cov",
Expand Down
4 changes: 4 additions & 0 deletions testapp/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,13 @@
"django.contrib.messages",
"django.contrib.admin",
"open_api_framework",
"sessionprofile",
"testapp",
]

MIDDLEWARE = [
"django.middleware.security.SecurityMiddleware",
"sessionprofile.middleware.SessionProfileMiddleware",
Coperh marked this conversation as resolved.
Show resolved Hide resolved
"django.contrib.sessions.middleware.SessionMiddleware",
"django.middleware.common.CommonMiddleware",
"django.middleware.csrf.CsrfViewMiddleware",
Expand Down Expand Up @@ -78,3 +80,5 @@
# These are excluded from generate_envvar_docs test by their group
VARIABLE_TO_BE_EXCLUDED = config("VARIABLE_TO_BE_EXCLUDED1", "foo", group="Excluded")
VARIABLE_TO_BE_EXCLUDED = config("VARIABLE_TO_BE_EXCLUDED2", "bar", group="Excluded")

SESSION_ENGINE = "django.contrib.sessions.backends.db"
17 changes: 17 additions & 0 deletions tests/factories.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import factory.fuzzy
from sessionprofile.models import SessionProfile

from open_api_framework.utils import get_session_store


class SessionProfileFactory(factory.django.DjangoModelFactory):

session_key = factory.fuzzy.FuzzyText(length=40)

class Meta:
model = SessionProfile

@factory.post_generation
def session(self, create, extracted, **kwargs):
SessionStore = get_session_store()
SessionStore(self.session_key).save(True)
197 changes: 197 additions & 0 deletions tests/test_admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
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 sessionprofile.models import SessionProfile

from open_api_framework.utils import get_session_store

from .factories import SessionProfileFactory


@pytest.fixture
def session_changelist_url():
return reverse("admin:sessionprofile_sessionprofile_changelist")


def test_session_profile_sanity(client, admin_user, session_changelist_url):

client.force_login(admin_user)
response = client.get(session_changelist_url)
assert response.status_code == 200

assert SessionProfile.objects.count() == 1

session = SessionProfile.objects.get()
assert client.session.session_key == session.session_key


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
):

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

client.force_login(other_admin)
response = client.get(session_changelist_url)
assert response.status_code == 200

client.force_login(admin_user)
response = client.get(session_changelist_url)
assert response.status_code == 200

# two sessions, one for each user
assert SessionProfile.objects.count() == 2

# Session created after response, needs to be called again
response = client.get(session_changelist_url)

admin_user_session = SessionProfile.objects.get(user=admin_user)
assert admin_user_session.session_key in response.content.decode()

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

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 DBSessionStore().exists(session.session_key)

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

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

# new session saved upon request
assert SessionProfile.objects.count() == 1
assert SessionProfile.objects.count() != session
assert Session.objects.count() == 1
assert not DBSessionStore().exists(session.session_key)


@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 CachedSessionStore().exists(session.session_key)

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

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

# new session saved upon request
assert SessionProfile.objects.count() == 1
assert SessionProfile.objects.count() != session
assert Session.objects.count() == 0
assert not CachedSessionStore().exists(session.session_key)


def test_delete_action_with_session_db_backend(
client, admin_user, session_changelist_url
):
client.force_login(admin_user)
sessions = SessionProfileFactory.create_batch(5, user=admin_user)

# one created from user login
assert Session.objects.count() == 6
assert SessionProfile.objects.count() == 5

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

response = client.post(
session_changelist_url,
{"action": "delete_selected", "_selected_action": session_keys, "post": "yes"},
)
assert response.status_code == 302

# one is created as the post request is sent
assert SessionProfile.objects.count() == 1
assert Session.objects.count() == 1

for session_key in session_keys:
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
):

client.force_login(admin_user)
sessions = SessionProfileFactory.create_batch(5, user=admin_user)

# no db sessions are created
assert Session.objects.count() == 0
assert SessionProfile.objects.count() == 5

session_keys = [session.session_key for session in sessions]

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

response = client.post(
session_changelist_url,
{"action": "delete_selected", "_selected_action": session_keys, "post": "yes"},
)
assert response.status_code == 302

# one is created as the post request is sent
assert SessionProfile.objects.count() == 1
assert Session.objects.count() == 0

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