OAuthTokenCache: Use account id as key

Current implementation assumes that OAuth provider always exposes
user name by using it for key in OAuth token cache. For some OAuth
providers (most notably Google OAuth provider) this is not the case.

Move from using user name to account id for token cache key. Postpone
populating the cache to the point when the authentication already took
place and use returned account id as cache key.

Bug: Issue 4627
Change-Id: I59f15b7c5ca8be6d52b59d21fac58cba88ba7fe3

gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java

@@ -62,6 +62,7 @@ class OAuthSession {
   private final OAuthTokenCache tokenCache;
   private OAuthServiceProvider serviceProvider;
   private OAuthUserInfo user;
+  private Account.Id accountId;
   private String redirectToken;
   private boolean linkMode;
 
@@ -80,7 +81,7 @@ class OAuthSession {
   }
 
   boolean isLoggedIn() {
-    return tokenCache.has(user);
+    return user != null;
   }
 
   boolean isOAuthFinal(HttpServletRequest request) {
@@ -101,13 +102,10 @@ class OAuthSession {
       OAuthToken token = oauth.getAccessToken(
           new OAuthVerifier(request.getParameter("code")));
       user = oauth.getUserInfo(token);
-      if (user != null && token != null) {
-        tokenCache.put(user, token);
-      }
 
       if (isLoggedIn()) {
         log.debug("Login-SUCCESS " + this);
-        authenticateAndRedirect(request, response);
+        authenticateAndRedirect(request, response, token);
         return true;
       }
       response.sendError(SC_UNAUTHORIZED);
@@ -128,7 +126,7 @@ class OAuthSession {
   }
 
   private void authenticateAndRedirect(HttpServletRequest req,
-      HttpServletResponse rsp) throws IOException {
+      HttpServletResponse rsp, OAuthToken token) throws IOException {
     AuthRequest areq = new AuthRequest(user.getExternalId());
     AuthResult arsp;
     try {
@@ -147,6 +145,9 @@ class OAuthSession {
       areq.setEmailAddress(user.getEmailAddress());
       areq.setDisplayName(user.getDisplayName());
       arsp = accountManager.authenticate(areq);
+
+      accountId = arsp.getAccountId();
+      tokenCache.put(accountId, token);
     } catch (AccountException e) {
       log.error("Unable to authenticate user \"" + user + "\"", e);
       rsp.sendError(HttpServletResponse.SC_FORBIDDEN);
@@ -215,7 +216,10 @@ class OAuthSession {
   }
 
   void logout() {
-    tokenCache.remove(user);
+    if (accountId != null) {
+      tokenCache.remove(accountId);
+      accountId = null;
+    }
     user = null;
     redirectToken = null;
     serviceProvider = null;
@@ -247,8 +251,8 @@ class OAuthSession {
 
   @Override
   public String toString() {
-    return "OAuthSession [token=" + tokenCache.get(user) + ", user=" + user
+    return "OAuthSession [token=" + tokenCache.get(accountId) + ", user="
-        + "]";
+        + user + "]";
   }
 
   public void setServiceProvider(OAuthServiceProvider provider) {

gerrit-server/src/main/java/com/google/gerrit/server/account/GetOAuthToken.java

@@ -18,6 +18,7 @@ import com.google.gerrit.extensions.auth.oauth.OAuthToken;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.auth.oauth.OAuthTokenCache;
 import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -52,16 +53,13 @@ class GetOAuthToken implements RestReadView<AccountResource>{
     if (self.get() != rsrc.getUser()) {
       throw new AuthException("not allowed to get access token");
     }
-    String username = rsrc.getUser().getAccount().getUserName();
+    Account a = rsrc.getUser().getAccount();
-    if (username == null) {
+    OAuthToken accessToken = tokenCache.get(a.getId());
-      throw new ResourceNotFoundException();
-    }
-    OAuthToken accessToken = tokenCache.get(username);
     if (accessToken == null) {
       throw new ResourceNotFoundException();
     }
     OAuthTokenInfo accessTokenInfo = new OAuthTokenInfo();
-    accessTokenInfo.username = username;
+    accessTokenInfo.username = a.getUserName();
     accessTokenInfo.resourceHost = hostName;
     accessTokenInfo.accessToken = accessToken.getToken();
     accessTokenInfo.providerId = accessToken.getProviderId();

gerrit-server/src/main/java/com/google/gerrit/server/auth/oauth/OAuthTokenCache.java

@@ -19,8 +19,8 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import com.google.common.cache.Cache;
 import com.google.gerrit.extensions.auth.oauth.OAuthToken;
 import com.google.gerrit.extensions.auth.oauth.OAuthTokenEncrypter;
-import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo;
 import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.cache.CacheModule;
 import com.google.inject.Inject;
 import com.google.inject.Module;
@@ -37,54 +37,39 @@ public class OAuthTokenCache {
     return new CacheModule() {
       @Override
       protected void configure() {
-        persist(OAUTH_TOKENS, String.class, OAuthToken.class);
+        persist(OAUTH_TOKENS, Account.Id.class, OAuthToken.class);
       }
     };
   }
 
-  private final Cache<String, OAuthToken> cache;
+  private final Cache<Account.Id, OAuthToken> cache;
 
   @Inject
-  OAuthTokenCache(@Named(OAUTH_TOKENS) Cache<String, OAuthToken> cache,
+  OAuthTokenCache(@Named(OAUTH_TOKENS) Cache<Account.Id, OAuthToken> cache,
       DynamicItem<OAuthTokenEncrypter> encrypter) {
     this.cache = cache;
     this.encrypter = encrypter;
   }
 
-  public boolean has(OAuthUserInfo user) {
+  public OAuthToken get(Account.Id id) {
-    return user != null
+    OAuthToken accessToken = cache.getIfPresent(id);
-      ? cache.getIfPresent(user.getUserName()) != null
-      : false;
-  }
-
-  public OAuthToken get(OAuthUserInfo user) {
-    return user != null
-      ? get(user.getUserName())
-      : null;
-  }
-
-  public OAuthToken get(String userName) {
-    OAuthToken accessToken = cache.getIfPresent(userName);
     if (accessToken == null) {
       return null;
     }
     accessToken = decrypt(accessToken);
     if (accessToken.isExpired()) {
-      cache.invalidate(userName);
+      cache.invalidate(id);
       return null;
     }
     return accessToken;
   }
 
-  public void put(OAuthUserInfo user, OAuthToken accessToken) {
+  public void put(Account.Id id, OAuthToken accessToken) {
-    cache.put(checkNotNull(user.getUserName()),
+    cache.put(id, encrypt(checkNotNull(accessToken)));
-        encrypt(checkNotNull(accessToken)));
   }
 
-  public void remove(OAuthUserInfo user) {
+  public void remove(Account.Id id) {
-    if (user != null) {
+    cache.invalidate(id);
-      cache.invalidate(user.getUserName());
-    }
   }
 
   private OAuthToken encrypt(OAuthToken token) {