Relax final method implementations on AbstractRememberMeServices

[okohub]

Expected Behavior

AbstractRememberMeServices implemented RememberMeServices methods with final modifier. This actually blocks overriding these methods. We can relax strictness on method implementations for further extensions.

Current Behavior

Any class that extends AbstractRememberMeServices can not override RememberMeServices methods.

Context

Is this behaviour necessary? I am not seeing any advantage for making these methods final, on an "abstract" service.

I can submit a PR for this.

[sjohnr]

Thanks for reaching out @okohub! The preferred approach in Spring Security would be to start with a closed implementation and open up the API only when needed. I can't speak to the specific design considerations of AbstractRememberMeServices but it appears that there are many methods that can be overridden.

For example, loginFail cannot be, but cancelCookie and onLoginFail can be, which are the only two methods called within the final method. Can you provide more context behind your use case and a specific method you feel should not be final?

[okohub]

@sjohnr hi,

We actually tried to create a new custom "RememberMeServices" implementation for some specific cases.

"AbstractRememberMeServices" is base class for "RememberMeServices" logic. Even with a new custom implementation, extending "AbstractRememberMeServices" class is inevitable, because main logic is embedded into abstract class.
(This is also another improvement context btw)

So here is the thing; hooking into "RememberMeServices" methods which are public interface methods, is prohibited because of final keyword on override.

Another point; if I want to delegate some methods to existing implementations (like PersistentTokenBasedRememberMeServices), I can't do this because of "AbstractRememberMeServices" limitations. Just implementing "RememberMeServices" is useless in this way, which I pointed above.

I think these public interface methods should not be implemented as final in abstract classes. This makes an ambiguity IMHO.

[sjohnr]

Hi @okohub!

Just implementing "RememberMeServices" is useless in this way, which I pointed above.

Can you implement RememberMeServices and delegate to two implementations, both PersistentTokenBasedRememberMeServices and a custom one that extends AbstractRememberMeServices?

Another option would be to extend PersistentTokenBasedRememberMeServices instead. All of the methods it overrides from AbstractRememberMeServices can be overridden by your class. Could this work for your use case?

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[okohub]

@sjohnr Hi again! Sorry for late response.

Extending PersistentTokenBasedRememberMeServices is not solving our case. Actually we are making a transition to persistent repository implementation. So we need to support legacy token based implementation, but transform to persistent token result on actions. (login/logout etc.)

Relaxing final methods should help us a bit. We made a package name workaround to use some protected methods :) Let me share a bit of code:

package org.springframework.security.web.authentication.rememberme;

class MyService extends PersistentTokenBasedRememberMeServices {

...

  @Override
  protected UserDetails processAutoLoginCookie(String[] cookieTokens, HttpServletRequest request,
                                               HttpServletResponse response) {
     //if new cookie structure, go on                                          
    if (isPersistentRememberMeCookie(cookieTokens)) {
      return processPersistentAutoLoginCookie(cookieTokens[0]);
    }
    //if legacy, use legacy process to support user
    if (isNonPersistentRememberMeCookie(cookieTokens)) {
      return tokenBasedRememberMeServices.processAutoLoginCookie(cookieTokens, request, response);
    }
    throw new InvalidCookieException("Cookie does not resemble persistent or non persistent remember me cookie");
  }

...

}

We will get rid of these code blocks soon because transition is nearly completed. But I just wanted to find an improvement point to help other coders.

We can discuss more about, or you can close it if you don't see a significant point :)

[sjohnr]

Hi @okohub!

Relaxing final methods should help us a bit. We made a package name workaround to use some protected methods :)

I'm not sure I understand what you mean. In my testing, the protected methods of PersistentTokenBasedRememberMeServices (including processAutoLoginCookie) can be overridden by an extending class without the need to use a package-name workaround. It actually seems you are already using the suggestion I made above to extend PersistentTokenBasedRememberMeServices.

It even seems you're using delegation with TokenBasedRememberMeServices, similar to what I suggested. That would seem to confirm that it's possible to build everything you need without relaxing final methods on AbstractRememberMeServices. Am I misunderstanding your example code?

[cgdsyilmaz]

Hi @sjohnr!

I want to elaborate more on the package name workaround that @okohub mentioned. We are also trying to delegate our processAutoLoginCookie logic to TokenBasedRememberMeServices to support legacy cookies. However, doing the delegation in a class in an arbitrary package is impossible as the processAutoLoginCookie method of TokenBasedRememberMeServices is also protected.

[sjohnr]

Hi @cgdsyilmaz!

We are also trying to delegate our processAutoLoginCookie logic to TokenBasedRememberMeServices to support legacy cookies. However, doing the delegation in a class in an arbitrary package is impossible as the processAutoLoginCookie method of TokenBasedRememberMeServices is also protected.

I think I see, thanks. It sounds like there are a couple of different improvements being suggested here. To clarify @cgdsyilmaz, are you and @okohub working on separate implementations, or is this all part of the same effort?

@okohub would you mind summarizing the improvements you're suggesting with specific class names and changes in each class, etc.? If there are multiple things here, we can open additional enhancements for items unrelated to the subject of this issue "Relax final method implementations on AbstractRememberMeServices".

[cgdsyilmaz]

Hi @sjohnr!

I think I see, thanks. It sounds like there are a couple of different improvements being suggested here. To clarify @cgdsyilmaz, are you and @okohub working on separate implementations, or is this all part of the same effort?

We are working on the same implementation, this is all part of the same effort.

We can think of two ways of solving our problem:

• The first choice is relaxing the final method implementations on AbstractRememberMeServices which we still prefer. By doing so, we can extend the abstract class instead of extending concrete implementation (PersistentTokenBasedRememberMeServices).

• The second one is to continue extending PersistentTokenBasedRememberMeServices as you have proposed. However, overriding autoLogin is still not possible because of the final implementation.

Also, for us to remove the package name workaround, relaxing the final method implementations or changing the access modifier of processAutoLoginCookie are needed.

You can select one solution and inform us of your selection.

[sjohnr]

@cgdsyilmaz sorry for the delay, and thanks for the great summary!

After reading the summary and understanding your challenge (thanks for providing all the details) I agree with your first choice and think it would make the most sense. I wanted to make sure there wasn't another way, but you've convinced me 😉! Since it's an either/or, let's stick with your first suggestion and not open additional issues.

Would you or @okohub be interested in submitting a PR for this?

[okohub]

@sjohnr Cool!

I will prepare a pull request for this. Thank you for your support.

[okohub]

I'm linking PR to this.

#12530