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;
+ }
+ }
+}
