Skip to content

Added support for the CAS gateway feature. #9881

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

yvangg
Copy link

@yvangg yvangg commented Jun 8, 2021

This is the CAS gateway feature.
This is a implementation based on the following discussion (PR): #40

I did some modifications in order to perform the CAS gateway redirection only if the CAS cookie is present on the client application request. Of course this will not fit in all situations (where CAS domain is different from client applications won't work), but for many of situations this is a pretty good approach.

Regards.

@pivotal-cla
Copy link

@yvangg Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@yvangg Thank you for signing the Contributor License Agreement!

@yvangg
Copy link
Author

yvangg commented Aug 16, 2021

Do we have any news about this? Why is not being merged?

@rwinch rwinch added in: cas An issue in spring-security-cas type: enhancement A general enhancement labels Nov 16, 2021
@rwinch rwinch removed their assignment Nov 16, 2021
@marcusdacoregio
Copy link
Contributor

Hi @yvangg.

The CAS module has been removed in Spring Security 6.0. See #10441.

So, for now, we have to wait to see if the java-cas-client is going to support Jakarta EE 9.

@yvangg
Copy link
Author

yvangg commented Nov 17, 2021

Hi, @marcusdacoregio I understand that you have removed that in Spring Security 6.0, but, Why don't you accept this request on the 5.5.x branch?

Our applications are using spring-security 5.5.x and we do not have in mind update to 6.0 soon.

It will be helpfull have support for the CAS gateway feature.

@marcusdacoregio marcusdacoregio self-assigned this Nov 17, 2021
@marcusdacoregio marcusdacoregio removed the status: waiting-for-triage An issue we've not yet triaged label Nov 17, 2021
@marcusdacoregio
Copy link
Contributor

Hi @yvangg. Unfortunately, we can't add new features to older branches, like 5.5.x.

We are gonna start planning features for the 5.7 release next month, so I'll add this PR to the next milestone and we can start working on this by that time.

@marcusdacoregio marcusdacoregio added this to the 5.7.0-M1 milestone Nov 17, 2021
@marcusdacoregio marcusdacoregio removed their assignment Nov 17, 2021
@sjohnr sjohnr modified the milestones: 5.7.0-M1, 5.7.0-M2 Jan 14, 2022
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've posted on here some changes. In general, I think the original PR was closer to what should be used. We just needed the Cookies to prevent making too many requests.

HttpServletResponse response = (HttpServletResponse) res;

if (requestMatcher.matches(request)) {
throw new TriggerCasGatewayException("Try a CAS gateway authentication");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a failed authentication attempt, so a subclass of AuthenticationException should not be thrown here

@@ -25,6 +25,7 @@

import org.springframework.beans.factory.InitializingBean;
import org.springframework.security.cas.ServiceProperties;
import org.springframework.security.cas.authentication.TriggerCasGatewayException;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Among other tangles, this introduces a tangle between between org.springframework.security.cas.authentication and org.springframework.security.cas.web. In general, authentication package should not rely on web.

@marcusdacoregio marcusdacoregio modified the milestones: 5.7.0-M2, 5.7.0-M3 Feb 21, 2022
@marcusdacoregio marcusdacoregio modified the milestones: 5.7.0-M3, 5.7.0-RC1 Mar 21, 2022
@marcusdacoregio
Copy link
Contributor

@yvangg have you had the chance to see @rwinch review and apply the suggested changes? Thanks!

@rwinch rwinch modified the milestones: 5.7.0-RC1, 5.8.x Apr 18, 2022
@rwinch rwinch removed this from the 5.8.x milestone Jun 9, 2022
@marcusdacoregio
Copy link
Contributor

I'll close this since we do not have more minor releases planned for 5.x and the CAS support has been removed in Spring Security 6. See #10441

@marcusdacoregio marcusdacoregio added the status: declined A suggestion or change that we don't feel we should currently apply label Nov 17, 2022
@Emkas
Copy link
Contributor

Emkas commented Nov 17, 2022

But there is also a ticket to re-add CAS... So why do not link to #11674.

This is an important feature and if I remember correctly original issue about it has probably +10 years.

@marcusdacoregio
Copy link
Contributor

@Emkas, I've just linked this PR with #11674. When the support is re-added this can be reconsidered. Thanks for the reminder.

@Emkas
Copy link
Contributor

Emkas commented Nov 18, 2022

Ok, I'll subscribe #11674.

@Emkas
Copy link
Contributor

Emkas commented Mar 8, 2023

#11674 is closed and CAS is re-added. Please, consider opening this issue again.

@marcusdacoregio
Copy link
Contributor

Hi @Emkas, I think creating a new PR targeting the main branch is the recommended approach here since it is a new feature. Also, the new PR should contain the changes requested by @rwinch. Is @yvangg still willing to work on this?

@yvangg
Copy link
Author

yvangg commented Mar 8, 2023

Hi @marcusdacoregio, I am willing to work on this. But probably I will need to test again the changes in the current 6.1.x branch, In addition, I did not receive any answer from @rwinch about some of my comments. I will checkout the main branch and I will test it again, if everything works as I expect I will do a new PR.

@SandwichCZ
Copy link

Hi, are the are news regarding this issue? This would be a quite helpful feature to have and it seems like there is interest in implementing it. Perhaps re-opening the ticket should be considered.

@marcusdacoregio
Copy link
Contributor

Hi @SandwichCZ, we haven't received any news from @yvangg yet and there is none from the team either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: cas An issue in spring-security-cas status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants