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

Use a real SessionBase object on FooterNoSessionMiddleware #5797

Merged
merged 1 commit into from Jun 17, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jun 12, 2019

Instead of forcing to be an empty dict {}, we force it to be a real SessionBase object which it's what Django uses internally. This avoid issues where other pieces expects the SessionBase.

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

This allows all Django internals method to keep working in the same
way but still using an empty session.
@humitos humitos requested review from and davidfischer Jun 12, 2019
@@ -205,7 +206,7 @@ def process_request(self, request):
settings.SESSION_COOKIE_NAME not in request.COOKIES
):
# Hack request.session otherwise the Authentication middleware complains.
request.session = {}
request.session = SessionBase() # create an empty session
Copy link
Member Author

@humitos humitos Jun 12, 2019

We could instantiate an empty instance of the same class that we are using in the backend.

from importlib import import_module
from django.conf import settings
SessionStore = import_module(settings.SESSION_ENGINE).SessionStore
request.session = SessionStore()

I suppose that won't be any difference since the session_key is None, though.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 12, 2019

I'm pretty sure this isn't the solution to our problem. We don't want a session on the request, anywhere downstream that thinks it's editing a session, that data won't be saved. We want that to error, and to stop trying to edit the session, not fake out users who think they are really editing it.

@humitos
Copy link
Member Author

@humitos humitos commented Jun 12, 2019

Hrm... I implemented this solution because we are already faking it with a dict (allowing get/set values). The comment says that otherwise Auth backend fails.

On the other hand, using this general SessionBase will behave similar to a dict (it's a dict-like object) but actually it will fail with a proper error (NotImplementedError) when calling the methods .save, .delete, .create, etc instead saying that the dict does not have this method.

Finally, for the use case I'm trying to fix solve, it will work since request.session.session_key will return None which it's expected when there is no session.

I'm not 100% sure, but seems to be the right way of having the "same behavior than a regular session object without saving anything and hard-failing if trying to save it".

@humitos
Copy link
Member Author

@humitos humitos commented Jun 12, 2019

I know that this affects a lot of requests. So, if we are in doubt we can just go in another direction that we consider safer. Although, at some point this will bite us again.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 12, 2019

Good point. I mostly have 2 reservations:

  • I tried something similar when I wrote this code, but I don't think it was quite this
  • Like you mentioned, the scale of the change. Though it should be pretty obvious if this breaks, so we can just roll it back.

So I guess I'm +0 on this proposed change, then.

@humitos
Copy link
Member Author

@humitos humitos commented Jun 13, 2019

OK. I think we can merge and deploy carefully being prepared to roll it back, as you said. Although, we can have another person reviewing this before merging.

@humitos humitos merged commit c03f839 into master Jun 17, 2019
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/use-real-session-object branch Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants