Import refactor for SAML #2689
Import refactor for SAML #2689
Conversation
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/186908994 The labels on this github issue will be updated when the story is started. |
* Without this, we depend on spring-security-saml2-core to bring in joda-time. also: fix: build error on URIUtil.getName * ExternalOAuthAuthenticationFilter imports org.apache.commons.httpclient.util.URIUtil. We used to get it from a transitive dependency in the SAML extension library. * Now we list it directly as a dependency. Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
* We're not getting Commons Lang from the SAML extension library any more. * We already have Commons Lang 3, so just use that. Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
* Replaces the use of the library that we used to get as part of the SAML extension library. Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
* Fixes a compile error in YamlServletProfileInitializerTest Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
- as opposed to relying to springSecuritySaml to transitively bring it in [#186822654] Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
- as opposed to relying on springSecuritySaml to transitively bring it in - extract common var [#186822654] Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
- use Java native string split as opposed to bouncycastle's (which is likely a mistaken import) [#186822654] Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
[#186822654] Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
- as opposed to relying on springSecuritySaml to transitively bring it in [#186822654] Co-authored-by: Peter Chen <peter-h.chen@broadcom.com>
swalchemist
force-pushed
the
import-refactor-186822654
branch
from
16db5f4
to
1b7ae09
Compare
| @@ -128,7 +128,9 @@ libraries.zxing = "com.google.zxing:javase:3.5.2" | |||
| libraries.nimbusJwt = "com.nimbusds:nimbus-jose-jwt:9.37.3" | |||
| libraries.xmlSecurity = "org.apache.santuario:xmlsec:4.0.1" | |||
| libraries.orgJson = "org.json:json:20231013" | |||
| libraries.spingSamlEsapiDependencyVersion = "org.owasp.esapi:esapi:2.5.3.1" | |||
| libraries.owaspEsapi = "org.owasp.esapi:esapi:2.5.3.1" | |||
There was a problem hiding this comment.
Can you please add comments to these libraries... because httpClient is not needed finally so we need these libraries only for now
There was a problem hiding this comment.
So you're asking about commons-httpclient:commons-httpclient:3.1 not esapi?
While the develop branch works fine without this explicit dependency, on our branch that has the SAML extension library removed, the code doesn't compile without this httpclient dependency.
Specifically, it's needed in ExternalOAuthAuthenticationFilter.java
import org.apache.commons.httpclient.util.URIUtil;
...
final String origin = URIUtil.getName(String.valueOf(request.getRequestURL()));
And in TokenMvcMockTests.java in the invalidScopeErrorMessageIsNotShowingAllClientScopes and
invalidScopeErrorMessageIsNotShowingAllUserScopes test methods, for the same import.
So unless we decide to change this code to use a different library, the need to declare this dependency is not temporary.
Cc: @peterhaochen47
There was a problem hiding this comment.
So you're asking about
commons-httpclient:commons-httpclient:3.1not esapi?
Yes httpclient:3.1 is a very only version of apache http and this wont be needed if we have no opensaml2, so I recommend this to mark with a todo or comment to remove it later...
In our CVE scanners this library is excluded for many years now ...
For esapi I am not sure if newer opensaml4 has it or not
There was a problem hiding this comment.
Our intention was to declare the dependency that the develop branch was already using for this import. I'm trying to double-check what that is.
There was a problem hiding this comment.
I know, but this will be obsolete ... however for me it is Ok as it is,
There was a problem hiding this comment.
ent (see usage here)
the usage in our code is something we should refactor....
but the other usage cannot be refactored without opensaml upgrade, e.g.
https://github.com/search?q=repo%3Acloudfoundry%2Fuaa%20org.apache.commons.httpclient&type=code
There was a problem hiding this comment.
We looked in httpclient 4.5.14, and it doesn't seem to have the same API, and we didn't see a getName() method there at all, which is what we're using from httpclient 3.1. Do you see one there?
There was a problem hiding this comment.
https://mvnrepository.com/artifact/commons-httpclient/commons-httpclient
vs.
https://mvnrepository.com/artifact/org.apache.httpcomponents/httpclient
But we can close this discussion and you can proceed....
There was a problem hiding this comment.
I think in this particular case, our best option is to migrate to using java.net.URI to replace URIUtil.getName, which then removes the need for commons-httpclient:3.1. I tried to do that earlier and found that handling the checked exceptions that the library throws was a pain.
There was a problem hiding this comment.
We are going to plan to merge this PR. Created this issue to track the future work to replace the old httpclient lib: #2691
These refactors will prepare the code for removing the old Spring SAML extension library. There are several libraries that are brought in only as a result of having the SAML extension library. These changes decouple these libraries that aren't directly related to SAML, by declaring them explicitly.
Changes made with the help of @peterhaochen47.