Improve ORCID JWT error handling and test robustness

Author: dauglyon

src/main/java/us/kbase/auth2/providers/OrcIDIdentityProviderFactory.java

@@ -51,6 +51,13 @@ public IdentityProvider configure(final IdentityProviderConfig cfg) {
 	}
 
 	/** An identity provider for OrcID accounts.
+	 *
+	 * Multi-Factor Authentication (MFA) Status Handling:
+	 * - Uses OpenID Connect JWT tokens to determine MFA status via AMR claims
+	 * - Missing JWT (non-member accounts): defaults to MfaStatus.UNKNOWN
+	 * - Malformed JWT during login: logs warning and defaults to MfaStatus.UNKNOWN for graceful degradation
+	 * - Valid JWT with AMR claim: returns MfaStatus.USED or MfaStatus.NOT_USED based on "mfa" presence
+	 *
 	 * @author [email protected]
 	 *
 	 */
@@ -72,6 +79,7 @@ public static class OrcIDIdentityProvider implements IdentityProvider {
 		private static final Client CLI = ClientBuilder.newClient();
 
 		private static final ObjectMapper MAPPER = new ObjectMapper();
+		private static final org.slf4j.Logger LOGGER = LoggerFactory.getLogger(OrcIDIdentityProviderFactory.class);
 
 		private final IdentityProviderConfig cfg;
 
@@ -214,13 +222,15 @@ public void handleError(final Response r, final Map<String, Object> response)
 		/**
 		 * Parses the Authentication Method Reference (AMR) claim from an OpenID Connect ID token
 		 * to determine if multi-factor authentication was used.
-		 * 
-		 * @param jwt the JWT ID token from ORCID
-		 * @return MfaStatus indicating whether MFA was used
+		 *
+		 * @param jwt the JWT ID token from ORCID (may be null/empty for non-member accounts)
+		 * @return MfaStatus indicating whether MFA was used, UNKNOWN if JWT is missing or unparseable
 		 */
 		private static MfaStatus parseAmrClaim(final String jwt) throws IdentityRetrievalException {
 			if (jwt == null || jwt.trim().isEmpty()) {
-				throw new IdentityRetrievalException("No JWT token provided by ORCID despite requesting OpenID scope");
+				// Missing JWT is expected for non-member ORCID accounts without OpenID Connect scope
+				LOGGER.debug("No JWT token provided by ORCID - defaulting MFA status to UNKNOWN");
+				return MfaStatus.UNKNOWN;
 			}
 
 			try {
@@ -256,10 +266,12 @@ private static MfaStatus parseAmrClaim(final String jwt) throws IdentityRetrieva
 				throw new IdentityRetrievalException("AMR claim from ORCID in unexpected format: " + amrClaim);
 
 			} catch (IllegalArgumentException e) {
-				// Base64 decoding failed - invalid JWT
+				// Base64 decoding failed - invalid JWT format
+				LOGGER.warn("Unable to decode JWT from ORCID: {}", e.getMessage());
 				throw new IdentityRetrievalException("Unable to decode JWT from ORCID: " + e.getMessage(), e);
 			} catch (IOException e) {
 				// JSON parsing failed - malformed payload
+				LOGGER.warn("Unable to parse JWT payload from ORCID: {}", e.getMessage());
 				throw new IdentityRetrievalException("Unable to parse JWT payload from ORCID: " + e.getMessage(), e);
 			}
 		}

src/test/java/us/kbase/test/auth2/lib/EmailAddressTest.java

@@ -18,8 +18,7 @@ public class EmailAddressTest {
 	@Test
 	public void construct() throws Exception {
 		final EmailAddress ea = new EmailAddress("   [email protected]   \n");
-		assertThat("incorrect email", ea.getAddress(), is("[email protected]"));
-		assertThat("incorrect equality", ea, is(new EmailAddress("[email protected]")));
+		assertThat("incorrect email content, failed equality", ea, is(new EmailAddress("[email protected]")));
 		assertThat("incorrect toString", ea.toString(), is("EmailAddress [[email protected]]"));
 	}
 

src/test/java/us/kbase/test/auth2/lib/UserNameTest.java

@@ -21,25 +21,22 @@ public class UserNameTest {
 	@Test
 	public void root() throws Exception {
 		final UserName un = new UserName("***ROOT***");
-		assertThat("incorrect username", un.getName(), is("***ROOT***"));
+		assertThat("incorrect root content, failed equality", un, is(UserName.ROOT));
 		assertThat("incorrect is root", un.isRoot(), is(true));
 		assertThat("incorrect toString", un.toString(), is("UserName [getName()=***ROOT***]"));
-		assertThat("incorrect equality", un, is(UserName.ROOT));
 
 		final UserName un2 = UserName.ROOT;
-		assertThat("incorrect username", un2.getName(), is("***ROOT***"));
+		assertThat("incorrect root content, failed equality", un2, is(new UserName("***ROOT***")));
 		assertThat("incorrect is root", un2.isRoot(), is(true));
 		assertThat("incorrect toString", un2.toString(), is("UserName [getName()=***ROOT***]"));
-		assertThat("incorrect equality", un2, is(new UserName("***ROOT***")));
 	}
 
 	@Test
 	public void construct() throws Exception {
 		final UserName un = new UserName("a8nba9");
-		assertThat("incorrect username", un.getName(), is("a8nba9"));
+		assertThat("incorrect username content, failed equality", un, is(new UserName("a8nba9")));
 		assertThat("incorrect is root", un.isRoot(), is(false));
 		assertThat("incorrect toString", un.toString(), is("UserName [getName()=a8nba9]"));
-		assertThat("incorrect equality", un, is(new UserName("a8nba9")));
 	}
 
 	@Test

src/test/java/us/kbase/test/auth2/lib/identity/RemoteIdentityTest.java

@@ -18,31 +18,19 @@ public class RemoteIdentityTest {
 	@Test
 	public void remoteDetailsWithAllFields() throws Exception {
 		final RemoteIdentityDetails dets = new RemoteIdentityDetails("user ", " full", "\temail");
-		assertThat("incorrect username", dets.getUsername(), is("user"));
-		assertThat("incorrect fullname", dets.getFullname(), is("full"));
-		assertThat("incorrect email", dets.getEmail(), is("email"));
-		assertThat("incorrect mfa authenticated", dets.getMfa(), is(MfaStatus.UNKNOWN));
-		assertThat("incorrect equality", dets, is(new RemoteIdentityDetails("user", "full", "email")));
+		assertThat("incorrect details content, failed equality", dets, is(new RemoteIdentityDetails("user", "full", "email")));
 		assertThat("incorrect toString()", dets.toString(),
 				is("RemoteIdentityDetails [username=user, fullname=full, email=email, mfa=UNKNOWN]"));
 	}
 
 	@Test
 	public void remoteDetailsWithEmptyFields() throws Exception {
 		final RemoteIdentityDetails dets = new RemoteIdentityDetails("user", "\t ", " \n");
-		assertThat("incorrect username", dets.getUsername(), is("user"));
-		assertThat("incorrect fullname", dets.getFullname(), is((String) null));
-		assertThat("incorrect email", dets.getEmail(), is((String) null));
-		assertThat("incorrect mfa authenticated", dets.getMfa(), is(MfaStatus.UNKNOWN));
 		assertThat("incorrect toString()", dets.toString(),
 				is("RemoteIdentityDetails [username=user, fullname=null, email=null, mfa=UNKNOWN]"));
 
 		final RemoteIdentityDetails dets2 = new RemoteIdentityDetails("user", null, null);
-		assertThat("incorrect username", dets2.getUsername(), is("user"));
-		assertThat("incorrect fullname", dets2.getFullname(), is((String) null));
-		assertThat("incorrect email", dets2.getEmail(), is((String) null));
-		assertThat("incorrect mfa authenticated", dets2.getMfa(), is(MfaStatus.UNKNOWN));
-		assertThat("incorrect equality", dets2, is(dets));
+		assertThat("incorrect empty field content, failed equality", dets2, is(dets));
 		assertThat("incorrect toString()", dets2.toString(),
 				is("RemoteIdentityDetails [username=user, fullname=null, email=null, mfa=UNKNOWN]"));
 	}
@@ -71,12 +59,10 @@ public void remoteDetailsEquals() throws Exception {
 	@Test
 	public void remoteId() throws Exception {
 		final RemoteIdentityID id = new RemoteIdentityID("foo", "bar");
-		assertThat("incorrect provider name", id.getProviderName(), is("foo"));
-		assertThat("incorrect provider id", id.getProviderIdentityId(), is("bar"));
+		assertThat("incorrect id content, failed equality", id, is(new RemoteIdentityID("foo", "bar")));
 		assertThat("incorrect unique id", id.getID(), is("5c7d96a3dd7a87850a2ef34087565a6e"));
 		// check unique id again to check memoization doesn't change result
 		assertThat("incorrect unique id", id.getID(), is("5c7d96a3dd7a87850a2ef34087565a6e"));
-		assertThat("incorrect equality", id, is(new RemoteIdentityID("foo", "bar")));
 		assertThat("incorrect toString()", id.toString(),
 				is("RemoteIdentityID [provider=foo, id=bar]"));
 	}
@@ -113,9 +99,7 @@ public void identity() throws Exception {
 		final RemoteIdentityID id = new RemoteIdentityID("p", "i");
 		final RemoteIdentityDetails dets = new RemoteIdentityDetails("u", "f", "e");
 		final RemoteIdentity ri = new RemoteIdentity(id, dets);
-		assertThat("incorrect id", ri.getRemoteID(), is(id));
-		assertThat("incorrect details", ri.getDetails(), is(dets));
-		assertThat("incorrect equality", ri, is(new RemoteIdentity(
+		assertThat("incorrect identity content, failed equality", ri, is(new RemoteIdentity(
 				new RemoteIdentityID("p", "i"), new RemoteIdentityDetails("u", "f", "e"))));
 		assertThat("incorrect toString()", ri.toString(),
 				is("RemoteIdentity [remoteID=RemoteIdentityID [provider=p, id=i], " +

src/test/java/us/kbase/test/auth2/lib/storage/mongo/MongoStorageTokensTest.java

@@ -42,9 +42,10 @@ TokenType.LOGIN, id, new UserName("bar"))
 					.withCustomContext("k1", "v1")
 					.withCustomContext("k2", "v2")
 					.build())
+			.withMfa(us.kbase.auth2.lib.identity.MfaStatus.NOT_USED)
 			.withTokenName(new TokenName("foo")).build();
 		storage.storeToken(store, "nJKFR6Xc4vzCeI3jT+FjlC9k5Q/qVw0zd0gi1erL8ew=");
-		
+
 		final StoredToken expected  = StoredToken.getBuilder(
 				TokenType.LOGIN, id, new UserName("bar"))
 			.withLifeTime(now, now.plusSeconds(20))
@@ -56,6 +57,7 @@ TokenType.LOGIN, id, new UserName("bar"))
 					.withCustomContext("k1", "v1")
 					.withCustomContext("k2", "v2")
 					.build())
+			.withMfa(us.kbase.auth2.lib.identity.MfaStatus.NOT_USED)
 			.withTokenName(new TokenName("foo")).build();
 		final StoredToken st = storage.getToken(new IncomingToken("sometoken").getHashedToken());
 		assertThat("incorrect token", st, is(expected));

src/test/java/us/kbase/test/auth2/lib/token/TokenTest.java

@@ -161,7 +161,7 @@ private void failCreateTemporaryToken(
 
 	@Test
 	public void storedTokenMSLifetime() throws Exception {
-		
+
 		final UUID id = UUID.randomUUID();
 		final StoredToken ht = StoredToken.getBuilder(TokenType.LOGIN, id, new UserName("whee"))
 				.withLifeTime(Instant.ofEpochMilli(1000), 4000).build();
@@ -175,6 +175,7 @@ public void storedTokenMSLifetime() throws Exception {
 				is(Instant.ofEpochMilli(5000)));
 		assertThat("incorrect context", ht.getContext(),
 				is(TokenCreationContext.getBuilder().build()));
+		assertThat("incorrect default mfa status", ht.getMfa(), is(us.kbase.auth2.lib.identity.MfaStatus.UNKNOWN));
 	}
 
 	@Test
@@ -183,6 +184,7 @@ public void storedTokenExpDateAndNameAndContext() throws Exception {
 		final StoredToken ht2 = StoredToken.getBuilder(TokenType.DEV, id2, new UserName("whee2"))
 				.withLifeTime(Instant.ofEpochMilli(27000), Instant.ofEpochMilli(42000))
 				.withContext(TokenCreationContext.getBuilder().withNullableDevice("d").build())
+				.withMfa(us.kbase.auth2.lib.identity.MfaStatus.USED)
 				.withTokenName(new TokenName("ugh")).build();
 		assertThat("incorrect token type", ht2.getTokenType(), is(TokenType.DEV));
 		assertThat("incorrect token name", ht2.getTokenName(),
@@ -195,6 +197,7 @@ public void storedTokenExpDateAndNameAndContext() throws Exception {
 				is(Instant.ofEpochMilli(42000)));
 		assertThat("incorrect context", ht2.getContext(),
 				is(TokenCreationContext.getBuilder().withNullableDevice("d").build()));
+		assertThat("incorrect mfa status", ht2.getMfa(), is(us.kbase.auth2.lib.identity.MfaStatus.USED));
 	}
 
 	@Test

src/test/java/us/kbase/test/auth2/providers/OrcIDIdentityProviderTest.java

@@ -542,22 +542,21 @@ public void getIdentityWithNoJWT() throws Exception {
 		final IdentityProviderConfig idconfig = getTestIDConfig();
 		final IdentityProvider idp = new OrcIDIdentityProvider(idconfig);
 		final String orcID = "0000-0001-1234-5678";
-		
+
 		setUpCallAuthToken(authCode, "footoken5", "https://ologinredir.com",
 				idconfig.getClientID(), idconfig.getClientSecret(), " My name ", orcID);
 		setupCallID("footoken5", orcID, APP_JSON, 200, MAPPER.writeValueAsString(
-				map("email", Arrays.asList(map("email", "[email protected]")))));
-		
-		// Now that JWT is required, this should fail with missing JWT error
-		try {
-			idp.getIdentities(authCode, "pkce", false, null);
-			fail("Expected IdentityRetrievalException");
-		} catch (IdentityRetrievalException e) {
-			assertThat("incorrect exception message", e.getMessage(), 
-					is("10030 Identity retrieval failed: No JWT token provided by ORCID despite requesting OpenID scope"));
-		}
+				map("email", Arrays.asList(map("email", "[email protected]")))));
+
+		// Missing JWT from non-member ORCID accounts should default to UNKNOWN MFA status
+		final Set<RemoteIdentity> rids = idp.getIdentities(authCode, "pkce", false, null);
+		assertThat("incorrect number of idents", rids.size(), is(1));
+		final Set<RemoteIdentity> expected = new HashSet<>();
+		expected.add(new RemoteIdentity(new RemoteIdentityID(ORCID, orcID),
+				new RemoteIdentityDetails(orcID, "My name", "[email protected]", MfaStatus.UNKNOWN)));
+		assertThat("incorrect ident set", rids, is(expected));
 	}
-	
+
 	@Test
 	public void getIdentityWithInvalidJWT() throws Exception {
 		final String authCode = "authcodeInvalidJWT";

src/test/java/us/kbase/test/auth2/service/ui/TokensTest.java

@@ -158,6 +158,7 @@ public void getTokensMinimalInput() throws Exception {
 						.with("name", null)
 						.with("user", "whoo")
 						.with("custom", Collections.emptyMap())
+						.with("mfa", "UNKNOWN")
 						.with("os", null)
 						.with("osver", null)
 						.with("agent", null)
@@ -260,6 +261,7 @@ public void getTokensMaximalInput() throws Exception {
 						.with("name", "wugga")
 						.with("user", "whoo")
 						.with("custom", ImmutableMap.of("foo", "bar"))
+						.with("mfa", "UNKNOWN")
 						.with("os", "o")
 						.with("osver", "osv")
 						.with("agent", "ag")
@@ -276,6 +278,7 @@ public void getTokensMaximalInput() throws Exception {
 								.with("name", "whee")
 								.with("user", "whoo")
 								.with("custom", Collections.emptyMap())
+								.with("mfa", "UNKNOWN")
 								.with("os", null)
 								.with("osver", null)
 								.with("agent", null)
@@ -291,6 +294,7 @@ public void getTokensMaximalInput() throws Exception {
 								.with("name", null)
 								.with("user", "whoo")
 								.with("custom", ImmutableMap.of("baz", "bat"))
+								.with("mfa", "UNKNOWN")
 								.with("os", null)
 								.with("osver", null)
 								.with("agent", "ag2")