Skip to content

Commit

Permalink
Fix login bug wrt anonymous IDs
Browse files Browse the repository at this point in the history
Somehow missed updating the `getUser(RemoteIdentity)` method when I
updated `getUser(UserName)`. Sloppy.

One other issue is I still don't understand is why the
`ExceptionHandler` didn't print out the full stacktrace from the NPE
thrown by `UUID.fromString` like it does for all the other exceptions in
the log.
  • Loading branch information
MrCreosote committed Jul 24, 2023
1 parent a91d0a9 commit 54e1523
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 18 deletions.
47 changes: 32 additions & 15 deletions src/us/kbase/auth2/lib/storage/mongo/MongoStorage.java
Original file line number Diff line number Diff line change
Expand Up @@ -748,33 +748,49 @@ private Document getUserDoc(
final boolean local)
throws AuthStorageException, NoSuchUserException {
requireNonNull(userName, "userName");
Document user = getUserDocWithoutPassword(collection, userName);
return getUserDoc(
collection, new Document(Fields.USER_NAME, userName.getName()), local, userName);

}

private Document getUserDoc(
final String collection,
final Document query,
final boolean local,
final UserName nameForException)
throws AuthStorageException, NoSuchUserException {
Document user = getUserDocWithoutPassword(collection, query);
if (user == null) {
throw new NoSuchUserException(userName.getName());
if (nameForException != null) {
throw new NoSuchUserException(nameForException.getName());
} else {
return null;
}
}
final String userName = user.getString(Fields.USER_NAME);
if (user.getString(Fields.USER_ANONYMOUS_ID) == null) {
user = updateUserAnonID(collection, userName);
}
if (local && !user.getBoolean(Fields.USER_LOCAL)) {
throw new NoSuchLocalUserException(userName.getName());
throw new NoSuchLocalUserException(userName);
}
return user;
}

private Document getUserDocWithoutPassword(final String collection, final UserName userName)
private Document getUserDocWithoutPassword(final String collection, final Document query)
throws AuthStorageException {
return findOne(
collection,
new Document(Fields.USER_NAME, userName.getName()),
query,
new Document(Fields.USER_PWD_HSH, 0).append(Fields.USER_SALT, 0));
}

private Document updateUserAnonID(final String collection, final UserName userName)
private Document updateUserAnonID(final String collection, final String userName)
throws AuthStorageException {
/* Added in version 0.6.0 when anonymous IDs were added to users. This method lazily
* backfills the anonymous ID on an as-needed basis.
*/
final Document query = new Document(Fields.USER_NAME, userName.getName())
final Document query = new Document(Fields.USER_NAME, userName)
// only modify if not already done to avoid race conditions
.append(Fields.USER_ANONYMOUS_ID, null);
final Document update = new Document(
Expand All @@ -788,10 +804,11 @@ private Document updateUserAnonID(final String collection, final UserName userNa
"Connection to database failed: " + e.getMessage(), e);
}
// pull the user from the DB again to avoid a race condition here
final Document userdoc = getUserDocWithoutPassword(collection, userName);
final Document userdoc = getUserDocWithoutPassword(
collection, new Document(Fields.USER_NAME, userName));
if (userdoc == null) {
throw new RuntimeException(
"User unexpectedly not found in database: " + userName.getName());
"User unexpectedly not found in database: " + userName);
}
return userdoc;
}
Expand Down Expand Up @@ -1526,12 +1543,12 @@ Fields.ROLES_ID, new Document("$in", roles))).stream()

@Override
public Optional<AuthUser> getUser(final RemoteIdentity remoteID) throws AuthStorageException {
final Document query = makeUserQuery(remoteID);
//note a user with identities should never have these fields, but
//doesn't hurt to be safe
final Document projection = new Document(Fields.USER_PWD_HSH, 0)
.append(Fields.USER_SALT, 0);
final Document u = findOne(COL_USERS, query, projection);
final Document u;
try {
u = getUserDoc(COL_USERS, makeUserQuery(remoteID), false, null);
} catch (NoSuchUserException e) {
throw new RuntimeException("This should be impossible", e);
}
if (u == null) {
return Optional.empty();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,35 @@ private void backfillMissingAnonIDStdUser(final Document update) throws Exceptio
assertThat("incorrect user", got, is(expected));
}

@Test
public void backFillMissingAnonIDNoFieldStdUserGetByIdentity() throws Exception {
backFillMissingAnonIDNoFieldStdUserGetByIdentity(
new Document("$unset", new Document("anonid", "")));
}

@Test
public void backFillMissingAnonIDWithFieldStdUserGetByIdentity() throws Exception {
backFillMissingAnonIDNoFieldStdUserGetByIdentity(
new Document("$set", new Document("anonid", null)));
}

private void backFillMissingAnonIDNoFieldStdUserGetByIdentity(final Document update)
throws Exception {
storage.createUser(NewUser.getBuilder(
new UserName("foo"), UID, new DisplayName("d"), NOW, REMOTE1).build());
db.getCollection("users").updateOne(new Document("user", "foo"), update);

final UUID uid2 = UUID.randomUUID();
when(manager.mockRand.randomUUID()).thenReturn(uid2, (UUID) null);

final AuthUser got = storage.getUser(REMOTE1).get();
final AuthUser expected = AuthUser.getBuilder(
new UserName("foo"), uid2, new DisplayName("d"), NOW)
.withIdentity(REMOTE1)
.build();
assertThat("incorrect user", got, is(expected));
}

/*
* The following tests test race conditions by accessing the internal update method
* directly. At this point the userdoc has already been pulled from the database and
Expand All @@ -125,7 +154,7 @@ private void backfillMissingAnonIDStdUser(final Document update) throws Exceptio

private Method getAnonIDUpdateMethod() throws Exception {
final Method m = MongoStorage.class
.getDeclaredMethod("updateUserAnonID", String.class, UserName.class);
.getDeclaredMethod("updateUserAnonID", String.class, String.class);
m.setAccessible(true);
return m;
}
Expand All @@ -143,7 +172,7 @@ public void updateUserAnonIDWithRaceOnUpdate() throws Exception {
when(manager.mockRand.randomUUID()).thenReturn(UUID.randomUUID(), (UUID) null);

final Document userdoc = (Document) getAnonIDUpdateMethod()
.invoke(storage, "users", new UserName("foo"));
.invoke(storage, "users", "foo");

// only check a few properties, the methods above test exhaustively
assertThat("incorrect user", userdoc.getString("user"), is("foo"));
Expand All @@ -167,7 +196,7 @@ public void updateUserAnonWithMissingDocument() throws Exception {
when(manager.mockRand.randomUUID()).thenReturn(UUID.randomUUID(), (UUID) null);

try {
getAnonIDUpdateMethod().invoke(storage, "users", new UserName("bar"));
getAnonIDUpdateMethod().invoke(storage, "users", "bar");
fail("expected exception");
} catch (InvocationTargetException e) {
assertExceptionCorrect(e.getCause(), new RuntimeException(
Expand Down

0 comments on commit 54e1523

Please sign in to comment.