[INJIWEB-1793] Technical documentation for VC Status check design#995
Open
jackjain wants to merge 23 commits into
Open
[INJIWEB-1793] Technical documentation for VC Status check design#995jackjain wants to merge 23 commits into
jackjain wants to merge 23 commits into
Conversation
Signed-off-by: jackjain <jackjain1995@gmail.com>
…check endpoints Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
…add caching strategy Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: jackjain <jackjain1995@gmail.com>
WalkthroughA new design document adds a complete Verifiable Credentials (VC) status-check system: workflows for view-time, user-initiated, and add-time checks; API contracts; DB schema additions and migration notes; a 24-hour status-list credential cache; and VC Verifier library adjustments to accept and return status-list credentials. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API
participant DB as Database
participant Cache as StatusListCache
participant Verifier as VCVerifier
participant External as ExternalStatusList
Client->>API: Request credential(s) with status
API->>DB: Fetch stored VC records
DB-->>API: Return VC(s)
API->>Cache: Check status-list credential (per VC)
alt Cache hit
Cache-->>API: Return cached status-list credential
else Cache miss
API->>Verifier: Request status-list credential (verify & fetch)
Verifier->>External: Fetch status-list credential
External-->>Verifier: Return status-list credential
Verifier-->>API: Return status-list credential result
API->>Cache: Store status-list credential (24h TTL)
end
API->>Verifier: Verify VC schema/signature and check status using status-list credential
Verifier-->>API: Return isSchemaAndSignatureValid, isExpired, statusChecks
API->>DB: Optionally update `status_checks` and `status_last_checked_at`
API-->>Client: Respond with VC(s) + status fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@docs/VCStatusCheckDesign.md`:
- Line 300: The NOT NULL constraint on the status_last_checked_at column will
block inserting newly created credentials before an async status check; update
the schema or docs so insertion succeeds by either making status_last_checked_at
nullable or assigning a sensible default (e.g., set status_last_checked_at =
created_at) during credential creation, and clearly document the chosen behavior
in the VCStatusCheckDesign.md (referencing status_last_checked_at and the
credential creation flow/Flow 3) so implementers know whether the check is async
and when the column is populated.
- Line 66: The doc shows inconsistent lastCheckedAt semantics: Flow 1 sets
lastCheckedAt to the cache creation time while Flow 2 sets it to the current
time; pick a single authoritative behavior (either always set lastCheckedAt to
the actual check time or always use the cache creation timestamp) and update the
design text to standardize it (or explicitly document the intentional
difference). Update the sections referencing lastCheckedAt (the Flow 1
description that ties it to "cache creation time" and the Flow 2 description
that sets it to "current time") so both flows use the chosen convention and add
a brief note explaining the rationale and any impact on cache staleness or
auditability.
- Around line 341-345: The field naming is inconsistent: update the data class
CredentialStatusResult to reflect that multiple status list credentials may be
returned by renaming statusListCredential to statusListCredentials and changing
its type to List<JsonLDObject>? (or MutableList/Sequence as appropriate), then
update all references, usages and documentation (design text lines mentioning
"status list credential(s)" and "status list credential cache") to use the
plural name and iterate over the list where multiple lists are expected so
callers handle zero/one/many uniformly; alternatively, if you intend a
single-entry result, adjust the design text to explicitly state it's only one
credential and keep the singular name across code and docs.
- Line 297: The is_expired column should not be nullable: change the schema for
is_expired to NOT NULL with a sensible default (e.g., DEFAULT FALSE) and add a
migration to backfill existing NULL values (compute expiration from the
credential's expires_at or set FALSE if unknown); update any code paths that
create or refresh credentials to explicitly compute and set is_expired based on
current time and the credential expiry field so it remains deterministic;
alternatively, if NULL must be preserved for a documented state, add
documentation explaining exactly when is_expired can be NULL instead of changing
the schema.
- Around line 302-305: The migration currently sets
verifiable_credential.is_schema_signature_valid = true and
status_last_checked_at = now() for all rows which misrepresents unverified
credentials; change the migration to initialize is_schema_signature_valid to
NULL (or add a new enum/status value like "not_yet_verified") and set
status_last_checked_at = NULL, enqueue all credential IDs for asynchronous
verification via the existing status-check worker (or create a background job)
to update is_schema_signature_valid and status_last_checked_at when verified,
and update docs/UI to surface the "not_yet_verified" state so users know which
credentials have not been verified yet.
- Around line 155-162: The sequence diagram's Flow 3 contradictorily returns an
error and then proceeds to store the credential and show a success—fix by
treating "status list not applicable" as a normal (non-error) path: remove the
error-return and user-error steps (the lines where "Mimoto-->>InjiWeb: return
error" and "InjiWeb-->User: Show error message to the user") so the flow remains
Mimoto->>Database: Store credential (verifiable_credential table) ->
Database-->>Mimoto: Acknowledgement -> Mimoto-->>InjiWeb: Success ->
InjiWeb-->User: Show success message; ensure only the success branch stores the
credential and not the error branch.
🧹 Nitpick comments (5)
docs/VCStatusCheckDesign.md (5)
15-17: Clarify the cache availability requirement and its implications.The design states that if the status list cache is missing or partially available, the status check will be skipped. Consider documenting:
- What happens to the user experience when status checks are skipped?
- Should there be a background job to populate missing cache entries?
- How will users be informed that their credential status wasn't verified?
16-16: Clarify the "minimum time" logic for multiple status lists.The statement "if there are multiple list involved then minimum time will be updated in the database" is ambiguous. Consider clarifying:
- Why is the minimum time chosen rather than the maximum or the time of the most recent check?
- Could this lead to confusion where
lastCheckedAtdoesn't reflect when some status lists were actually verified?
320-346: Clarify whystatusListCredentialis null when using cache.Line 347 states: "The new field will be null when status list credential is fetched from cache or if its not applicable for the credential."
However, when the status list credential is fetched from cache, the actual credential data is available in the cache. Why not return it in
CredentialStatusResult? This would:
- Provide consistency regardless of cache hits/misses
- Allow callers to access the status list credential without additional cache lookups
- Simplify downstream logic
If there's a specific reason for this design choice (e.g., performance, avoiding duplication), please document it in the design.
318-318: Document the rationale for 24-hour cache TTL.A 24-hour TTL for revocation status seems long from a security perspective, as credentials could remain accessible for up to a day after being revoked. While this may be acceptable for performance reasons, consider:
- Documenting the security vs. performance trade-off
- Noting whether this aligns with industry standards for VC status checking
- Clarifying whether this TTL is appropriate for all status purposes (revocation, suspension, etc.)
9-9: Address documentation formatting issues.Static analysis tools have identified minor formatting issues:
- Line 9: "High level" should be hyphenated as "high-level" when used as an adjective
- Line 5: Heading level jumps from h1 to h3, skipping h2 (should use
##instead of###)- Line 314: Bare URL should be enclosed in angle brackets or proper markdown link syntax
These are minor documentation quality issues that improve readability.
📝 Proposed formatting fixes
-### Overview +## Overview -### High level user flow +### High-level user flow -- **Value**: - - `statusListCredential`: The actual bitstring retrieved from the status list credential +- **Value**: + - `statusListCredential`: The actual bitstring retrieved from the status list credential (e.g., <https://injicertify-farmer.dev-int-inji.mosip.net/v1/certify/credentials/status-list/a7b3ac9a-861a-4fa3-9c85-27e79d35fad3>)Also applies to: 5-5, 314-314
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/VCStatusCheckDesign.md
🧰 Additional context used
🪛 LanguageTool
docs/VCStatusCheckDesign.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...rent status of the credential. ### High level user flow #### Flow : User lands ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/VCStatusCheckDesign.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
314-314: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-apitest-mimoto / maven-build
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: Durgesh Konga <kongadurgesh20@gmail.com>
…n deployment scripts and docker image version updates (inji#1070) * [INJIWEB-1883] Add placeholder db script files, helm chart versions for deployment and update images used in the docker-compose Signed-off-by: jackjain <jackjain1995@gmail.com> * [INJIWEB-1883] Update chart version to 1.5.0 for mimoto-apitrstrig Signed-off-by: jackjain <jackjain1995@gmail.com> --------- Signed-off-by: jackjain <jackjain1995@gmail.com>
… hostname and port (inji#1071) Signed-off-by: jackjain <jackjain1995@gmail.com>
Signed-off-by: Nitin Hegde <nitin.k@cyberpwn.com>
Signed-off-by: Nitin Hegde <165893206+hegdenitin@users.noreply.github.com>
Signed-off-by: Rudhhi Shah <rudhhi.shah@thoughtworks.com>
…nji#1086) * fix(issuers-config): Revert Issuers V1 Endpoint attributes Signed-off-by: Durgesh Konga <kongadurgesh20@gmail.com> * fix(issuers-config): Revert Issuers V1 Endpoint attributes in release Signed-off-by: Durgesh Konga <kongadurgesh20@gmail.com> --------- Signed-off-by: Durgesh Konga <kongadurgesh20@gmail.com>
Signed-off-by: Prafulrakhade <prafulrakhade02@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/mimoto/keygen-mimoto.yaml`:
- Around line 7-10: The environment variable INJI_MIMOTO_DATABASE_HOSTNAME in
the keygen deployment defaults is set to a misspelled hostname
("postgres-postresql.postgres"); update the value to match the correct sibling
hostname ("postgres-postgresql.postgres") so the deployment uses the correct DB
service name—locate the INJI_MIMOTO_DATABASE_HOSTNAME entry in the
keygen-mimoto.yaml and correct the hostname string accordingly.
In `@docs/GoogleOauth2LoginIntegration.md`:
- Around line 145-146: The property name "googleIdToken" is Google-specific and
may confuse users switching identity providers; update the docs and any
references to use a provider-agnostic name like "idpTokenEndpoint" (or
explicitly state that "googleIdToken" is used for all providers) by renaming the
property in the Global Properties section and adjusting all mentions/examples to
"idpTokenEndpoint" (or add a clear note next to "googleIdToken" explaining its
provider-agnostic use) so the configuration is unambiguous.
🪄 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: CHILL
Plan: Pro
Run ID: 7caab332-adfb-47fa-ad7f-2391407318a8
📒 Files selected for processing (22)
api-test/README.mdapi-test/pom.xmldb_scripts/init_values.yamldb_upgrade_script/inji_mimoto/sql/0.21.0_to_0.22.0_rollback.sqldb_upgrade_script/inji_mimoto/sql/0.21.0_to_0.22.0_upgrade.sqldeploy/mimoto-apitestrig/install.shdeploy/mimoto-apitestrig/values.yamldeploy/mimoto/install.shdeploy/mimoto/keygen-mimoto.yamldeploy/mimoto/values.yamldocker-compose/config/mimoto-issuers-config.jsondocker-compose/docker-compose.ymldocs/GoogleOauth2LoginIntegration.mddocs/UserDataEncryptionWithPinBasedKey.mddocs/WalletUnlockProcess.mdhelm/mimoto/Chart.yamlhelm/mimoto/values.yamlpartner-onboarder/install.shpom.xmlsrc/main/resources/application-default.propertiessrc/main/resources/application-local.propertiessrc/main/resources/mimoto-issuers-config.json
✅ Files skipped from review due to trivial changes (10)
- db_scripts/init_values.yaml
- deploy/mimoto/install.sh
- partner-onboarder/install.sh
- deploy/mimoto-apitestrig/install.sh
- helm/mimoto/Chart.yaml
- src/main/resources/mimoto-issuers-config.json
- docs/WalletUnlockProcess.md
- api-test/README.md
- api-test/pom.xml
- docker-compose/config/mimoto-issuers-config.json
Comment on lines
+7
to
10
| - name: INJI_MIMOTO_DATABASE_HOSTNAME | ||
| value: postgres-postresql.postgres | ||
| - name: MOSIP_MIMOTO_DATABASE_PORT | ||
| - name: INJI_MIMOTO_DATABASE_PORT | ||
| value: "5432" |
There was a problem hiding this comment.
Potential DB hostname typo in keygen env defaults (Line 8).
postgres-postresql.postgres appears misspelled and differs from the sibling deployment value (postgres-postgresql.postgres). If this default is ever used, DB connectivity will fail.
Suggested fix
- - name: INJI_MIMOTO_DATABASE_HOSTNAME
- value: postgres-postresql.postgres
+ - name: INJI_MIMOTO_DATABASE_HOSTNAME
+ value: postgres-postgresql.postgres📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: INJI_MIMOTO_DATABASE_HOSTNAME | |
| value: postgres-postresql.postgres | |
| - name: MOSIP_MIMOTO_DATABASE_PORT | |
| - name: INJI_MIMOTO_DATABASE_PORT | |
| value: "5432" | |
| - name: INJI_MIMOTO_DATABASE_HOSTNAME | |
| value: postgres-postgresql.postgres | |
| - name: INJI_MIMOTO_DATABASE_PORT | |
| value: "5432" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/mimoto/keygen-mimoto.yaml` around lines 7 - 10, The environment
variable INJI_MIMOTO_DATABASE_HOSTNAME in the keygen deployment defaults is set
to a misspelled hostname ("postgres-postresql.postgres"); update the value to
match the correct sibling hostname ("postgres-postgresql.postgres") so the
deployment uses the correct DB service name—locate the
INJI_MIMOTO_DATABASE_HOSTNAME entry in the keygen-mimoto.yaml and correct the
hostname string accordingly.
Comment on lines
+145
to
+146
| #### D. Global Properties (in `application.properties`) | ||
| * `googleIdToken` – Update this URL to point to the new provider's token endpoint |
There was a problem hiding this comment.
Clarify the googleIdToken property naming for provider-agnostic context.
The property googleIdToken has a Google-specific name but is being used in a section that discusses switching to any identity provider. This could confuse implementers when configuring non-Google providers.
Consider either:
- Renaming the property to something provider-agnostic (e.g.,
idpTokenEndpoint), or - Adding a note that despite the Google-specific name, this property is used for all providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/GoogleOauth2LoginIntegration.md` around lines 145 - 146, The property
name "googleIdToken" is Google-specific and may confuse users switching identity
providers; update the docs and any references to use a provider-agnostic name
like "idpTokenEndpoint" (or explicitly state that "googleIdToken" is used for
all providers) by renaming the property in the Global Properties section and
adjusting all mentions/examples to "idpTokenEndpoint" (or add a clear note next
to "googleIdToken" explaining its provider-agnostic use) so the configuration is
unambiguous.
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
5 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.
Summary by CodeRabbit