Spring Security
  1. Spring Security
  2. SEC-1167

Introduce more flexible SavedRequest handling

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.4
    • Fix Version/s: 3.0.0 RC1
    • Component/s: Web
    • Labels:
      None

      Description

      The current SavedRequest approach is rather restricted. It would be better if a strategy was introduced which handled the generation of a SavedRequest. With that in mind, SecurityContextHolderAwareRequestFilter should also use a strategy for generating the wrapper, rather than being configured with a wrapper class. This would handle the request matching and generation of the wrapped request, if required.

      Interfaces:

      RequestPersister {
      saveRequest(HttpServletRequest request);
      }

      (default implementation produces a SavedRequest and stores it in the session). Possibly make SavedRequest an interface to allow customization.

      RequestFactory {
      HttpServletRequest createRequest(HttpServletRequest request);
      }

      (reloads the saved request and returns a wrapper delegating to it). The existing SavedRequestAwareWrapper would be used in a modified form (it currently does the matching itself but that would be done by the factory implementation).

      The default SavedRequestHandler could implement both interfaces.

      It should also be possible to disable SavedRequest use completely in ExceptionTranslationFilter when the application always uses a default target, for example.

        Issue Links

          Activity

          Hide
          Luke Taylor added a comment -

          I've committed changes with a new RequestCache interface which handles storage and retrieval of the SavedRequest (and also of a request matching wrapper). The functionality has also been separated into a specific filter (RequestCacheAwareFilter) from the SecurityContextHolderAwareFilter as the requirements are orthogonal and there is no connection with providing servlet API support. They can thus be used independently and it makes the wrapper creation much simpler (configuration with a wrapper class and the use of reflection to create the wrapper is no longer supported as it would be trivial to insert an alternative filter). The default implementation of RequestCache is HttpSessionRequestCache. Properties such as allowSessionCreation and justUseSavedRequestOnGet have been moved to this class from ExceptionTranslationFilter.

          The SavedRequestWrapper use should also be simpler as it will not be used if there is no matching request in the cache. This class can therefore be modified to assume that the savedRequest it contains is not null. At the moment it has a lot of differing logic based on whether it is null or not. This was the cause of quite a few bugs in the past with forwarded requests etc. Hopefully things should now be much more straightforward.

          It remains to be seen whether we need a public SavedRequest interface/class at all since most of the logic is now within the RequestCache implementation. It may be preferable in order to allow people to customize the implementation to support POST bodies etc but using the remaining classes unchanged.

          Show
          Luke Taylor added a comment - I've committed changes with a new RequestCache interface which handles storage and retrieval of the SavedRequest (and also of a request matching wrapper). The functionality has also been separated into a specific filter (RequestCacheAwareFilter) from the SecurityContextHolderAwareFilter as the requirements are orthogonal and there is no connection with providing servlet API support. They can thus be used independently and it makes the wrapper creation much simpler (configuration with a wrapper class and the use of reflection to create the wrapper is no longer supported as it would be trivial to insert an alternative filter). The default implementation of RequestCache is HttpSessionRequestCache. Properties such as allowSessionCreation and justUseSavedRequestOnGet have been moved to this class from ExceptionTranslationFilter. The SavedRequestWrapper use should also be simpler as it will not be used if there is no matching request in the cache. This class can therefore be modified to assume that the savedRequest it contains is not null. At the moment it has a lot of differing logic based on whether it is null or not. This was the cause of quite a few bugs in the past with forwarded requests etc. Hopefully things should now be much more straightforward. It remains to be seen whether we need a public SavedRequest interface/class at all since most of the logic is now within the RequestCache implementation. It may be preferable in order to allow people to customize the implementation to support POST bodies etc but using the remaining classes unchanged.
          Hide
          Luke Taylor added a comment -

          Postponing final work/design decisions till after M2.

          Show
          Luke Taylor added a comment - Postponing final work/design decisions till after M2.
          Hide
          Luke Taylor added a comment -

          SavedRequest is now an interface, with methods to access the request data and a getRedirectUrl() method to allow the authentication mechanism to obtain the URL it should redirect to. DefaultSavedRequest is the standard implementation (the equivalent of the original SavedRequest class).

          Show
          Luke Taylor added a comment - SavedRequest is now an interface, with methods to access the request data and a getRedirectUrl() method to allow the authentication mechanism to obtain the URL it should redirect to. DefaultSavedRequest is the standard implementation (the equivalent of the original SavedRequest class).

            People

            • Assignee:
              Luke Taylor
              Reporter:
              Luke Taylor
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: