Merge branch 'stable-3.6' into stable-3.7
* stable-3.6:
SamlWebFilterIT: throw Exception from tests
Do not set full name when not allowed by Realm
Do not swallow the user provisioning exception
Fix formatting issues
Do not load the SAML plugin when auth.httpDisplaynameHeader is set
Change-Id: I000922f4d26015821a5d01643bd24900f3f8c91a
diff --git a/src/main/java/com/googlesource/gerrit/plugins/saml/HttpServletBufferedStatusResponse.java b/src/main/java/com/googlesource/gerrit/plugins/saml/HttpServletBufferedStatusResponse.java
new file mode 100644
index 0000000..6c593df
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/saml/HttpServletBufferedStatusResponse.java
@@ -0,0 +1,68 @@
+// 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 java.io.IOException;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpServletResponseWrapper;
+
+class HttpServletBufferedStatusResponse extends HttpServletResponseWrapper {
+ private int status;
+ private String statusMsg;
+ private int error;
+ private String errorMsg;
+
+ HttpServletBufferedStatusResponse(HttpServletResponse response) {
+ super(response);
+ }
+
+ @Override
+ public void sendError(int sc) throws IOException {
+ sendError(sc, null);
+ }
+
+ @Override
+ public void sendError(int sc, String msg) throws IOException {
+ error = sc;
+ errorMsg = msg;
+ }
+
+ @Override
+ public void setStatus(int sc) {
+ setStatus(sc, null);
+ }
+
+ @Override
+ public void setStatus(int sc, String msg) {
+ status = sc;
+ statusMsg = msg;
+ }
+
+ void commit() throws IOException {
+ if (error > 0) {
+ if (errorMsg != null) {
+ super.sendError(error, errorMsg);
+ } else {
+ super.sendError(error);
+ }
+ } else if (status > 0) {
+ if (statusMsg != null) {
+ super.setStatus(status, statusMsg);
+ } else {
+ super.setStatus(status);
+ }
+ }
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlMembership.java b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlMembership.java
index 7b91f1e..1fd8a1f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlMembership.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlMembership.java
@@ -87,8 +87,7 @@
public void sync(AuthenticatedUser user, SAML2Profile profile) throws IOException {
Set<AccountGroup.UUID> samlMembership =
Optional.ofNullable((List<?>) profile.getAttribute(memberAttr, List.class))
- .orElse(Collections.emptyList())
- .stream()
+ .orElse(Collections.emptyList()).stream()
.map(m -> getOrCreateGroup(m.toString()))
.filter(Optional::isPresent)
.map(Optional::get)
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..2a73b44 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/saml/SamlWebFilter.java
@@ -24,14 +24,17 @@
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.client.AccountFieldName;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.server.account.Realm;
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.ProvisionException;
import com.google.inject.Singleton;
import java.io.IOException;
import java.nio.file.Files;
@@ -83,10 +86,12 @@
private final GerritApi gApi;
private final Accounts accounts;
private final OneOffRequestContext oneOffRequestContext;
+ private final boolean realmAllowsFullNameEditing;
@Inject
SamlWebFilter(
AuthConfig auth,
+ Realm realm,
@CanonicalWebUrl @Nullable String canonicalUrl,
SitePaths sitePaths,
SamlConfig samlConfig,
@@ -96,7 +101,13 @@
OneOffRequestContext oneOffRequestContext)
throws IOException {
this.auth = auth;
+ if (auth.getHttpDisplaynameHeader() != null) {
+ throw new ProvisionException(
+ "auth.httpdisplaynameheader is not compatible with SAML: remove the config and restart");
+ }
+
this.samlConfig = samlConfig;
+ this.realmAllowsFullNameEditing = realm.allowsEdit(AccountFieldName.FULL_NAME);
this.samlMembership = samlMembership;
log.debug("Max Authentication Lifetime: " + samlConfig.getMaxAuthLifetimeAttr());
SAML2Configuration samlClientConfig =
@@ -176,13 +187,22 @@
redirectToIdentityProvider(context);
} 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);
+
+ if (realmAllowsFullNameEditing) {
+ HttpServletBufferedStatusResponse respWrapper =
+ new HttpServletBufferedStatusResponse(httpResponse);
+ chain.doFilter(req, respWrapper);
+ try (ManualRequestContext ignored =
+ oneOffRequestContext.openAs(
+ Account.id(accounts.id(user.getUsername()).get()._accountId))) {
+ gApi.accounts().id(user.getUsername()).setName(user.getDisplayName());
+ respWrapper.commit();
+ } catch (RestApiException e) {
+ log.error("Saml plugin could not set account name", e);
+ httpResponse.sendError(HttpServletResponse.SC_FORBIDDEN);
+ }
+ } else {
+ chain.doFilter(req, httpResponse);
}
}
} else if (isGerritLogout(httpRequest)) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java b/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java
index 7701b46..0b99d26 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java
@@ -16,19 +16,36 @@
import static com.google.common.base.Strings.nullToEmpty;
import static com.google.common.truth.Truth.assertThat;
+import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static javax.servlet.http.HttpServletResponse.SC_OK;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.api.accounts.Accounts;
+import com.google.gerrit.extensions.client.AccountFieldName;
import com.google.gerrit.extensions.common.AccountDetailInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.AccountsUpdate;
+import com.google.gerrit.server.account.Realm;
+import com.google.gerrit.server.config.AuthConfig;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.util.http.testutil.FakeHttpServletRequest;
import com.google.gerrit.util.http.testutil.FakeHttpServletResponse;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import com.google.inject.Provider;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
import javax.servlet.FilterChain;
-import javax.servlet.ServletException;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -37,6 +54,8 @@
public class SamlWebFilterIT extends AbstractDaemonTest {
+ @Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdateProvider;
+
@ConfigSuite.Default
public static Config setupSaml() throws ConfigInvalidException {
Config cfg = new Config();
@@ -56,7 +75,7 @@
}
@Test
- public void supportAccountNamesWithNonIso88591Characters() throws IOException, ServletException, RestApiException {
+ public void supportAccountNamesWithNonIso88591Characters() throws Exception {
SamlWebFilter samlWebFilter = server.getTestInjector().getInstance(SamlWebFilter.class);
String samlDisplayName = nullToEmpty(user.displayName()) + " Saml Test 合覺那加情力心";
@@ -70,14 +89,116 @@
req.setPathInfo(SamlWebFilter.GERRIT_LOGIN);
HttpServletResponse res = new FakeHttpServletResponse();
- samlWebFilter.doFilter(req, res, mock(FilterChain.class));
-
+ samlWebFilter.doFilter(req, res, mockFilterReturningStatusOK());
assertThat(res.getStatus()).isEqualTo(SC_OK);
AccountDetailInfo account = gApi.accounts().id(user.username()).detail();
assertThat(account.name).isEqualTo(samlDisplayName);
}
+ @Test
+ public void failAuthenticationWhenAccountManipulationFails() throws Exception {
+ SamlWebFilter samlWebFilter =
+ newSamlWebFilter(
+ server.getTestInjector().getInstance(Realm.class),
+ newGerritApiMockFailingOnAccountsApi());
+ List<Integer> responseStatuses = new ArrayList<>();
+
+ HttpServletResponse res =
+ new FakeHttpServletResponse() {
+ @Override
+ public synchronized void setStatus(int sc) {
+ super.setStatus(sc);
+ responseStatuses.add(sc);
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public synchronized void setStatus(int sc, String msg) {
+ super.setStatus(sc, msg);
+ responseStatuses.add(sc);
+ }
+
+ @Override
+ public synchronized void sendError(int sc) {
+ super.sendError(sc);
+ responseStatuses.add(sc);
+ }
+
+ @Override
+ public synchronized void sendError(int sc, String msg) {
+ super.sendError(sc, msg);
+ responseStatuses.add(sc);
+ }
+ };
+
+ samlWebFilter.doFilter(newFinalLoginFakeHttpRequest(), res, mockFilterReturningStatusOK());
+
+ assertThat(res.getStatus()).isEqualTo(SC_FORBIDDEN);
+ assertThat(responseStatuses).containsExactly(SC_FORBIDDEN);
+ }
+
+ private FilterChain mockFilterReturningStatusOK() {
+ return (request, response) ->
+ ((HttpServletResponse) response).setStatus(HttpServletResponse.SC_OK);
+ }
+
+ private FakeHttpServletRequest newFinalLoginFakeHttpRequest() {
+ HttpSession httpSession = mock(HttpSession.class);
+ AuthenticatedUser authenticatedUser =
+ new AuthenticatedUser(user.username(), "User Fullname", user.email(), "externalId");
+ doReturn(authenticatedUser).when(httpSession).getAttribute(SamlWebFilter.SESSION_ATTR_USER);
+ FakeHttpServletRequest req = new FakeHttpServletRequestWithSession(httpSession);
+ req.setPathInfo(SamlWebFilter.GERRIT_LOGIN);
+ return req;
+ }
+
+ private GerritApi newGerritApiMockFailingOnAccountsApi() throws RestApiException {
+ GerritApi apiMock = mock(GerritApi.class);
+ Accounts accountsApiMock = mock(Accounts.class);
+ doReturn(accountsApiMock).when(apiMock).accounts();
+ doThrow(RestApiException.class).when(accountsApiMock).id(any());
+ return apiMock;
+ }
+
+ private SamlWebFilter newSamlWebFilter(Realm realm, GerritApi gerritApi) throws IOException {
+ Injector testInjector = server.getTestInjector();
+ return new SamlWebFilter(
+ testInjector.getInstance(AuthConfig.class),
+ realm,
+ "",
+ testInjector.getInstance(SitePaths.class),
+ testInjector.getInstance(SamlConfig.class),
+ testInjector.getInstance(SamlMembership.class),
+ gerritApi,
+ testInjector.getInstance(Accounts.class),
+ testInjector.getInstance(OneOffRequestContext.class));
+ }
+
+ @Test
+ public void shouldSucceedAndNotSetFullNameWhenNotAllowedByRealm() throws Exception {
+ Realm realmMock = mock(Realm.class);
+ doReturn(false).when(realmMock).allowsEdit(eq(AccountFieldName.FULL_NAME));
+ SamlWebFilter samlWebFilter = newSamlWebFilter(realmMock, gApi);
+
+ String samlDisplayName = "Test display name";
+
+ 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).isNotEqualTo(samlDisplayName);
+ }
+
private static class FakeHttpServletRequestWithSession extends FakeHttpServletRequest {
HttpSession session;
