Fix --re=bob to match bob@example.com when using HTTP_LDAP
When we introduced HTTP_LDAP support we broke expansion of the local
username in situations like the --reviewer flag during receive. This
was because the EmailExpander wasn't configured, as we usually rely on
the LDAP directory to return the address for us.
Bug: GERRIT-263
Change-Id: I8c8d5ce9e5ac1cd1a8b1dc85422257b00e1114ab
Signed-off-by: Shawn O. Pearce <sop@google.com>
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index aac7f87..13384e1 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -281,6 +281,12 @@
low maxAge setting, to ensure LDAP modifications are picked up in
a timely fashion.
+cache `"ldap_usernames"`::
++
+Caches a mapping of LDAP username to Gerrit account identity. The
+cache automatically updates when a user first creates their account
+within Gerrit, so the cache expire time is largely irrelevant.
+
cache `"openid"`::
+
If OpenID authentication is enabled, caches the OpenID discovery
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 299f546..d45110e 100644
--- a/src/main/java/com/google/gerrit/server/account/AccountManager.java
+++ b/src/main/java/com/google/gerrit/server/account/AccountManager.java
@@ -221,6 +221,7 @@
txn.commit();
byEmailCache.evict(account.getPreferredEmail());
+ realm.onCreateAccount(who, account);
return new AuthResult(newId, true);
}
diff --git a/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/src/main/java/com/google/gerrit/server/account/AccountResolver.java
index 3e229ee..ff36fae 100644
--- a/src/main/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/src/main/java/com/google/gerrit/server/account/AccountResolver.java
@@ -25,16 +25,15 @@
import java.util.Set;
public class AccountResolver {
- private final EmailExpander emailExpander;
+ private final Realm realm;
private final AccountByEmailCache byEmail;
private final AccountCache byId;
private final Provider<ReviewDb> schema;
@Inject
- AccountResolver(final EmailExpander emailExpander,
- final AccountByEmailCache byEmail, final AccountCache byId,
- final Provider<ReviewDb> schema) {
- this.emailExpander = emailExpander;
+ AccountResolver(final Realm realm, final AccountByEmailCache byEmail,
+ final AccountCache byId, final Provider<ReviewDb> schema) {
+ this.realm = realm;
this.byEmail = byEmail;
this.byId = byId;
this.schema = schema;
@@ -64,11 +63,9 @@
return findByEmail(nameOrEmail);
}
- if (emailExpander.canExpand(nameOrEmail)) {
- final Account a = findByEmail(emailExpander.expand(nameOrEmail));
- if (a != null) {
- return a;
- }
+ final Account.Id id = realm.lookup(nameOrEmail);
+ if (id != null) {
+ return byId.get(id).getAccount();
}
return oneAccount(schema.get().accounts().byFullName(nameOrEmail));
diff --git a/src/main/java/com/google/gerrit/server/account/DefaultRealm.java b/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
index 9a833dd..5dbd445 100644
--- a/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
+++ b/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
@@ -22,10 +22,13 @@
public final class DefaultRealm implements Realm {
private final EmailExpander emailExpander;
+ private final AccountByEmailCache byEmail;
@Inject
- DefaultRealm(final EmailExpander emailExpander) {
+ DefaultRealm(final EmailExpander emailExpander,
+ final AccountByEmailCache byEmail) {
this.emailExpander = emailExpander;
+ this.byEmail = byEmail;
}
@Override
@@ -43,7 +46,22 @@
}
@Override
+ public void onCreateAccount(final AuthRequest who, final Account account) {
+ }
+
+ @Override
public Set<AccountGroup.Id> groups(final AccountState who) {
return who.getInternalGroups();
}
+
+ @Override
+ public Account.Id lookup(final String accountName) {
+ if (emailExpander.canExpand(accountName)) {
+ final Set<Account.Id> c = byEmail.get(emailExpander.expand(accountName));
+ if (1 == c.size()) {
+ return c.iterator().next();
+ }
+ }
+ return null;
+ }
}
diff --git a/src/main/java/com/google/gerrit/server/account/Realm.java b/src/main/java/com/google/gerrit/server/account/Realm.java
index 37ba044..129a977 100644
--- a/src/main/java/com/google/gerrit/server/account/Realm.java
+++ b/src/main/java/com/google/gerrit/server/account/Realm.java
@@ -25,5 +25,17 @@
public AuthRequest authenticate(AuthRequest who) throws AccountException;
+ public void onCreateAccount(AuthRequest who, Account account);
+
public Set<AccountGroup.Id> groups(AccountState who);
+
+ /**
+ * Locate an account whose local username is the given account name.
+ * <p>
+ * Generally this only works for local realms, such as one backed by an LDAP
+ * directory, or where there is an {@link EmailExpander} configured that knows
+ * how to convert the accountName into an email address, and then locate the
+ * user by that email address.
+ */
+ public Account.Id lookup(String accountName);
}
diff --git a/src/main/java/com/google/gerrit/server/ldap/LdapModule.java b/src/main/java/com/google/gerrit/server/ldap/LdapModule.java
index b708b77..9305756 100644
--- a/src/main/java/com/google/gerrit/server/ldap/LdapModule.java
+++ b/src/main/java/com/google/gerrit/server/ldap/LdapModule.java
@@ -16,6 +16,7 @@
import static java.util.concurrent.TimeUnit.HOURS;
+import com.google.gerrit.client.reviewdb.Account;
import com.google.gerrit.client.reviewdb.AccountGroup;
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.cache.Cache;
@@ -26,14 +27,18 @@
import java.util.Set;
public class LdapModule extends CacheModule {
+ static final String USERNAME_CACHE = "ldap_usernames";
static final String GROUP_CACHE = "ldap_groups";
@Override
protected void configure() {
- final TypeLiteral<Cache<String, Set<AccountGroup.Id>>> type =
+ final TypeLiteral<Cache<String, Set<AccountGroup.Id>>> groups =
new TypeLiteral<Cache<String, Set<AccountGroup.Id>>>() {};
+ final TypeLiteral<Cache<String, Account.Id>> usernames =
+ new TypeLiteral<Cache<String, Account.Id>>() {};
- core(type, GROUP_CACHE).timeToIdle(1, HOURS).timeToLive(1, HOURS);
+ core(groups, GROUP_CACHE).timeToIdle(1, HOURS).timeToLive(1, HOURS);
+ core(usernames, USERNAME_CACHE);
bind(Realm.class).to(LdapRealm.class).in(Scopes.SINGLETON);
}
}
diff --git a/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java b/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java
index 90f7297..2d0d2a0 100644
--- a/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java
+++ b/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java
@@ -17,6 +17,7 @@
import com.google.gerrit.client.reviewdb.Account;
import com.google.gerrit.client.reviewdb.AccountExternalId;
import com.google.gerrit.client.reviewdb.AccountGroup;
+import com.google.gerrit.client.reviewdb.ReviewDb;
import com.google.gerrit.server.account.AccountException;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.AuthRequest;
@@ -26,6 +27,8 @@
import com.google.gerrit.server.cache.Cache;
import com.google.gerrit.server.cache.SelfPopulatingCache;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gwtorm.client.OrmException;
+import com.google.gwtorm.client.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.google.inject.name.Named;
@@ -57,11 +60,13 @@
private final String username;
private final String password;
+ private final SchemaFactory<ReviewDb> schema;
private final EmailExpander emailExpander;
private final String accountFullName;
private final String accountEmailAddress;
private final String accountSshUserName;
private final LdapQuery accountQuery;
+ private final SelfPopulatingCache<String, Account.Id> usernameCache;
private final GroupCache groupCache;
private final String groupName;
@@ -73,10 +78,13 @@
LdapRealm(
final GroupCache groupCache,
final EmailExpander emailExpander,
+ final SchemaFactory<ReviewDb> schema,
@Named(LdapModule.GROUP_CACHE) final Cache<String, Set<AccountGroup.Id>> rawGroup,
+ @Named(LdapModule.USERNAME_CACHE) final Cache<String, Account.Id> rawUsername,
@GerritServerConfig final Config config) {
- this.emailExpander = emailExpander;
this.groupCache = groupCache;
+ this.emailExpander = emailExpander;
+ this.schema = schema;
this.server = required(config, "server");
this.username = optional(config, "username");
@@ -138,6 +146,13 @@
if (accountQuery.getParameters().length == 0) {
throw new IllegalArgumentException("No variables in ldap.accountPattern");
}
+
+ usernameCache = new SelfPopulatingCache<String, Account.Id>(rawUsername) {
+ @Override
+ public Account.Id createEntry(final String username) throws Exception {
+ return queryForUsername(username);
+ }
+ };
}
private static String optional(final Config config, final String name) {
@@ -230,6 +245,11 @@
}
@Override
+ public void onCreateAccount(final AuthRequest who, final Account account) {
+ usernameCache.put(who.getLocalUser(), account.getId());
+ }
+
+ @Override
public Set<AccountGroup.Id> groups(final AccountState who) {
final HashSet<AccountGroup.Id> r = new HashSet<AccountGroup.Id>();
r.addAll(membershipCache.get(findId(who.getExternalIds())));
@@ -293,6 +313,31 @@
return null;
}
+ @Override
+ public Account.Id lookup(final String accountName) {
+ return usernameCache.get(accountName);
+ }
+
+ private Account.Id queryForUsername(final String username) {
+ try {
+ final ReviewDb db = schema.open();
+ try {
+ final List<AccountExternalId> candidates =
+ db.accountExternalIds().byExternal(
+ AccountExternalId.SCHEME_GERRIT + username).toList();
+ if (candidates.size() == 1) {
+ return candidates.get(0).getAccountId();
+ }
+ return null;
+ } finally {
+ db.close();
+ }
+ } catch (OrmException e) {
+ log.warn("Cannot query for username in database", e);
+ return null;
+ }
+ }
+
private DirContext open() throws NamingException {
final Properties env = new Properties();
env.put(Context.INITIAL_CONTEXT_FACTORY, LDAP);
