You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by GitBox <gi...@apache.org> on 2022/09/05 05:18:04 UTC
[GitHub] [shiro] lprimak opened a new pull request, #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
lprimak opened a new pull request, #372:
URL: https://github.com/apache/shiro/pull/372
Fixes [SHIRO-512](https://issues.apache.org/jira/browse/SHIRO-512)
Following this checklist to help us incorporate your contribution quickly and easily:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SHIRO) 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.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] 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.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] 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.
- [X] 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](https://github.com/apache/shiro/blob/master/CONTRIBUTING.md)
If your pull request is about ~20 lines of code you don't need to sign an [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf)
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](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
bdemers commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r993606906
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,16 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ // There could be a rare race condition when a session is invalidated in another thread,
+ // this thread could throw this exception, so we catch it
+ // similar issue as in clearRunAsIdentitiesInternal()
+ // See https://issues.apache.org/jira/browse/SHIRO-512
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
Review Comment:
@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
1. Thread 2 - calls `session.invalidate()`; // somewhere else in the code
1. Thread 1 - calls `session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);` // that throws an exception, the session is invalid.
1. 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
1. Thread 1 - calls `session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);` // object returned
1. 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
1. Thread 1 - calls `Session session = getSession(false);` // null is returned
1. 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] sunshineandy commented on pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
sunshineandy commented on PR #372:
URL: https://github.com/apache/shiro/pull/372#issuecomment-1236549170
这是自动回复邮件。你好，您的邮件已经发送到我的邮箱，我看过后会尽快给您回复。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak closed pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
lprimak closed pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
URL: https://github.com/apache/shiro/pull/372
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] adamenveil commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
adamenveil commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r993395430
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,16 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ // There could be a rare race condition when a session is invalidated in another thread,
+ // this thread could throw this exception, so we catch it
+ // similar issue as in clearRunAsIdentitiesInternal()
+ // See https://issues.apache.org/jira/browse/SHIRO-512
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
lprimak commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r963026734
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,12 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
+ + "can generally safely be ignored.", se);
+ }
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon merged pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
fpapon merged PR #372:
URL: https://github.com/apache/shiro/pull/372
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] rzo1 commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
rzo1 commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r962718372
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,12 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
+ + "can generally safely be ignored.", se);
+ }
Review Comment:
The underlying `IllegalStateException` is thrown, if the session is invalidated (servlet spec).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
fpapon commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r962590771
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,12 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
+ + "can generally safely be ignored.", se);
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
lprimak commented on PR #372:
URL: https://github.com/apache/shiro/pull/372#issuecomment-1236554223
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
fpapon commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r962590771
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,12 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
+ + "can generally safely be ignored.", se);
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
lprimak commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r993627018
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,16 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ // There could be a rare race condition when a session is invalidated in another thread,
+ // this thread could throw this exception, so we catch it
+ // similar issue as in clearRunAsIdentitiesInternal()
+ // See https://issues.apache.org/jira/browse/SHIRO-512
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
Review Comment:
@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 not`getRunAsPrincipalsStack` 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
lprimak commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r993636993
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,16 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ // There could be a rare race condition when a session is invalidated in another thread,
+ // this thread could throw this exception, so we catch it
+ // similar issue as in clearRunAsIdentitiesInternal()
+ // See https://issues.apache.org/jira/browse/SHIRO-512
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on a diff in pull request #372: [SHIRO-512] catch SessionException in getRunAsPrincipalsStack
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #372:
URL: https://github.com/apache/shiro/pull/372#discussion_r962521867
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -471,7 +471,12 @@ public PrincipalCollection releaseRunAs() {
private List<PrincipalCollection> getRunAsPrincipalsStack() {
Session session = getSession(false);
if (session != null) {
- return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ try {
+ return (List<PrincipalCollection>) session.getAttribute(RUN_AS_PRINCIPALS_SESSION_KEY);
+ } catch (SessionException se) {
+ log.debug("Encountered session exception trying to get 'runAs' principal stack. This "
+ + "can generally safely be ignored.", se);
+ }
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org