SEC-1341: CasProcessingFilterEntryPoint more extensible #1571

Closed
spring-issuemaster opened this Issue Dec 21, 2009 · 5 comments

1 participant

@spring-issuemaster

Sébastien Launay (Migrated from SEC-1341) said:

I need to extend CasAuthenticationEntryPoint in order to add additional parameter to the CAS server.

Because all the code reside in one method I need to duplicate the code just to change the redirected URL.
Moreover the encodeServiceUrlWithSessionId property is not accessible therefore I need to do that:
public class CustomCasProcessingFilterEntryPoint extends CasProcessingFilterEntryPoint {
private boolean encodeServiceUrlWithSessionId;

...

public void setEncodeServiceUrlWithSessionId(final boolean encodeServiceUrlWithSessionId) {
super.setEncodeServiceUrlWithSessionId(encodeServiceUrlWithSessionId);
this.encodeServiceUrlWithSessionId = encodeServiceUrlWithSessionId;
}
}

Patch against trunk attached to provide more extensibility.

@spring-issuemaster

Luke Taylor said:

I don't think you should need encodeServiceUrlWithSessionId if you are using an up to date CAS server. There are only 3 lines in the commence method so writing a custom end point should be trivial.

@spring-issuemaster

Sébastien Launay said:

Yes encodeServiceUrlWithSessionId is not necessary anymore but in order to properly extends the entry point I wanted to keep the existing features.
I agree also that there are only 3 lines but here also I wanted to keep the same code for an easy maintenance in case of a (unlikely) modification or maybe a refactoring.

My custom entry point is working it's just that my implementation can be less intrusive ;).

@spring-issuemaster

Luke Taylor said:

Extending a class is really more fragile in terms of the impact of potential modifications to the parent class and that's one of the main reasons we prefer to encourage people to implement interfaces directly. It gives us more freedom to make changes to the existing implementations without worrying about breaking existing code.

In fact, this raises the possibility that we should maybe deprecate this property for 3.0, since most people are probably already on a CAS version that doesn't require it, certainly in the case of new developments.

Scott will be able to comment more on this. What do you think Scott? And when is CAS 4.0 due ;).

@spring-issuemaster

Scott Battaglia said:

Yes, we should be able to deprecate that property. I'll take a look at it now actually, and see if I can work any magic to make the class more extensible too (no promises)

@spring-issuemaster

Scott Battaglia said:

all, please check out what I just committed for this issue. Luke, not sure what version we'd target this for so I didn't mark it as resolved.

@spring-issuemaster spring-issuemaster added this to the 3.0.1 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment