Request with invalid session after concurrent logout or session timeout is considered a BadRequest

When working with multiple tabs, if a user logs out or his session times out, any concurrent request happening in another tab will be considered a bad request. See the SessionInterrupted ​exception raised.

I see that ​@carltongibson was slightly worried about the status code and I feel the same. This for me should be handled as forbidden (SessionInterrupted being a subclass of PermissionDenied) because the request is actually well-formed, but it’s not allowed anymore.

What do you think?

1 Like

Hi @danielfbnunes — Yes… I still lean towards your view from a correctness POV, but I’m not sure what disruption it would cause, and whether that’s worth it. :thinking:

We handle PermissionDenied already (with an error view and all) so maybe it’d be feasible. Q: Who’s really leveraging the exact status code for this scenario?

On the other hand: 500 to 400 seems like a good win. 400 to 403, less so.

That’s not to come down either way. It being something you want to work on would be the first hurdle.

Kind regards,
Carlton

Hey @carltongibson, thanks for answering.

Let’s think about the case where a user has multiple tabs open. If he logs out in one tab while another request is happening on a different tab, he will likely get a 400 error page if the response to that request comes before the page reload (the page is automatically reloaded due to the invalid session). As a user, this is not what I would expect to see. That’s why I suggested the 403 approach using PermissionDenied because it makes more sense from the user’s standpoint.

For the described use case, why do you think this?

I can work on it and it would be my first time contributing. If you think the change is too disruptive, I’m happy to have this assigned to someone else.

Yes, I get the use-case. (It’s the same as the original ticket.)

I guess I have two hunches:

  1. I don’t suppose this occurs very often in real life. (It’s developers experimenting that hit it most I’d wager.)
  2. I’d suspect most people aren’t building behaviour around this, such that a change of status code would be a problem.

If 2 is correct, then likely we can change it without too much disruption. If.

I can work on it and it would be my first time contributing.

It’s a nicely scoped issue, so it might be a good starting point from that perspective if you’re interested to take it on. I don’t know if you’ll get lots of strong opinions either way though without opening a ticket. Perhaps give it some more time here to see if there are other opinions.

You could make a proof-of-concept PR, using the original as a model for what’s needed — code often helps to make decisions easier — but it might be that that doesn’t get accepted in the end. (I can’t immediately see Why not but I can’t just say one way or the other.)

I agree with both.

I have a ticket that was closed until I get some positive feedback from the topic here. Do you think I can link this thread there, re-open the ticket and assign it to me? It’s fine if I end up closing the PR but I feel I have to try it :smile:

Ha! :sweat_smile: OK, in that case I think you should probably have a go at it. I do agree with your take in principle, and if you want to take it on as a new contributor, I don’t think we should stand in the way there. So :+1:

I can’t 100% recall whether there was a strict reason (beyond Tim’s comment on the earlier PR) why 400 settled on, rather than 403, or whether that was because the PR was finished, and I didn’t want to put up an extra barrier for (what I think is) a pretty minor niggle. I need to chat with @felixxm about it — probably at DjangoCon Europe — it was ≈3 years ago, so I can’t recall whether we discussed this explicitly. (I half imagine we did, but … :woman_shrugging: )

Good hustle! And welcome aboard! :sailboat:

(You can @mention me on the PR if you’d like some input)

I will tag you there for sure!

Thanks, Carlton! Happy DjangoCon :smile:

1 Like

We should take into account that PermissionDenied subclasses are ​treated differently than BadRequest subclasses.

Correct! On the other hand, this should be less impactful if we’re talking under the assumptions posted by @carltongibson.

(Replying to Daniel) But it may be significant…

If I’m a developer experimenting locally, getting the informative debug error page, rather than a terse Forbidden, may well be worth more to me (in the rare occurrences of this) than the difference in the status code. :thinking:

What do think @danielfbnunes ?

For the use case described, I really think the Forbidden makes more sense, especially from the user’s perspective. The 400 is perceived to be a client error which is not the case considering the fact that the request is valid on any other occasion.

The developer would understand the error due to the error message, not the status code. Having both “correct” – 403 and the same error message – makes more sense IMO and should be the goal.

@danielfbnunes Could you share any extra details about the motivation behind this request? Specifically, do you have a web service where this was an issue for a “real user”? Or is this about a theoretical user that logs out in a tab?

In my experience working with large web services with web fronts, (real/average) users would usually not logout, from anywhere. There may be the case where the web service logs the user out (for whatever reason, password was changed, token was expired, suspicions action was detected, etc), but in those cases is arguably the responsibility of the web service itself to keep track of this fact and inform the user with explicit error messages about what happened.

I think that knowing the real-life driver for this change could help us make a better decision. Thanks!

Yes, this was an issue with a real user. In this case, this happened after a logout. It would be identical if we were talking about an expired session, for example.

I agree with the explicit error messages and for the logout case, there’s nothing more explicit than the actual user doing the logout :smile: The point here is not if the user is aware of what happened or not, it’s that we’re showing a wrong error and error page informing about a client error (according to the status code).

“The request’s session was deleted before the request completed. The user may have logged out in a concurrent request, for example”
:point_up: This is the error message coming with the “BadRequest” response. Nothing here shows that the user is responsible for a “client error”.

Little note: I’m not bringing this up as a huge bug that is causing a lot of trouble for me. Instead, I’m looking at this as an inconsistency that I think we could fix.

I agree with @danielfbnunes that 403 seems more appropriate. We have a web application that’s being strictly observed and almost each day we get a few alerts about these 400 errors.

It feels like a race condition whether 400 or 403 will be returned and really the client(browser) didn’t do anything wrong to deserve a 400 response.

About 500 I see it as if something is wrong with the server, which is also not the case.

403 is actional response, because it’s clear that the the session has to be (re)authenticated in order to keep using the endpoint. 500 and 400 just tell the client: “Sorry, there is nothing you can do.”

What’s the decision here?(cc @carltongibson @felixxm @nessita)

@danielfbnunes I’m afraid there does not seem to be a consensus just yet. There seems to be two clear +1, one -1 and two -0, though it would be good to confirm from @carltongibson’s whether his latest message it’s a -0 or not (I’m honestly not sure).

Yep, I’m a bit +-0. I’m not sure it’s worth the disruption or the lack of the error page…

:woman_shrugging:

Do you know anyone else who could provide more feedback and help us break the tie? :smile:

@danielfbnunes anyone contributing to/using Django can comment in this post.
Separately, if this would be a tie, we could resort to the Django Steering Council to make a final decision. So far this is 2 votes in favor and 3 against, so no tie (yet?).

I was talking about tagging people familiar with the related code that could provide some valuable feedback. Let’s wait a few more days and see if anyone else participates.