Robustly clear sessions on user deletion to cause immediate logout #13757
Conversation
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.
github-actions
bot
added
DEV: backend
Build Artifacts
|
github-actions
bot
added
APP: Device
rtibbles
force-pushed
the
unhandle_my_lod
branch
from
c9b9896 to
b6dfa77
Compare
rtibbles
force-pushed
the
unhandle_my_lod
branch
from
b6dfa77 to
59e4106
Compare
There was a problem hiding this comment.
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.
| 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. | ||
| """ |
| @@ -0,0 +1,69 @@ | |||
| from abc import ABC | |||
| from abc import abstractmethod | |||
There was a problem hiding this comment.
(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
There was a problem hiding this comment.
Love how this whole module DRYs things up
There was a problem hiding this comment.
Yes, ABC and abstractmethods are the more explicit way to do this.
| user=self.full_name or self.username, facility=self.facility | ||
| ) | ||
|
|
||
| def __init__(self, *args, **kwargs): |
There was a problem hiding this comment.
(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 😄
There was a problem hiding this comment.
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.
| # 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) | ||
| ) |
There was a problem hiding this comment.
Nice to see code like this in the red :)
There was a problem hiding this comment.
It has been redundant for quite some time, because of the router conditionality being in settings/base.py instead!
|
Hi @rtibbles, Logs: |
3 participants
Summary
References
FIxes #13542
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.
The result here should be that the user is logged out identically if the user's session timed out.