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;