From 54e152365a5ecd024f29330c9288b30873cb3c69 Mon Sep 17 00:00:00 2001 From: Gavin Date: Mon, 24 Jul 2023 15:41:56 -0700 Subject: [PATCH] Fix login bug wrt anonymous IDs 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. --- .../auth2/lib/storage/mongo/MongoStorage.java | 47 +++++++++++++------ .../mongo/MongoStorageAnonymousIDTest.java | 35 ++++++++++++-- 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/src/us/kbase/auth2/lib/storage/mongo/MongoStorage.java b/src/us/kbase/auth2/lib/storage/mongo/MongoStorage.java index 73373804..e1c2a287 100644 --- a/src/us/kbase/auth2/lib/storage/mongo/MongoStorage.java +++ b/src/us/kbase/auth2/lib/storage/mongo/MongoStorage.java @@ -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( @@ -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; } @@ -1526,12 +1543,12 @@ Fields.ROLES_ID, new Document("$in", roles))).stream() @Override public Optional 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(); } diff --git a/src/us/kbase/test/auth2/lib/storage/mongo/MongoStorageAnonymousIDTest.java b/src/us/kbase/test/auth2/lib/storage/mongo/MongoStorageAnonymousIDTest.java index 72d802e0..9265a454 100644 --- a/src/us/kbase/test/auth2/lib/storage/mongo/MongoStorageAnonymousIDTest.java +++ b/src/us/kbase/test/auth2/lib/storage/mongo/MongoStorageAnonymousIDTest.java @@ -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 @@ -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; } @@ -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")); @@ -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(