Spring Security
  1. Spring Security
  2. SEC-1648

Require explicit enabling of targetUrlParameter parameter in AbstractAuthenticationTargetUrlRequestHandler

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Complete
    • Affects Version/s: 3.1.0.M2
    • Fix Version/s: 3.1.0.RC1
    • Component/s: Web
    • Labels:
      None

      Description

      It's possible that an AuthenticationSuccessHandler or other class extending from this class may be used in scenarios where it isn't desirable to have a parameter used to determine the redirect location, though when it is invoked during the login request, this shouldn't be a problem.

      A user might create a subclass or use SimpleUrlAuthenticationSuccessHandler without realising that the parameter-based functionality exists. It would therefore probably be preferable to require that the functionality is explicitly enabled (as with the use of the "referer" header).

        Activity

        Hide
        Luke Taylor added a comment -

        I've added a useTargetUrlParameter property which defaults to false, thus requiring that the user explicitly enable this functionality.

        Show
        Luke Taylor added a comment - I've added a useTargetUrlParameter property which defaults to false, thus requiring that the user explicitly enable this functionality.
        Hide
        Rob Winch added a comment -

        Might we consider just flexing this if targetUrlParameter is null and defaulting it to null? This would reduce the state in the class. This does cause some additional passivity concerns, since targetUrlParameter has a getter. However, changing the default behaviour of the class (i.e. not using the parameter) is also a non-passive change. Another observation is that subclasses will likely have to inspect something (useTargetUrlParameter or targetUrlParameter!=null) in order to use AbstractAuthenticationTargetUrlRequestHandler without surprising results. This means that a getter for useTargetUrlParameter may need to be exposed if it is kept around. Just some thoughts (that may have already been considered).

        Show
        Rob Winch added a comment - Might we consider just flexing this if targetUrlParameter is null and defaulting it to null? This would reduce the state in the class. This does cause some additional passivity concerns, since targetUrlParameter has a getter. However, changing the default behaviour of the class (i.e. not using the parameter) is also a non-passive change. Another observation is that subclasses will likely have to inspect something (useTargetUrlParameter or targetUrlParameter!=null) in order to use AbstractAuthenticationTargetUrlRequestHandler without surprising results. This means that a getter for useTargetUrlParameter may need to be exposed if it is kept around. Just some thoughts (that may have already been considered).
        Hide
        Luke Taylor added a comment -

        Yes, I think that sounds like a better option.

        Show
        Luke Taylor added a comment - Yes, I think that sounds like a better option.
        Hide
        Luke Taylor added a comment -

        Re-implemented following Rob's suggestion. The default value is now null and the functionality is disabled unless the targetUrlParameter property is set.

        Show
        Luke Taylor added a comment - Re-implemented following Rob's suggestion. The default value is now null and the functionality is disabled unless the targetUrlParameter property is set.
        Hide
        Piotr Jagielski added a comment -

        It took me a while to figure out. There are a lot of tutorials and blog entries on the web that mention the parameter "spring-security-redirect". Is the fact that it needs to be explicitly enabled now and how to enable it in the configuration covered somewhere in the reference documentation?

        Show
        Piotr Jagielski added a comment - It took me a while to figure out. There are a lot of tutorials and blog entries on the web that mention the parameter "spring-security-redirect". Is the fact that it needs to be explicitly enabled now and how to enable it in the configuration covered somewhere in the reference documentation?

          People

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

            Dates

            • Created:
              Updated:
              Resolved: