Robustly clear sessions on user deletion to cause immediate logout #13757

rtibbles · 2025-09-18T00:53:29Z

Summary

Abstracts our existing database routers into a base class and reuses it in all existing implementations - cleaning up unnecessary code in the process
Migrates Kolibri from file based sessions to database based sessions using a custom model and backend based on Django Database Session backend in order to track the user id on the session model
For SQLite backends, it adds an additional database to limit the additional bottleneck that using the database backend might add
Adds logic to the queryset and model for FacilityUser to ensure that any time a user is deleted or soft deleted, we also clear any associated sessions
Cleans up previous API specific logic in favour of this approach
Adds tests for the new model and queryset updates

References

Note that this reverts #3944 which was originally implemented for a very modest performance improvement. I think the combination of a wide range of other performance improvements, plus the use of a separate database in this implementation means that any performance degradation from changing back here should be negligible.

Reviewer guidance

I used Claude Code to do the initial model and queryset updates and write tests for it, and then heavily edited it to reduce code verbosity, fix errors and ensure the tests were asserting the right things. All other code was hand written.

Both the deletion flows in the issue itself, when deleting in an LoD and also when deleting using the new bulk user management flows should work here. Also, if a user is deleted due to a facility being deleted from a device that should also work. Additionally, it is important to test both the soft deletion that happens by default and the permanent deletion, to make sure both work.

The result here should be that the user is logged out identically if the user's session timed out.

github-actions · 2025-09-18T01:27:45Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.19.0b1.dev0_git.35.g8bf43225.pex
Windows Installer (EXE)	kolibri-0.19.0b1.dev0+git.35.g8bf43225-windows-setup-unsigned.exe
Debian Package	kolibri_0.19.0b1.dev0+git.35.g8bf43225-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.19.0b1.dev0+git.35.g8bf43225.dmg
Android Package (APK)	kolibri-0.19.0b1.dev0+git.35.g8bf43225-0.1.6-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.19.0b1.dev0+git.35.g8bf43225.zip
TAR file	kolibri-0.19.0b1.dev0+git.35.g8bf43225.tar.gz
WHL file	kolibri-0.19.0b1.dev0+git.35.g8bf43225-py2.py3-none-any.whl

nucleogenesis

I left some questions / thoughts - nothing blocking or urgent to reply to.

Overall I think that I got a good sense of how the changes work. The Session* classes were a bit weird to grok at first but seems the crux is in customizing the SessionStore so you can override create_model_instance to handle the user_id business. The rest then seems like glue code w/ the new Session model and the DB.

I didn't spin it up yet but don't want to forget to submit this review so I'll come back for further review after some manual testing.

nucleogenesis · 2025-09-19T19:51:10Z

kolibri/core/auth/upgrade.py

+def cleanup_legacy_file_sessions():
+    """
+    Clean up legacy file-based sessions when upgrading to database-backed sessions.
+    Removes the sessions directory from KOLIBRI_HOME if it exists.
+    """


nucleogenesis · 2025-09-19T20:00:16Z

kolibri/core/utils/model_router.py

@@ -0,0 +1,69 @@
+from abc import ABC
+from abc import abstractmethod


(non-blocking curiosity) The reason I'm seeing for using ABC/abstractmethod here is to ensure that classes inheriting this will be required to override specific methods or throw an error.

Are there any other benefits to using an abstract class or would it be functionally similar to:

def MODEL_CLASSES(self): raise "Not implemented"

I think this came to mind because that's how I've done abstract classes in Ruby

Love how this whole module DRYs things up

Yes, ABC and abstractmethods are the more explicit way to do this.

nucleogenesis · 2025-09-19T20:07:31Z

kolibri/core/auth/models.py

            user=self.full_name or self.username, facility=self.facility
        )

+    def __init__(self, *args, **kwargs):


(non blocking opinion) - __init__ feels like it should always be at the top of a class to me but as I type this I'm feeling like this is largely based on vibes 😆 -- I think maybe I just like to see overrides at the top of a class so I can first answer the question "what is the class doing in relation to it's parent's implementation" then the second question of "now what does this class do uniquely".

In any case - __str__ is overridden down here too so the rest fit in well enough 😄

Yeah - I think I have a similar intuition - the only advantage to it being here is that its closer to where the intialized value is used.

nucleogenesis · 2025-09-19T20:17:45Z

kolibri/core/notifications/models.py

-# Remove NotificationsRouter if sqlite is not being used:
-if settings.DATABASES["default"]["ENGINE"] != "django.db.backends.sqlite3":
-    ROUTER_ID = "kolibri.core.notifications.models.NotificationsRouter"
-    if ROUTER_ID in settings.DATABASE_ROUTERS:
-        settings.DATABASE_ROUTERS = tuple(
-            filter(lambda x: x != ROUTER_ID, settings.DATABASE_ROUTERS)
-        )


Nice to see code like this in the red :)

It has been redundant for quite some time, because of the router conditionality being in settings/base.py instead!

pcenov · 2025-09-22T12:47:25Z

Hi @rtibbles,
Unfortunately the issue reported in #13542 is still present.
Also when on device A I created a server with 2 facilities, and then on device B I went through the LOD user flow and imported a user from the 2nd facility and then deleted the entire facility the user remains signed in with no indication whatsoever that the facility has been deleted other than syncing not working anymore.
The same scenario is valid also for a single device with multiple facilities, where if I add a new facility, create and assign a learner to a class, sign in as the learner and then delete the entire facility - the learner still remains signed in.

Logs:
server-logs.zip
lod-logs.zip

Update all implementations. Remove redundant unregistering of NotificationsRouter.

To allow sessions to be cleared via user id. Use separate sessions database to prevent bottlenecks.

…eted users also get deleted.

…d users!

pcenov · 2025-10-31T16:18:14Z

Hi @rtibbles, LGTM in general, just noting the following:

When I delete a signed in user the user does get signed out, but for a brief moment there's the generic "Sorry! Something went wrong!" error screen:

error.mp4

The error details:

{
  "data": [
    {
      "id": "NOT_AUTHENTICATED",
      "metadata": {
        "view": "Learner Classroom Viewset Instance"
      }
    }
  ],
  "status": 403,
  "statusText": "Forbidden",
  "headers": {
    "allow": "GET, HEAD, OPTIONS",
    "content-length": "85",
    "content-security-policy": "script-src 'self' blob:; style-src 'self' data: blob: 'unsafe-inline'; frame-src 'self' data: blob: http: https:; default-src 'self' data: blob:",
    "content-type": "application/json",
    "date": "Fri, 31 Oct 2025 16:03:53 GMT",
    "referrer-policy": "same-origin",
    "server": "Cheroot/10.0.1",
    "vary": "Accept, Cookie",
    "x-content-type-options": "nosniff",
    "x-frame-options": "DENY"
  },
  "config": {
    "transitional": {
      "silentJSONParsing": true,
      "forcedJSONParsing": true,
      "clarifyTimeoutError": false
    },
    "adapter": [
      "xhr",
      "http",
      "fetch"
    ],
    "transformRequest": [
      null
    ],
    "transformResponse": [
      null
    ],
    "timeout": 0,
    "xsrfCookieName": "kolibri_csrftoken",
    "xsrfHeaderName": "X-CSRFToken",
    "maxContentLength": -1,
    "maxBodyLength": -1,
    "env": {},
    "headers": {
      "Accept": "application/json, text/plain, */*",
      "X-Requested-With": "XMLHttpRequest"
    },
    "paramsSerializer": {},
    "url": "/learn/api/learnerclassroom/3c43d8f06a7bf5c8dbc3e1c058bf9ff7/",
    "params": {},
    "allowAbsoluteUrls": true,
    "method": "get"
  },
  "request": {}
}

The points from my previous comment (based on the following part of your reviewer guidance: Also, if a user is deleted due to a facility being deleted from a device that should also work. ) are still valid - if the user of the deleted facility is on a LOD, then the app just stops syncing, while otherwise the user is seeing a 500 error.

server.error.mp4

nucleogenesis

The code overall looks great and made sense as I read through. I left a few questions, mostly for my own understanding of the changes.

I'll leave approval to @pcenov 's testing

nucleogenesis · 2025-10-31T22:01:11Z

kolibri/core/auth/backends.py

+        return obj
+
+    @classmethod
+    def delete_all_sessions(cls, user_ids):


(nitpick) - If this isn't overriding an existing method, would be clearer to call it delete_sessions as delete_all_sessions doesn't read like it should take any params but just delete all of the sessions

The reason it's called delete_all_sessions is because each user can have multiple active sessions, if they have signed in from different devices, so this is to emphasize that this is what is happening.

nucleogenesis · 2025-10-31T22:07:16Z

kolibri/core/auth/models.py

    all_objects = AllObjectsFacilityUserModelManager()
    objects = FacilityUserModelManager()
-
+    syncing_objects = BaseFacilityUserModelManager()


I only see this referenced here an in a migration - is this used internally to morango or somewhere else?

Yes, this is the manager used specifically by Morango. Practically, I'm not sure this is actually needed, because of the additional morango sync hooks we have, but it means if things are getting deleted via regular queryset methods in morango we have this happening for sure.

nucleogenesis · 2025-10-31T22:12:31Z

kolibri/core/auth/models.py

        )

+    def save(self, *args, **kwargs):
+        # Call the parent save method first


Just a question for my own understanding - this & the delete method below are very similar to the update and delete methods in the FacilityUserQuerySet up above. What are the primary differences in the use cases for the two classes' handling of clearing sessions of deleted users?

I'm not sure I understand the question - models can be updated and deleted either via the queryset, or by model methods - we have to do this in both places to ensure that the proper clearing of session data is not conditional on how it happens.

One possible weakness with the current implementation is that we are not catching bulk_update calls - but I am not aware of anywhere we use that in Kolibri currently.

nucleogenesis · 2025-10-31T22:16:04Z

kolibri/core/utils/model_router.py

+    def DB_NAME(self):
+        pass
+
+    def db_for_read(self, model, **hints):


What's **hints here and why isn't it used in the methods?

It's a standard Django thing to provide additional context to the database router. We do not currently use it except in the allow_migrations method.

https://docs.djangoproject.com/en/3.2/topics/db/multi-db/#database-routers

rtibbles · 2025-11-04T03:01:57Z

@pcenov I think for both these cases, I'd rather file follow up issues - the use case of whole facility deletion is important, but I think significantly less common than individual user deletion, and I think we can push the minor 'error page' flash as a follow up in a patch release.

pcenov

Thanks @rtibbles - I've filled the notes from my comment as follow-up issues. This one is good to go!

AlexVelezLl

I'm not sure if an extra pair of eyes is helpful, but just a note that I read this entire PR critically (I learned quite a bit) and didn't find anything unusual.

rtibbles added this to the Kolibri 0.19: Bulk User Management milestone Sep 18, 2025

github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: large labels Sep 18, 2025

nucleogenesis self-assigned this Sep 18, 2025

rtibbles force-pushed the unhandle_my_lod branch 2 times, most recently from b6dfa77 to 59e4106 Compare September 18, 2025 19:55

nucleogenesis reviewed Sep 19, 2025

View reviewed changes

AlexVelezLl mentioned this pull request Oct 1, 2025

Check unusable device on user_auth plugin #13759

Merged

rtibbles added 2 commits October 29, 2025 12:32

Abstract our database router pattern into an abstract base class.

8a45c29

Update all implementations. Remove redundant unregistering of NotificationsRouter.

Change from file backed sessions to custom cached_db sessions.

e52629d

To allow sessions to be cleared via user id. Use separate sessions database to prevent bottlenecks.

rtibbles force-pushed the unhandle_my_lod branch from 59e4106 to 9c88a32 Compare October 30, 2025 00:58

rtibbles added 5 commits October 29, 2025 18:03

Add session cleanup on hard and soft deletion of users.

d917a26

Fix potential regression in facility deletion to ensure that soft del…

ae4e64b

…eted users also get deleted.

Update all tests that use the sessions database to "__all__" databases.

6cda77f

Add soud sync queue handling for deleted users.

bf2eb1d

Add session cleanup on sync of hard deleted, deleted, and soft delete…

a7848c7

…d users!

rtibbles force-pushed the unhandle_my_lod branch from 9c88a32 to a7848c7 Compare October 30, 2025 01:04

nucleogenesis reviewed Oct 31, 2025

View reviewed changes

pcenov self-requested a review November 4, 2025 14:46

This was referenced Nov 4, 2025

A deleted user's session expiration shouldn't result in an error page #13873

Open

Unclear workflow - What should happen if a facility is deleted while it's in use by some of the users #13874

Open

pcenov approved these changes Nov 4, 2025

View reviewed changes

AlexVelezLl reviewed Nov 5, 2025

View reviewed changes

rtibbles merged commit 01dfa44 into learningequality:develop Nov 5, 2025
51 checks passed

rtibbles deleted the unhandle_my_lod branch November 5, 2025 21:44

		@@ -0,0 +1,69 @@
		from abc import ABC
		from abc import abstractmethod

Robustly clear sessions on user deletion to cause immediate logout #13757

Robustly clear sessions on user deletion to cause immediate logout #13757

Uh oh!

Conversation

rtibbles commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Uh oh!

github-actions bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nucleogenesis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcenov commented Sep 22, 2025

Uh oh!

pcenov commented Oct 31, 2025

Uh oh!

nucleogenesis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibbles commented Nov 4, 2025

Uh oh!

pcenov left a comment

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rtibbles commented Sep 18, 2025 •

edited

Loading

github-actions bot commented Sep 18, 2025 •

edited

Loading