Shiro
  1. Shiro
  2. SHIRO-380

runAs feature (still) doesn't work

    Details

      Description

      Right after SecurityUtils.getSubject().runAs(new new SimplePrincipalCollection()

      {...}

      )

      SecurityUtils.getSubject().getPrincipal() returns correct new Principal
      SecurityUtils.getSubject()..getPreviousPrincipals() returns correct original Principal

      but DefaultSubjectDAO merge principals in method

      protected void mergePrincipals(Subject subject) {
      PrincipalCollection currentPrincipals = subject.getPrincipals();
      ...
      if (session == null)

      { ... }

      else {
      PrincipalCollection existingPrincipals = (PrincipalCollection) session.getAttribute(DefaultSubjectContext.PRINCIPALS_SESSION_KEY);
      if (CollectionUtils.isEmpty(currentPrincipals))

      { ... }

      else {
      if (!currentPrincipals.equals(existingPrincipals))

      { session.setAttribute(DefaultSubjectContext.PRINCIPALS_SESSION_KEY, currentPrincipals); }

      }
      }

      and after that
      SecurityUtils.getSubject().getPrincipal() and SecurityUtils.getSubject().getPreviousPrincipals() both returns new Principal - this is wrong behavior

      1. shiro_380_webapp.tgz
        4 kB
        Jochen Munz
      2. SHIRO-380-patch1.diff
        3 kB
        Elijah Korneckis

        Issue Links

          Activity

          Jochen Munz created issue -
          Jochen Munz made changes -
          Field Original Value New Value
          Link This issue is a clone of SHIRO-344 [ SHIRO-344 ]
          Hide
          Jochen Munz added a comment -

          I have cloned the SHIRO-344 issue, because using shiro 1.2.1, I still experience this issue - the principals get overwritten in the mergePrincipals() method.

          Running the runAs() tests for the DelegatingSubject gives me no errors.

          Also noteworthy, when entering a second runAs-Level, the previous principals are preserved. It is only on the first runAs level that the previous principal gets overwritten.

          When I am using the subclassed SubjectDAO of SHIRO-344 the problem does not occur, runAs works on all levels as expected.

          My setup is web-based, maybe this leads to some subtle differences?

          Show
          Jochen Munz added a comment - I have cloned the SHIRO-344 issue, because using shiro 1.2.1, I still experience this issue - the principals get overwritten in the mergePrincipals() method. Running the runAs() tests for the DelegatingSubject gives me no errors. Also noteworthy, when entering a second runAs-Level, the previous principals are preserved. It is only on the first runAs level that the previous principal gets overwritten. When I am using the subclassed SubjectDAO of SHIRO-344 the problem does not occur, runAs works on all levels as expected. My setup is web-based, maybe this leads to some subtle differences?
          Jochen Munz made changes -
          Affects Version/s 1.2.1 [ 12319511 ]
          Affects Version/s 1.2.0 [ 12315478 ]
          Fix Version/s 1.3.0 [ 12317961 ]
          Fix Version/s 1.2.1 [ 12319511 ]
          Hide
          Jochen Munz added a comment -

          Attached sample webapp to demonstrate the behaviour - see the README file explanations

          Show
          Jochen Munz added a comment - Attached sample webapp to demonstrate the behaviour - see the README file explanations
          Jochen Munz made changes -
          Attachment shiro_380_webapp.tgz [ 12539664 ]
          Hide
          Les Hazlewood added a comment -

          Thanks for the update Jochen, and the sample app. Hopefully we can use this to create more test cases beyond the ones we already have.

          Do you know how the behavior differs from the DelegatingSubjectTest testRunAs() test case (other than the web scenario)?

          http://svn.apache.org/repos/asf/shiro/trunk/core/src/test/java/org/apache/shiro/subject/DelegatingSubjectTest.java

          Show
          Les Hazlewood added a comment - Thanks for the update Jochen, and the sample app. Hopefully we can use this to create more test cases beyond the ones we already have. Do you know how the behavior differs from the DelegatingSubjectTest testRunAs() test case (other than the web scenario)? http://svn.apache.org/repos/asf/shiro/trunk/core/src/test/java/org/apache/shiro/subject/DelegatingSubjectTest.java
          Hide
          Jochen Munz added a comment -

          I am not aware of significant differences between sample app and the unit test (besides the web setup).

          The steps are quite similar:

          • 3 users are defined upfront
          • login as user 1 (1st request)
          • runAs user2 (2nd request, status shows up ok)
          • check status (3rd request, status shows user2 as current and previous principal)

          I could think of different behaviour because of (web) session handling and/or threading (as the unit test runs in one go).

          Can you (or somebody else) please verify the behaviour/setup with the attached sample app, so that we can exclude misconfiguration as a possible cause? I'll be happy to investigate further.

          Show
          Jochen Munz added a comment - I am not aware of significant differences between sample app and the unit test (besides the web setup). The steps are quite similar: 3 users are defined upfront login as user 1 (1st request) runAs user2 (2nd request, status shows up ok) check status (3rd request, status shows user2 as current and previous principal) I could think of different behaviour because of (web) session handling and/or threading (as the unit test runs in one go). Can you (or somebody else) please verify the behaviour/setup with the attached sample app, so that we can exclude misconfiguration as a possible cause? I'll be happy to investigate further.
          Hide
          Elijah Korneckis added a comment -

          Hi,

          Here's what I've been able to piece together. I think the main difference between the test scenario and a web app setup is that the Subject that is bound to the tread and saved in the session is recreated between requests - in the filter chain (see AbstractShiroFilter, line 359).

          During the creation process the DefaultSecurityManager.createSubject calls it's save method (at line 350).
          That, in turn, calls DefaultSubjectDAO.mergePrincipals (at line 163): save -> saveToSession -> mergePrincipals.

          Here's where things get interesting. Consider Jochen's scenario, right after runAs is executed:
          1. The session now contains the following attributes:

          • DefaultSubjectContext.PRINCIPALS_SESSION_KEY = user1
          • DelegatingSubject.RUN_AS_PRINCIPALS_SESSION_KEY = [user2];
            2. When the next request is fired, we enter the filter chain and get to mergePrincipals.

          At this point
          PrincipalCollection currentPrincipals = subject.getPrincipals(); (DefaultSubjectDAO, line 177)
          will return "user2" as it is the top item in the runAs stack.

          After that
          PrincipalCollection existingPrincipals = (PrincipalCollection) session.getAttribute(DefaultSubjectContext.PRINCIPALS_SESSION_KEY); (line 187)
          will return "user1" as it is saved in the session.

          And here the initial principal is overwritten (lines 196 to 198):

          // currentPrincipals == user2, existingPrincipals = user1
          if (!currentPrincipals.equals(existingPrincipals))

          { session.setAttribute(DefaultSubjectContext.PRINCIPALS_SESSION_KEY, currentPrincipals); }

          Whew, hope I got that right. I've attached a diff with changes that solved this issue for me. The changes are rather minor - initial prinicpal is saved to session during login and restored when the runAs stack is emptied.

          P.S. All line numbers and the diff file are taken from 1.2.1 relase tag (https://svn.apache.org/repos/asf/shiro/tags/shiro-root-1.2.1)

          Show
          Elijah Korneckis added a comment - Hi, Here's what I've been able to piece together. I think the main difference between the test scenario and a web app setup is that the Subject that is bound to the tread and saved in the session is recreated between requests - in the filter chain (see AbstractShiroFilter, line 359). During the creation process the DefaultSecurityManager.createSubject calls it's save method (at line 350). That, in turn, calls DefaultSubjectDAO.mergePrincipals (at line 163): save -> saveToSession -> mergePrincipals. Here's where things get interesting. Consider Jochen's scenario, right after runAs is executed: 1. The session now contains the following attributes: DefaultSubjectContext.PRINCIPALS_SESSION_KEY = user1 DelegatingSubject.RUN_AS_PRINCIPALS_SESSION_KEY = [user2] ; 2. When the next request is fired, we enter the filter chain and get to mergePrincipals. At this point PrincipalCollection currentPrincipals = subject.getPrincipals(); (DefaultSubjectDAO, line 177) will return "user2" as it is the top item in the runAs stack. After that PrincipalCollection existingPrincipals = (PrincipalCollection) session.getAttribute(DefaultSubjectContext.PRINCIPALS_SESSION_KEY); (line 187) will return "user1" as it is saved in the session. And here the initial principal is overwritten (lines 196 to 198): // currentPrincipals == user2, existingPrincipals = user1 if (!currentPrincipals.equals(existingPrincipals)) { session.setAttribute(DefaultSubjectContext.PRINCIPALS_SESSION_KEY, currentPrincipals); } Whew, hope I got that right. I've attached a diff with changes that solved this issue for me. The changes are rather minor - initial prinicpal is saved to session during login and restored when the runAs stack is emptied. P.S. All line numbers and the diff file are taken from 1.2.1 relase tag ( https://svn.apache.org/repos/asf/shiro/tags/shiro-root-1.2.1 )
          Elijah Korneckis made changes -
          Attachment SHIRO-380-patch1.diff [ 12539882 ]
          Hide
          Les Hazlewood added a comment -

          Jochen and Elijah thanks for your help!!! Jochen, the sample web app was a HUGE help, I wish all bug reports had one of these Great stuff!

          I just committed a hotfix for this using a sneaky bit of reflection to ensure point-version forwards and backwards compatibility (new non-private fields, constants and methods can only be introduced during minor point revision releases (i.e. 1.2 -> 1.3, but not 1.2.1 -> 1.2.2).

          I tested with Jochen's web app and all appears to be well.

          Jochen, I also added another action to your servlet, "pop", i.e. localhost:8080/shiro380/login?action=pop which calls subject.releaseRunAs();

          This allows me to action=runas or action=runas2 as many times as I wanted. Each action=pop request would pop the stack and, when depleted, would return the original Subject principals as expected. This would have been a bit harder to test without your webapp - thanks again!

          For those following along, if you're willing, please try the latest 1.2.x branch (1.2.2-SNAPSHOT) and feel free to test it out.

          Thanks again,

          Les

          Show
          Les Hazlewood added a comment - Jochen and Elijah thanks for your help!!! Jochen, the sample web app was a HUGE help, I wish all bug reports had one of these Great stuff! I just committed a hotfix for this using a sneaky bit of reflection to ensure point-version forwards and backwards compatibility (new non-private fields, constants and methods can only be introduced during minor point revision releases (i.e. 1.2 -> 1.3, but not 1.2.1 -> 1.2.2). I tested with Jochen's web app and all appears to be well. Jochen, I also added another action to your servlet, "pop", i.e. localhost:8080/shiro380/login?action=pop which calls subject.releaseRunAs(); This allows me to action=runas or action=runas2 as many times as I wanted. Each action=pop request would pop the stack and, when depleted, would return the original Subject principals as expected. This would have been a bit harder to test without your webapp - thanks again! For those following along, if you're willing, please try the latest 1.2.x branch (1.2.2-SNAPSHOT) and feel free to test it out. Thanks again, Les
          Hide
          Les Hazlewood added a comment -

          Fixed in the 1.2.x branch (with test cases) and ported to trunk (1.3.0-SNAPSHOT). Thanks to Jochen and Elijah for their debugging help!

          If there are any further issues related to this prior to a 1.2.2 and/or 1.3.0 release, please re-open this issue.

          Show
          Les Hazlewood added a comment - Fixed in the 1.2.x branch (with test cases) and ported to trunk (1.3.0-SNAPSHOT). Thanks to Jochen and Elijah for their debugging help! If there are any further issues related to this prior to a 1.2.2 and/or 1.3.0 release, please re-open this issue.
          Les Hazlewood made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 1.2.2 [ 12323469 ]
          Fix Version/s 1.3.0 [ 12317961 ]
          Resolution Fixed [ 1 ]

            People

            • Assignee:
              Les Hazlewood
              Reporter:
              Jochen Munz
            • Votes:
              2 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development