Support display names with non-ISO-8859-1 characters The SAML plugin relied on request headers to pass the display name to Gerrit. This was problematic as Gerrit follows RFC-7230's recommendation [1] of using ISO-8859-1 characters - ideally US-ASCII only. This worked until change 94918 enforced ISO-8859-1 encoding in the display name header, and unintentionally (presumably, as there are no tests) also broke characters that are present but have different encodings in ISO-8859-1 and UTF-8 (a bug report was raised for this [2]). This change implements Luca's suggestion [3] not to use headers at all, but instead make a call via Gerrit's API once the user has been authenticated. This happens after login and circumvents the issue around header charsets. [1] https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.4 [2] https://issues.gerritcodereview.com/issues/296001714 [3] https://gerrit-review.googlesource.com/c/plugins/saml/+/369676/comments/2da3a9f3_1f3c9af3 Bug: Issue 40012596 Change-Id: I321cb9d17d8013a042ea7a4822a606dde8329303
diff --git a/BUILD b/BUILD index 3454255..00a847d 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", @@ -37,3 +38,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/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java index a58ef8a..5b5c969 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java +++ b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java
@@ -14,12 +14,19 @@ package com.googlesource.gerrit.plugins.saml; +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.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.GerritServerConfig; 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; @@ -60,10 +67,10 @@ 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; @@ -74,13 +81,19 @@ private final HashSet<String> authHeaders; private final boolean userNameToLowerCase; private final SamlMembership samlMembership; + private final GerritApi gApi; + private final Accounts accounts; + private final OneOffRequestContext oneOffRequestContext; @Inject SamlWebFilter( @GerritServerConfig Config gerritConfig, SitePaths sitePaths, SamlConfig samlConfig, - SamlMembership samlMembership) + SamlMembership samlMembership, + GerritApi gApi, + Accounts accounts, + OneOffRequestContext oneOffRequestContext) throws IOException { this.samlConfig = samlConfig; this.samlMembership = samlMembership; @@ -128,6 +141,10 @@ userNameToLowerCase = gerritConfig.getBoolean("auth", "userNameToLowerCase", false); saml2Client.setCallbackUrl(callbackUrl); + + this.gApi = gApi; + this.accounts = accounts; + this.oneOffRequestContext = oneOffRequestContext; } @Override @@ -164,6 +181,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);
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..98aa3b6 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java
@@ -0,0 +1,81 @@ +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" + + " httpDisplaynameHeader = X-SAML-DisplayName\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; + } + } +}