Fix OpenID delegate authentication
OpenID delegate support was accidentally broken back when we switched
to openid4java. Issue 38 claims it was broken in commit 818fa7568021
(v2.0.19~85) when we did a refactoring on how accounts are handled,
but I can't seem to fault that commit with the actual problem of
not honoring the claimed identity supplied by the user.
We now store both the claimed identity and the delegate identity into
the database. This makes it easier to link back to the same account
if the user enters through one or the other. It also permits the
us to only grant extended access to a user account if both their
claimed and delegate provider are trusted by the site administrator.
If an account has only the delegate identity (e.g. because it
was made before this fix was put into production use) we add the
claimed identity as an additional identity during the next login.
If an account has the claimed identity but not the delegate, we
add the delegate on the next login under the assumption that the
user has updated their delegation rule stored at the claimed address.
Bug: issue 38
Change-Id: Ie1e1265c94e5cafc75e8d71c9a1cbfa4df4337b7
Signed-off-by: Shawn O. Pearce <sop@google.com>
diff --git a/src/main/java/com/google/gerrit/server/account/AccountManager.java b/src/main/java/com/google/gerrit/server/account/AccountManager.java
index e8b47a5..0224403 100644
--- a/src/main/java/com/google/gerrit/server/account/AccountManager.java
+++ b/src/main/java/com/google/gerrit/server/account/AccountManager.java
@@ -50,14 +50,15 @@
}
/**
- * True if user identified by this external identity string has an account.
+ * @return user identified by this external identity string, or null.
*/
- public boolean exists(final String externalId) throws AccountException {
+ public Account.Id lookup(final String externalId) throws AccountException {
try {
final ReviewDb db = schema.open();
try {
- return db.accountExternalIds().get(
- new AccountExternalId.Key(externalId)) != null;
+ final AccountExternalId ext =
+ db.accountExternalIds().get(new AccountExternalId.Key(externalId));
+ return ext != null ? ext.getAccountId() : null;
} finally {
db.close();
}
diff --git a/src/main/java/com/google/gerrit/server/openid/OpenIdServiceImpl.java b/src/main/java/com/google/gerrit/server/openid/OpenIdServiceImpl.java
index 40fb987..02e2f44 100644
--- a/src/main/java/com/google/gerrit/server/openid/OpenIdServiceImpl.java
+++ b/src/main/java/com/google/gerrit/server/openid/OpenIdServiceImpl.java
@@ -20,6 +20,7 @@
import com.google.gerrit.client.auth.openid.DiscoveryResult;
import com.google.gerrit.client.auth.openid.OpenIdService;
import com.google.gerrit.client.auth.openid.OpenIdUtil;
+import com.google.gerrit.client.reviewdb.Account;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.UrlEncoded;
import com.google.gerrit.server.account.AccountException;
@@ -73,6 +74,7 @@
private static final String P_MODE = "gerrit.mode";
private static final String P_TOKEN = "gerrit.token";
private static final String P_REMEMBER = "gerrit.remember";
+ private static final String P_CLAIMED = "gerrit.claimed";
private static final int LASTID_AGE = 365 * 24 * 60 * 60; // seconds
private static final String OPENID_MODE = "openid.mode";
@@ -170,7 +172,7 @@
// We might already have this account on file. Look for it.
//
try {
- return !accountManager.exists(aReq.getIdentity());
+ return accountManager.lookup(aReq.getIdentity()) == null;
} catch (AccountException e) {
log.warn("Cannot determine if user account exists", e);
return true;
@@ -189,11 +191,14 @@
//
final SignInDialog.Mode mode = signInMode(req);
final String openidIdentifier = req.getParameter("openid.identity");
+ final String claimedIdentifier = req.getParameter(P_CLAIMED);
final String returnToken = req.getParameter(P_TOKEN);
final boolean remember = "1".equals(req.getParameter(P_REMEMBER));
+ final String rediscoverIdentifier =
+ claimedIdentifier != null ? claimedIdentifier : openidIdentifier;
final State state;
- state = init(openidIdentifier, mode, remember, returnToken);
+ state = init(rediscoverIdentifier, mode, remember, returnToken);
if (state == null) {
// Re-discovery must have failed, we can't run a login.
//
@@ -214,8 +219,7 @@
final VerificationResult result =
manager.verify(state.retTo.toString(), new ParameterList(req
.getParameterMap()), state.discovered);
- final Identifier user = result.getVerifiedId();
- if (user == null /* authentication failure */) {
+ if (result.getVerifiedId() == null /* authentication failure */) {
if ("Nonce verification failed.".equals(result.getStatusMsg())) {
// We might be suffering from clock skew on this system.
//
@@ -258,7 +262,7 @@
}
final com.google.gerrit.server.account.AuthRequest areq =
- new com.google.gerrit.server.account.AuthRequest(user.getIdentifier());
+ new com.google.gerrit.server.account.AuthRequest(openidIdentifier);
if (sregRsp != null) {
areq.setDisplayName(sregRsp.getAttributeValue("fullname"));
@@ -281,6 +285,51 @@
areq.setEmailAddress(fetchRsp.getAttributeValue("Email"));
}
+ if (claimedIdentifier != null) {
+ // The user used a claimed identity which has delegated to the verified
+ // 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());
+
+ if (claimedId != null && actualId != null) {
+ if (claimedId.equals(actualId)) {
+ // 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 "
+ + areq.getExternalId());
+ cancelWithError(req, rsp, "Contact site administrator");
+ return;
+ }
+
+ } else if (claimedId == null && actualId != null) {
+ // Older account, the actual was already created but the claimed
+ // was missing due to a bug in Gerrit. Link the claimed.
+ //
+ final com.google.gerrit.server.account.AuthRequest linkReq =
+ new com.google.gerrit.server.account.AuthRequest(claimedIdentifier);
+ linkReq.setDisplayName(areq.getDisplayName());
+ linkReq.setEmailAddress(areq.getEmailAddress());
+ accountManager.link(actualId, linkReq);
+
+ } else if (claimedId != null && actualId == null) {
+ // 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);
+
+ } else {
+ // Both are null, we are going to create a new account below.
+ }
+ }
+
try {
switch (mode) {
case REGISTER:
@@ -291,13 +340,21 @@
final Cookie lastId = new Cookie(OpenIdUtil.LASTID_COOKIE, "");
lastId.setPath(req.getContextPath() + "/");
if (remember) {
- lastId.setValue(user.getIdentifier());
+ lastId.setValue(rediscoverIdentifier);
lastId.setMaxAge(LASTID_AGE);
} else {
lastId.setMaxAge(0);
}
rsp.addCookie(lastId);
webSession.get().login(arsp.getAccountId(), remember);
+ if (arsp.isNew() && claimedIdentifier != null) {
+ final com.google.gerrit.server.account.AuthRequest linkReq =
+ new com.google.gerrit.server.account.AuthRequest(
+ claimedIdentifier);
+ linkReq.setDisplayName(areq.getDisplayName());
+ linkReq.setEmailAddress(areq.getEmailAddress());
+ accountManager.link(arsp.getAccountId(), linkReq);
+ }
callback(arsp.isNew(), req, rsp);
break;
@@ -392,6 +449,9 @@
if (remember) {
retTo.put(P_REMEMBER, "1");
}
+ if (discovered.hasClaimedIdentifier()) {
+ retTo.put(P_CLAIMED, discovered.getClaimedIdentifier().getIdentifier());
+ }
return new State(discovered, retTo, contextUrl);
}
