diff --git a/java/com/google/gerrit/httpd/ContainerAuthFilter.java b/java/com/google/gerrit/httpd/ContainerAuthFilter.java index 07893ba7ea..ac66845a2e 100644 --- a/java/com/google/gerrit/httpd/ContainerAuthFilter.java +++ b/java/com/google/gerrit/httpd/ContainerAuthFilter.java @@ -31,6 +31,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; import java.util.Locale; +import java.util.Optional; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -98,13 +99,14 @@ class ContainerAuthFilter implements Filter { if (config.getBoolean("auth", "userNameToLowerCase", false)) { username = username.toLowerCase(Locale.US); } - final AccountState who = accountCache.getByUsername(username); - if (who == null || !who.getAccount().isActive()) { + Optional who = + accountCache.getByUsername(username).filter(a -> a.getAccount().isActive()); + if (!who.isPresent()) { rsp.sendError(SC_UNAUTHORIZED); return false; } WebSession ws = session.get(); - ws.setUserAccountId(who.getAccount().getId()); + ws.setUserAccountId(who.get().getAccount().getId()); ws.setAccessPathOk(AccessPath.GIT, true); ws.setAccessPathOk(AccessPath.REST_API, true); return true; diff --git a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java index 1c7d508dc6..55bd4d5937 100644 --- a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java +++ b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java @@ -36,6 +36,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; import java.util.Locale; +import java.util.Optional; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -127,8 +128,9 @@ class ProjectBasicAuthFilter implements Filter { username = username.toLowerCase(Locale.US); } - final AccountState who = accountCache.getByUsername(username); - if (who == null || !who.getAccount().isActive()) { + Optional accountState = + accountCache.getByUsername(username).filter(a -> a.getAccount().isActive()); + if (!accountState.isPresent()) { log.warn( "Authentication failed for " + username @@ -137,6 +139,7 @@ class ProjectBasicAuthFilter implements Filter { return false; } + AccountState who = accountState.get(); GitBasicAuthPolicy gitBasicAuthPolicy = authConfig.getGitBasicAuthPolicy(); if (gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP || gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP_LDAP) { @@ -157,7 +160,7 @@ class ProjectBasicAuthFilter implements Filter { setUserIdentified(whoAuthResult.getAccountId()); return true; } catch (NoSuchUserException e) { - if (who.checkPassword(password, who.getUserName())) { + if (who.checkPassword(password, username)) { return succeedAuthentication(who); } log.warn(authenticationFailedMsg(username, req), e); diff --git a/java/com/google/gerrit/httpd/ProjectOAuthFilter.java b/java/com/google/gerrit/httpd/ProjectOAuthFilter.java index b9105093d5..2b37378e08 100644 --- a/java/com/google/gerrit/httpd/ProjectOAuthFilter.java +++ b/java/com/google/gerrit/httpd/ProjectOAuthFilter.java @@ -25,6 +25,7 @@ import com.google.gerrit.extensions.auth.oauth.OAuthLoginProvider; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.registration.DynamicMap.Entry; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.AccessPath; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountException; @@ -40,6 +41,7 @@ import java.io.UnsupportedEncodingException; import java.net.URLDecoder; import java.util.Locale; import java.util.NoSuchElementException; +import java.util.Optional; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -151,8 +153,9 @@ class ProjectOAuthFilter implements Filter { return false; } - AccountState who = accountCache.getByUsername(authInfo.username); - if (who == null || !who.getAccount().isActive()) { + Optional who = + accountCache.getByUsername(authInfo.username).filter(a -> a.getAccount().isActive()); + if (!who.isPresent()) { log.warn( authenticationFailedMsg(authInfo.username, req) + ": account inactive or not provisioned in Gerrit"); @@ -160,9 +163,10 @@ class ProjectOAuthFilter implements Filter { return false; } + Account account = who.get().getAccount(); AuthRequest authRequest = AuthRequest.forExternalUser(authInfo.username); - authRequest.setEmailAddress(who.getAccount().getPreferredEmail()); - authRequest.setDisplayName(who.getAccount().getFullName()); + authRequest.setEmailAddress(account.getPreferredEmail()); + authRequest.setDisplayName(account.getFullName()); authRequest.setPassword(authInfo.tokenOrSecret); authRequest.setAuthPlugin(authInfo.pluginName); authRequest.setAuthProvider(authInfo.exportName); diff --git a/java/com/google/gerrit/server/account/AccountCache.java b/java/com/google/gerrit/server/account/AccountCache.java index 72c00d03a3..9474f8c4d3 100644 --- a/java/com/google/gerrit/server/account/AccountCache.java +++ b/java/com/google/gerrit/server/account/AccountCache.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.account; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; import java.io.IOException; +import java.util.Optional; /** Caches important (but small) account state to avoid database hits. */ public interface AccountCache { @@ -50,10 +51,9 @@ public interface AccountCache { * * @param username username of the account that should be retrieved * @return {@code AccountState} instance for the given username, if no account with this username - * exists or if loading the external ID fails {@code null} is returned + * exists or if loading the external ID fails {@link Optional#empty()} is returned */ - @Nullable - AccountState getByUsername(String username); + Optional getByUsername(String username); /** * Evicts the account from the cache and triggers a reindex for it. diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java index 57e14d614d..a78940236c 100644 --- a/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -98,14 +98,13 @@ public class AccountCacheImpl implements AccountCache { } @Override - @Nullable - public AccountState getByUsername(String username) { + public Optional getByUsername(String username) { try { ExternalId extId = externalIds.get(ExternalId.Key.create(SCHEME_USERNAME, username)); if (extId == null) { - return null; + return Optional.empty(); } - return getOrNull(extId.accountId()); + return Optional.ofNullable(getOrNull(extId.accountId())); } catch (IOException | ConfigInvalidException e) { log.warn("Cannot load AccountState for username " + username, e); return null; diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java index ae2bee63bb..7843f606fc 100644 --- a/java/com/google/gerrit/server/account/AccountResolver.java +++ b/java/com/google/gerrit/server/account/AccountResolver.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.account; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.stream.Collectors.toSet; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Streams; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.query.account.InternalAccountQuery; @@ -28,6 +29,7 @@ import java.io.IOException; import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -106,9 +108,9 @@ public class AccountResolver { } if (nameOrEmail.matches(Account.USER_NAME_PATTERN)) { - AccountState who = byId.getByUsername(nameOrEmail); - if (who != null) { - return Collections.singleton(who.getAccount().getId()); + Optional who = byId.getByUsername(nameOrEmail); + if (who.isPresent()) { + return ImmutableSet.of(who.map(a -> a.getAccount().getId()).get()); } } diff --git a/java/com/google/gerrit/server/auth/InternalAuthBackend.java b/java/com/google/gerrit/server/auth/InternalAuthBackend.java index 508bf31445..7ac06d829c 100644 --- a/java/com/google/gerrit/server/auth/InternalAuthBackend.java +++ b/java/com/google/gerrit/server/auth/InternalAuthBackend.java @@ -54,10 +54,10 @@ public class InternalAuthBackend implements AuthBackend { username = req.getUsername(); } - final AccountState who = accountCache.getByUsername(username); - if (who == null) { - throw new UnknownUserException(); - } else if (!who.getAccount().isActive()) { + AccountState who = + accountCache.getByUsername(username).orElseThrow(() -> new UnknownUserException()); + + if (!who.getAccount().isActive()) { throw new UserNotAllowedException( "Authentication failed for " + username diff --git a/java/com/google/gerrit/sshd/GerritGSSAuthenticator.java b/java/com/google/gerrit/sshd/GerritGSSAuthenticator.java index 8e4be7891e..adb50852f6 100644 --- a/java/com/google/gerrit/sshd/GerritGSSAuthenticator.java +++ b/java/com/google/gerrit/sshd/GerritGSSAuthenticator.java @@ -23,6 +23,7 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.inject.Inject; import com.google.inject.Singleton; import java.util.Locale; +import java.util.Optional; import org.apache.sshd.server.auth.gss.GSSAuthenticator; import org.apache.sshd.server.session.ServerSession; import org.eclipse.jgit.lib.Config; @@ -52,7 +53,7 @@ class GerritGSSAuthenticator extends GSSAuthenticator { @Override public boolean validateIdentity(ServerSession session, String identity) { - final SshSession sd = session.getAttribute(SshSession.KEY); + SshSession sd = session.getAttribute(SshSession.KEY); int at = identity.indexOf('@'); String username; if (at == -1) { @@ -63,18 +64,19 @@ class GerritGSSAuthenticator extends GSSAuthenticator { if (config.getBoolean("auth", "userNameToLowerCase", false)) { username = username.toLowerCase(Locale.US); } - AccountState state = accounts.getByUsername(username); - Account account = state == null ? null : state.getAccount(); - boolean active = account != null && account.isActive(); - if (active) { - return SshUtil.success( - username, - session, - sshScope, - sshLog, - sd, - SshUtil.createUser(sd, userFactory, account.getId())); + + Optional account = + accounts.getByUsername(username).map(AccountState::getAccount).filter(Account::isActive); + if (!account.isPresent()) { + return false; } - return false; + + return SshUtil.success( + username, + session, + sshScope, + sshLog, + sd, + SshUtil.createUser(sd, userFactory, account.get().getId())); } } diff --git a/java/com/google/gerrit/testing/FakeAccountCache.java b/java/com/google/gerrit/testing/FakeAccountCache.java index 58cc48fcad..454e51453a 100644 --- a/java/com/google/gerrit/testing/FakeAccountCache.java +++ b/java/com/google/gerrit/testing/FakeAccountCache.java @@ -23,6 +23,7 @@ import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.AllUsersNameProvider; import java.util.HashMap; import java.util.Map; +import java.util.Optional; /** Fake implementation of {@link AccountCache} for testing. */ public class FakeAccountCache implements AccountCache { @@ -50,9 +51,8 @@ public class FakeAccountCache implements AccountCache { } @Override - @Nullable - public synchronized AccountState getByUsername(String username) { - return byUsername.get(username); + public synchronized Optional getByUsername(String username) { + return Optional.ofNullable(byUsername.get(username)); } @Override diff --git a/plugins/singleusergroup b/plugins/singleusergroup index 73cfc73077..7dc0ce9c48 160000 --- a/plugins/singleusergroup +++ b/plugins/singleusergroup @@ -1 +1 @@ -Subproject commit 73cfc73077d249a5d92ae3d31f1949b816bd98c3 +Subproject commit 7dc0ce9c4868af2e3fcb5244ade8e19db9f0f1d4