Merge branch 'stable-3.4' * stable-3.4: Add missing copyright header to SamlWebFilterIT.java Remove httpDisplaynameHeader config option Support display names with non-ISO-8859-1 characters Change-Id: I80f3619e05364f9bc2eef1df2194e98596b219f3
diff --git a/BUILD b/BUILD index a13b9ed..36550aa 100644 --- a/BUILD +++ b/BUILD
@@ -1,4 +1,5 @@ -load("//tools/bzl:plugin.bzl", "gerrit_plugin") +load("//tools/bzl:plugin.bzl", "PLUGIN_TEST_DEPS", "gerrit_plugin") +load("//tools/bzl:junit.bzl", "junit_tests") gerrit_plugin( name = "saml", @@ -38,3 +39,13 @@ "@woodstox-core//jar", ], ) + +junit_tests( + name = "saml_tests", + srcs = glob(["src/test/java/**/*.java"]), + tags = ["saml"], + deps = PLUGIN_TEST_DEPS + [ + ":saml__plugin", + "//javatests/com/google/gerrit/util/http/testutil", + ], +)
diff --git a/README.md b/README.md index 3575060..e9fa614 100644 --- a/README.md +++ b/README.md
@@ -94,7 +94,6 @@ type = HTTP logoutUrl = https://mysso.example.com/logout httpHeader = X-SAML-UserName - httpDisplaynameHeader = X-SAML-DisplayName httpEmailHeader = X-SAML-EmailHeader httpExternalIdHeader = X-SAML-ExternalId ```
diff --git a/adfs/README.md b/adfs/README.md index 693838c..c589619 100644 --- a/adfs/README.md +++ b/adfs/README.md
@@ -17,7 +17,6 @@ type = HTTP_LDAP logoutUrl = https://fs.hc.sct/adfs/ls/?wa=wsignout1.0 httpHeader = X-SAML-UserName - httpDisplaynameHeader = X-SAML-DisplayName httpEmailHeader = X-SAML-EmailHeader httpExternalIdHeader = X-SAML-ExternalId [saml]
diff --git a/keycloak/README.md b/keycloak/README.md index bf9939d..c88f2c4 100644 --- a/keycloak/README.md +++ b/keycloak/README.md
@@ -38,7 +38,6 @@ type = HTTP logoutUrl = http://localhost:8080/auth/realms/master/protocol/openid-connect/logout httpHeader = X-SAML-UserName - httpDisplaynameHeader = X-SAML-DisplayName httpEmailHeader = X-SAML-EmailHeader httpExternalIdHeader = X-SAML-ExternalId
diff --git a/simplesamlphp/README.md b/simplesamlphp/README.md index 44646f4..3f15824 100644 --- a/simplesamlphp/README.md +++ b/simplesamlphp/README.md
@@ -43,7 +43,6 @@ type = HTTP logoutUrl = http://localhost:8080/simplesaml/saml2/idp/SingleLogoutService.php?ReturnTo=http://localhost:8081 httpHeader = X-SAML-UserName - httpDisplaynameHeader = X-SAML-DisplayName httpEmailHeader = X-SAML-EmailHeader httpExternalIdHeader = X-SAML-ExternalId autoUpdateAccountActiveStatus = true
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 7e51f36..5714315 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java +++ b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java
@@ -16,14 +16,21 @@ 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.Singleton; import java.io.IOException; @@ -63,16 +70,19 @@ private static final Logger log = LoggerFactory.getLogger(SamlWebFilter.class); private static final String GERRIT_LOGOUT = "/logout"; - private static final String GERRIT_LOGIN = "/login"; + @VisibleForTesting static final String GERRIT_LOGIN = "/login"; private static final String SAML = "saml"; private static final String SAML_CALLBACK = "plugins/" + SAML + "/callback"; - private static final String SESSION_ATTR_USER = "Gerrit-Saml-User"; + @VisibleForTesting static final String SESSION_ATTR_USER = "Gerrit-Saml-User"; private final SAML2Client saml2Client; private final SamlConfig samlConfig; private final AuthConfig auth; private final HashSet<String> authHeaders; private final SamlMembership samlMembership; + private final GerritApi gApi; + private final Accounts accounts; + private final OneOffRequestContext oneOffRequestContext; @Inject SamlWebFilter( @@ -80,7 +90,10 @@ @CanonicalWebUrl @Nullable String canonicalUrl, SitePaths sitePaths, SamlConfig samlConfig, - SamlMembership samlMembership) + SamlMembership samlMembership, + GerritApi gApi, + Accounts accounts, + OneOffRequestContext oneOffRequestContext) throws IOException { this.auth = auth; this.samlConfig = samlConfig; @@ -112,20 +125,22 @@ authHeaders = Sets.newHashSet( auth.getLoginHttpHeader().toUpperCase(), - auth.getHttpDisplaynameHeader().toUpperCase(), auth.getHttpEmailHeader().toUpperCase(), auth.getHttpExternalIdHeader().toUpperCase()); if (authHeaders.contains("") || authHeaders.contains(null)) { throw new RuntimeException("All authentication headers must be set."); } - if (authHeaders.size() != 4) { + if (authHeaders.size() != 3) { throw new RuntimeException( "Unique values for httpUserNameHeader, " - + "httpDisplaynameHeader, httpEmailHeader and httpExternalIdHeader " - + "are required."); + + "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; + this.oneOffRequestContext = oneOffRequestContext; } @Override @@ -162,6 +177,13 @@ } else { HttpServletRequest req = new AuthenticatedHttpRequest(httpRequest, user); chain.doFilter(req, response); + try (ManualRequestContext ignored = + oneOffRequestContext.openAs( + Account.id(accounts.id(user.getUsername()).get()._accountId))) { + gApi.accounts().id(user.getUsername()).setName(user.getDisplayName()); + } catch (RestApiException e) { + log.error("Saml plugin could not set account name", e); + } } } else if (isGerritLogout(httpRequest)) { httpRequest.getSession().removeAttribute(SESSION_ATTR_USER); @@ -322,8 +344,6 @@ String nameUpperCase = name.toUpperCase(); if (auth.getLoginHttpHeader().toUpperCase().equals(nameUpperCase)) { return user.getUsername(); - } else if (auth.getHttpDisplaynameHeader().toUpperCase().equals(nameUpperCase)) { - return user.getDisplayName(); } else if (auth.getHttpEmailHeader().toUpperCase().equals(nameUpperCase)) { return user.getEmail(); } else if (auth.getHttpExternalIdHeader().toUpperCase().equals(nameUpperCase)) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java b/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java new file mode 100644 index 0000000..7701b46 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java
@@ -0,0 +1,94 @@ +// 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.Strings.nullToEmpty; +import static com.google.common.truth.Truth.assertThat; +import static javax.servlet.http.HttpServletResponse.SC_OK; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.extensions.common.AccountDetailInfo; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.testing.ConfigSuite; +import com.google.gerrit.util.http.testutil.FakeHttpServletRequest; +import com.google.gerrit.util.http.testutil.FakeHttpServletResponse; +import java.io.IOException; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; +import org.junit.Test; + +public class SamlWebFilterIT extends AbstractDaemonTest { + + @ConfigSuite.Default + public static Config setupSaml() throws ConfigInvalidException { + Config cfg = new Config(); + cfg.fromText( + "" + + "[httpd]\n" + + " filterClass = com.googlesource.gerrit.plugins.saml.SamlWebFilter\n" + + "[saml]\n" + + " keystorePath = etc/samlKeystore.jks\n" + + " metadataPath = http://localhost:8080/auth/realms/master/protocol/saml/descriptor\n" + + "[auth]\n" + + " type = HTTP\n" + + " httpHeader = X-SAML-UserName\n" + + " httpEmailHeader = X-SAML-EmailHeader\n" + + " httpExternalIdHeader = X-SAML-ExternalId"); + return cfg; + } + + @Test + public void supportAccountNamesWithNonIso88591Characters() throws IOException, ServletException, RestApiException { + SamlWebFilter samlWebFilter = server.getTestInjector().getInstance(SamlWebFilter.class); + + String samlDisplayName = nullToEmpty(user.displayName()) + " Saml Test 合覺那加情力心"; + + HttpSession httpSession = mock(HttpSession.class); + AuthenticatedUser authenticatedUser = + new AuthenticatedUser(user.username(), samlDisplayName, user.email(), "externalId"); + doReturn(authenticatedUser).when(httpSession).getAttribute(SamlWebFilter.SESSION_ATTR_USER); + + FakeHttpServletRequest req = new FakeHttpServletRequestWithSession(httpSession); + req.setPathInfo(SamlWebFilter.GERRIT_LOGIN); + HttpServletResponse res = new FakeHttpServletResponse(); + + samlWebFilter.doFilter(req, res, mock(FilterChain.class)); + + assertThat(res.getStatus()).isEqualTo(SC_OK); + + AccountDetailInfo account = gApi.accounts().id(user.username()).detail(); + assertThat(account.name).isEqualTo(samlDisplayName); + } + + private static class FakeHttpServletRequestWithSession extends FakeHttpServletRequest { + HttpSession session; + + public FakeHttpServletRequestWithSession(HttpSession session) { + super(); + this.session = session; + } + + @Override + public HttpSession getSession() { + return session; + } + } +}