diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 4dfc4809..ba235ddd 100644 --- a/RELEASE_NOTES.md +++ b/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. diff --git a/src/main/java/us/kbase/auth2/Version.java b/src/main/java/us/kbase/auth2/Version.java index 30993a69..bd75936f 100644 --- a/src/main/java/us/kbase/auth2/Version.java +++ b/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"; } diff --git a/src/main/java/us/kbase/auth2/lib/Authentication.java b/src/main/java/us/kbase/auth2/lib/Authentication.java index 93ba621d..72fc0b7d 100644 --- a/src/main/java/us/kbase/auth2/lib/Authentication.java +++ b/src/main/java/us/kbase/auth2/lib/Authentication.java @@ -1181,10 +1181,17 @@ private Optional 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 users = storage.getUserDisplayNames(spec, -1); final boolean match = users.containsKey(suggestedUserName); final boolean hasNumSuffix = sugStrip.length() != sugName.length(); diff --git a/src/main/java/us/kbase/auth2/lib/UserName.java b/src/main/java/us/kbase/auth2/lib/UserName.java index 97af8559..7b94c51d 100644 --- a/src/main/java/us/kbase/auth2/lib/UserName.java +++ b/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,8 +79,7 @@ public boolean isRoot() { */ public static Optional 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) { @@ -84,6 +87,29 @@ public static Optional sanitizeName(final String suggestedUserName) { } } + 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 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(); diff --git a/src/main/java/us/kbase/auth2/lib/UserSearchSpec.java b/src/main/java/us/kbase/auth2/lib/UserSearchSpec.java index 613fe483..b51c2675 100644 --- a/src/main/java/us/kbase/auth2/lib/UserSearchSpec.java +++ b/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 prefixes; + private final List userNamePrefixes; + private final List displayPrefixes; private final String regex; private final boolean searchUser; private final boolean searchDisplayName; @@ -34,7 +37,8 @@ public class UserSearchSpec { private final boolean includeDisabled; private UserSearchSpec( - final List prefixes, + final List userNamePrefixes, + final List displayPrefixes, final String regex, final boolean searchUser, final boolean searchDisplayName, @@ -42,7 +46,10 @@ private UserSearchSpec( final Set 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 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 getSearchPrefixes() { - return prefixes == null ? Collections.emptyList() : prefixes; + public List 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 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 userNamePrefixes = null; + List 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); } } } diff --git a/src/main/java/us/kbase/auth2/lib/storage/mongo/MongoStorage.java b/src/main/java/us/kbase/auth2/lib/storage/mongo/MongoStorage.java index e1c2a287..f2e520f6 100644 --- a/src/main/java/us/kbase/auth2/lib/storage/mongo/MongoStorage.java +++ b/src/main/java/us/kbase/auth2/lib/storage/mongo/MongoStorage.java @@ -1131,6 +1131,11 @@ public Map testModeGetUserDisplayNames(final Set getDisplayNames( final String collection, final Document query, @@ -1156,6 +1161,7 @@ private Map getDisplayNames( } } + // Sort on a field we're querying otherwise a table scan could occur private static final Map SEARCHFIELD_TO_FIELD; static { final Map m = new HashMap<>(); @@ -1166,9 +1172,9 @@ private Map getDisplayNames( SEARCHFIELD_TO_FIELD = m; } - private Document andRegexes(final String field, final List regexes) { - return new Document("$and", regexes.stream() - .map(regex -> new Document(field, regex)) + private Document andRegexes(final String field, final List prefixes) { + return new Document("$and", prefixes.stream() + .map(t -> new Document(field, new Document("$regex", "^" + Pattern.quote(t)))) .collect(Collectors.toList())); } @@ -1180,16 +1186,14 @@ public Map getUserDisplayNames( requireNonNull(spec, "spec"); final Document query = new Document(); if (spec.hasSearchPrefixes()) { - final List regexes = spec.getSearchPrefixes().stream() - .map(token -> new Document("$regex", "^" + Pattern.quote(token))) - .collect(Collectors.toList()); final List queries = new LinkedList<>(); if (spec.isDisplayNameSearch()) { - queries.add(andRegexes(Fields.USER_DISPLAY_NAME_CANONICAL, regexes)); + queries.add(andRegexes( + Fields.USER_DISPLAY_NAME_CANONICAL, spec.getSearchDisplayPrefixes())); } if (spec.isUserNameSearch() ) { // this means if there's > 1 token nothing will match, but that seems right - queries.add(andRegexes(Fields.USER_NAME, regexes)); + queries.add(andRegexes(Fields.USER_NAME, spec.getSearchUserNamePrefixes())); } query.put("$or", queries); } diff --git a/src/test/java/us/kbase/test/auth2/lib/UserNameTest.java b/src/test/java/us/kbase/test/auth2/lib/UserNameTest.java index 6a99f5e0..d5085a63 100644 --- a/src/test/java/us/kbase/test/auth2/lib/UserNameTest.java +++ b/src/test/java/us/kbase/test/auth2/lib/UserNameTest.java @@ -3,6 +3,7 @@ import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; +import static us.kbase.test.auth2.TestCommon.list; import org.junit.Test; @@ -104,8 +105,8 @@ public void equals() throws Exception { @Test public void sanitize() throws Exception { - assertThat("incorrect santize", UserName.sanitizeName(" 999aFA8 ea6t \t ѱ ** J(())"), - is(Optional.of(new UserName("afa8ea6tj")))); + assertThat("incorrect santize", UserName.sanitizeName(" 999aF_A8 ea6t \t ѱ ** J(())"), + is(Optional.of(new UserName("af_a8ea6tj")))); assertThat("incorrect santize", UserName.sanitizeName("999 8 6 \t ѱ ** (())"), is(Optional.empty())); } @@ -120,4 +121,32 @@ public void failSanitize() { } } + @Test + public void getCanonicalNames() throws Exception { + assertThat("incorrect canonicalize", + UserName.getCanonicalNames( + " 999aF_A8 ea6t \t foo _ѱaatѱ(*) 891**ѱ \n fo-o x\n"), + is(list("af_a8", "ea6t", "foo", "aat", "x"))); + assertThat("incorrect canonicalize", + UserName.getCanonicalNames( + " 999_8 6 \t _ѱѱ(*) 891**ѱ \n \n"), + is(list())); + } + + @Test + public void getCanonicalNamesFail() throws Exception { + failGetCanonicalNames(null); + failGetCanonicalNames(" \t "); + } + + private void failGetCanonicalNames(final String names) { + try { + UserName.getCanonicalNames(names); + fail("expected exception"); + } catch (Exception got) { + TestCommon.assertExceptionCorrect(got, new IllegalArgumentException( + "names cannot be null or whitespace only")); + } + } + } diff --git a/src/test/java/us/kbase/test/auth2/lib/UserSearchSpecTest.java b/src/test/java/us/kbase/test/auth2/lib/UserSearchSpecTest.java index b5c738a5..3a1e1137 100644 --- a/src/test/java/us/kbase/test/auth2/lib/UserSearchSpecTest.java +++ b/src/test/java/us/kbase/test/auth2/lib/UserSearchSpecTest.java @@ -20,6 +20,7 @@ import us.kbase.auth2.lib.UserSearchSpec; import us.kbase.auth2.lib.UserSearchSpec.Builder; import us.kbase.auth2.lib.UserSearchSpec.SearchField; +import us.kbase.auth2.lib.exceptions.IllegalParameterException; import us.kbase.test.auth2.TestCommon; public class UserSearchSpecTest { @@ -32,9 +33,9 @@ public void equals() { } @Test - public void buildWithEverything() { + public void buildWithEverything() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() - .withSearchPrefix("F*oo bar *()") + .withSearchPrefix("F*oo bar *() baz_bat") .withSearchOnUserName(true) .withSearchOnDisplayName(true) .withSearchOnRole(Role.ADMIN) @@ -45,7 +46,10 @@ public void buildWithEverything() { .withIncludeDisabled(true) .build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list("foo", "bar"))); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), + is(list("foo", "bar", "baz_bat"))); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), + is(list("foo", "bar", "bazbat"))); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(true)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -60,13 +64,13 @@ public void buildWithEverything() { assertThat("incorrect orderby", uss.orderBy(), is(SearchField.USERNAME)); assertThat("incorrect include root", uss.isRootIncluded(), is(true)); assertThat("incorrect include disabled", uss.isDisabledIncluded(), is(true)); - } @Test - public void buildWithNothing() { + public void buildWithNothing() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder().build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list())); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list())); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list())); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(false)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -83,10 +87,11 @@ public void buildWithNothing() { } @Test - public void buildWithPrefixOnly() { + public void buildWithPrefixOnly() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() .withSearchPrefix("foO").build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list("foo"))); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list("foo"))); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list("foo"))); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(true)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -103,11 +108,12 @@ public void buildWithPrefixOnly() { } @Test - public void buildUserSearch() { + public void buildUserSearch() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() .withSearchPrefix("foo") .withSearchOnUserName(true).build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list("foo"))); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list("foo"))); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list("foo"))); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(true)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -124,13 +130,14 @@ public void buildUserSearch() { } @Test - public void buildDisplaySearch() { + public void buildDisplaySearch() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() .withSearchPrefix("foo") .withSearchOnDisplayName(true) .withSearchOnCustomRole("bar") .withSearchOnRole(Role.SERV_TOKEN).build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list("foo"))); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list("foo"))); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list("foo"))); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(true)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -146,11 +153,12 @@ public void buildDisplaySearch() { } @Test - public void buildCustomRoleSearch() { + public void buildCustomRoleSearch() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() .withSearchOnCustomRole("foo") .withSearchOnRole(Role.DEV_TOKEN).build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list())); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list())); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list())); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(false)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -166,10 +174,11 @@ public void buildCustomRoleSearch() { } @Test - public void buildRoleSearch() { + public void buildRoleSearch() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() .withSearchOnRole(Role.DEV_TOKEN).build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list())); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list())); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list())); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(false)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -186,10 +195,11 @@ public void buildRoleSearch() { } @Test - public void resetSearch() { + public void resetSearch() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder().withSearchPrefix("foo") .withSearchOnUserName(false).withSearchOnDisplayName(false).build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list("foo"))); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list("foo"))); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list("foo"))); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(true)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -210,7 +220,8 @@ public void regex() throws Exception { final Builder b = UserSearchSpec.getBuilder(); setRegex(b, "\\Qfoo.bar\\E"); final UserSearchSpec uss = b.build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list())); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list())); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list())); assertThat("incorrect regex", uss.getSearchRegex(), is(opt("\\Qfoo.bar\\E"))); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(false)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(true)); @@ -238,7 +249,8 @@ public void prefixToRegex() throws Exception { final Builder b = UserSearchSpec.getBuilder().withSearchPrefix("foo"); setRegex(b, "\\Qfoo.bar\\E"); final UserSearchSpec uss = b.build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list())); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list())); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list())); assertThat("incorrect regex", uss.getSearchRegex(), is(opt("\\Qfoo.bar\\E"))); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(false)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(true)); @@ -259,7 +271,8 @@ public void regexToPrefix() throws Exception { final Builder b = UserSearchSpec.getBuilder(); setRegex(b, "\\Qfoo.bar\\E"); final UserSearchSpec uss = b.withSearchPrefix("foo").build(); - assertThat("incorrect prefix", uss.getSearchPrefixes(), is(list("foo"))); + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list("foo"))); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list("foo"))); assertThat("incorrect regex", uss.getSearchRegex(), is(MT)); assertThat("incorrect has prefixes", uss.hasSearchPrefixes(), is(true)); assertThat("incorrect has regex", uss.hasSearchRegex(), is(false)); @@ -276,10 +289,16 @@ public void regexToPrefix() throws Exception { } @Test - public void immutablePrefixes() { + public void immutablePrefixes() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder().withSearchPrefix("foo bar").build(); try { - uss.getSearchPrefixes().add("baz"); + uss.getSearchUserNamePrefixes().add("baz"); + fail("expected exception"); + } catch (UnsupportedOperationException e) { + //test passed + } + try { + uss.getSearchDisplayPrefixes().add("baz"); fail("expected exception"); } catch (UnsupportedOperationException e) { //test passed @@ -287,7 +306,7 @@ public void immutablePrefixes() { } @Test - public void immutableRoles() { + public void immutableRoles() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() .withSearchOnRole(Role.DEV_TOKEN).build(); try { @@ -299,7 +318,7 @@ public void immutableRoles() { } @Test - public void immutableCustomRoles() { + public void immutableCustomRoles() throws Exception { final UserSearchSpec uss = UserSearchSpec.getBuilder() .withSearchOnCustomRole("foo").build(); try { @@ -310,6 +329,66 @@ public void immutableCustomRoles() { } } + private static final String ERR_USER_SEARCH = "The search prefix %s contains no valid " + + "username prefix and a user name search was requested"; + + @Test + public void buildUserSearchWithInvalidAndValidPrefixes() throws Exception { + // if the user search spec is good, the display spec must be good. The reverse is not true. + final Exception e = new IllegalParameterException(String.format(ERR_USER_SEARCH, "98_7")); + final Builder b = UserSearchSpec.getBuilder() + .withSearchPrefix("98_7"); // valid display spec, not user spec + + // test that no exception is thrown + UserSearchSpec uss = b.build(); + buildUserSearchWithInvalidAndValidPrefixesAssertOnPass(uss); + + buildFail(b.withSearchOnUserName(true), e); + + // test that no exception is thrown + uss = b.withSearchOnDisplayName(true).build(); + buildUserSearchWithInvalidAndValidPrefixesAssertOnPass(uss); + + // test that no exception is thrown + uss = b.withSearchOnUserName(false).build(); + buildUserSearchWithInvalidAndValidPrefixesAssertOnPass(uss); + } + + private void buildUserSearchWithInvalidAndValidPrefixesAssertOnPass(final UserSearchSpec uss) { + assertThat("incorrect user prefix", uss.getSearchUserNamePrefixes(), is(list())); + assertThat("incorrect display prefix", uss.getSearchDisplayPrefixes(), is(list("987"))); + assertThat("incorrect user search", uss.isUserNameSearch(), is(false)); + assertThat("incorrect display name search", uss.isDisplayNameSearch(), is(true)); + } + + private static final String ERR_DISPLAY_SEARCH = "The search prefix &*^(%(^*&) contains only " + + "punctuation and a display name search was requested"; + + @Test + public void buildDisplaySearchFail() throws Exception { + // if the user search spec is good, the display spec must be good. The reverse is not true. + final Exception e = new IllegalParameterException(ERR_DISPLAY_SEARCH); + final Exception euser = new IllegalParameterException( + String.format(ERR_USER_SEARCH, ("&*^(%(^*&)"))); + final Builder b = UserSearchSpec.getBuilder() + .withSearchPrefix("&*^(%(^*&)"); + buildFail(b, e); + + buildFail(b.withSearchOnDisplayName(true), e); + buildFail(b.withSearchOnUserName(true), e); + buildFail(b.withSearchOnDisplayName(false), euser); + } + + + private void buildFail(final Builder b, final Exception expected) { + try { + b.build(); + fail("expected exception"); + } catch (Exception got) { + TestCommon.assertExceptionCorrect(got, expected); + } + } + @Test public void addPrefixFail() { failAddPrefix(null, new IllegalArgumentException( diff --git a/src/test/java/us/kbase/test/auth2/lib/storage/mongo/MongoStorageGetDisplayNamesTest.java b/src/test/java/us/kbase/test/auth2/lib/storage/mongo/MongoStorageGetDisplayNamesTest.java index cb7c6d39..3703d207 100644 --- a/src/test/java/us/kbase/test/auth2/lib/storage/mongo/MongoStorageGetDisplayNamesTest.java +++ b/src/test/java/us/kbase/test/auth2/lib/storage/mongo/MongoStorageGetDisplayNamesTest.java @@ -17,6 +17,8 @@ import org.junit.Test; +import com.google.common.collect.ImmutableMap; + import us.kbase.auth2.lib.CustomRole; import us.kbase.auth2.lib.DisplayName; import us.kbase.auth2.lib.Role; @@ -655,6 +657,25 @@ public void canonicalSearchPunctuation4() throws Exception { is(Collections.emptyMap())); } + @Test + public void userSearchUnderscore() throws Exception { + storage.createUser(NewUser.getBuilder( + new UserName("foo_bar"), UID1, new DisplayName("1"), + NOW, REMOTE1) + .build()); + storage.createUser(NewUser.getBuilder( + new UserName("fo_obar"), UID2, new DisplayName("2"), NOW, REMOTE2) + .build()); + + assertThat("incorrect users found", storage.getUserDisplayNames(UserSearchSpec.getBuilder() + .withSearchPrefix("fo_").build(), -1), + is(ImmutableMap.of(new UserName("fo_obar"), new DisplayName("2")))); + + assertThat("incorrect users found", storage.getUserDisplayNames(UserSearchSpec.getBuilder() + .withSearchPrefix("foo_b").build(), -1), + is(ImmutableMap.of(new UserName("foo_bar"), new DisplayName("1")))); + } + @Test public void canonicalSearch1() throws Exception { createUsersForCanonicalSearch(); diff --git a/src/test/java/us/kbase/test/auth2/service/api/UserEndpointTest.java b/src/test/java/us/kbase/test/auth2/service/api/UserEndpointTest.java index 200c866c..08e79047 100644 --- a/src/test/java/us/kbase/test/auth2/service/api/UserEndpointTest.java +++ b/src/test/java/us/kbase/test/auth2/service/api/UserEndpointTest.java @@ -3,12 +3,12 @@ import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertThat; import static us.kbase.test.auth2.service.ServiceTestUtils.failRequestJSON; +import static us.kbase.test.auth2.TestCommon.inst; import java.net.HttpURLConnection; import java.net.URI; import java.net.URL; import java.nio.file.Path; -import java.time.Instant; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -154,12 +154,12 @@ public void testModeFail() throws Exception { public void getMeMinimalInput() throws Exception { final UUID uid = UUID.randomUUID(); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("foobar"), - uid, new DisplayName("bleah"), Instant.ofEpochMilli(20000)).build(), + uid, new DisplayName("bleah"), inst(20000)).build(), new PasswordHashAndSalt("foobarbazbing".getBytes(), "aa".getBytes())); final IncomingToken token = new IncomingToken("whee"); manager.storage.storeToken(StoredToken.getBuilder(TokenType.LOGIN, UUID.randomUUID(), - new UserName("foobar")).withLifeTime(Instant.ofEpochMilli(10000), - Instant.ofEpochMilli(1000000000000000L)).build(), + new UserName("foobar")).withLifeTime(inst(10000), + inst(1000000000000000L)).build(), token.getHashedToken().getTokenHash()); final URI target = UriBuilder.fromUri(host).path("/api/V2/me").build(); @@ -197,25 +197,25 @@ public void getMeMaximalInput() throws Exception { manager.storage.setCustomRole(new CustomRole("whoo", "a")); manager.storage.setCustomRole(new CustomRole("whee", "b")); manager.storage.createUser(NewUser.getBuilder(new UserName("foobar"), UID, - new DisplayName("bleah"), Instant.ofEpochMilli(20000), + new DisplayName("bleah"), inst(20000), new RemoteIdentity(new RemoteIdentityID("prov", "id"), new RemoteIdentityDetails("user1", "full1", "f@h.com"))) .withCustomRole("whoo") .withCustomRole("whee") .withEmailAddress(new EmailAddress("a@g.com")) - .withLastLogin(Instant.ofEpochMilli(30000)) + .withLastLogin(inst(30000)) .withRole(Role.ADMIN) .withRole(Role.DEV_TOKEN) - .withPolicyID(new PolicyID("wugga"), Instant.ofEpochMilli(40000)) - .withPolicyID(new PolicyID("wubba"), Instant.ofEpochMilli(50000)) + .withPolicyID(new PolicyID("wugga"), inst(40000)) + .withPolicyID(new PolicyID("wubba"), inst(50000)) .build()); manager.storage.link(new UserName("foobar"), new RemoteIdentity( new RemoteIdentityID("prov2", "id2"), new RemoteIdentityDetails("user2", "full2", "f2@g.com"))); final IncomingToken token = new IncomingToken("whee"); manager.storage.storeToken(StoredToken.getBuilder(TokenType.LOGIN, UUID.randomUUID(), - new UserName("foobar")).withLifeTime(Instant.ofEpochMilli(10000), - Instant.ofEpochMilli(1000000000000000L)).build(), + new UserName("foobar")).withLifeTime(inst(10000), + inst(1000000000000000L)).build(), token.getHashedToken().getTokenHash()); final URI target = UriBuilder.fromUri(host).path("/api/V2/me").build(); @@ -295,13 +295,13 @@ public void getMeFailBadToken() throws Exception { @Test public void putMeNoUpdate() throws Exception { manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("foobar"), - UID, new DisplayName("bleah"), Instant.ofEpochMilli(20000)) + UID, new DisplayName("bleah"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), new PasswordHashAndSalt("foobarbazbing".getBytes(), "aa".getBytes())); final IncomingToken token = new IncomingToken("whee"); manager.storage.storeToken(StoredToken.getBuilder(TokenType.LOGIN, UUID.randomUUID(), - new UserName("foobar")).withLifeTime(Instant.ofEpochMilli(10000), - Instant.ofEpochMilli(1000000000000000L)).build(), + new UserName("foobar")).withLifeTime(inst(10000), + inst(1000000000000000L)).build(), token.getHashedToken().getTokenHash()); final URI target = UriBuilder.fromUri(host).path("/api/V2/me").build(); @@ -316,7 +316,7 @@ UID, new DisplayName("bleah"), Instant.ofEpochMilli(20000)) assertThat("user modified unexpectedly", manager.storage.getUser(new UserName("foobar")), is(AuthUser.getBuilder(new UserName("foobar"), UID, new DisplayName("bleah"), - Instant.ofEpochMilli(20000)) + inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")) .build())); } @@ -324,13 +324,13 @@ UID, new DisplayName("bleah"), Instant.ofEpochMilli(20000)) @Test public void putMeFullUpdate() throws Exception { manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("foobar"), - UID, new DisplayName("bleah"), Instant.ofEpochMilli(20000)) + UID, new DisplayName("bleah"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), new PasswordHashAndSalt("foobarbazbing".getBytes(), "aa".getBytes())); final IncomingToken token = new IncomingToken("whee"); manager.storage.storeToken(StoredToken.getBuilder(TokenType.LOGIN, UUID.randomUUID(), - new UserName("foobar")).withLifeTime(Instant.ofEpochMilli(10000), - Instant.ofEpochMilli(1000000000000000L)).build(), + new UserName("foobar")).withLifeTime(inst(10000), + inst(1000000000000000L)).build(), token.getHashedToken().getTokenHash()); final URI target = UriBuilder.fromUri(host).path("/api/V2/me").build(); @@ -346,7 +346,7 @@ UID, new DisplayName("bleah"), Instant.ofEpochMilli(20000)) assertThat("user not modified", manager.storage.getUser(new UserName("foobar")), is(AuthUser.getBuilder(new UserName("foobar"), UID, new DisplayName("whee"), - Instant.ofEpochMilli(20000)) + inst(20000)) .withEmailAddress(new EmailAddress("x@g.com")) .build())); } @@ -421,13 +421,13 @@ public void getGlobusUserSelfWithGlobusHeader() throws Exception { final PasswordHashAndSalt creds = new PasswordHashAndSalt( "foobarbazbing".getBytes(), "aa".getBytes()); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("foobar"), - UID, new DisplayName("bleah"), Instant.ofEpochMilli(20000)) + UID, new DisplayName("bleah"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), creds); final IncomingToken token = new IncomingToken("whee"); manager.storage.storeToken(StoredToken.getBuilder(TokenType.LOGIN, UUID.randomUUID(), - new UserName("foobar")).withLifeTime(Instant.ofEpochMilli(10000), - Instant.ofEpochMilli(1000000000000000L)).build(), + new UserName("foobar")).withLifeTime(inst(10000), + inst(1000000000000000L)).build(), token.getHashedToken().getTokenHash()); final URI target = UriBuilder.fromUri(host).path("/api/legacy/globus/users/foobar") @@ -465,17 +465,17 @@ public void getGlobusUserOtherWithAuthHeader() throws Exception { final PasswordHashAndSalt creds = new PasswordHashAndSalt( "foobarbazbing".getBytes(), "aa".getBytes()); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("foobar"), - UID, new DisplayName("bleah"), Instant.ofEpochMilli(20000)) + UID, new DisplayName("bleah"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), creds); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("foobaz"), - UUID.randomUUID(),new DisplayName("bleah2"), Instant.ofEpochMilli(20000)) + UUID.randomUUID(),new DisplayName("bleah2"), inst(20000)) .withEmailAddress(new EmailAddress("f2@g.com")).build(), creds); final IncomingToken token = new IncomingToken("whee"); manager.storage.storeToken(StoredToken.getBuilder(TokenType.LOGIN, UUID.randomUUID(), - new UserName("foobar")).withLifeTime(Instant.ofEpochMilli(10000), - Instant.ofEpochMilli(1000000000000000L)).build(), + new UserName("foobar")).withLifeTime(inst(10000), + inst(1000000000000000L)).build(), token.getHashedToken().getTokenHash()); final URI target = UriBuilder.fromUri(host).path("/api/legacy/globus/users/foobaz") @@ -604,31 +604,35 @@ private IncomingToken setUpUsersForTesting() throws Exception { "foobarbazbing".getBytes(), "aa".getBytes()); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("foo"), - uuid(), new DisplayName("bar *thing*"), Instant.ofEpochMilli(20000)) + uuid(), new DisplayName("bar *thing*"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), creds); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("baz"), - uuid(), new DisplayName("fuz"), Instant.ofEpochMilli(20000)) + uuid(), new DisplayName("fuz"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), creds); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("puz"), - uuid(), new DisplayName("mup"), Instant.ofEpochMilli(20000)) + uuid(), new DisplayName("mup"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), creds); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("mua"), - uuid(), new DisplayName("paz"), Instant.ofEpochMilli(20000)) + uuid(), new DisplayName("paz"), inst(20000)) + .withEmailAddress(new EmailAddress("f@h.com")).build(), + creds); + manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("under_score"), + uuid(), new DisplayName("zzznevermind"), inst(20000)) .withEmailAddress(new EmailAddress("f@h.com")).build(), creds); manager.storage.createLocalUser(LocalUser.getLocalUserBuilder(new UserName("toobar"), - uuid(), new DisplayName("bleah2"), Instant.ofEpochMilli(20000)) + uuid(), new DisplayName("bleah2"), inst(20000)) .withEmailAddress(new EmailAddress("f2@g.com")).build(), creds); final IncomingToken token = new IncomingToken("whee"); manager.storage.storeToken(StoredToken.getBuilder(TokenType.LOGIN, UUID.randomUUID(), - new UserName("toobar")).withLifeTime(Instant.ofEpochMilli(10000), - Instant.ofEpochMilli(1000000000000000L)).build(), + new UserName("toobar")).withLifeTime(inst(10000), + inst(1000000000000000L)).build(), token.getHashedToken().getTokenHash()); return token; } @@ -682,6 +686,15 @@ public void searchUsersBlankFields() throws Exception { searchUsers("f", " \t , ", ImmutableMap.of("foo", "bar *thing*", "baz", "fuz")); } + @Test + public void searchUsersUnderscore() throws Exception { + // The display name canonicalization previously applied to the user name as well, which + // caused a bug since the username in the database is not canonicalized. This would cause + // searches to fail when the one allowed punctuation symbol `_`, was included in + // the search term. + searchUsers("9un$der_s", "", ImmutableMap.of("under_score", "zzznevermind")); + } + @Test public void searchUsersUserName() throws Exception { searchUsers("f", " username , \t ", ImmutableMap.of("foo", "bar *thing*")); @@ -742,17 +755,27 @@ private void searchUsers( @Test public void searchUsersFailBadToken() throws Exception { - failSearchUsers(null, 400, "Bad Request", + failSearchUsers("f", null, 400, "Bad Request", new NoTokenProvidedException("No user token provided")); - failSearchUsers("foobar", 401, "Unauthorized", new InvalidTokenException()); + failSearchUsers("f", "foobar", 401, "Unauthorized", new InvalidTokenException()); + } + + @Test + public void searchUsersFailBadInput() throws Exception { + final IncomingToken token = setUpUsersForTesting(); + failSearchUsers("*^&)*^)", token.getToken(), 400, "Bad Request", + new IllegalParameterException( + "The search prefix *^&)*^) contains only " + + "punctuation and a display name search was requested")); } private void failSearchUsers( + final String prefix, final String token, final int code, final String error, final AuthException e) throws Exception { - final URI target = UriBuilder.fromUri(host).path("/api/V2/users/search/f").build(); + final URI target = UriBuilder.fromUri(host).path("/api/V2/users/search/" + prefix).build(); final WebTarget wt = CLI.target(target); final Builder req = wt.request() diff --git a/src/test/java/us/kbase/test/auth2/service/common/ServiceCommonTest.java b/src/test/java/us/kbase/test/auth2/service/common/ServiceCommonTest.java index bf662eb9..5f4983dc 100644 --- a/src/test/java/us/kbase/test/auth2/service/common/ServiceCommonTest.java +++ b/src/test/java/us/kbase/test/auth2/service/common/ServiceCommonTest.java @@ -47,7 +47,7 @@ public class ServiceCommonTest { public static final String SERVICE_NAME = "Authentication Service"; - public static final String SERVER_VER = "0.7.1"; + public static final String SERVER_VER = "0.7.2"; public static final String GIT_ERR = "Missing git commit file gitcommit, should be in us.kbase.auth2";