Factor out SamlClient creation The SAML client creation was completely done in the constructor of the filter. To make it more readable extract it into its own class. Change-Id: If938e283f2a338a3503f76c41d09261d1f0ed5ef
diff --git a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlClientProvider.java b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlClientProvider.java new file mode 100644 index 0000000..f5e593d --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlClientProvider.java
@@ -0,0 +1,91 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.googlesource.gerrit.plugins.saml; + +import static com.google.common.base.Preconditions.checkNotNull; +import static com.googlesource.gerrit.plugins.saml.SamlWebFilter.SAML; +import static com.googlesource.gerrit.plugins.saml.SamlWebFilter.SAML_CALLBACK; + +import com.google.common.base.Strings; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.server.config.CanonicalWebUrl; +import com.google.gerrit.server.config.SitePaths; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import org.pac4j.saml.client.SAML2Client; +import org.pac4j.saml.config.SAML2Configuration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Singleton +public class SamlClientProvider implements Provider<SAML2Client> { + private static final Logger log = LoggerFactory.getLogger(SamlClientProvider.class); + + private final SamlConfig samlConfig; + private final String canonicalUrl; + private final SitePaths sitePaths; + + @Inject + public SamlClientProvider( + @CanonicalWebUrl @Nullable String canonicalUrl, SitePaths sitePaths, SamlConfig samlConfig) { + this.samlConfig = samlConfig; + this.canonicalUrl = canonicalUrl; + this.sitePaths = sitePaths; + } + + @Override + public SAML2Client get() { + SAML2Configuration samlClientConfig = + new SAML2Configuration( + samlConfig.getKeystorePath(), samlConfig.getKeystorePassword(), + samlConfig.getPrivateKeyPassword(), samlConfig.getMetadataPath()); + + if (!Strings.isNullOrEmpty(samlConfig.getIdentityProviderEntityId())) { + if (!Strings.isNullOrEmpty(samlConfig.getServiceProviderEntityId())) { + log.warn( + "Both identityProviderEntityId as serviceProviderEntityId are set, ignoring serviceProviderEntityId."); + } + samlClientConfig.setIdentityProviderEntityId(samlConfig.getIdentityProviderEntityId()); + } else { + samlClientConfig.setServiceProviderMetadataPath( + ensureExists(sitePaths.data_dir).resolve("sp-metadata.xml").toString()); + if (!Strings.isNullOrEmpty(samlConfig.getServiceProviderEntityId())) { + samlClientConfig.setServiceProviderEntityId(samlConfig.getServiceProviderEntityId()); + } + } + + samlClientConfig.setUseNameQualifier(samlConfig.useNameQualifier()); + samlClientConfig.setMaximumAuthenticationLifetime(samlConfig.getMaxAuthLifetimeAttr()); + + SAML2Client saml2Client = new SAML2Client(samlClientConfig); + + checkNotNull(canonicalUrl, "gerrit.canonicalWebUrl must be set in gerrit.config"); + saml2Client.setCallbackUrl(canonicalUrl + SAML_CALLBACK); + + return saml2Client; + } + + private static Path ensureExists(Path dataDir) { + try { + return Files.createDirectories(dataDir.resolve(SAML)); + } catch (IOException e) { + throw new IllegalStateException("Unable to create data directory for the SAML-plugin.", e); + } + } +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java index 5714315..2796b08 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java +++ b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java
@@ -14,28 +14,21 @@ package com.googlesource.gerrit.plugins.saml; -import static com.google.common.base.Preconditions.checkNotNull; - import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Strings; import com.google.common.collect.Iterators; import com.google.common.collect.Sets; -import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Account; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.api.accounts.Accounts; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.server.config.AuthConfig; -import com.google.gerrit.server.config.CanonicalWebUrl; -import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.util.ManualRequestContext; import com.google.gerrit.server.util.OneOffRequestContext; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; @@ -58,7 +51,6 @@ import org.pac4j.core.exception.HttpAction; import org.pac4j.core.exception.TechnicalException; import org.pac4j.saml.client.SAML2Client; -import org.pac4j.saml.config.SAML2Configuration; import org.pac4j.saml.credentials.SAML2Credentials; import org.pac4j.saml.profile.SAML2Profile; import org.pac4j.saml.state.SAML2StateGenerator; @@ -71,8 +63,8 @@ private static final String GERRIT_LOGOUT = "/logout"; @VisibleForTesting static final String GERRIT_LOGIN = "/login"; - private static final String SAML = "saml"; - private static final String SAML_CALLBACK = "plugins/" + SAML + "/callback"; + public static final String SAML = "saml"; + public static final String SAML_CALLBACK = "plugins/" + SAML + "/callback"; @VisibleForTesting static final String SESSION_ATTR_USER = "Gerrit-Saml-User"; private final SAML2Client saml2Client; @@ -87,42 +79,19 @@ @Inject SamlWebFilter( AuthConfig auth, - @CanonicalWebUrl @Nullable String canonicalUrl, - SitePaths sitePaths, SamlConfig samlConfig, SamlMembership samlMembership, GerritApi gApi, Accounts accounts, - OneOffRequestContext oneOffRequestContext) - throws IOException { + OneOffRequestContext oneOffRequestContext, + Provider<SAML2Client> samlClientProvider) { this.auth = auth; this.samlConfig = samlConfig; this.samlMembership = samlMembership; log.debug("Max Authentication Lifetime: " + samlConfig.getMaxAuthLifetimeAttr()); - SAML2Configuration samlClientConfig = - new SAML2Configuration( - samlConfig.getKeystorePath(), samlConfig.getKeystorePassword(), - samlConfig.getPrivateKeyPassword(), samlConfig.getMetadataPath()); + this.saml2Client = samlClientProvider.get(); - if (!Strings.isNullOrEmpty(samlConfig.getIdentityProviderEntityId())) { - if (!Strings.isNullOrEmpty(samlConfig.getServiceProviderEntityId())) { - log.warn( - "Both identityProviderEntityId as serviceProviderEntityId are set, ignoring serviceProviderEntityId."); - } - samlClientConfig.setIdentityProviderEntityId(samlConfig.getIdentityProviderEntityId()); - } else { - samlClientConfig.setServiceProviderMetadataPath( - ensureExists(sitePaths.data_dir).resolve("sp-metadata.xml").toString()); - if (!Strings.isNullOrEmpty(samlConfig.getServiceProviderEntityId())) { - samlClientConfig.setServiceProviderEntityId(samlConfig.getServiceProviderEntityId()); - } - } - - samlClientConfig.setUseNameQualifier(samlConfig.useNameQualifier()); - samlClientConfig.setMaximumAuthenticationLifetime(samlConfig.getMaxAuthLifetimeAttr()); - - saml2Client = new SAML2Client(samlClientConfig); - authHeaders = + this.authHeaders = Sets.newHashSet( auth.getLoginHttpHeader().toUpperCase(), auth.getHttpEmailHeader().toUpperCase(), @@ -135,8 +104,6 @@ "Unique values for httpUserNameHeader, " + "httpEmailHeader and httpExternalIdHeader are required."); } - checkNotNull(canonicalUrl, "gerrit.canonicalWebUrl must be set in gerrit.config"); - saml2Client.setCallbackUrl(canonicalUrl + SAML_CALLBACK); this.gApi = gApi; this.accounts = accounts; @@ -317,10 +284,6 @@ return auth.isUserNameToLowerCase() ? username.toLowerCase(Locale.US) : username; } - private static Path ensureExists(Path dataDir) throws IOException { - return Files.createDirectories(dataDir.resolve(SAML)); - } - private class AuthenticatedHttpRequest extends HttpServletRequestWrapper { private AuthenticatedUser user;