Skip to content

Import refactor for SAML #2689

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Jan 25, 2024
Merged

Import refactor for SAML #2689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d313924
swalchemist Jan 18, 2024
fb80720
swalchemist Jan 22, 2024
970c651
swalchemist Jan 23, 2024
c75aee0
swalchemist Jan 23, 2024
5c105ae
swalchemist Jan 23, 2024
d0c83d5
refactor: import commonsIo directly
swalchemist Jan 23, 2024
0a75689
refactor: import apache.commons.httpclient directly
swalchemist Jan 23, 2024
c56dbfc
refactor: simplify code
swalchemist Jan 24, 2024
b33b09b
refactor: rename var
swalchemist Jan 24, 2024
1b7ae09
refactor: import owasp.esapi directly
swalchemist Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion dependencies.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

@strehle strehle Jan 25, 2024

Choose a reason for hiding this comment

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

Can you please add comments to these libraries... because httpClient is not needed finally so we need these libraries only for now

Copy link
Contributor Author

@swalchemist swalchemist Jan 25, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@strehle strehle Jan 25, 2024

Choose a reason for hiding this comment

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

So you're asking about commons-httpclient:commons-httpclient:3.1 not 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

Copy link
Contributor Author

@swalchemist swalchemist Jan 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@strehle strehle Jan 25, 2024

Choose a reason for hiding this comment

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

I know, but this will be obsolete ... however for me it is Ok as it is,

Copy link
Member

@strehle strehle Jan 25, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@swalchemist swalchemist Jan 25, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

@strehle strehle Jan 25, 2024

Choose a reason for hiding this comment

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

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....

Copy link
Contributor Author

@swalchemist swalchemist Jan 25, 2024

Choose a reason for hiding this comment

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

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.

https://marc.info/?l=httpclient-users&m=125425095705062&w=2

Copy link
Member

@peterhaochen47 peterhaochen47 Jan 25, 2024

Choose a reason for hiding this comment

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

We are going to plan to merge this PR. Created this issue to track the future work to replace the old httpclient lib: #2691

libraries.jodaTime = "joda-time:joda-time:2.12.6"
libraries.commonsHttpClient = "commons-httpclient:commons-httpclient:3.1"

// gradle plugins
libraries.testRetryPlugin = "org.gradle:test-retry-gradle-plugin:1.5.8"
Expand Down
6 changes: 5 additions & 1 deletion server/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ dependencies {
implementation(libraries.springSecurityCore)
implementation(libraries.springSecurityWeb)
implementation(libraries.springBootStarterMail)
implementation(libraries.spingSamlEsapiDependencyVersion) {
implementation(libraries.owaspEsapi) {
transitive = false
}
implementation(libraries.springSecuritySaml) {
exclude(module: "bcprov-ext-jdk15on")
exclude(module: "xalan")
}
implementation(libraries.jodaTime)
implementation(libraries.commonsHttpClient)
implementation(libraries.xmlSecurity)
implementation(libraries.springSessionJdbc)

Expand Down Expand Up @@ -116,6 +118,8 @@ dependencies {

testImplementation(libraries.jsonPathAssert)
testImplementation(libraries.guavaTestLib)

implementation(libraries.commonsIo)
}

configurations.all {
Expand Down
2 changes: 1 addition & 1 deletion .../cloudfoundry/identity/uaa/authentication/manager/ExternalLoginAuthenticationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.authentication.manager;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.StringUtils;
import org.cloudfoundry.identity.uaa.authentication.AccountNotPreCreatedException;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthentication;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails;
Expand Down
2 changes: 1 addition & 1 deletion ...main/java/org/cloudfoundry/identity/uaa/mfa/JdbcUserGoogleMfaCredentialsProvisioning.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.mfa;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.StringUtils;
import org.cloudfoundry.identity.uaa.audit.event.SystemDeletable;
import org.cloudfoundry.identity.uaa.cypto.EncryptionKeyService;
import org.cloudfoundry.identity.uaa.cypto.EncryptionServiceException;
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.cloudfoundry.identity.uaa.oauth.beans;

import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.ArrayUtils;
import org.apache.commons.lang3.StringUtils;
import org.cloudfoundry.identity.uaa.util.UaaUrlUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down
2 changes: 1 addition & 1 deletion ...ain/java/org/cloudfoundry/identity/uaa/provider/saml/LoginSamlAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.provider.saml;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.StringUtils;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthentication;
import org.cloudfoundry.identity.uaa.authentication.UaaPrincipal;
import org.cloudfoundry.identity.uaa.authentication.event.IdentityProviderAuthenticationSuccessEvent;
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/cloudfoundry/identity/uaa/user/JdbcUaaUserDatabase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.user;

import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang3.ArrayUtils;
import org.cloudfoundry.identity.uaa.util.beans.DbUtils;
import org.cloudfoundry.identity.uaa.db.DatabaseUrlModifier;
import org.cloudfoundry.identity.uaa.db.Vendor;
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/org/cloudfoundry/identity/uaa/zone/ZoneEndpointsClientDetailsValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.zone;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.StringUtils;
import org.cloudfoundry.identity.uaa.client.ClientDetailsValidator;
import org.cloudfoundry.identity.uaa.client.InvalidClientDetailsException;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/cloudfoundry/identity/uaa/db/TestSchemaValidation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.db;

import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang3.ArrayUtils;
import org.cloudfoundry.identity.uaa.annotations.WithDatabaseContext;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down
2 changes: 1 addition & 1 deletion ...rc/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/LimitSqlAdapterFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.resources.jdbc;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.StringUtils;
import org.cloudfoundry.identity.uaa.extensions.SpringProfileCleanupExtension;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.ExtensionContext;
Expand Down
2 changes: 1 addition & 1 deletion ...est/java/org/cloudfoundry/identity/uaa/scim/validate/UaaPasswordPolicyValidatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.scim.validate;

import org.apache.commons.lang.RandomStringUtils;
import org.apache.commons.lang3.RandomStringUtils;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.provider.IdentityProvider;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/cloudfoundry/identity/uaa/test/RandomParametersJunitExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.test;

import org.apache.commons.lang.RandomStringUtils;
import org.apache.commons.lang3.RandomStringUtils;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.ParameterContext;
import org.junit.jupiter.api.extension.ParameterResolutionException;
Expand Down
4 changes: 4 additions & 0 deletions uaa/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ dependencies {
testImplementation(libraries.tomcatJdbc)
testImplementation(libraries.springRestdocs)
testImplementation(libraries.greenmail)
testImplementation(libraries.jodaTime)
testImplementation(libraries.commonsIo)
testImplementation(libraries.commonsHttpClient)
testImplementation(libraries.owaspEsapi)
}

ext {
Expand Down
2 changes: 1 addition & 1 deletion uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/clients/ClientAdminEndpointDocs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.mock.clients;

import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang3.ArrayUtils;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.login.util.RandomValueStringGenerator;
import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants;
Expand Down
2 changes: 1 addition & 1 deletion ...rc/test/java/org/cloudfoundry/identity/uaa/mock/mfa_provider/MfaProviderEndpointDocs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.cloudfoundry.identity.uaa.mock.mfa_provider;

import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang3.ArrayUtils;
import org.cloudfoundry.identity.uaa.login.util.RandomValueStringGenerator;
import org.cloudfoundry.identity.uaa.mfa.GoogleMfaProviderConfig;
import org.cloudfoundry.identity.uaa.mfa.MfaProvider;
Expand Down
4 changes: 2 additions & 2 deletions .../test/java/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointDocs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.mock.providers;

import org.apache.commons.collections.map.HashedMap;
import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.collections4.map.HashedMap;
import org.apache.commons.lang3.ArrayUtils;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
import org.cloudfoundry.identity.uaa.integration.util.IntegrationTestUtils;
import org.cloudfoundry.identity.uaa.login.Prompt;
Expand Down
2 changes: 1 addition & 1 deletion ...a/org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsMockMvcTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
package org.cloudfoundry.identity.uaa.mock.providers;

import com.fasterxml.jackson.core.type.TypeReference;
import org.apache.commons.lang.RandomStringUtils;
import org.apache.commons.lang3.RandomStringUtils;
import org.cloudfoundry.identity.uaa.DefaultTestContext;
import org.cloudfoundry.identity.uaa.audit.AuditEventType;
import org.cloudfoundry.identity.uaa.constants.OriginKeys;
Expand Down
2 changes: 1 addition & 1 deletion uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/token/TokenMvcMockTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.cloudfoundry.identity.uaa.mock.token;

import com.fasterxml.jackson.core.type.TypeReference;
import org.apache.commons.collections.map.HashedMap;
import org.apache.commons.collections4.map.HashedMap;
import org.apache.commons.httpclient.util.URIUtil;
import org.cloudfoundry.identity.uaa.DefaultTestContext;
import org.cloudfoundry.identity.uaa.account.UserInfoResponse;
Expand Down
2 changes: 1 addition & 1 deletion uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/util/MockMvcUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import com.warrenstrange.googleauth.GoogleAuthenticator;
import com.warrenstrange.googleauth.GoogleAuthenticatorConfig;
import org.apache.commons.codec.binary.Base64;
import org.apache.commons.lang.RandomStringUtils;
import org.apache.commons.lang3.RandomStringUtils;
import org.cloudfoundry.identity.uaa.audit.event.AbstractUaaEvent;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthentication;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails;
Expand Down
3 changes: 1 addition & 2 deletions uaa/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenServicesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.appender.AbstractAppender;
import org.bouncycastle.util.Strings;
import org.cloudfoundry.identity.uaa.DefaultTestContext;
import org.cloudfoundry.identity.uaa.approval.Approval;
import org.cloudfoundry.identity.uaa.approval.JdbcApprovalStore;
Expand Down Expand Up @@ -230,7 +229,7 @@ void ensureAnIdTokenIsReturned() {

@Test
void ensureJKUHeaderIsSetWhenBuildingAnAccessToken() {
AuthorizationRequest authorizationRequest = constructAuthorizationRequest(clientId, GRANT_TYPE_CLIENT_CREDENTIALS, Strings.split(clientScopes, ','));
AuthorizationRequest authorizationRequest = constructAuthorizationRequest(clientId, GRANT_TYPE_CLIENT_CREDENTIALS, clientScopes.split(","));

OAuth2Authentication authentication = new OAuth2Authentication(authorizationRequest.createOAuth2Request(), null);

Expand Down
2 changes: 1 addition & 1 deletion ...test/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpointsMockMvcTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import com.fasterxml.jackson.core.type.TypeReference;
import com.google.common.collect.Lists;
import org.apache.commons.lang.RandomStringUtils;
import org.apache.commons.lang3.RandomStringUtils;
import org.apache.http.NameValuePair;
import org.apache.http.client.utils.URLEncodedUtils;
import org.cloudfoundry.identity.uaa.DefaultTestContext;
Expand Down