Skip to content

Add Saml2AuthenticationRequestRepository #9185

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
jzheaux opened this issue Nov 3, 2020 · 9 comments · Fixed by #10060
Closed

Add Saml2AuthenticationRequestRepository #9185

jzheaux opened this issue Nov 3, 2020 · 9 comments · Fixed by #10060
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Nov 3, 2020

Related to #9174

It would be nice to remember the AuthNRequest so that data like InResponseTo can be validated.

To do this will require a new interface that both Saml2WebSsoAuthenticationRequestFilter and Saml2WebSsoAuthenticationFilter can use. The first will store the AuthNRequest and the second will read and remove it.

The interface will look something like this:

public interface Saml2AuthenticationRequestRepository<T extends AbstractSaml2AuthenticationRequest> {
    T loadAuthenticationRequest(HttpServletRequest request);

    void saveAuthenticationRequest(T authenticationRequest, HttpServletRequest request, HttpServletResponse response);

    T removeAuthenticationRequest(HttpServletRequest request, HttpServletResponse response);
}

And it should initially have a single implementation HttpSessionSaml2AuthenticationRequestRepository that stores the request in the session.

Saml2AuthenticationToken should also be altered to have a new constructor that takes the RelyingPartyRegistration, the SAML 2.0 response, and the AbstractSaml2AuthenticationRequest loaded from the repository.

@jzheaux jzheaux added type: enhancement A general enhancement in: saml2 An issue in SAML2 modules status: ideal-for-contribution An issue that we actively are looking for someone to help us with labels Nov 3, 2020
@fast-reflexes
Copy link
Contributor

@jzheaux Why are the responses needed? Also, can we assume that a session exists?

@jzheaux
Copy link
Contributor Author

jzheaux commented Nov 12, 2020

@fast-reflexes, thanks for the questions

Why are the responses needed?

I'm not sure I understand the question, are you referring to SAML 2.0 responses? They are needed as that is the response an IDP gives in order to complete the authentication process.

Also, can we assume that a session exists?

No, we should not assume it exists. But, if it does exist, then we can attempt to look up an associated AuthnRequest from it.

@fast-reflexes
Copy link
Contributor

@jzheaux Thanks for answering! Nah, I meant the responses in the function signatures of your interface? The SAML2 response is in the request and so is a potential session so why do we need to include the responses as argument? I can do it, just curious :)

Also in the Saml2WebSsoAuthenticationRequestFilter, if a session doesn't exist, we just don't save the request or do we take any measures to create a session there and then?

@jzheaux
Copy link
Contributor Author

jzheaux commented Nov 12, 2020

Ah, got it, @fast-reflexes.

The reason is from a lesson we learned in OAuth support. Most will store the AuthNrequest in the session, but some may want to store it in the browser, in a cookie, for example. Applications will need the HttpServletResponse to do that. The HttpSession implementation wouldn't use that parameter.

@jzheaux
Copy link
Contributor Author

jzheaux commented Dec 1, 2020

@fast-reflexes are you able to contribute a PR that adds this feature?

@fast-reflexes
Copy link
Contributor

@jzheaux yes, I have started working on it but a lot of other things going on as well. Also, this is my first PR for Spring Security so I haven't fully read up on the procedures. Nonetheless, the implementation itself is done, just need to ...

  • Fix existing tests
  • Possibly write new tests
  • Check guidelines and adhere to formatting and other stuff

@jzheaux jzheaux removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Dec 2, 2020
@jzheaux
Copy link
Contributor Author

jzheaux commented Dec 2, 2020

No problem, @fast-reflexes, take what time you need.

Regarding fixing existing tests, please do your best to leave them as-is. When adding a feature, all other behavior in Spring Security should remain the same.

@fast-reflexes
Copy link
Contributor

fast-reflexes commented Jan 28, 2021

@jzheaux I really want to work on this but setting up the tool suite to work properly is just a mess... I come from IntelliJ and I haven't used Eclipse in a long time and the few rows of text on how to set up the IDE is far from enough for me to get rolling. Currently, I have tried to set up the project both in IntelliJ and Spring Tool Suite but the error I'm getting is that all resources return 401's when trying to build. I find no information on this anywhere but I will look some more when I have time.. could you provide me with some slick hints? :)

@jzheaux
Copy link
Contributor Author

jzheaux commented Jan 28, 2021

Hi, @fast-reflexes, sorry to hear you are having trouble, let's see if I can help.

I personally use IntelliJ, and since you also come from an IntelliJ background, let's see if we can get that working.

Are you on the latest? Some infrastructural changes were recently made that required an adjustment in the way Spring Security manages its dependencies. You can check for this line in your build.gradle file that sets spring-build-conventions to version 0.0.37. Building using an earlier version of spring-build-conventions will cause the 401 behavior that you are describing.

If you are still getting 401s you might see if you are getting them on the command line, too.

marcusdacoregio added a commit to marcusdacoregio/spring-security that referenced this issue Jul 27, 2021
jzheaux pushed a commit that referenced this issue Jul 27, 2021
jzheaux added a commit that referenced this issue Jul 27, 2021
- Moved docs into AuthnRequest section, changed links to be more
semantically valuable to search engines
- Moved tests to be nearer to similar tests

Issue gh-9185
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
- Moved docs into AuthnRequest section, changed links to be more
semantically valuable to search engines
- Moved tests to be nearer to similar tests

Issue spring-projectsgh-9185
marcusdacoregio added a commit that referenced this issue Sep 29, 2021
Moving to solve package tangles

Issue gh-9185
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants