feat(jans-fido2): retry MDS TOC download at startup when the blob is … - #14483
Conversation
…missing Signed-off-by: imran <imranishaq7071@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable FIDO2 MDS startup retry settings, asynchronous TOC loading, missing-content detection, thread-safe state handling, download outcome mapping, tests, schema updates, defaults, and documentation. ChangesMDS TOC startup retry
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 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 |
✅ 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. |
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}
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>
GitHub Actions
Inactive
GitHub Actions
Inactive
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>
GitHub Actions
Error
GitHub Actions
Inactive
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 (1)
docs/janssen-server/config-guide/fido2-config/janssen-fido2-configuration.md (1)
172-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank lines around the new headings.
Insert a blank line after Line 172 and before Line 185 to satisfy Markdown linting and keep section boundaries consistent.
Also applies to: 185-185
🤖 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 `@docs/janssen-server/config-guide/fido2-config/janssen-fido2-configuration.md` at line 172, Add blank lines around the “MySQL / PostgreSQL Layout” heading and the corresponding heading at the referenced later section, ensuring each heading is separated from surrounding content to satisfy Markdown linting.Source: Linters/SAST tools
🤖 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 `@docs/janssen-server/fido/fido2-server-properties-config.md`:
- Around line 58-59: Update TocService.java’s startup MDS TOC download loop to
treat mdsDownloadStartupRetries as retries after the initial attempt, setting
the maximum attempts to one plus the configured value while preserving the
existing retry interval behavior.
---
Outside diff comments:
In
`@docs/janssen-server/config-guide/fido2-config/janssen-fido2-configuration.md`:
- Line 172: Add blank lines around the “MySQL / PostgreSQL Layout” heading and
the corresponding heading at the referenced later section, ensuring each heading
is separated from surrounding content to satisfy Markdown linting.
🪄 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 Plus
Run ID: fda5a278-b3e2-44a2-9b0d-590dd9ff6bd2
📒 Files selected for processing (2)
docs/janssen-server/config-guide/fido2-config/janssen-fido2-configuration.mddocs/janssen-server/fido/fido2-server-properties-config.md
…h the cached-TOC fallback Signed-off-by: imran <imranishaq7071@gmail.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
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 (1)
jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java (1)
277-288: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInterrupted sleep doesn't stop the retry loop.
sleepBeforeRetry()restores the interrupt flag but returns normally, sofetchMetadata(boolean)'s loop (Lines 209-225) keeps issuing further download attempts even after the thread has been asked to stop (e.g. executor/app shutdown). Worth checking interruption after the call and breaking out.🔧 Suggested fix
log.warn("Attempt {}/{} to download the MDS TOC failed", attempt, maxAttempts); sleepBeforeRetry(); + if (Thread.currentThread().isInterrupted()) { + break; + } }🤖 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 277 - 288, Update fetchMetadata(boolean) to check the thread’s interrupted state immediately after sleepBeforeRetry() returns and exit the retry loop without issuing another download attempt when interrupted. Preserve the existing retry behavior for non-interrupted waits, using sleepBeforeRetry()’s restored interrupt status.
♻️ Duplicate comments (1)
jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java (1)
128-129: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
digesteris still not safely published across threads.
digesteris written byloadGlobalVariables()on the async startup/background thread and read viagetDigester()from request-handling threads, but it lostvolatileagain (unliketocEntries/nextUpdateright above it, which were correctly fixed). This reintroduces the same JMM visibility race raised on a prior commit of this PR; PMD'sAvoidMessageDigestFieldhint also still fires here, andMessageDigestitself is stateful/not thread-safe if concurrently shared.🔒 Minimal fix
- private MessageDigest digester; + private volatile MessageDigest digester;This restores safe publication; consider also avoiding a shared mutable
MessageDigestinstance across concurrentgetDigester()callers (e.g. resolve a fresh instance per use) sinceupdate()/digest()carry mutable state.Also applies to: 536-538
🤖 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 128 - 129, Update the digester field used by loadGlobalVariables() and getDigester() to be safely published across threads, and avoid sharing one mutable MessageDigest instance between concurrent callers by resolving a fresh digest per use where required. Preserve the existing tocEntries and nextUpdate publication behavior.Source: Linters/SAST tools
🤖 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 277-288: Update fetchMetadata(boolean) to check the thread’s
interrupted state immediately after sleepBeforeRetry() returns and exit the
retry loop without issuing another download attempt when interrupted. Preserve
the existing retry behavior for non-interrupted waits, using
sleepBeforeRetry()’s restored interrupt status.
---
Duplicate comments:
In `@jans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.java`:
- Around line 128-129: Update the digester field used by loadGlobalVariables()
and getDigester() to be safely published across threads, and avoid sharing one
mutable MessageDigest instance between concurrent callers by resolving a fresh
digest per use where required. Preserve the existing tocEntries and nextUpdate
publication behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30c4e05e-6977-415a-848b-597868c6474a
📒 Files selected for processing (3)
docs/janssen-server/fido/fido2-server-properties-config.mdjans-fido2/server/src/main/java/io/jans/fido2/service/mds/TocService.javajans-fido2/server/src/test/java/io/jans/fido2/service/mds/TocServiceTest.java
|
|
GitHub Actions
Failure
GitHub Actions
Failure
|
8 participants
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
mdsDownloadStartupRetriesandmdsDownloadStartupRetryInterval(defaults: 3 and 30 seconds) to tune FIDO2 MDS TOC download retries.