Rework session ids

[hmpf]

When they are set, when they are changed, when they are deleted...

[stveit]

https://docs.djangoproject.com/en/5.0/topics/http/sessions/#django.contrib.sessions.backends.base.SessionBase.cycle_key

cycle_key()

Creates a new session key while retaining the current session data. django.contrib.auth.login() calls this method to mitigate against session fixation.

Im guessing we dont use django.contrib.auth in NAV, so the automatic session rotation you should get in django doesnt actually happen for us.

[stveit]

Calling request.session.cycle_key() On login (and i think its meta to do it on logout too) should do the trick

EDIT: request.session.flush() seems to be the method Django uses on logout. This doesnt just rotate the key but deletes the session and cookie stuff altogether

[stveit]

When testing manually and logging in and out with admin user on current master, it does actually already rotate session ID on logout (but not on login, #2813 fixes that).

This is what the logout does:

        del request.session[ACCOUNT_ID_VAR]
        del request.account
        request.session.set_expiry(datetime.now())

Not entirely sure what here causes a new session ID to be created, but its probably the session being expired on the next request that causes it

Those lines should probably be replaced with request.session.flush() (del request.account might have to be kept since thats a nav-only thing)

[stveit]

I generally dont understand why request.account is a thing. This seems to be a nav-way of saving an account to the session, but that is effectively already done by request.session[ACCOUNT_ID_VAR]. While this doesnt save the account object directly, you can easily retrieve it since you have the ID.

My guess is request.account is so you have the account object there directly without having to get it from the DB, but then why use request.session[ACCOUNT_ID_VAR] as well? seems redundant and complicates things unnecessarily

[lunkwill42]

I generally dont understand why request.account is a thing. This seems to be a nav-way of saving an account to the session, but that is effectively already done by request.session[ACCOUNT_ID_VAR]. While this doesnt save the account object directly, you can easily retrieve it since you have the ID.

My guess is request.account is so you have the account object there directly without having to get it from the DB, but then why use request.session[ACCOUNT_ID_VAR] as well? seems redundant and complicates things unnecessarily

I don't see the neither the redundancies nor the complications that you apparently do. This is all to do with separation of concerns.

Remember, the request object is something Django creates each time an incoming HTTP request arrives, and it passes this object around to middleware and view functions to describe the incoming request. It has nothing to do with sessions, it just describes an HTTP request.

The session layer is implemented as a separate Django middleware, which concerns itself with identifying which session an incoming request should be associated with. It reads and sets cookies with session IDs to do this. Each session has an ID and an associated dictionary of arbitrary values. The middleware persists the session dictionary by serializing it and storing it in the database, and it retrieves and de-serializes session dictionaries from the database based on the session identified by cookies in the incoming requests. It attaches the de-serialized session dictionary to request.session at the start of each request, and at the end of each request cycle, it will persist changes to request.session back to the database.

What a Django application chooses to store in request.session is entirely up to itself, but none of the view functions will need to concern themselves with how this information is persisted and correctly retrieved.

django.contrib.auth provides middleware to store a user ID in request.session, and provides request.user as a shorthand for other views and middleware to fetch the currently logged in user. Again, this is due to separation of concerns. View function should not concern themselves with how a user was associated with an incoming request, they should just rely on the readily available request.user object.

NAV doesn't use django.contrib.auth, since NAV predates Django by many years, so NAV has developed its own Django middleware for authentication. It essentially does the same as the Django auth middleware, but using NAV's account model and supported authentication methods. In order to avoid screwing up any Django code that expects request.user to be a django.contrib.auth User object, NAV instead sets the authenticated account in request.account, so that NAV views can find it there. Again, those views shouldn't concern themselves with the fact that the session dictionary happens to contain a magic variable with the authenticated account's primary key in it. It would be extremely redundant if every view was to keep an algorithm to look up the user object based on this, and the implementation details could change at any time, which would necessitate changing the redundant code contained in hundreds of view functions.

[stveit]

#2828 related