Shiro
  1. Shiro
  2. SHIRO-312

DefaultSecurityManager.setSessionManager can get out of sync with DefaultSecurityManager.setSessionMode

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.0
    • Fix Version/s: 1.2.0
    • Component/s: Web
    • Labels:
      None

      Description

      So, I've run into a bit of a pickle with DefaultWebSecurityManager and
      native vs http sessions.

      The DefaultWebSecurityManager exposes two methods, ostensibly for the
      purposes of determining how sessions are managed:

      setSessionManager(SessionManager)

      and

      setSessionMode(String)

      However, it would appear that if I call:

      setSessionManager(new MyCustomSessionManager())

      and then

      setSessionMode("native")

      the SessionManager is overridden.

      This is a bit of a gotcha, but can be easily avoided by not calling
      setSessionMode. (calling them in the reverse order seems contrary to
      the nature of setters) The problem with not calling setSessionMode is
      that it appears to actually matter - if I leave it to it's default
      (http), but set a DefaultWebSessionManager, then things break horribly
      (apparently due to the use of isHttpSessionMode by AbstractShiroFilter
      for redirect rewriting). Sessions get forgotten, etc. This also seems
      contrary to the nature of setters.

      1. SessionManager_SHIRO-312.patch
        7 kB
        Jared Bunting
      2. SessionManager_SHIRO-312_b.patch
        7 kB
        Jared Bunting

        Activity

        Jared Bunting created issue -
        Hide
        Jared Bunting added a comment -

        After discussion on the dev list, I will be adding a new WebSessionManager interface that a web-specific SessionManager can optionally implement, with a method, "isServletContainerSessions". DefaultSecurityManager's isHttpMode will then use this method to return it's value, and will not use the saved sessionMode string.

        Show
        Jared Bunting added a comment - After discussion on the dev list, I will be adding a new WebSessionManager interface that a web-specific SessionManager can optionally implement, with a method, "isServletContainerSessions". DefaultSecurityManager's isHttpMode will then use this method to return it's value, and will not use the saved sessionMode string.
        Hide
        Jared Bunting added a comment -

        Adds the WebSessionManager interface with a usesServletContainerSessions method. This interfaces is not required to be implemented, and if the SessionManager does not implement it, the behavior is the same as previously when the sessionMode was null.

        I have not deprecated the setSessionMode convenience method, but I have deprecated use of that field in an attempt to avoid it being used for any logic outside of the "setSessionMode" method.

        I have also added a test case to ensure that isHttpSessionMode is actually using the return value of the WebSessionManager's method.

        Show
        Jared Bunting added a comment - Adds the WebSessionManager interface with a usesServletContainerSessions method. This interfaces is not required to be implemented, and if the SessionManager does not implement it, the behavior is the same as previously when the sessionMode was null. I have not deprecated the setSessionMode convenience method, but I have deprecated use of that field in an attempt to avoid it being used for any logic outside of the "setSessionMode" method. I have also added a test case to ensure that isHttpSessionMode is actually using the return value of the WebSessionManager's method.
        Jared Bunting made changes -
        Field Original Value New Value
        Attachment SessionManager_SHIRO-312.patch [ 12487102 ]
        Hide
        Les Hazlewood added a comment -

        Hi Jared,

        I can't apply the patch - I believe I'm having errors due to recent changes in the ServletContainerSessionManager class. Would you mind updating to the latest trunk and applying a new patch?

        My apologies!

        Show
        Les Hazlewood added a comment - Hi Jared, I can't apply the patch - I believe I'm having errors due to recent changes in the ServletContainerSessionManager class. Would you mind updating to the latest trunk and applying a new patch? My apologies!
        Hide
        Jared Bunting added a comment -

        Sure thing. This is against svn revision 1149681.

        Show
        Jared Bunting added a comment - Sure thing. This is against svn revision 1149681.
        Jared Bunting made changes -
        Attachment SessionManager_SHIRO-312_b.patch [ 12487462 ]
        Les Hazlewood made changes -
        Assignee Les Hazlewood [ lhazlewood ]
        Hide
        Les Hazlewood added a comment -

        Thanks Jared - this one worked.

        I applied the patch with some minor adjustments:

        • I moved the WebSessionManager to the org.apache.shiro.web.session.mgt package to be with the other web session management interfaces/components
        • I changed the WebSessionManager's method to be isServletContainerSessions() to be JavaBeans compatible (in case any tools introspect the API).
        • Minor JavaDoc additions.

        Please feel free to review and offer any further suggestions/adjustments.

        Show
        Les Hazlewood added a comment - Thanks Jared - this one worked. I applied the patch with some minor adjustments: I moved the WebSessionManager to the org.apache.shiro.web.session.mgt package to be with the other web session management interfaces/components I changed the WebSessionManager's method to be isServletContainerSessions() to be JavaBeans compatible (in case any tools introspect the API). Minor JavaDoc additions. Please feel free to review and offer any further suggestions/adjustments.
        Hide
        Les Hazlewood added a comment -

        I also added some more Deprecation notices and one log message. The 'sessionMode' property was added as a convenience to automagically set the SessionManager based on which session mechanism one wanted to use. However, this property has caused more than enough problems due to configuration out-of-order problems based on when you set the property - not nice. It's better to just remove it alltogether, so maybe we can do that in 1.3 or 2.0.

        Show
        Les Hazlewood added a comment - I also added some more Deprecation notices and one log message. The 'sessionMode' property was added as a convenience to automagically set the SessionManager based on which session mechanism one wanted to use. However, this property has caused more than enough problems due to configuration out-of-order problems based on when you set the property - not nice. It's better to just remove it alltogether, so maybe we can do that in 1.3 or 2.0.
        Hide
        Les Hazlewood added a comment -

        I had to make one final adjustment. I changed the implementation of isHttpSessionMode() to be the following:

        SessionManager sessionManager = getSessionManager();
        return sessionManager instanceof WebSessionManager && ((WebSessionManager)sessionManager).isServletContainerSessions();

        If the end-user configures something that is not a WebSessionManager, we can't assume that Servlet Container sessions are being used. So this method will return 'false' in that case.

        Show
        Les Hazlewood added a comment - I had to make one final adjustment. I changed the implementation of isHttpSessionMode() to be the following: SessionManager sessionManager = getSessionManager(); return sessionManager instanceof WebSessionManager && ((WebSessionManager)sessionManager).isServletContainerSessions(); If the end-user configures something that is not a WebSessionManager, we can't assume that Servlet Container sessions are being used. So this method will return 'false' in that case.
        Hide
        Jared Bunting added a comment -

        Works for me. I had named it "uses" b/c it "is" just doesn't make sense to me, but for the purposes of JavaBeans introspection, that seems reasonable.

        As far as the isHttpSessionMode, I agree with that logic...makes more sense - I was just trying to maintain current behavior, where anything other than "native" returned true.

        Show
        Jared Bunting added a comment - Works for me. I had named it "uses" b/c it "is" just doesn't make sense to me, but for the purposes of JavaBeans introspection, that seems reasonable. As far as the isHttpSessionMode, I agree with that logic...makes more sense - I was just trying to maintain current behavior, where anything other than "native" returned true.
        Hide
        Les Hazlewood added a comment -

        Sounds good - thanks for the review.

        Show
        Les Hazlewood added a comment - Sounds good - thanks for the review.
        Jared Bunting made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Les Hazlewood made changes -
        Fix Version/s 1.2.0 [ 12315478 ]
        Hide
        Les Hazlewood added a comment -

        Closing with the 1.2.0 release.

        Show
        Les Hazlewood added a comment - Closing with the 1.2.0 release.
        Les Hazlewood made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Les Hazlewood
            Reporter:
            Jared Bunting
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development