-
Notifications
You must be signed in to change notification settings - Fork 0
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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): | ||
|
||
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) |
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 |
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) |
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 tbhThere was a problem hiding this comment.
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 :/