fix(broker): replay terminal auth on stale Entra MFA requests

[shardool-patil]

Summary

When an Entra ID authentication succeeds through MFA (entra_mfa_wait or entra_mfa_code), clearEntraMFAState() is invoked to free the underlying C memory, setting mfaFlowActive = nil. A late or duplicate polling/code request arriving shortly after AuthGranted was encountering mfaFlowActive == nil and falling through to an error branch, returning an unexpected error ("no active MFA flow").

This PR introduces a safe replay mechanism that records the completed MFA mode and response upon successful finalization, allowing subsequent duplicate requests for that exact MFA mode to replay AuthGranted idempotently without re-invoking the single-use MFA flow.

Changes

• Session State Tracking: Added completedMFAMode and completedMFAResponse to the session struct to record terminal Entra MFA results.
• Flow-Specific Finalization: Scoped terminal state recording strictly to finishEntraAuth, keeping the shared finishAuth helper generic and unaffected.
• Centralized Replay Helper: Added replayCompletedMFA() to validate that incoming duplicate requests match the completed mode before returning the cached AuthGranted payload.
• Guarded Execution: Replay checks in entraMFAWaitAuth and entraMFACodeAuth execute only after provider validation and when session.mfaFlowActive == nil.
• State Reset: Updated clearEntraMFAState() to clear completedMFAMode and completedMFAResponse on cancellation, restart, or terminal failure.
• Preserved Error Routing: Maintained AADSTS500121 handling in routeMFAInitError with a retry delay notice when previous MFA requests were abandoned.
• Unit Tests: Added regression tests covering stale wait replays, stale code replays, and mode mismatch denials (TestIsAuthenticatedEntraMFA*).

Related Issues

Closes #1810

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.53%. Comparing base (48be1e6) to head (7b0e3b2).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1817      +/-   ##
==========================================
- Coverage   88.19%   85.53%   -2.67%     
==========================================
  Files          96       26      -70     
  Lines        7092     1963    -5129     
  Branches      112        0     -112     
==========================================
- Hits         6255     1679    -4576     
+ Misses        781      284     -497     
+ Partials       56        0      -56     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Fixes stale Entra MFA requests by replaying the completed authentication result.

Changes:

• Caches terminal authentication responses and clears follow-up modes.
• Replays successful MFA wait/code results.
• Adds duplicate-request regression tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
broker.go	Adds terminal result caching and replay guards.
broker_test.go	Tests stale MFA wait and code requests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[nooreldeenmansour]

Thanks a lot @shardool-patil for the contribution!

Apart from the copilot finding which you addressed, here are a few comments to consider.

This patch (relative to main) addresses the inline comments, you can adopt it, but you will need to update the tests along with it

Proposed diff

diff --git i/authd-oidc-brokers/internal/broker/broker.go w/authd-oidc-brokers/internal/broker/broker.go
index 4d3b3a181..49c3ec92d 100644
--- i/authd-oidc-brokers/internal/broker/broker.go
+++ w/authd-oidc-brokers/internal/broker/broker.go
@@ -101,6 +101,13 @@ type session struct {
 	mfaChallengeInfo   *himmelblau.MFAChallengeInfo
 	entraPasswordHash  string // pre-computed hash (not plaintext) for offline use
 
+	// completedMFAMode and completedMFAResponse record a terminal Entra MFA
+	// result after all finalization steps succeed. They let a late follow-up
+	// request for the same MFA mode replay the result without invoking the
+	// single-use MFA flow again.
+	completedMFAMode     string
+	completedMFAResponse isAuthenticatedDataResponse
+
 	isAuthenticating *isAuthenticatedCtx
 }
 
@@ -1595,6 +1602,20 @@ func clearEntraMFAState(session *session) {
 	himmelblau.FreeMFAFlowState(session.mfaFlowActive)
 	session.mfaFlowActive = nil
 	session.mfaChallengeInfo = nil
+	session.completedMFAMode = ""
+	session.completedMFAResponse = nil
+}
+
+// replayCompletedMFA returns the cached terminal result for mode if an Entra
+// MFA flow for that exact mode has already completed successfully. It is a
+// fallback for stale or duplicate client requests that arrive after the
+// single-use MFA flow state has been released.
+func replayCompletedMFA(session *session, mode string) (string, isAuthenticatedDataResponse, bool) {
+	if session.completedMFAMode != mode || session.completedMFAResponse == nil {
+		return "", nil, false
+	}
+	log.Debugf(context.Background(), "Stale %s request after successful authentication for user %q, replaying terminal result", mode, session.username)
+	return AuthGranted, session.completedMFAResponse, true
 }
 
 // cachedDeviceRegistrationData returns the device registration data from the
@@ -1615,6 +1636,9 @@ func (b *Broker) entraMFAWaitAuth(ctx context.Context, session *session) (string
 	}
 
 	if session.mfaFlowActive == nil {
+		if access, data, ok := replayCompletedMFA(session, authmodes.EntraMFAWait); ok {
+			return access, data
+		}
 		log.Error(context.Background(), "MFA wait mode selected but no active MFA flow")
 		return AuthDenied, unexpectedErrMsg("no active MFA flow")
 	}
@@ -1719,6 +1743,9 @@ func (b *Broker) entraMFACodeAuth(ctx context.Context, session *session, code st
 	}
 
 	if session.mfaFlowActive == nil {
+		if access, data, ok := replayCompletedMFA(session, authmodes.EntraMFACode); ok {
+			return access, data
+		}
 		log.Error(context.Background(), "MFA code mode selected but no active MFA flow")
 		return AuthDenied, unexpectedErrMsg("no active MFA flow")
 	}
@@ -1884,6 +1911,9 @@ func (b *Broker) finishEntraAuth(ctx context.Context, session *session, mfaToken
 		}
 	}
 
+	session.completedMFAMode = session.selectedMode
+	session.completedMFAResponse = data
+	session.nextAuthModes = nil
 	return access, data
 }

[shardool-patil]

@nooreldeenmansour please take a look, i have fixed the code and now it is ready for review.

[nooreldeenmansour]

LGTM! Thanks for your contribution @shardool-patil

Can you please update the PR description (and the title if necessary), considering that the PR description is stale now.

FYI, I've added two tests to improve the coverage.

[shardool-patil]

LGTM! Thanks for your contribution @shardool-patil

Can you please update the PR description (and the title if necessary), considering that the PR description is stale now.

FYI, I've added two tests to improve the coverage.

Updated the PR description to include what has been added since the initial code and thanks for the review and additional tests @nooreldeenmansour .

[adombeck]

See #1810 (comment)