[SHIRO-512] catch SessionException in getRunAsPrincipalsStack

[lprimak]

Fixes SHIRO-512
Fixes flowlogix/flowlogix#284

Following this checklist to help us incorporate your contribution quickly and easily:

• Make sure there is a JIRA issue filed
  for the change (usually before you start working on it). Trivial changes like typos do not
  require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [SHIRO-XXX] - Fixes bug in SessionManager,
  where you replace SHIRO-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in the pull request title and in the first line of the commit message.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean install apache-rat:check to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• If you have a group of commits related to the same change, please squash your commits into one and force push your branch using git rebase -i.

Trivial changes like typos do not require a JIRA issue (javadoc, comments...).
In this case, just format the pull request title like (DOC) - Add javadoc in SessionManager.

If this is your first contribution, you have to read the Contribution Guidelines

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[sunshineandy]

这是自动回复邮件。你好，您的邮件已经发送到我的邮箱，我看过后会尽快给您回复。

[lprimak]

Something's wrong with Jenkins environment:

com.github.mjeanroy.junit.servers.exceptions.ServerStartException: java.io.IOException: Failed to bind to 0.0.0.0/0.0.0.0:37081
Caused by: java.io.IOException: Failed to bind to 0.0.0.0/0.0.0.0:37081
Caused by: java.net.BindException: Address already in use

[fpapon]

LGTM
Thanks for your contribution!

[bmarwell]

Can you add some more information? Not sure whether it should be a comment or go into the exception message, though. Can you elaboare, WHY this can be ignored and WHY and WHEN this occurs? Is it true it only occurs on Tomcat? Shouldn't we file an issue there instead?

[fpapon]

This exception seems to be a race condition on Tomcat but appear randomly so I'm not sure we can find more information in the exception and it could be hard to reproduce.

[rzo1]

The underlying IllegalStateException is thrown, if the session is invalidated (servlet spec).

[lprimak]

Please see the underlying JIRA. It's not only Tomcat, but any other servlet container.
I have reproduced it on both Payara, Glassfish and Jetty.
There is also the same try/catch block on clearRunAsIdentitiesInternal() for the same reason.

I did add some comments to the code

[adamenveil]

I'm not trying to be rude, I'm legitimately trying to understand, how is this safe to ignore? This appears to be the reason one of my unit tests is failing now after bumping Shiro up to 1.10.0

So if Thread_1 invalidates the subject, then Thread_2 attempts to get the session, this block here would originally throw an exception to Thread_2 informing that the session has been invalidated. But now Thread_2 will catch and just log the exception instead. This would essentially let Thread_2 believe the subject is still authenticated, when the session should actually be invalidated.

[bdemers]

@adamenveil
I can sort of see both sides of this. I think what this change was trying to prevent was:

1. Thread 1 - calls Session session = getSession(false); // valid session returned
2. Thread 2 - calls session.invalidate(); // somewhere else in the code
3. Thread 1 - calls session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY); // that throws an exception, the session is invalid.
4. Thread 1 - the getRunAsPrincipalsStack method returns null

If this same flow of events happened in a different order:

1. Thread 1 - calls Session session = getSession(false); // valid session returned
2. Thread 1 - calls session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY); // object returned
3. Thread 2 - calls session.invalidate(); // somewhere else in the code

The caller of getRunAsPrincipalsStack() doesn't know the session has been invalidated.

Or in the order of:

1. Thread 2 - calls session.invalidate(); // somewhere else in the code
2. Thread 1 - calls Session session = getSession(false); // null is returned
3. Thread 1 - the getRunAsPrincipalsStack method returns null

Can you give us an idea of what your code is doing maybe there is something else we could do?

[lprimak]

@adamenveil Think of a session is an implementation detail, and that's all.
Previous code had an exception leak through because of the session invalidation.
Some of which could never be caught (if it was in the web flow) and thus random exception logs would appear,
as stated in the multiple commenters in the JIRA
It's notgetRunAsPrincipalsStack job to check if the session is invalid. If it is, it just returns null in the new code,
which is the correct behavior.

Think about it like this: old code forced to check an (unrelated) session validity in every call to Shiro, which is not the right thing for an API to do.

API needs to handle it's own errors.

[lprimak]

i.e. it's not specified that the subject is automatically logged out when session is invalidated.
Proper way (according to the API) to log a subject out is SecurityUtils.getSubject().invalidate() and not session.invalidate()

It is true that Shiro could add HttpSession listeners and automatically log the subject out if a session gets invalidated, but neither the old nor new code does this, perhaps this could be an additional feature in the future, but certainly not a showstopper