AccountCache#getByUsername: Return Optional<AccountState>

This makes it more explicit that callers must handle the case where the
returned AccountState is absent.

Change-Id: Ib679cdd6d05f712c0d0c8780eb607780a4a0a79d
Signed-off-by: Edwin Kempin <ekempin@google.com>

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<AccountState> 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;

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> 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);

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<AccountState> 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);

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<AccountState> getByUsername(String username);
 
   /**
    * Evicts the account from the cache and triggers a reindex for it.

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<AccountState> 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;

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<AccountState> who = byId.getByUsername(nameOrEmail);
+      if (who.isPresent()) {
+        return ImmutableSet.of(who.map(a -> a.getAccount().getId()).get());
       }
     }
 

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

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) {
+
+    Optional<Account> account =
+        accounts.getByUsername(username).map(AccountState::getAccount).filter(Account::isActive);
+    if (!account.isPresent()) {
+      return false;
+    }
+
     return SshUtil.success(
         username,
         session,
         sshScope,
         sshLog,
         sd,
-          SshUtil.createUser(sd, userFactory, account.getId()));
-    }
-    return false;
+        SshUtil.createUser(sd, userFactory, account.get().getId()));
   }
 }

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<AccountState> getByUsername(String username) {
+    return Optional.ofNullable(byUsername.get(username));
   }
 
   @Override