opendev/gerrit
Code Issues Proposed changes

Return Optional<Account.Id> from AccountManager.lookup(String)

Browse Source 
This makes it more clear that a lookup may find no account.

Change-Id: Ia0ffe7547c365be1526eb959ff4b5e25cfa1d37e
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2016-12-28 16:12:21 +01:00
parent 80f054f8b8
commit 45814b1fe3
4 changed files with 44 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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<Account.Id> claimedId = accountManager.lookup(claimedIdentifier);
Optional<Account.Id> 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;
}

22
gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java
View File

@ -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<Account.Id> actualId =
accountManager.lookup(user.getExternalId());
Optional<Account.Id> 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;
}

22
gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java
View File

@ -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<Account.Id> claimedId = accountManager.lookup(claimedIdentifier);
Optional<Account.Id> 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.

14
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
View File

@ -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<Account.Id> 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);