feat(jans-fido2): retry MDS TOC download at startup when the blob is …#14483
Open
imran-ishaq wants to merge 3 commits into
Open
feat(jans-fido2): retry MDS TOC download at startup when the blob is …#14483imran-ishaq wants to merge 3 commits into
imran-ishaq wants to merge 3 commits into
Conversation
…missing Signed-off-by: imran <imranishaq7071@gmail.com>
imran-ishaq
marked this pull request as ready for review
July 2, 2026 14:09
Contributor
📝 WalkthroughWalkthroughAdds two new FIDO2 configuration properties, updates defaults and documentation, and reworks TocService to fetch MDS TOC metadata asynchronously at startup with retry behavior when the TOC blob is missing. ChangesMDS TOC Startup Retry
Estimated code review effort: 3 (Moderate) | ~25 minutes Related issues: Suggested labels: fido2, enhancement, configuration Suggested reviewers: yuriyz, mzavgorodniy 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
imran-ishaq
requested review from
devrimyatar,
iromli,
moabu,
pujavs,
yurem,
yuriyz and
yuriyzz
as code owners
July 2, 2026 14:09
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
imran-ishaq
had a problem deploying
to
integration-tests
July 2, 2026 14:09 — with 
GitHub Actions
Error
GitHub Actions
Error
imran-ishaq
had a problem deploying
to
integration-tests
July 2, 2026 14:09 — with
GitHub Actions
Error
GitHub Actions
Error
mo-auto
added
area-documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java`:
- Around line 141-228: The new retry/startup flow in TocService is complex and
currently untested, especially the branching in fetchMetadata(boolean
retryWhenTocMissing), the missing-state check in isTocContentMissing(), and the
wait/interrupt behavior in sleepBeforeRetry(). Add focused unit tests for these
methods to cover the stale-vs-missing TOC path, max-attempt bounds, early exit
once the TOC appears, and interrupted sleep handling using the existing
TocService and its collaborators.
- Around line 103-116: The asynchronous `init()` in `TocService` now writes
`tocEntries` from a background thread without any publication guarantee, so
request threads in `getAuthenticatorsMetadata()` may keep seeing stale or null
data. Fix this by making `tocEntries` safely published, such as marking the
field `volatile` or switching it to an `AtomicReference`, and keep the write in
`refreshTOCEntries()` and reads in `getAuthenticatorsMetadata()` using that same
field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54d49c2d-b0ec-4a6f-bce3-cd811e5b9fa8
📒 Files selected for processing (7)
docker-jans-fido2/scripts/upgrade.pydocs/janssen-server/config-guide/fido2-config/janssen-fido2-configuration.mddocs/janssen-server/fido/config.mdjans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-fido2/model/src/main/java/io/jans/fido2/model/conf/Fido2Configuration.javajans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.javajans-linux-setup/jans_setup/templates/jans-fido2/dynamic-conf.json
Signed-off-by: imran <imranishaq7071@gmail.com>
imran-ishaq
temporarily deployed
to
integration-tests
July 2, 2026 15:04 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
imran-ishaq
temporarily deployed
to
integration-tests
July 2, 2026 15:04 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java (3)
120-126: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPublish
tocEntriesonly after it is fully populated.Line 121 assigns the new map before
parseTOCs()completes, so request threads can observe an empty TOC during refresh. Build a local map first, then assign it once.Suggested fix
public void refreshTOCEntries() { - this.tocEntries = Collections.synchronizedMap(new HashMap<>()); + Map<String, JsonNode> loadedTocEntries = Collections.synchronizedMap(new HashMap<>()); if (appConfiguration.getFido2Configuration().isDisableMetadataService()) { log.debug("SkipDownloadMds is enabled"); } else { - tocEntries.putAll(parseTOCs()); + loadedTocEntries.putAll(parseTOCs()); } + this.tocEntries = loadedTocEntries; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java` around lines 120 - 126, The refreshTOCEntries() method in TocService publishes tocEntries too early by assigning a new synchronized map before parseTOCs() finishes, which can expose an empty TOC to concurrent readers. Build and populate a local map first (including the parseTOCs() result), then assign it to the tocEntries field only after it is fully populated, keeping the existing disableMetadataService branch behavior intact.
149-167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat configured retries as retries, not total attempts.
The PR objective and config name describe
mdsDownloadStartupRetriesas retries. This loop uses it as total attempts, so the default3produces only 2 retries after the first attempt.Suggested fix
int maxAttempts = 1; if (retryWhenTocMissing && isTocContentMissing()) { - maxAttempts = Math.max(1, appConfiguration.getFido2Configuration().getMdsDownloadStartupRetries()); - log.info("MDS TOC blob is missing at startup, will attempt to download it up to {} time(s)", maxAttempts); + int retries = Math.max(0, appConfiguration.getFido2Configuration().getMdsDownloadStartupRetries()); + maxAttempts = 1 + retries; + log.info("MDS TOC blob is missing at startup, will attempt the initial download plus {} retry(ies)", retries); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java` around lines 149 - 167, The retry loop in TocService is treating mdsDownloadStartupRetries as total attempts instead of retries, so the first fetch is consuming one of the configured retries. Update the logic around fetchMetadataOnce() to always perform the initial attempt, then allow the configured number of additional retry attempts when isTocContentMissing() remains true. Keep the existing retry guard and logging, but adjust maxAttempts/loop bounds so the configured value reflects retries rather than total attempts.
371-383: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd explicit connect/read timeouts before opening the MDS URL.
URL.openStream()can block indefinitely on a slow or unresponsive endpoint, which can stall the retry loop and tie up the async worker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java` around lines 371 - 383, The downloadMdsFromServer method currently uses URL.openStream() without any network timeouts, which can block indefinitely. Update this path in TocService so the metadataUrl connection is opened explicitly with connect and read timeouts before reading the stream, then keep the existing IOUtils/base64Service/persistTocDocument flow unchanged. Make the timeout values configurable or use sensible defaults, and preserve the current error handling in the catch block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java`:
- Around line 120-126: The refreshTOCEntries() method in TocService publishes
tocEntries too early by assigning a new synchronized map before parseTOCs()
finishes, which can expose an empty TOC to concurrent readers. Build and
populate a local map first (including the parseTOCs() result), then assign it to
the tocEntries field only after it is fully populated, keeping the existing
disableMetadataService branch behavior intact.
- Around line 149-167: The retry loop in TocService is treating
mdsDownloadStartupRetries as total attempts instead of retries, so the first
fetch is consuming one of the configured retries. Update the logic around
fetchMetadataOnce() to always perform the initial attempt, then allow the
configured number of additional retry attempts when isTocContentMissing()
remains true. Keep the existing retry guard and logging, but adjust
maxAttempts/loop bounds so the configured value reflects retries rather than
total attempts.
- Around line 371-383: The downloadMdsFromServer method currently uses
URL.openStream() without any network timeouts, which can block indefinitely.
Update this path in TocService so the metadataUrl connection is opened
explicitly with connect and read timeouts before reading the stream, then keep
the existing IOUtils/base64Service/persistTocDocument flow unchanged. Make the
timeout values configurable or use sensible defaults, and preserve the current
error handling in the catch block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1da8c2e-9e21-4766-8a80-7d418d42e6a3
📒 Files selected for processing (3)
jans-fido2/model/src/main/java/io/jans/fido2/model/conf/Fido2Configuration.javajans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.javajans-fido2/server/src/test/java/io/jans/fido2/service/mds/TocServiceTest.java
Signed-off-by: imran <imranishaq7071@gmail.com>
imran-ishaq
had a problem deploying
to
integration-tests
July 3, 2026 05:27 — with
GitHub Actions
Error
GitHub Actions
Error
imran-ishaq
temporarily deployed
to
integration-tests
July 3, 2026 05:27 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java (2)
154-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
mdsDownloadStartupRetriesis used as total attempts, not retries.
maxAttempts = getMdsDownloadStartupRetries()means a value of3yields 3 total downloads (1 initial + 2 retries), yet the property name and@DocPropertyinFido2Configurationdescribe it as the number of times the download is retried. This off-by-one between the documented semantics and behavior can mislead operators tuning the value. Either treat it asmaxAttempts = retries + 1, or align the property description to state it is the total number of attempts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java` around lines 154 - 158, The TOC startup download limit in TocService is treating mdsDownloadStartupRetries as total attempts instead of retries, which is inconsistent with the Fido2Configuration property documentation. Update the logic in TocService so the loop limit reflects retries plus the initial attempt, or alternatively change the property’s documented semantics to clearly state it configures total attempts; keep the behavior and docs aligned around getMdsDownloadStartupRetries().
229-239: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInterrupt during retry wait doesn't stop the retry loop.
sleepBeforeRetry()correctly restores the interrupt flag, but control returns to theforloop infetchMetadata(boolean), which continues to the next attempt. On the following iterationThread.sleep(...)will immediately re-throwInterruptedException, so the remaining attempts run back-to-back with no delay (busy retrying during e.g. shutdown). Consider aborting the loop when interrupted.🔧 Suggested fix
- sleepBeforeRetry(); + sleepBeforeRetry(); + if (Thread.currentThread().isInterrupted()) { + log.warn("MDS TOC download retry loop interrupted, aborting remaining attempts"); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java` around lines 229 - 239, The interrupt handling in sleepBeforeRetry is restoring the flag but still allows fetchMetadata to continue retrying, which causes the remaining attempts to run immediately. Update the retry flow in fetchMetadata and/or sleepBeforeRetry so that an InterruptedException causes the retry loop to abort early, using the existing sleepBeforeRetry and fetchMetadata symbols to locate the control flow. Keep the interrupt status restored, then return or break out of the loop instead of proceeding to the next retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java`:
- Around line 105-106: Restore safe cross-thread publication for the shared
MessageDigest in TocService: the digester field is initialized in
loadGlobalVariables() on the async startup path and read later by getDigester(),
so it needs volatile again or another safe-publication mechanism. Also address
the thread-safety risk of sharing a single MessageDigest instance across request
threads by avoiding a mutable singleton digest state or by creating/using a
per-call instance within getDigester().
---
Outside diff comments:
In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java`:
- Around line 154-158: The TOC startup download limit in TocService is treating
mdsDownloadStartupRetries as total attempts instead of retries, which is
inconsistent with the Fido2Configuration property documentation. Update the
logic in TocService so the loop limit reflects retries plus the initial attempt,
or alternatively change the property’s documented semantics to clearly state it
configures total attempts; keep the behavior and docs aligned around
getMdsDownloadStartupRetries().
- Around line 229-239: The interrupt handling in sleepBeforeRetry is restoring
the flag but still allows fetchMetadata to continue retrying, which causes the
remaining attempts to run immediately. Update the retry flow in fetchMetadata
and/or sleepBeforeRetry so that an InterruptedException causes the retry loop to
abort early, using the existing sleepBeforeRetry and fetchMetadata symbols to
locate the control flow. Keep the interrupt status restored, then return or
break out of the loop instead of proceeding to the next retry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bb9f826-5f74-485c-8522-231730eead68
📒 Files selected for processing (2)
jans-fido2/model/src/main/java/io/jans/fido2/model/conf/Fido2Configuration.javajans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java
|
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
At startup, the FIDO2 server (jans-fido2) fetches the FIDO Alliance MDS TOC blob (toc.jwt) exactly once via TocService.init() → fetchMetadata(). There is no retry loop. If that single download fails — e.g. a transient network glitch or a temporary outage of https://mds.fidoalliance.org/ — the server comes up with a missing/empty TOC in the DB and cannot validate authenticator attestations. The only subsequent attempt is the 24-hour MDS3UpdateTimer, so the server can remain without metadata for up to a day.
Additionally, the download runs synchronously on the ApplicationInitialized observer thread, so a slow/hanging MDS endpoint blocks application initialization.
A missing TOC specifically breaks the FIDO2 server (attestation validation), whereas a merely stale-but-present TOC still works — so the two cases should be treated differently.
Target issue
When the TOC blob is missing at startup, the server should retry the download a few times (configurable) before giving up, to ride out transient MDS outages.
Retries should apply only to the missing-TOC case, not to a stale-but-present TOC (which keeps the single-attempt behavior, matching the daily timer).
The startup download/retry should not block application initialization — it should run in the background.
closes #14482
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit