Merge pull request #439 from kbase/dev-user_search

PTV-1911: Fix user search bugs re underscores + mongo errors

Author: MrCreosote

RELEASE_NOTES.md

@@ -1,5 +1,13 @@
 # Authentication Service MKII release notes
 
+## 0.7.2
+
+* Fixed a bug where usernames with underscores would not be matched in username searches if an
+  underscore was an interior character of a search prefix.
+* Fixed a bug where a MongoDB error would be thrown if a user search prefix resulted in no search
+  terms if it had no valid characters for the requested search, whether user name or display
+  name. Now a service error is thrown.
+
 ## 0.7.1
 
 * Publishes a shadow jar on jitpack.io for supporting tests in other repos.

src/main/java/us/kbase/auth2/Version.java

@@ -5,6 +5,6 @@
 public class Version {
 
 	/** The version of the KBase Auth2 service. */
-	public static final String VERSION = "0.7.1";
+	public static final String VERSION = "0.7.2";
 
 }

src/main/java/us/kbase/auth2/lib/Authentication.java

@@ -1181,10 +1181,17 @@ private Optional<UserName> getAvailableUserName(
 		 * problems. Make this smarter if necessary. E.g. could store username and numeric suffix
 		 * db side and search and sort db side.
 		 */
-		final UserSearchSpec spec = UserSearchSpec.getBuilder()
-				// checked that this does indeed use an index for the mongo implementation
-				.withSearchRegex("^" + Pattern.quote(sugStrip) + "\\d*$")
-				.withSearchOnUserName(true).withIncludeDisabled(true).build();
+		final UserSearchSpec spec;
+		try {
+			spec = UserSearchSpec.getBuilder()
+					// checked that this does indeed use an index for the mongo implementation
+					.withSearchRegex("^" + Pattern.quote(sugStrip) + "\\d*$")
+					.withSearchOnUserName(true)
+					.withIncludeDisabled(true)
+					.build();
+		} catch (IllegalParameterException e) {
+			throw new RuntimeException("this is impossible", e);
+		}
 		final Map<UserName, DisplayName> users = storage.getUserDisplayNames(spec, -1);
 		final boolean match = users.containsKey(suggestedUserName);
 		final boolean hasNumSuffix = sugStrip.length() != sugName.length();

src/main/java/us/kbase/auth2/lib/UserName.java

@@ -1,10 +1,14 @@
 package us.kbase.auth2.lib;
 
 import static java.util.Objects.requireNonNull;
+import static us.kbase.auth2.lib.Utils.checkStringNoCheckedException;
 
+import java.util.Arrays;
+import java.util.List;
 import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 import us.kbase.auth2.lib.exceptions.ErrorType;
 import us.kbase.auth2.lib.exceptions.IllegalParameterException;
@@ -35,8 +39,8 @@ public class UserName extends Name {
 		}
 	}
 
-	private static final String INVALID_CHARS_REGEX = "[^a-z\\d_]+";
-	private final static Pattern INVALID_CHARS = Pattern.compile(INVALID_CHARS_REGEX);
+	private static final Pattern FORCE_ALPHA_FIRST_CHAR = Pattern.compile("^[^a-z]+");
+	private final static Pattern INVALID_CHARS = Pattern.compile("[^a-z\\d_]+");
 	public final static int MAX_NAME_LENGTH = 100;
 
 	/** Create a new user name.
@@ -75,15 +79,37 @@ public boolean isRoot() {
 	 */
 	public static Optional<UserName> sanitizeName(final String suggestedUserName) {
 		requireNonNull(suggestedUserName, "suggestedUserName");
-		final String s = suggestedUserName.toLowerCase().replaceAll(INVALID_CHARS_REGEX, "")
-				.replaceAll("^[^a-z]+", "");
+		final String s = cleanUserName(suggestedUserName);
 		try {
 			return s.isEmpty() ? Optional.empty() : Optional.of(new UserName(s));
 		} catch (IllegalParameterException | MissingParameterException e) {
 			throw new RuntimeException("This should be impossible", e);
 		}
 	}
 
+	private static String cleanUserName(final String putativeName) {
+		return FORCE_ALPHA_FIRST_CHAR.matcher(
+						INVALID_CHARS.matcher(
+								putativeName.toLowerCase())
+						.replaceAll(""))
+				.replaceAll("");
+	}
+	
+	/** Given a string, splits the string by whitespace, strips all illegal
+	 * characters from the tokens, and returns the resulting strings,
+	 * discarding repeats.
+	 * @param names the names string to process.
+	 * @return the list of canonicalized names.
+	 */
+	public static List<String> getCanonicalNames(final String names) {
+		checkStringNoCheckedException(names, "names");
+		return Arrays.asList(names.toLowerCase().split("\\s+")).stream()
+				.map(u -> cleanUserName(u))
+				.filter(u -> !u.isEmpty())
+				.distinct()
+				.collect(Collectors.toList());
+	}
+
 	@Override
 	public String toString() {
 		StringBuilder builder = new StringBuilder();

src/main/java/us/kbase/auth2/lib/UserSearchSpec.java

@@ -10,6 +10,8 @@
 import java.util.Optional;
 import java.util.Set;
 
+import us.kbase.auth2.lib.exceptions.IllegalParameterException;
+
 /** A specification for how a user search should be conducted.
  * 
  * If a search prefix or regex is supplied and neither withSearchOnUserName() nor
@@ -24,7 +26,8 @@ public class UserSearchSpec {
 
 	//TODO ZLATER CODE don't expose regex externally. Not sure how best to do this without duplicating a lot of the class. For now setting regex is default access (package only).
 
-	private final List<String> prefixes;
+	private final List<String> userNamePrefixes;
+	private final List<String> displayPrefixes;
 	private final String regex;
 	private final boolean searchUser;
 	private final boolean searchDisplayName;
@@ -34,15 +37,19 @@ public class UserSearchSpec {
 	private final boolean includeDisabled;
 
 	private UserSearchSpec(
-			final List<String> prefixes,
+			final List<String> userNamePrefixes,
+			final List<String> displayPrefixes,
 			final String regex,
 			final boolean searchUser,
 			final boolean searchDisplayName,
 			final Set<Role> searchRoles,
 			final Set<String> searchCustomRoles,
 			final boolean includeRoot,
 			final boolean includeDisabled) {
-		this.prefixes = prefixes == null ? null : Collections.unmodifiableList(prefixes);
+		this.userNamePrefixes = userNamePrefixes == null ? null :
+				Collections.unmodifiableList(userNamePrefixes);
+		this.displayPrefixes = displayPrefixes == null ? null :
+				Collections.unmodifiableList(displayPrefixes);
 		this.regex = regex;
 		this.searchUser = searchUser;
 		this.searchDisplayName = searchDisplayName;
@@ -52,13 +59,21 @@ private UserSearchSpec(
 		this.includeDisabled = includeDisabled;
 	}
 
-	/** Returns the user and/or display name prefixes for the search, if any.
-	 * The prefixes match the start of the username or the start of any part of the whitespace
+	/** Returns the user name prefixes for the search, if any.
+	 * The prefixes match the start of the user name.
+	 * @return the search prefix.
+	 */
+	public List<String> getSearchUserNamePrefixes() {
+		return userNamePrefixes == null ? Collections.emptyList() : userNamePrefixes;
+	}
+	
+	/** Returns the display name prefixes for the search, if any.
+	 * The prefixes match the start of any part of the whitespace
 	 * tokenized display name.
 	 * @return the search prefix.
 	 */
-	public List<String> getSearchPrefixes() {
-		return prefixes == null ? Collections.emptyList() : prefixes;
+	public List<String> getSearchDisplayPrefixes() {
+		return displayPrefixes == null ? Collections.emptyList() : displayPrefixes;
 	}
 
 	/** Returns the user and/or display name regex for the search, if any.
@@ -80,35 +95,33 @@ public boolean hasSearchRegex() {
 	 * @return true if the search prefixes are set.
 	 */
 	public boolean hasSearchPrefixes() {
-		return prefixes != null;
+		return displayPrefixes != null;
 	}
 
-	private boolean hasSearchString() {
-		return prefixes != null || regex != null;
-	}
-
 	/** Returns true if a search should occur on the user's user name.
 	 * 
-	 * True when a) a prefix or regex is provided and b) withSearchOnUserName() was called with a
-	 * true argument or neither withSearchOnUserName() nor withSearchOnDisplayName() were called
-	 * with a true argument.
+	 * True when
+	 * a) a prefix with a valid format for a username or regex is provided and
+	 * b) withSearchOnUserName() was called with a true argument or neither or both of
+	 * withSearchOnUserName() and withSearchOnDisplayName() were called with a true argument.
 	 * @return whether the search should occur on the user's user name with the provided prefix or
 	 * regex.
 	 */
 	public boolean isUserNameSearch() {
-		return searchUser || (hasSearchString() && !searchDisplayName);
+		return (regex != null || userNamePrefixes != null) && (searchUser || !searchDisplayName);
 	}
 
 	/** Returns true if a search should occur on the user's tokenized display name.
 	 * 
-	 * True when a) a prefix or regex is provided and b) withSearchOnDisplayName() was called with
-	 * a true argument or neither withSearchOnUserName() nor withSearchOnDisplayName() were
-	 * called with a true argument.
+	 * True when
+	 * a) a prefix or regex is provided and
+	 * b) withSearchOnDisplayName() was called with a true argument or neither or both of
+	 * withSearchOnUserName() and withSearchOnDisplayName() were called with a true argument.
 	 * @return whether the search should occur on the users's display name with the provided
 	 * prefix or regex.
 	 */
 	public boolean isDisplayNameSearch() {
-		return searchDisplayName || (hasSearchString() && !searchUser);
+		return (regex != null || displayPrefixes != null) && (searchDisplayName || !searchUser);
 	}
 
 	/** Returns true if a search should occur on the user's roles.
@@ -202,8 +215,8 @@ public static Builder getBuilder() {
 
 	@Override
 	public int hashCode() {
-		return Objects.hash(includeDisabled, includeRoot, prefixes, regex,
-				searchCustomRoles, searchDisplayName, searchRoles, searchUser);
+		return Objects.hash(displayPrefixes, includeDisabled, includeRoot, regex,
+				searchCustomRoles, searchDisplayName, searchRoles, searchUser, userNamePrefixes);
 	}
 
 	@Override
@@ -218,14 +231,15 @@ public boolean equals(Object obj) {
 			return false;
 		}
 		UserSearchSpec other = (UserSearchSpec) obj;
-		return includeDisabled == other.includeDisabled
+		return Objects.equals(displayPrefixes, other.displayPrefixes)
+				&& includeDisabled == other.includeDisabled
 				&& includeRoot == other.includeRoot
-				&& Objects.equals(prefixes, other.prefixes)
 				&& Objects.equals(regex, other.regex)
 				&& Objects.equals(searchCustomRoles, other.searchCustomRoles)
 				&& searchDisplayName == other.searchDisplayName
 				&& Objects.equals(searchRoles, other.searchRoles)
-				&& searchUser == other.searchUser;
+				&& searchUser == other.searchUser
+				&& Objects.equals(userNamePrefixes, other.userNamePrefixes);
 	}
 
 	/** A builder for a UserSearchSpec.
@@ -234,7 +248,7 @@ public boolean equals(Object obj) {
 	 */
 	public static class Builder {
 
-		private List<String> prefixes = null;
+		private String prefix;
 		private String regex = null;
 		private boolean searchUser = false;
 		private boolean searchDisplayName = false;
@@ -249,15 +263,16 @@ private Builder() {}
 		 * The prefix will replace the search regex, if any.
 		 * The prefix matches the start of the username or the start of any part of the whitespace
 		 * and hyphen tokenized display name.
-		 * The prefix is always split by whitespace and hyphens, punctuation removed, and
-		 * converted to lower case.
+		 * The user name prefix is split by whitespace and all illegal characters removed.
+		 * The display name prefix is split by whitespace and hyphens, punctuation removed,
+		 * and converted to lower case.
 		 * Once the prefix or search regex is set in this builder it cannot be removed.
 		 * @param prefix the prefix.
 		 * @return this builder.
 		 */
 		public Builder withSearchPrefix(final String prefix) {
 			checkStringNoCheckedException(prefix, "prefix");
-			this.prefixes = DisplayName.getCanonicalDisplayName(prefix);
+			this.prefix = prefix;
 			this.regex = null;
 			return this;
 		}
@@ -273,12 +288,14 @@ public Builder withSearchPrefix(final String prefix) {
 		 */
 		Builder withSearchRegex(final String regex) {
 			this.regex = checkStringNoCheckedException(regex, "regex");
-			this.prefixes = null;
+			this.prefix = null;
 			return this;
 		}
 
 		/** Specify whether a search on a users's user name should occur.
 		 * A prefix must be set prior to calling this method.
+		 * If neither a user nor a display search is set (the default) and a prefix is set, then
+		 * the search occurs on both fields.
 		 * @param search whether the search should occur on the user's user name.
 		 * @return this builder.
 		 */
@@ -290,6 +307,8 @@ public Builder withSearchOnUserName(final boolean search) {
 
 		/** Specify whether a search on a users's display name should occur.
 		 * A prefix must be set prior to calling this method.
+		 * If neither a user nor a display search is set (the default) and a prefix is set, then
+		 * the search occurs on both fields.
 		 * @param search whether the search should occur on the user's display name.
 		 * @return this builder.
 		 */
@@ -300,7 +319,7 @@ public Builder withSearchOnDisplayName(final boolean search) {
 		}
 
 		private void checkSearchPrefix(final boolean search) {
-			if (search && prefixes == null && regex == null) {
+			if (search && prefix == null && regex == null) {
 				throw new IllegalStateException(
 						"Must provide a prefix or regex if a name search is to occur");
 			}
@@ -353,10 +372,50 @@ public Builder withIncludeDisabled(final boolean include) {
 
 		/** Build a UserSearchSpec instance.
 		 * @return a UserSearchSpec.
+		 * @throws IllegalParameterException if a prefix is set that, after normalizing, contains
+		 * no characters for the requested search(es).
 		 */
-		public UserSearchSpec build() {
-			return new UserSearchSpec(prefixes, regex, searchUser, searchDisplayName, searchRoles,
-					searchCustomRoles, includeRoot, includeDisabled);
+		public UserSearchSpec build() throws IllegalParameterException {
+			List<String> userNamePrefixes = null;
+			List<String> displayPrefixes = null;
+			if (this.prefix != null) {
+				/*	UsrSrch	DisSrch	UsrOK	DisOK	Throw exception?
+				 *	T		T		Y implies Y		
+				 *	T		T		No		Y		No, just go with display search
+				 *	T		T		No		No		Display or user exception
+				 *
+				 *	T		F		Y implies Y		
+				 *	T		F		No		Y		User exception
+				 *	T		F		No		No		User exception
+				 *
+				 *	F		T		Y implies Y		
+				 * 	F		T		No		Y		
+				 * 	F		T		No		No		Display exception
+				 * 
+				 * Note that:
+				 *   * If the user search is ok (UsrOK) the display search must be ok since the
+				 *     user search has at least one a-z char.
+				 *   * The first block where UsrSrch and DisSrch are all true is equivalent
+				 *     to a block where they're all false, and so that block is omitted.
+				 */
+				userNamePrefixes = UserName.getCanonicalNames(prefix);
+				userNamePrefixes = userNamePrefixes.isEmpty() ? null : userNamePrefixes;
+				displayPrefixes = DisplayName.getCanonicalDisplayName(prefix);
+				displayPrefixes = displayPrefixes.isEmpty() ? null : displayPrefixes;
+				if (searchUser && !searchDisplayName && userNamePrefixes == null) {
+					throw new IllegalParameterException(String.format(
+							"The search prefix %s contains no valid username prefix "
+									+ "and a user name search was requested", this.prefix));
+				}
+				if (displayPrefixes == null) {
+					throw new IllegalParameterException(String.format(
+							"The search prefix %s contains only punctuation and a "
+							+ "display name search was requested", this.prefix));
+				}
+			}
+			return new UserSearchSpec(userNamePrefixes, displayPrefixes, regex, searchUser,
+					searchDisplayName, searchRoles, searchCustomRoles,
+					includeRoot, includeDisabled);
 		}
 	}
 }