Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add isValidUsername method #11

Merged
merged 2 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,5 @@ jobs:
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
with:
token: ${{ secrets.CODECOV_TOKEN }}
fail_ci_if_error: true
64 changes: 60 additions & 4 deletions src/main/java/us/kbase/auth/client/AuthClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,20 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.slf4j.LoggerFactory;

import com.fasterxml.jackson.databind.ObjectMapper;

import us.kbase.auth.AuthException;
import us.kbase.auth.AuthToken;
import us.kbase.auth.client.internal.StringCache;
import us.kbase.auth.client.internal.TokenCache;

/** A client for the KBase Auth2 authentication server (https://github.com/kbase/auth2).
Expand All @@ -31,8 +37,11 @@ public class AuthClient {

private static final ObjectMapper MAPPER = new ObjectMapper();

// make this configurable? Add a builder if so
// make these configurable? Add a builder if so
private final TokenCache tokenCache = new TokenCache(1000, 2000); // same as old auth client
private final StringCache userCache = new StringCache(1000, 2000); // same as old auth client

final static Pattern INVALID_USERNAME = Pattern.compile("[^a-z\\d_]+");

private final URI rootURI;

Expand Down Expand Up @@ -164,9 +173,7 @@ public String getServerVersion() throws IOException, AuthException {
* @throws AuthException if an auth exception occurs communicating with the auth service.
*/
public AuthToken validateToken(final String token) throws IOException, AuthException {
if (token == null || token.trim().isEmpty()) {
throw new IllegalArgumentException("token must be a non-whitespace string");
}
checkToken(token);
final AuthToken t = tokenCache.getToken(token);
if (t != null) {
return t;
Expand All @@ -179,4 +186,53 @@ public AuthToken validateToken(final String token) throws IOException, AuthExcep
return authToken;
}

private void checkToken(final String token) {
if (token == null || token.trim().isEmpty()) {
throw new IllegalArgumentException("token must be a non-whitespace string");
}
}

/** Check if usernames are valid accounts in the auth service.
* @param users the list of usernames to check. If they contain any invalid characters
* (e.g. anything other than a-z, 0-9, or _, an exception will be thrown.
* @param token any valid auth token.
* @return a mapping of each username to whether it's valid or not.
* @throws IOException if an IOException occurs communicating with the auth service.
* @throws AuthException if an auth exception occurs communicating with the auth service.
*/
public Map<String, Boolean> isValidUserName(final List<String> users, final String token)
throws IOException, AuthException {
// theoretically someone could submit hundreds of users and hit the url size limit
// don't worry about that for now.
checkToken(token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a valid token for this? I can see a case (mainly at account creation) where you don't have your own username yet, thus no token (maybe a temp token via an auth provider?), and you want to see if your username is available or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this endpoint requires a token. For the use case you're talking about there's https://github.com/kbase/auth2/blob/develop/src/main/java/us/kbase/auth2/service/ui/Login.java#L136-L150

if (users == null || users.isEmpty()) {
throw new IllegalArgumentException("users cannot be null or empty");
}
final List<String> badlist = new LinkedList<String>();
final Map<String, Boolean> result = new HashMap<>();
for (String user: users) {
if (user == null || user.trim().isEmpty()) {
throw new IllegalArgumentException("each user must be a non-whitespace string");
}
user = user.trim();
final Matcher m = INVALID_USERNAME.matcher(user);
if (m.find()) {
// this matches the prior behavior, but later could have an validity enum
// one for bad chars, one for does not exist
throw new IllegalArgumentException(
"username " + user + " has invalid character: " + m.group(0));
}
if (userCache.hasString(user)) {
result.put(user, true);
} else {
badlist.add(user);
}
}
final URI target = rootURI.resolve("api/V2/users/?list=" + String.join(",", badlist));
Copy link

@Xiangs18 Xiangs18 Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if my understanding is correct.

badlist may consist gibberish usernames and expired ones. And you call api/V2/users/ to distinguish them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

badlist contains any usernames that don't exist in the cache and are valid usernames, and the endpoint returns extant usernames mapped to their display names

Copy link

@Xiangs18 Xiangs18 Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw your test examples. Make sense now. Thanks!

final Map<String, Object> res = request(target, token.trim());
res.keySet().stream().forEach(u -> userCache.putString(u));
badlist.stream().forEach(u -> result.put(u, res.containsKey(u)));
return result;
}

}
6 changes: 3 additions & 3 deletions src/main/java/us/kbase/auth/client/internal/TokenCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public void putValidToken(AuthToken token) {
}
Collections.sort(dts);
for(int i = size; i < dts.size(); i++) {
cache.remove(dts.get(i).token);
cache.remove(dts.get(i).tokenHash);
}
}
}
Expand All @@ -135,11 +135,11 @@ class UserDate {

class DateToken implements Comparable<DateToken>{

final String token;
final String tokenHash;
final Date date;

DateToken(long date, String token) {
this.token = token;
this.tokenHash = token;
this.date = new Date(date);
}

Expand Down
75 changes: 75 additions & 0 deletions src/test/java/us/kbase/test/auth/client/AuthClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
import static org.junit.Assert.fail;

import java.net.URI;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;

import org.junit.AfterClass;
import org.junit.Before;
Expand Down Expand Up @@ -179,4 +184,74 @@ private void validateTokenFail(final URI uri, final String token, final Exceptio
TestCommon.assertExceptionCorrect(got, expected);
}
}

@Test
public void isValidUserName() throws Exception {
final String token1 = TestCommon.getAuthToken1();
final List<String> goodUsers = TestCommon.getGoodUsers();

final AuthClient c = AuthClient.from(new URI(TestCommon.getAuthURI()));

final List<String> badUsers = Arrays.asList(
"superfakeuserthatdoesntexistihope",
"anothersuperfakeuserrighthereimfake");

final List<String> allUsers = new LinkedList<>(badUsers);
goodUsers.stream().forEach(u -> allUsers.add(String.format(" \t %s ", u)));

final Map<String, Boolean> expected = new HashMap<>();
goodUsers.stream().forEach(u -> expected.put(u.trim(), true));
badUsers.stream().forEach(u -> expected.put(u, false));

final Map<String, Boolean> res = c.isValidUserName(allUsers, token1);

assertThat("incorrect users", res, is(expected));

// 2nd time from cache. Again no good way to test this w/o a client mock
final Map<String, Boolean> res2 = c.isValidUserName(allUsers, token1);

assertThat("incorrect users", res2, is(expected));
}

@Test
public void isValidUserNameFailBadArgs() throws Exception {
final URI uri = new URI(TestCommon.getAuthURI());
final List<String> u = Arrays.asList("u");
isValidUserNameFail(uri, null, "foo",
new IllegalArgumentException("users cannot be null or empty"));
isValidUserNameFail(uri, Collections.emptyList(), "foo",
new IllegalArgumentException("users cannot be null or empty"));
isValidUserNameFail(uri, Arrays.asList("a", null), "foo",
new IllegalArgumentException("each user must be a non-whitespace string"));
isValidUserNameFail(uri, Arrays.asList("a", " "), "foo",
new IllegalArgumentException("each user must be a non-whitespace string"));
isValidUserNameFail(uri, Arrays.asList("a", " foo*bar "), "foo",
new IllegalArgumentException("username foo*bar has invalid character: *"));
isValidUserNameFail(uri, u, null,
new IllegalArgumentException("token must be a non-whitespace string"));
isValidUserNameFail(uri, u, " \t ",
new IllegalArgumentException("token must be a non-whitespace string"));
}

@Test
public void isValidUserNameFailBadToken() throws Exception {
final URI uri = new URI(TestCommon.getAuthURI());
final List<String> u = Arrays.asList("u");
isValidUserNameFail(uri, u, "badtoken", new AuthException(
"Auth service returned an error: 10020 Invalid token"));
}

private void isValidUserNameFail(
final URI uri,
final List<String> usernames,
final String token,
final Exception expected) {
try {
AuthClient.from(uri).isValidUserName(usernames, token);
fail("expected exception");
} catch (Exception got) {
TestCommon.assertExceptionCorrect(got, expected);
}
}

}
1 change: 1 addition & 0 deletions test.cfg.example
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ auth_user1 = <user name goes here>
auth_token2 = <token goes here>
auth_user2 = <user name goes here>

# existing names from the auth server
good_users = <comma separated list of user names>
Loading