Make email validation case insensitive
While RFC 5321 section 2.3.11 allows for the local-part (the part left of the '@') of an email address to be case sensitive, the domain portion is case insensitive according to RFC 1035 section 3.1. And in practice, even the local-part is typically case insensitive also. HashSet and equal() was used for email matching/validation, which is overly strict. For example, if the email-address-on-record is FirstName.LastName@SomeCorp.com and the user committed a change using firstname.lastname@somecorp.com, then the commit would be rejected unless Forge Author permission is enabled or the user registered the "second" email address. TreeSet is used in place of HashSet and equalsIgnoreCase() is used in place of equals() to resolve this issue. While TreeSet has the time complexity of O(logN), which is worse than that of HashSet (O(1)), the performance impact should be negligible given the problem at hand. IdentifiedUserTest unit test can be executed by running: buck test //gerrit-server:server_tests Change-Id: I820f183a7024d16284ad0a2e7698179384fe6920
This commit is contained in:

committed by
David Pursehouse

parent
58f76e406e
commit
efd882024b
@@ -196,7 +196,8 @@ public class IdentifiedUser extends CurrentUser {
|
||||
private final GroupBackend groupBackend;
|
||||
private final String anonymousCowardName;
|
||||
private final Boolean disableReverseDnsLookup;
|
||||
private final Set<String> validEmails = Sets.newHashSetWithExpectedSize(4);
|
||||
private final Set<String> validEmails =
|
||||
Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER);
|
||||
|
||||
@Nullable
|
||||
private final Provider<SocketAddress> remotePeerProvider;
|
||||
@@ -284,7 +285,7 @@ public class IdentifiedUser extends CurrentUser {
|
||||
validEmails.add(email);
|
||||
return true;
|
||||
} else if (invalidEmails == null) {
|
||||
invalidEmails = Sets.newHashSetWithExpectedSize(4);
|
||||
invalidEmails = Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER);
|
||||
}
|
||||
invalidEmails.add(email);
|
||||
return false;
|
||||
|
@@ -25,7 +25,6 @@ import com.google.inject.Inject;
|
||||
|
||||
import java.util.Collection;
|
||||
import java.util.HashSet;
|
||||
import java.util.Objects;
|
||||
import java.util.Set;
|
||||
|
||||
/** Basic implementation of {@link Realm}. */
|
||||
@@ -57,7 +56,7 @@ public abstract class AbstractRealm implements Realm {
|
||||
@Override
|
||||
public boolean hasEmailAddress(IdentifiedUser user, String email) {
|
||||
for (AccountExternalId ext : user.state().getExternalIds()) {
|
||||
if (Objects.equals(ext.getEmailAddress(), email)) {
|
||||
if (email != null && email.equalsIgnoreCase(ext.getEmailAddress())) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
@@ -0,0 +1,131 @@
|
||||
// Copyright (C) 2015 The Android Open Source Project
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package com.google.gerrit.server;
|
||||
|
||||
import static com.google.common.truth.Truth.assertThat;
|
||||
import static com.google.inject.Scopes.SINGLETON;
|
||||
|
||||
import com.google.gerrit.common.TimeUtil;
|
||||
import com.google.gerrit.reviewdb.client.Account;
|
||||
import com.google.gerrit.server.account.AccountCache;
|
||||
import com.google.gerrit.server.account.CapabilityControl;
|
||||
import com.google.gerrit.server.account.FakeRealm;
|
||||
import com.google.gerrit.server.account.GroupBackend;
|
||||
import com.google.gerrit.server.account.Realm;
|
||||
import com.google.gerrit.server.config.AnonymousCowardName;
|
||||
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
|
||||
import com.google.gerrit.server.config.CanonicalWebUrl;
|
||||
import com.google.gerrit.server.config.DisableReverseDnsLookup;
|
||||
import com.google.gerrit.server.config.GerritServerConfig;
|
||||
import com.google.gerrit.server.group.SystemGroupBackend;
|
||||
import com.google.gerrit.testutil.ConfigSuite;
|
||||
import com.google.gerrit.testutil.FakeAccountCache;
|
||||
import com.google.inject.AbstractModule;
|
||||
import com.google.inject.Guice;
|
||||
import com.google.inject.Inject;
|
||||
import com.google.inject.Injector;
|
||||
import com.google.inject.util.Providers;
|
||||
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
import org.junit.Before;
|
||||
import org.junit.Test;
|
||||
import org.junit.runner.RunWith;
|
||||
|
||||
import java.util.Arrays;
|
||||
import java.util.HashSet;
|
||||
import java.util.Set;
|
||||
|
||||
@RunWith(ConfigSuite.class)
|
||||
public class IdentifiedUserTest {
|
||||
@ConfigSuite.Parameter
|
||||
public Config config;
|
||||
|
||||
private IdentifiedUser identifiedUser;
|
||||
|
||||
@Inject
|
||||
private IdentifiedUser.GenericFactory identifiedUserFactory;
|
||||
|
||||
private static final String[] TEST_CASES = {
|
||||
"",
|
||||
"FirstName.LastName@Corporation.com",
|
||||
"!#$%&'+-/=.?^`{|}~@[IPv6:0123:4567:89AB:CDEF:0123:4567:89AB:CDEF]"
|
||||
};
|
||||
|
||||
@Before
|
||||
public void setUp() throws Exception {
|
||||
final FakeAccountCache accountCache = new FakeAccountCache();
|
||||
final Realm mockRealm = new FakeRealm() {
|
||||
HashSet<String> emails = new HashSet<>(Arrays.asList(TEST_CASES));
|
||||
|
||||
@Override
|
||||
public boolean hasEmailAddress(IdentifiedUser who, String email) {
|
||||
return emails.contains(email);
|
||||
}
|
||||
|
||||
@Override
|
||||
public Set<String> getEmailAddresses(IdentifiedUser who) {
|
||||
return emails;
|
||||
}
|
||||
};
|
||||
|
||||
AbstractModule mod = new AbstractModule() {
|
||||
@Override
|
||||
protected void configure() {
|
||||
bind(Boolean.class).annotatedWith(DisableReverseDnsLookup.class)
|
||||
.toInstance(Boolean.FALSE);
|
||||
bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(config);
|
||||
bind(String.class).annotatedWith(AnonymousCowardName.class)
|
||||
.toProvider(AnonymousCowardNameProvider.class);
|
||||
bind(String.class).annotatedWith(CanonicalWebUrl.class)
|
||||
.toInstance("http://localhost:8080/");
|
||||
bind(AccountCache.class).toInstance(accountCache);
|
||||
bind(GroupBackend.class).to(SystemGroupBackend.class).in(SINGLETON);
|
||||
bind(CapabilityControl.Factory.class)
|
||||
.toProvider(Providers.<CapabilityControl.Factory>of(null));
|
||||
bind(Realm.class).toInstance(mockRealm);
|
||||
|
||||
}
|
||||
};
|
||||
|
||||
Injector injector = Guice.createInjector(mod);
|
||||
injector.injectMembers(this);
|
||||
|
||||
Account account = new Account(new Account.Id(1), TimeUtil.nowTs());
|
||||
Account.Id ownerId = account.getId();
|
||||
|
||||
identifiedUser = identifiedUserFactory.create(ownerId);
|
||||
|
||||
/* Trigger identifiedUser to load the email addresses from mockRealm */
|
||||
identifiedUser.getEmailAddresses();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testEmailsExistence() {
|
||||
assertThat(identifiedUser.hasEmailAddress(TEST_CASES[0])).isTrue();
|
||||
assertThat(identifiedUser.hasEmailAddress(TEST_CASES[1].toLowerCase())).isTrue();
|
||||
assertThat(identifiedUser.hasEmailAddress(TEST_CASES[1])).isTrue();
|
||||
assertThat(identifiedUser.hasEmailAddress(TEST_CASES[1].toUpperCase())).isTrue();
|
||||
/* assert again to test cached email address by IdentifiedUser.validEmails */
|
||||
assertThat(identifiedUser.hasEmailAddress(TEST_CASES[1])).isTrue();
|
||||
|
||||
assertThat(identifiedUser.hasEmailAddress(TEST_CASES[2])).isTrue();
|
||||
assertThat(identifiedUser.hasEmailAddress(TEST_CASES[2].toLowerCase())).isTrue();
|
||||
|
||||
|
||||
assertThat(identifiedUser.hasEmailAddress("non-exist@email.com")).isFalse();
|
||||
/* assert again to test cached email address by IdentifiedUser.invalidEmails */
|
||||
assertThat(identifiedUser.hasEmailAddress("non-exist@email.com")).isFalse();
|
||||
}
|
||||
}
|
Reference in New Issue
Block a user