[ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response `mfa` key #471

dauglyon · 2025-07-15T17:56:59Z

Adds MFA boolean to RemoteIdentity and StoredToken classes, which get stored in Mongo. Adds JWT parsing to extract the AMR claim from the Orcid login token during login. Modifies the token to include MFA status.

A nice summary of all the changes from claude:

Key Changes

Core Implementation:

Added mfa field to StoredToken (stored as mfa in MongoDB)
Extended RemoteIdentityDetails to capture MFA status from identity providers
Updated APIToken to expose MFA status via /api/V2/token endpoint

ORCID Provider Enhancement:

Changed scope from /authenticate to openid to receive JWT ID tokens
Parse Authentication Method Reference (AMR) claim to detect MFA usage
Set mfa=USED when AMR contains "mfa"

Authentication Flow:

OAuth login: MFA status determined from identity provider and stored on LOGIN token
Password login: MFA status set to uknown (not applicable)
Agent/Dev/Service tokens: MFA status set to uknown (no authentication context)

API Response

The /api/V2/token endpoint now returns mfa with tri-state semantics:

USED: User authenticated with MFA during token creation (e.g. Orcid with MFA)
NOT_USED: User explicitly chose not to use MFA when available (e.g. Orcid without MFA)
UNKNOWN: MFA status unknown or not applicable to authentication method (e.g. Google (mfa inspection not supported), a non-member Orcid API client account (openid token not supported), or a dev token)

Database Schema

Tokens collection: Added mfa field
Users collection: Added mfa field to identity objects

…; add MFA tests

src/main/java/us/kbase/auth2/lib/identity/RemoteIdentityDetails.java

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

src/main/java/us/kbase/auth2/service/api/MfaStatus.java

…p code

Update tests to handle MFA field additions and ORCID OpenID Connect changes: - Fix hash codes in RemoteIdentityTest for new MFA field - Update ORCID provider tests for openid scope instead of /authenticate - Add mfaAuthenticated field to API response expectations - Fix non-ORCID provider test to use null MFA status - Update identity ordering in user endpoint tests

codecov · 2025-07-16T02:57:53Z

Codecov Report

❌ Patch coverage is 94.56522% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.35%. Comparing base (47f6a01) to head (a4a0262).

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #471      +/-   ##
=============================================
- Coverage      93.37%   93.35%   -0.02%     
- Complexity      2151     2171      +20     
=============================================
  Files            126      127       +1     
  Lines           7558     7634      +76     
  Branches        1184     1207      +23     
=============================================
+ Hits            7057     7127      +70     
- Misses           458      459       +1     
- Partials          43       48       +5

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Move MFA status from being computed from user identities to being stored as a boolean field on tokens themselves. This provides per-token MFA tracking and eliminates the need for complex identity lookups.

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

dauglyon · 2025-08-13T23:08:35Z

See I need another test or two to have complete coverage, will add.

- Add getMfaStatus helper method to handle null MFA fields in database - Update token and identity deserialization to use helper method - Add tests for backward compatibility with existing database records - Prevents NullPointerException when MFA field is missing from stored data

- Update scope in login URL tests from 'openid' to 'openid+%2Fauthenticate' - Add JWT tokens to tests that were missing them after OpenID Connect changes - Update getIdentityWithNoJWT test to expect new error when JWT is missing - Fix error condition tests to provide valid JWTs so they test intended errors

- Add MFA field to ExternalToken base class with UNKNOWN default - Remove duplicate MFA handling from APIToken (now inherited) - Ensures all token endpoints return MFA status for UI compatibility - Non-ORCID providers default to MFA status UNKNOWN

…lsverifier contract test. this resolves the hardcoded hashcodes causing stochastic test failures depending on execution order/enviroment, test cleanup to follow

MrCreosote

Review is already huge so skipping tests until next time

MrCreosote · 2025-09-23T21:03:22Z

RELEASE_NOTES.md

According to codecov there's still some uncovered code, which should be coverable since the orcid interactions are mocked and everything else can be unit tested

MrCreosote · 2025-09-23T21:04:47Z

src/main/java/us/kbase/auth2/lib/Authentication.java

+	private NewToken login(final UserName userName, final TokenCreationContext tokenCtx, 
+			final MfaStatus mfa) throws AuthStorageException {


Suggested change

private NewToken login(final UserName userName, final TokenCreationContext tokenCtx,

final MfaStatus mfa) throws AuthStorageException {

private NewToken login(

final UserName userName,

final TokenCreationContext tokenCtx,

final MfaStatus mfa

) throws AuthStorageException {

My rule is either all args on 1 line or each arg on its own line. More readable

MrCreosote · 2025-09-23T21:13:15Z

src/main/java/us/kbase/auth2/lib/identity/MfaStatus.java

You might want to rework this in a similar way to TokenType so that we can change the enum name without breaking things later:

https://github.com/kbase/auth2/blob/47f6a0160a6c3bec09f49a6623b3decb3d647a8d/src/main/java/us/kbase/auth2/lib/token/TokenType.java

MrCreosote · 2025-09-23T21:13:40Z

src/main/java/us/kbase/auth2/lib/identity/RemoteIdentityDetails.java

 		} else {
 			this.email = email.trim();
 		}
+		this.mfa = mfa;


Suggested change

this.mfa = mfa;

this.mfa = requireNonNull(mfa, "mfa");

MrCreosote · 2025-09-23T21:14:47Z

src/main/java/us/kbase/auth2/lib/identity/RemoteIdentityDetails.java

 		int result = 1;
 		result = prime * result + ((email == null) ? 0 : email.hashCode());
 		result = prime * result + ((fullname == null) ? 0 : fullname.hashCode());
+		result = prime * result + ((mfa == null) ? 0 : mfa.name().hashCode());


Suggested change

result = prime * result + ((mfa == null) ? 0 : mfa.name().hashCode());

result = prime * result + ((mfa == null) ? 0 : mfa.hashCode());

Getting the hash directly from the enum class is fine, it's done this way in other places in the codebase.

There's a couple other places in the PR where this should be fixed

MrCreosote · 2025-09-23T21:19:56Z

src/main/java/us/kbase/auth2/lib/storage/mongo/MongoStorage.java

 				.append(pre + Fields.IDENTITIES_EMAIL, rid.getEmail())
-				.append(pre + Fields.IDENTITIES_NAME, rid.getFullname()));
+				.append(pre + Fields.IDENTITIES_NAME, rid.getFullname())
+				.append(pre + Fields.IDENTITIES_MFA, rid.getMfa().name()));


This is updating the user document, which I don't think you want to do

MrCreosote · 2025-09-23T21:23:14Z

src/main/java/us/kbase/auth2/lib/token/StoredToken.java


+		@Override
+		public OptionalsStep withMfa(final MfaStatus mfa) {
+			this.mfa = mfa;


Suggested change

this.mfa = mfa;

this.mfa = requireNonNull(mfa, "mfa");

MrCreosote · 2025-09-23T21:25:29Z

src/main/java/us/kbase/auth2/lib/Authentication.java

 	}

+
 	/** Get the external configuration without providing any credentials.


Suggested change

}

/** Get the external configuration without providing any credentials.

}

/** Get the external configuration without providing any credentials.

MrCreosote · 2025-09-23T21:26:25Z

src/main/java/us/kbase/auth2/service/common/ExternalToken.java

 		created = storedToken.getCreationDate().toEpochMilli();
 		custom = storedToken.getContext().getCustomContext();
+		// For tokens from non-ORCID providers, MFA status defaults to UNKNOWN
+		mfa = storedToken.getMfa() != null ? storedToken.getMfa() : MfaStatus.UNKNOWN;


Suggested change

mfa = storedToken.getMfa() != null ? storedToken.getMfa() : MfaStatus.UNKNOWN;

mfa = storedToken.getMfa()

Can't be null with the other changes in this review

MrCreosote · 2025-09-23T21:27:02Z

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

 	}

+
 }


Suggested change

}

}

}

}

MrCreosote

Sorry, hit the submit button too early

MrCreosote · 2025-09-23T21:32:43Z

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

 		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);


Can you handle logging like this:

auth2/src/main/java/us/kbase/auth2/lib/Authentication.java

Lines 325 to 327 in 47f6a01

private void logInfo(final String format, final Object... params) {

LoggerFactory.getLogger(getClass()).info(format, params);

}

There's some weirdness I've encountered in the past where a static logger could cause (I think) permgen OOMs. Probably won't happen here but just to be safe

MrCreosote · 2025-09-23T21:35:42Z

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

 					(String) m.get("access_token"),
-					(String) m.get("name"),
-					(String) m.get("orcid"));
+					fullName != null ? fullName : "",


Why this change? It's backwards incompatible and all the other providers return null if there's no full name

A mistake, artifact from when I was trying to use only one auth call

MrCreosote · 2025-09-23T21:37:05Z

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

+		 * to determine if multi-factor authentication 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


Suggested change

* @return MfaStatus indicating whether MFA was used, UNKNOWN if JWT is missing or unparseable

* @return MfaStatus indicating whether MFA was used, UNKNOWN if JWT is missing

although from a comment from review # 1, it sounds like missing should throw an error?

I couldn't figure out how to make the error informative to the user and not just crap-out without a lot of changes to other parts of the code. Perhaps an mfa:ERROR state? Right now I'm just printing to logs that the error occurred

MrCreosote · 2025-09-23T21:38:18Z

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

+			} catch (IllegalArgumentException e) {
+				// 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);
+			}


Can you move these up to specifically catch errors from the lines that throw them? Ensures we don't accidentally mask unexpected errors

MrCreosote · 2025-09-23T21:44:35Z

It's been a while since the first time I reviewed it but I get the feeling the updated implementation is much more straightforward

Add openid role and mfa parsing to Orcid provider; support MFA status…

e78616e

…; add MFA tests

dauglyon commented Jul 15, 2025

View reviewed changes

src/main/java/us/kbase/auth2/lib/identity/RemoteIdentityDetails.java Outdated Show resolved Hide resolved

dauglyon commented Jul 15, 2025

View reviewed changes

src/main/java/us/kbase/auth2/providers/OrcIDIdentityProviderFactory.java Outdated Show resolved Hide resolved

dauglyon commented Jul 15, 2025

View reviewed changes

src/main/java/us/kbase/auth2/service/api/MfaStatus.java Outdated Show resolved Hide resolved

dauglyon assigned dauglyon and unassigned dauglyon Jul 15, 2025

This comment has been minimized.

Sign in to view

dauglyon added 4 commits July 15, 2025 12:05

move MFA status to token endpoint; remove mfaStatus endpoint; clean u…

984fdb2

…p code

fix build errors

e23c5dd

reorder expected html for test

99f0573

dauglyon added 6 commits July 15, 2025 20:23

Improve test coverage; improve error handling

748c8a2

fix exception errors

589d52a

Update ORCID provider tests to use MockServer

099ca8d

remove duplicate tests

4cbf2ec

fix TokenEndpointTest.getToken

ef094fd

fix TokenEndpointTest.getToken

ac606a5

dauglyon marked this pull request as ready for review July 17, 2025 17:17

dauglyon changed the title ~~MFA Support~~ Orcid Provider MFA Support & Token response mfaAuthenticated key Jul 17, 2025

dauglyon changed the title ~~Orcid Provider MFA Support & Token response mfaAuthenticated key~~ [ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response mfaAuthenticated key Jul 17, 2025

dauglyon added 2 commits July 17, 2025 12:31

Store MFA authentication status directly on tokens

9dfc0a7

Move MFA status from being computed from user identities to being stored as a boolean field on tokens themselves. This provides per-token MFA tracking and eliminates the need for complex identity lookups.

update tests

53ed176

dauglyon commented Jul 17, 2025

View reviewed changes

src/main/java/us/kbase/auth2/providers/OrcIDIdentityProviderFactory.java Outdated Show resolved Hide resolved

fix tests

ae74a21

dauglyon force-pushed the MFAStatus branch from 202a9b8 to ae74a21 Compare July 17, 2025 21:46

dauglyon added 2 commits July 17, 2025 15:17

slightly better documentation

935f193

update token field name

7bbf759

dauglyon force-pushed the MFAStatus branch from 1f38446 to ff8e201 Compare July 17, 2025 22:57

consolidate test logic

3d9e93e

dauglyon added 8 commits July 18, 2025 13:49

Change mfa value to enum, from boolean

8d85ad9

rename all uses MfaAuthenticated -> mfa

2db7814

fix tests

328704c

rename all uses MfaAuthenticated -> mfa

90ce1af

rename jwt field for clarity

58a2aec

fix tests, equalities

c766704

throw errors for jwt parsing issues

37d2ef6

add check for null/missing AMR claim, fix tests

62d6066

dauglyon force-pushed the MFAStatus branch from 591dbbb to 62d6066 Compare July 18, 2025 23:12

dauglyon added 3 commits August 12, 2025 16:10

Fix hashCode implementations

f45fb6e

Fix hascode expectations

bd080e6

Fix test ordering expectation for identity list

ec619c6

dauglyon changed the title ~~[ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response mfaAuthenticated key~~ [ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response mfa key Aug 13, 2025

dauglyon requested a review from MrCreosote August 13, 2025 23:02

dauglyon added 3 commits August 28, 2025 15:28

include both scopes, minor refactor

d43d01e

Fix null fullname handling in ORCID OAuth flow

6768eaf

dauglyon force-pushed the MFAStatus branch 3 times, most recently from 545f705 to e8fdd6a Compare September 8, 2025 21:00

dauglyon force-pushed the MFAStatus branch from e8fdd6a to c46b2da Compare September 8, 2025 21:15

dauglyon and others added 3 commits September 8, 2025 18:10

Add MFA status to all token responses

697b8c9

- Add MFA field to ExternalToken base class with UNKNOWN default - Remove duplicate MFA handling from APIToken (now inherited) - Ensures all token endpoints return MFA status for UI compatibility - Non-ORCID providers default to MFA status UNKNOWN

Replace hashcode assertions with equaility assertions, backed by equa…

164e3cb

…lsverifier contract test. this resolves the hardcoded hashcodes causing stochastic test failures depending on execution order/enviroment, test cleanup to follow

Improve ORCID JWT error handling and test robustness

a4a0262

dauglyon force-pushed the MFAStatus branch from 869cecf to a4a0262 Compare September 22, 2025 21:02

MrCreosote reviewed Sep 23, 2025

View reviewed changes

		private NewToken login(final UserName userName, final TokenCreationContext tokenCtx,
		final MfaStatus mfa) throws AuthStorageException {

	result = prime * result + ((mfa == null) ? 0 : mfa.name().hashCode());
	result = prime * result + ((mfa == null) ? 0 : mfa.hashCode());

		}


		/** Get the external configuration without providing any credentials.

	mfa = storedToken.getMfa() != null ? storedToken.getMfa() : MfaStatus.UNKNOWN;
	mfa = storedToken.getMfa()

-	}
-}
+	}
+}

	private void logInfo(final String format, final Object... params) {
	LoggerFactory.getLogger(getClass()).info(format, params);
	}

	* @return MfaStatus indicating whether MFA was used, UNKNOWN if JWT is missing or unparseable
	* @return MfaStatus indicating whether MFA was used, UNKNOWN if JWT is missing

[ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response mfa key #471

Are you sure you want to change the base?

[ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response mfa key #471

Conversation

dauglyon commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

codecov bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

dauglyon commented Aug 13, 2025

Uh oh!

MrCreosote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCreosote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCreosote commented Sep 23, 2025

Uh oh!

Uh oh!

[ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response `mfa` key #471

[ CDM-243 ] [ CDM-245 ] Orcid Provider MFA Support & Token response `mfa` key #471

dauglyon commented Jul 15, 2025 •

edited

Loading

codecov bot commented Jul 16, 2025 •

edited

Loading