From 45814b1fe3baddb03bd8e27ae2f900afdea17028 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 28 Dec 2016 16:12:21 +0100 Subject: [PATCH] Return Optional from AccountManager.lookup(String) This makes it more clear that a lookup may find no account. Change-Id: Ia0ffe7547c365be1526eb959ff4b5e25cfa1d37e Signed-off-by: Edwin Kempin --- .../gerrit/httpd/auth/oauth/OAuthSession.java | 21 +++++++++--------- .../auth/openid/OAuthSessionOverOpenID.java | 22 ++++++++++--------- .../httpd/auth/openid/OpenIdServiceImpl.java | 22 ++++++++++--------- .../gerrit/server/account/AccountManager.java | 14 +++++++----- 4 files changed, 44 insertions(+), 35 deletions(-) diff --git a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java index 35f79c942f..9deec4434b 100644 --- a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java +++ b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java @@ -44,6 +44,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; +import java.util.Optional; import javax.servlet.ServletRequest; import javax.servlet.http.HttpServletRequest; @@ -165,10 +166,10 @@ class OAuthSession { private boolean authenticateWithIdentityClaimedDuringHandshake( AuthRequest req, HttpServletResponse rsp, String claimedIdentifier) throws AccountException, IOException { - Account.Id claimedId = accountManager.lookup(claimedIdentifier); - Account.Id actualId = accountManager.lookup(user.getExternalId()); - if (claimedId != null && actualId != null) { - if (claimedId.equals(actualId)) { + Optional claimedId = accountManager.lookup(claimedIdentifier); + Optional actualId = accountManager.lookup(user.getExternalId()); + if (claimedId.isPresent() && actualId.isPresent()) { + if (claimedId.get().equals(actualId.get())) { // Both link to the same account, that's what we expected. log.debug("OAuth2: claimed identity equals current id"); } else { @@ -176,23 +177,23 @@ class OAuthSession { // for what might be the same user. // log.error("OAuth accounts disagree over user identity:\n" - + " Claimed ID: " + claimedId + " is " + claimedIdentifier - + "\n" + " Delgate ID: " + actualId + " is " + + " Claimed ID: " + claimedId.get() + " is " + claimedIdentifier + + "\n" + " Delgate ID: " + actualId.get() + " is " + user.getExternalId()); rsp.sendError(HttpServletResponse.SC_FORBIDDEN); return false; } - } else if (claimedId != null && actualId == null) { + } else if (claimedId.isPresent() && !actualId.isPresent()) { // Claimed account already exists: link to it. // log.info("OAuth2: linking claimed identity to {}", - claimedId.toString()); + claimedId.get().toString()); try { - accountManager.link(claimedId, req); + accountManager.link(claimedId.get(), req); } catch (OrmException e) { log.error("Cannot link: " + user.getExternalId() + " to user identity:\n" - + " Claimed ID: " + claimedId + " is " + claimedIdentifier); + + " Claimed ID: " + claimedId.get() + " is " + claimedIdentifier); rsp.sendError(HttpServletResponse.SC_FORBIDDEN); return false; } diff --git a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java index 67ac8959f0..dccb6f7a2e 100644 --- a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java +++ b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java @@ -43,6 +43,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; +import java.util.Optional; import javax.servlet.ServletRequest; import javax.servlet.http.HttpServletRequest; @@ -122,8 +123,9 @@ class OAuthSessionOverOpenID { AuthResult arsp = null; try { String claimedIdentifier = user.getClaimedIdentity(); - Account.Id actualId = accountManager.lookup(user.getExternalId()); - Account.Id claimedId = null; + Optional actualId = + accountManager.lookup(user.getExternalId()); + Optional claimedId = Optional.empty(); // We try to retrieve claimed identity. // For some reason, for example staging instance @@ -133,17 +135,17 @@ class OAuthSessionOverOpenID { // That why we query it here, not to lose linking mode. if (!Strings.isNullOrEmpty(claimedIdentifier)) { claimedId = accountManager.lookup(claimedIdentifier); - if (claimedId == null) { + if (!claimedId.isPresent()) { log.debug("Claimed identity is unknown"); } } // Use case 1: claimed identity was provided during handshake phase // and user account exists for this identity - if (claimedId != null) { + if (claimedId.isPresent()) { log.debug("Claimed identity is set and is known"); - if (actualId != null) { - if (claimedId.equals(actualId)) { + if (actualId.isPresent()) { + if (claimedId.get().equals(actualId.get())) { // Both link to the same account, that's what we expected. log.debug("Both link to the same account. All is fine."); } else { @@ -151,8 +153,8 @@ class OAuthSessionOverOpenID { // for what might be the same user. The admin would have to // link the accounts manually. log.error("OAuth accounts disagree over user identity:\n" - + " Claimed ID: " + claimedId + " is " + claimedIdentifier - + "\n" + " Delgate ID: " + actualId + " is " + + " Claimed ID: " + claimedId.get() + " is " + claimedIdentifier + + "\n" + " Delgate ID: " + actualId.get() + " is " + user.getExternalId()); rsp.sendError(HttpServletResponse.SC_FORBIDDEN); return; @@ -161,11 +163,11 @@ class OAuthSessionOverOpenID { // Claimed account already exists: link to it. log.debug("Claimed account already exists: link to it."); try { - accountManager.link(claimedId, areq); + accountManager.link(claimedId.get(), areq); } catch (OrmException e) { log.error("Cannot link: " + user.getExternalId() + " to user identity:\n" - + " Claimed ID: " + claimedId + " is " + claimedIdentifier); + + " Claimed ID: " + claimedId.get() + " is " + claimedIdentifier); rsp.sendError(HttpServletResponse.SC_FORBIDDEN); return; } diff --git a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java index 36947a9e86..f3130e7a4d 100644 --- a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java +++ b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java @@ -62,6 +62,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.net.URL; import java.util.List; +import java.util.Optional; import java.util.concurrent.TimeUnit; import javax.servlet.http.Cookie; @@ -364,25 +365,26 @@ class OpenIdServiceImpl { // identity we have in our AuthRequest above. We still should have a // link between the two, so set one up if not present. // - Account.Id claimedId = accountManager.lookup(claimedIdentifier); - Account.Id actualId = accountManager.lookup(areq.getExternalId()); + Optional claimedId = accountManager.lookup(claimedIdentifier); + Optional actualId = + accountManager.lookup(areq.getExternalId()); - if (claimedId != null && actualId != null) { - if (claimedId.equals(actualId)) { + if (claimedId.isPresent() && actualId.isPresent()) { + if (claimedId.get().equals(actualId.get())) { // Both link to the same account, that's what we expected. } else { // This is (for now) a fatal error. There are two records // for what might be the same user. // log.error("OpenID accounts disagree over user identity:\n" - + " Claimed ID: " + claimedId + " is " + claimedIdentifier - + "\n" + " Delgate ID: " + actualId + " is " + + " Claimed ID: " + claimedId.get() + " is " + claimedIdentifier + + "\n" + " Delgate ID: " + actualId.get() + " is " + areq.getExternalId()); cancelWithError(req, rsp, "Contact site administrator"); return; } - } else if (claimedId == null && actualId != null) { + } else if (!claimedId.isPresent() && actualId.isPresent()) { // Older account, the actual was already created but the claimed // was missing due to a bug in Gerrit. Link the claimed. // @@ -390,14 +392,14 @@ class OpenIdServiceImpl { new com.google.gerrit.server.account.AuthRequest(claimedIdentifier); linkReq.setDisplayName(areq.getDisplayName()); linkReq.setEmailAddress(areq.getEmailAddress()); - accountManager.link(actualId, linkReq); + accountManager.link(actualId.get(), linkReq); - } else if (claimedId != null && actualId == null) { + } else if (claimedId.isPresent() && !actualId.isPresent()) { // Claimed account already exists, but it smells like the user has // changed their delegate to point to a different provider. Link // the new provider. // - accountManager.link(claimedId, areq); + accountManager.link(claimedId.get(), areq); } else { // Both are null, we are going to create a new account below. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java index 3e5a0467b9..674273ef15 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java @@ -45,6 +45,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; /** Tracks authentication related details for user accounts. */ @@ -90,22 +91,25 @@ public class AccountManager { } /** - * @return user identified by this external identity string, or null. + * @return user identified by this external identity string */ - public Account.Id lookup(String externalId) throws AccountException { + public Optional lookup(String externalId) + throws AccountException { try { if (accountIndexes.getSearchIndex() != null) { AccountState accountState = accountQueryProvider.get().oneByExternalId(externalId); return accountState != null - ? accountState.getAccount().getId() - : null; + ? Optional.of(accountState.getAccount().getId()) + : Optional.empty(); } try (ReviewDb db = schema.open()) { AccountExternalId ext = db.accountExternalIds().get(new AccountExternalId.Key(externalId)); - return ext != null ? ext.getAccountId() : null; + return ext != null + ? Optional.of(ext.getAccountId()) + : Optional.empty(); } } catch (OrmException e) { throw new AccountException("Cannot lookup account " + externalId, e);