Do not set full name when not allowed by Realm
Gerrit API for setting the full name may not be allowed
by the authentication Realm, therefore the execution of
the REST-API to set that information from the SAML
plugin may fail.
It is better to avoid setting the full name rather
than having a failed authentication: the underlying Realm
in Gerrit will have a way to populate the missing information.
Change-Id: I4a5687df90c177551cf405ff09e99ab26c659d86
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 cb6b769..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,8 +24,10 @@
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;
@@ -84,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,
@@ -103,6 +107,7 @@
}
this.samlConfig = samlConfig;
+ this.realmAllowsFullNameEditing = realm.allowsEdit(AccountFieldName.FULL_NAME);
this.samlMembership = samlMembership;
log.debug("Max Authentication Lifetime: " + samlConfig.getMaxAuthLifetimeAttr());
SAML2Configuration samlClientConfig =
@@ -183,17 +188,21 @@
} else {
HttpServletRequest req = new AuthenticatedHttpRequest(httpRequest, user);
- 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);
+ 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 c1e41e5..e12e4a9 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/saml/SamlWebFilterIT.java
@@ -18,7 +18,8 @@
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.Mockito.any;
+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;
@@ -26,15 +27,21 @@
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;
@@ -48,6 +55,8 @@
public class SamlWebFilterIT extends AbstractDaemonTest {
+ @Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdateProvider;
+
@ConfigSuite.Default
public static Config setupSaml() throws ConfigInvalidException {
Config cfg = new Config();
@@ -92,7 +101,10 @@
@Test
public void failAuthenticationWhenAccountManipulationFails()
throws IOException, ServletException, RestApiException {
- SamlWebFilter samlWebFilter = newSamlWebFilter(newGerritApiMockFailingOnAccountsApi());
+ SamlWebFilter samlWebFilter =
+ newSamlWebFilter(
+ server.getTestInjector().getInstance(Realm.class),
+ newGerritApiMockFailingOnAccountsApi());
List<Integer> responseStatuses = new ArrayList<>();
HttpServletResponse res =
@@ -152,10 +164,11 @@
return apiMock;
}
- private SamlWebFilter newSamlWebFilter(GerritApi gerritApi) throws IOException {
+ 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),
@@ -165,6 +178,30 @@
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;
