feat: auth and draft 15 #1496
feat: auth and draft 15 #1496
Conversation
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
…etails Signed-off-by: Tipu_Singh <[email protected]>
… method Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
WalkthroughAdds X.509 Docker build and broad OID4VC issuance changes: new template formats (SD‑JWT, MDOC), credential validity windows, authorizationServerUrl propagation, template-driven builders and validators, DTO/interface restructures, service/repository agent interactions with rollback, DateOnly utility, and a DB migration for authorizationServerUrl. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Repo
participant Builder
participant Agent
participant DB
Client->>Controller: POST /credential-offer (dto)
Controller->>Service: createOidcCredentialOffer(dto)
Service->>Repo: getIssuerDetailsByIssuerId(issuerId)
Repo-->>Service: issuerDetails { publicIssuerId, authorizationServerUrl }
Service->>Builder: buildCredentialOfferPayload(dto, templates, issuerDetails, signerOptions, activeCerts)
alt format == SD-JWT
Builder->>Builder: validatePayloadAgainstTemplate(...)
Builder->>Builder: buildSdJwtCredential(...)
Builder->>Builder: buildDisclosureFrameFromTemplate(...)
else format == MDOC
Builder->>Builder: validatePayloadAgainstTemplate(...)
Builder->>Builder: buildMdocCredential(...)
end
Builder-->>Service: CredentialOfferPayload
Service->>DB: save credential offer
Service->>Agent: push issuer/template state
alt Agent OK
Agent-->>Service: ack
Service-->>Controller: 201 {credentialOfferUri}
Controller-->>Client: 201 Created
else Agent Fail
Service->>DB: rollback template state
Service-->>Controller: RpcException {agentError, rollbackError}
Controller-->>Client: 500 Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (2)
76-101: Swagger status 200 vs actual 201 CREATED.@apiresponse declares 200, but you return 201 and message says “created”. Align for accurate API docs.
- @ApiResponse({ status: HttpStatus.OK, description: 'Issuer created successfully.', type: ApiResponseDto }) + @ApiResponse({ status: HttpStatus.CREATED, description: 'Issuer created successfully.', type: ApiResponseDto })
579-585: Remove body from 204 No Content response to comply with HTTP specification.Per RFC 7231, 204 responses MUST NOT include a message body. The current code violates this by calling
.json(finalResponse)with a 204 status code.- return res.status(HttpStatus.NO_CONTENT).json(finalResponse); + return res.status(HttpStatus.NO_CONTENT).send();Alternatively, if clients depend on receiving the response data, change to HTTP 200 instead and keep the
.json()call.apps/oid4vc-issuance/src/oid4vc-issuance.repository.ts (1)
148-175: Nullability mismatch on certificate lookup.findFirst can return null, but the signature promises X509CertificateRecord.
- async getCurrentActiveCertificate(orgId: string, keyType: x5cKeyType): Promise<X509CertificateRecord> { + async getCurrentActiveCertificate(orgId: string, keyType: x5cKeyType): Promise<X509CertificateRecord | null> {Propagate null handling to callers.
apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.ts (3)
33-42: Claims type conflicts with builder output.
CredentialConfiguration.claims: Claim[]but builder produces a nested object (see issuer.metadata.ts). This will break consumers and typings.Option A (nested object, recommended):
export interface CredentialConfiguration { format: string; vct?: string; doctype?: string; scope: string; - claims: Claim[]; + // Nested claims tree keyed by attribute (and namespace for mdoc) + claims: Record<string, Claim>; credential_signing_alg_values_supported: string[]; cryptographic_binding_methods_supported: string[]; display: Display[]; }Apply corresponding changes in issuer.metadata.ts (
CredentialConfig) and extendClaimto support children (see template interfaces comment). Based on learnings
93-98:IssuerUpdation.displaylacks a type.Unannotated property degrades type safety and can allow wrong shapes.
export interface IssuerUpdation { issuerId: string; accessTokenSignerKeyType: AccessTokenSignerKeyType; - display; + display: Display[]; batchCredentialIssuanceSize?: number; }
112-133: Duplicate interface declarations (Logo,ClientAuthentication,AuthorizationServerConfig).These are defined earlier in the same file; duplication relies on declaration merging and is confusing.
-export interface Logo { - uri: string; - alt_text: string; -} -... -export interface ClientAuthentication { - clientId: string; - clientSecret: string; -} -... -export interface AuthorizationServerConfig { - issuer: string; - clientAuthentication: ClientAuthentication; -} +// Duplicate declarations removed; using the definitions above.apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts (2)
42-51:CredentialConfig.claimstype mismatches actual value.Builder assigns a nested object from
buildClaimsFromTemplate, but the type here isClaim[]. This is internally inconsistent and will mislead consumers and break typings.-type CredentialConfig = { +type CredentialConfig = { format: string; vct?: string; scope: string; doctype?: string; - claims: Claim[]; + // Nested claims tree + claims: Record<string, Claim>; credential_signing_alg_values_supported: string[]; cryptographic_binding_methods_supported: string[]; display: { name: string; description?: string; locale?: string }[]; };Also update
apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.tsaccordingly. Based on learnings
171-179: Wrong fallback type forcredentialConfigurationsSupported.Defaulting to
[]should be{}to match object shape.- credentialConfigurationsSupported: credentialConfigurations.credentialConfigurationsSupported ?? [], + credentialConfigurationsSupported: credentialConfigurations.credentialConfigurationsSupported ?? {},apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (1)
362-371: Example property naming mismatch (inputModevsinput_mode).DTO uses
input_mode; example showsinputMode. Align the example to prevent client confusion.- txCode: { - length: 8, - inputMode: 'numeric' - }, + txCode: { + length: 8, + input_mode: 'numeric' + },apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts (2)
70-77: ValidateAuthorizationServerConfigDto.issueras a URL, not a string.This field is documented as a URL; enforce it.
Apply:
- @IsString() + @IsUrl() issuer: string;
30-44: Example includesdescriptionbutIssuerDisplayDtodoes not.Requests with
descriptionwill be stripped or rejected (depending on ValidationPipe). Add it or remove from examples.Apply:
export class IssuerDisplayDto { @@ @IsString({ message: 'Error from DisplayDto -> name' }) name: string; + @ApiPropertyOptional({ + description: 'Description for the issuer display (localized)', + example: 'Accredited institution issuing verified student credentials' + }) + @IsOptional() + @IsString() + description?: string;Also applies to: 101-125
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (2)
195-201: Null-check logic bug makes 404 unreachable in some cases.
if (!getIssuerDetails && getIssuerDetails.publicIssuerId)can never be true. It should use OR.Apply:
- if (!getIssuerDetails && getIssuerDetails.publicIssuerId) { + if (!getIssuerDetails || !getIssuerDetails.publicIssuerId) { throw new NotFoundException(ResponseMessages.oidcIssuer.error.notFound); }
514-575: Per-credential signer options mapping is ambiguous.A single
signerOptionsarray is built for all templates, but the builder usessignerOptions[0]for every credential. Mixed signer methods (DID and X5C) will be misapplied.
- Pass signer options per credential (e.g., keyed by
templateId), or- Ensure the builder selects a signer option based on each template’s stored
signerOption.
I can draft a minimal change set once you confirm the preferred direction.
🧹 Nitpick comments (36)
libs/enum/src/enum.ts (1)
326-330: Remove commented-outSignerOptionenum or convert to active code.Commented-out code should not be committed to version control; it adds maintenance overhead and obscures intent. Either uncomment and activate this enum if it's needed, or remove it entirely.
If this is planned for future use, open an issue or leave a structured TODO comment instead.
Dockerfiles/Dockerfile.x509 (3)
40-40: Optimize final image by excluding unnecessary files fromlibs/directory.Copying the entire
libs/directory to the final image includes source code, tests, and build artifacts that are not needed at runtime. This bloats the image and increases attack surface.Consider using a
.dockerignorefile to exclude unnecessary paths:# .dockerignore node_modules dist .git .env* **/*.test.ts **/*.spec.ts **/src **/tsconfig.jsonAlternatively, selectively copy only the runtime-needed libs (e.g.,
libs/prisma-servicefor schema) rather than the entire directory.
12-12: Clean up commented-out code.Lines 12, 21, 32, and 41 contain commented-out code. Remove these to improve maintainability or, if they represent intentional decisions (e.g., not using
package-lock.json), add a brief comment explaining why.Also applies to: 21-21, 32-32, 41-41
2-2: Pin the Node.js version and add a non-root user for security.Using
node:18-alpinewithout a specific patch version allows pulling potentially vulnerable patch versions. Additionally, running as root is a security risk.Apply these changes:
-FROM node:18-alpine as build +FROM node:18.20-alpine as build +USER node # Install OpenSSL RUN apk add --no-cache openssl-FROM node:18-alpine +FROM node:18.20-alpine # Install OpenSSL RUN apk add --no-cache openssl + +# Create a non-root user to run the application +RUN addgroup -g 1001 -S nodejs && adduser -S nodejs -u 1001 # Set the working directory WORKDIR /app +RUN chown -R nodejs:nodejs /app +USER nodejsNote: Adjust the patch version to match your security posture and testing requirements.
Also applies to: 29-29
apps/api-gateway/src/agent-service/dto/create-schema.dto.ts (3)
5-8: tenantId: consider stricter ID validation (UUID/MongoId) and optional trimmingIf tenantId is a UUID or MongoId, prefer the specific validator to prevent invalid IDs slipping through.
Option (UUID v4):
@ApiProperty() - @IsString({ message: 'tenantId must be a string' }) + @IsUUID('4', { message: 'tenantId must be a valid UUID v4' }) @IsNotEmpty({ message: 'please provide valid tenantId' }) tenantId: string;Also update imports (outside this hunk):
-import { IsString, IsNotEmpty, IsArray } from 'class-validator'; +import { IsString, IsNotEmpty, IsArray, IsUUID } from 'class-validator';
10-13: schemaVersion: optionally enforce format (semver) instead of free-form stringIf versions follow semver, validate it to catch mistakes early.
@ApiProperty() @IsString({ message: 'schema version must be a string' }) - @IsNotEmpty({ message: 'please provide valid schema version' }) + @IsSemVer({ message: 'schemaVersion must be valid semver (e.g., 1.0.0)' }) + @IsNotEmpty({ message: 'please provide valid schema version' }) schemaVersion: string;Add import (outside this hunk) if used:
-import { IsString, IsNotEmpty, IsArray } from 'class-validator'; +import { IsString, IsNotEmpty, IsArray, IsSemVer } from 'class-validator';
15-18: schemaName: optionally bound length and prevent blank-only stringsHelps keep names reasonable and avoids " " passing validation.
@ApiProperty() @IsString({ message: 'schema name must be a string' }) @IsNotEmpty({ message: 'please provide valid schema name' }) + @MaxLength(128, { message: 'schema name must be <= 128 chars' }) schemaName: string;Import (outside this hunk) if used:
-import { IsString, IsNotEmpty, IsArray } from 'class-validator'; +import { IsString, IsNotEmpty, IsArray, MaxLength } from 'class-validator';Optional (if trimming is enabled with class-transformer):
// @Transform(({ value }) => typeof value === 'string' ? value.trim() : value)libs/prisma-service/prisma/data/credebl-master-table.json (1)
3-9: Avoid env-specific values in seed; use neutral placeholders.
- externalIp/inboundEndpoint: private IP ties seed to one LAN.
- emailFrom: real domain may be unintended.
- apiEndpoint: 0.0.0.0 is a bind address, not a client URL; prefer localhost.
- tailsFileServer: empty string may cause runtime checks to fail.
Apply minimal safe defaults:
- "externalIp": "192.168.1.25", - "inboundEndpoint": "192.168.1.25", + "externalIp": "127.0.0.1", + "inboundEndpoint": "127.0.0.1", - "emailFrom": "[email protected]", + "emailFrom": "[email protected]", - "apiEndpoint": "http://0.0.0.0:5000", + "apiEndpoint": "http://localhost:5000", - "tailsFileServer": "" + "tailsFileServer": "http://localhost:6543"If these are intentional environment values, consider moving to per-env seed or .env-driven overrides.
libs/prisma-service/prisma/schema.prisma (1)
581-582: Be explicit with DB type and validate upstream.Prisma will map String→text; for clarity and parity with migration, consider:
- authorizationServerUrl String + authorizationServerUrl String @db.TextAlso ensure DTO validation enforces a well-formed URL.
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (3)
440-441: Doc wording LGTM; align terminology across file.You switched to “OID4VC”. Elsewhere, descriptions still say “OIDC4VCI” (e.g., Lines 471-475, 587-591). Recommend unifying to “OID4VC” for consistency.
103-136: Use PATCH/PUT for update endpoint instead of POST.Semantics and tooling (SDKs, gateways) expect PATCH/PUT for updates.
- @Post('/orgs/:orgId/oid4vc/issuers/:issuerId') + @Patch('/orgs/:orgId/oid4vc/issuers/:issuerId')If you must keep POST for backward compatibility, add the PATCH route as canonical and deprecate POST.
246-254: Validate UUIDs consistently.Other endpoints use ParseUUIDPipe; this one doesn’t. Apply it to orgId and issuerId to catch bad inputs early.
- async createTemplate( - @Param('orgId') orgId: string, - @Param('issuerId') issuerId: string, + async createTemplate( + @Param('orgId', new ParseUUIDPipe()) orgId: string, + @Param('issuerId', new ParseUUIDPipe()) issuerId: string,apps/oid4vc-issuance/src/oid4vc-issuance.repository.ts (2)
180-190: Ensure authorizationServerUrl is validated upstream; fail fast if missing.DB column is NOT NULL; add DTO validation (URL, non-empty) and/or guard here to avoid runtime DB errors.
Example guard at service/repo boundary:
if (!authorizationServerUrl) { throw new Error('authorizationServerUrl is required'); }
310-319: Duplicate issuer getters; consider consolidating.You have both getOidcIssuerDetailsById (throws) and getIssuerDetailsByIssuerId (nullable). Unify the API or clearly document the difference to reduce confusion.
Option A: keep only one and accept a flag to control throw-on-miss.
apps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.ts (2)
38-43: Use wire-friendly types for validityInfo.Date in interfaces can be ambiguous over the wire. Prefer ISO string or epoch seconds.
- validityInfo?: { - validFrom: Date; - validUntil: Date; - }; + validityInfo?: { + validFrom: string; // ISO 8601 + validUntil: string; // ISO 8601 + };Ensure serializers/validators align.
30-34: Remove unusedDisclosureFrametype from interfaces file.Verification confirms
DisclosureFrameat line 30 is not imported by any module in the codebase. While the type is actively used elsewhere via a duplicate definition incredential-sessions.builder.ts, the declaration in the interfaces file is orphaned and should be removed to reduce surface area.libs/common/src/date-only.ts (1)
30-33: Clarify year constant and startup time intent.
oneYearInMilliseconds = 365 * oneDayInMillisecondsignores leap years; document as approximate or rename.serverStartupTimeInMilliseconds = Date.now()is evaluated at import time and can make tests non-deterministic; consider moving to a function or annotate usage.-export const oneYearInMilliseconds = 365 * oneDayInMilliseconds; +// Approximate (ignores leap years); use calendar math if exact bounds matter. +export const oneYearInMilliseconds = 365 * oneDayInMilliseconds;apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.ts (2)
54-62: Plural name with singular type.
authorizationServerConfigs: AuthorizationServerConfig;is singular but named plural. Align naming or make it an array, consistent withIssuerResponse.authorizationServerConfigs?: AuthorizationServerConfig[].- authorizationServerConfigs: AuthorizationServerConfig; + // If only one config is supported: + authorizationServerConfig: AuthorizationServerConfig; + // Or if multiple are intended: + // authorizationServerConfigs: AuthorizationServerConfig[];Would you confirm if multiple AS configs are supported at creation time?
138-151: Response shape vs creation shape mismatch.
IssuerResponse.authorizationServerConfigs?: AuthorizationServerConfig[]conflicts withIssuerCreation.authorizationServerConfigs(singular). Ensure parity or document why they differ.apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts (4)
282-301: Return type usesany; tighten to the nested claims tree.Avoid
anyhere to keep the builder contract strong.-export function buildClaimsFromTemplate(template: SdJwtTemplate | MdocTemplate): Record<string, any> { +export function buildClaimsFromTemplate(template: SdJwtTemplate | MdocTemplate): Record<string, Claim> | Record<string, Record<string, Claim>> {And adjust the MDOC branch to return
Record<string, Record<string, Claim>>. Based on learnings
378-417: Loosened input typing and unchecked appearance parsing.
- Parameter is
any; useTemplateRowPrisma[].appearanceis cast without guards; add a type guard to avoid runtime errors.-export function buildCredentialConfigurationsSupported(templateRows: any): Record<string, CredentialConfig> { +export function buildCredentialConfigurationsSupported(templateRows: TemplateRowPrisma[]): Record<string, CredentialConfig> {- const appearanceJson = coerceJsonObject<unknown>(templateRow.appearance); + const appearanceJson = coerceJsonObject<unknown>(templateRow.appearance); + const isAppearance = (x: unknown): x is Appearance => + !!x && typeof x === 'object' && Array.isArray((x as Appearance).display);- const displayConfigurations = - (appearanceJson as Appearance).display?.map((displayEntry) => ({ + const displayConfigurations = + (isAppearance(appearanceJson) ? appearanceJson.display : []).map((displayEntry) => ({ name: displayEntry.name, description: displayEntry.description, locale: displayEntry.locale, logo: displayEntry.logo ? { uri: displayEntry.logo.uri, alt_text: displayEntry.logo.alt_text } : undefined })) ?? [];
100-113: DB coercion forattributes.
TemplateRowPrisma.attributesis typed asSdJwtTemplate | MdocTemplate, but DB rows may carry JSON strings. Ensure upstream coercion (or coerce here) to avoid runtime type errors.
331-357: Minor: avoid commented dead code.Large commented blocks make maintenance harder. Remove or move to docs.
apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (3)
24-25: ImportIsISO8601for the updated validators.-import { +import { IsArray, IsNotEmpty, IsObject, IsOptional, IsString, ValidateNested, registerDecorator, ValidationOptions, ArrayMinSize, IsInt, Min, IsIn, IsUrl, + IsISO8601, ValidatorConstraint, ValidatorConstraintInterface, ValidationArguments, Validate } from 'class-validator';
284-313: Resolve ESLint mixed-operator and prefer shared constant.Use
oneYearInMillisecondsfrom common lib to avoid precedence ambiguity and magic numbers.-import { dateToSeconds } from '@credebl/common/date-only'; +import { dateToSeconds, oneYearInMilliseconds } from '@credebl/common/date-only';- exp: dateToSeconds(new Date(Date.now() + 5 * 365 * 24 * 60 * 60 * 1000)) + exp: dateToSeconds(new Date(Date.now() + 5 * oneYearInMilliseconds))
314-316:payloadis a plain object;ValidateNestedhas no effect.Replace with
@IsObject()to validate shape generically.- @ValidateNested() - payload: object; + @IsObject() + payload: Record<string, unknown>;apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts (1)
40-47: Strengthenvalue_typetyping.Constrain to a union to improve validation/tooling and reuse across formats.
-export interface CredentialAttribute { +export interface CredentialAttribute { key: string; mandatory?: boolean; - value_type: string; + value_type: 'string' | 'number' | 'boolean' | 'date' | string; disclose?: boolean; children?: CredentialAttribute[]; display?: ClaimDisplay[]; }apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts (3)
156-158: Add validation forIssuerUpdationDto.issuerId.Keep it optional but typed for consistency with creation DTOs.
Apply:
export class IssuerUpdationDto { - issuerId?: string; + @IsOptional() + @IsString() + issuerId?: string;
54-60: MarkclientSecretas write-only in Swagger to avoid echoing secrets.Improves API hygiene in generated docs.
Apply:
- @ApiProperty({ + @ApiProperty({ description: 'OAuth2 client secret for the authorization server', - example: '2qKMWulZpMBzXIdfPO5AEs0xaTaKs1uk' + example: '••••••••', + writeOnly: true })
138-154: Naming:authorizationServerConfigsis singular.The type is a single
AuthorizationServerConfigDto, but the name is plural. Consider renaming toauthorizationServerConfigfor clarity. Non-blocking.apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (2)
180-181: Replaceconsole.logwith structured logger.Use
this.logger.debug()/log()to avoid noisy stdout in prod.Apply:
- console.log('This is the issuerConfig:', JSON.stringify(issuerConfig, null, 2)); + this.logger.debug(`issuerConfig=${JSON.stringify(issuerConfig)}`); @@ - console.log('createTemplateOnAgent::::::::::::::', createTemplateOnAgent); + this.logger.debug(`createTemplateOnAgent=${JSON.stringify(createTemplateOnAgent)}`); @@ - console.log('This is the createCredentialOfferOnAgent:', JSON.stringify(createCredentialOfferOnAgent, null, 2)); + this.logger.debug(`createCredentialOfferOnAgent=${JSON.stringify(createCredentialOfferOnAgent)}`); @@ - console.log('Storing OID4VC Credential Webhook:', CredentialOfferWebhookPayload); + this.logger.debug(`storeOidcCredentialWebhook payload=${JSON.stringify(CredentialOfferWebhookPayload)}`);Also applies to: 351-352, 662-663, 958-958
911-935: RxJStoPromise()is deprecated; uselastValueFrom.Prevents deprecation and subtle behavior changes.
Apply:
- return this.issuanceServiceProxy - .send<string>(pattern, payload) - .pipe( - map((response) => ({ - response - })) - ) - .toPromise() - .catch((error) => { + const obs$ = this.issuanceServiceProxy + .send<string>(pattern, payload) + .pipe(map((response) => ({ response }))); + return await lastValueFrom(obs$).catch((error) => { this.logger.error(`catch: ${JSON.stringify(error)}`); throw new HttpException( { status: error.statusCode, error: error.message }, error.error ); });Also add import:
import { lastValueFrom } from 'rxjs';apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (3)
80-86: ValidateLogoDto.urias URL.Apply:
- @IsString() - @IsNotEmpty() + @IsUrl() uri: string;
253-259: Guard against invalidformatwhen resolvingtemplatetype.If
formatis missing/invalid, resolver returnsundefined, breaking validation. Throw early.Apply:
- @Type(({ object }) => { - if (object.format === CredentialFormat.Mdoc) { + @Type(({ object }) => { + if (object.format === CredentialFormat.Mdoc) { return MdocTemplateDto; - } else if (object.format === CredentialFormat.SdJwtVc) { + } else if (object.format === CredentialFormat.SdJwtVc) { return SdJwtTemplateDto; } + throw new Error('Invalid or missing credential format'); })
239-246: The_doctypeAbsentGuard/_vctAbsentGuardpattern doesn’t enforce anything.These sentinels don’t validate presence of unknown fields. Either remove them or add a custom validator that checks for forbidden fields in the raw body. Non-blocking.
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (1)
140-165: URL builder is fine; minor: keep param order predictable.If deterministic ordering matters for caching/tests, sort keys before join. Optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Dockerfiles/Dockerfile.x509(1 hunks)apps/api-gateway/src/agent-service/dto/create-schema.dto.ts(1 hunks)apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts(5 hunks)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts(4 hunks)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts(4 hunks)apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts(1 hunks)apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.ts(4 hunks)apps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.ts(1 hunks)apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts(1 hunks)apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts(3 hunks)apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts(5 hunks)apps/oid4vc-issuance/src/oid4vc-issuance.repository.ts(3 hunks)apps/oid4vc-issuance/src/oid4vc-issuance.service.ts(12 hunks)libs/common/src/date-only.ts(1 hunks)libs/common/src/response-messages/index.ts(3 hunks)libs/enum/src/enum.ts(1 hunks)libs/prisma-service/prisma/data/credebl-master-table.json(1 hunks)libs/prisma-service/prisma/migrations/20251023120134_added_authorization_server_url/migration.sql(1 hunks)libs/prisma-service/prisma/schema.prisma(1 hunks)
🧰 Additional context used
🪛 ESLint
apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts
[error] 310-310: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
[error] 310-310: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
🔇 Additional comments (6)
libs/enum/src/enum.ts (1)
321-324: ✅ New credential format types properly defined.The
CredentialFormatenum cleanly introduces SD-JWT and MDOC format support, aligning with the PR's credential template refactoring objectives.libs/common/src/response-messages/index.ts (2)
555-556: LGTM!These error messages are clear, descriptive, and follow the established patterns in the codebase. The
missingValidityInfomessage appropriately specifies the required fields.
528-528: LGTM!The success message is clear and consistent with other fetch messages in the codebase.
apps/oid4vc-issuance/src/oid4vc-issuance.repository.ts (1)
124-129: No issues found—commenting out templates include is safe.The call to
getAllOidcIssuersByOrgat line 236 returnsgetIssuersdirectly without accessing anytemplatesproperty. Additionally, a comprehensive search found zero references to.templatesanywhere in the codebase, confirming no consumers depend on the inclusion. The behavior change poses no risk to downstream code.apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (1)
186-194: Auth flow change: ensure controller/services updated to readauthorizationType.You removed XOR at this DTO level in favor of
authorizationType. Confirm the issuing flow branches solely on this field and old config paths are not required here.apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (1)
401-454: Agent template command is functionally correct—no verification issues found.The agent's
oidcIssuerTemplateimplementation uses an HTTP PUT request (idempotent), not HTTP POST, so despite the misleadingagent-create-oid4vc-templatenaming, it properly handles template updates. No separate update command exists in the codebase for templates, confirming this is the intended pattern. The code works correctly as-is.
| @ApiProperty() | ||
| @IsArray({ message: 'attributes must be an array' }) | ||
| @IsString({ each: true }) | ||
| // TODO: IsNotEmpty won't work for array. Must use @ArrayNotEmpty() instead | ||
| @IsNotEmpty({ message: 'please provide valid attributes' }) | ||
| attributes: string[]; |
There was a problem hiding this comment.
Fix array validation: @isnotempty doesn’t enforce non-empty arrays; use @ArrayNotEmpty and element checks
Empty arrays will currently pass. Enforce at least one attribute and non-empty strings per element. Also set Swagger type for arrays.
Apply:
- @ApiProperty()
+ @ApiProperty({ type: [String] })
@IsArray({ message: 'attributes must be an array' })
- @IsString({ each: true })
- // TODO: IsNotEmpty won't work for array. Must use @ArrayNotEmpty() instead
- @IsNotEmpty({ message: 'please provide valid attributes' })
+ @IsString({ each: true })
+ @ArrayNotEmpty({ message: 'please provide at least one attribute' })
+ @IsNotEmpty({ each: true, message: 'attribute must not be empty' })
attributes: string[];And update imports (outside this hunk):
-import { IsString, IsNotEmpty, IsArray } from 'class-validator';
+import { IsString, IsNotEmpty, IsArray, ArrayNotEmpty } from 'class-validator';Optional (if duplicates are not allowed):
// @ArrayUnique({ message: 'attributes must be unique' })🤖 Prompt for AI Agents
In apps/api-gateway/src/agent-service/dto/create-schema.dto.ts around lines
20–25, replace the incorrect @IsNotEmpty on the array with @ArrayNotEmpty to
enforce a non-empty array, add element-level non-empty checks by using
@IsNotEmpty({ each: true }) alongside the existing @IsString({ each: true }),
and set the Swagger type with @ApiProperty({ type: [String] }); also update
imports at the top to import ArrayNotEmpty (and optionally ArrayUnique if you
want uniqueness) from class-validator and ensure ApiProperty is configured for
array types.
| @ApiProperty() | ||
| @IsNotEmpty({ message: 'please provide orgId' }) | ||
| orgId: string; |
There was a problem hiding this comment.
orgId missing type validation: add @IsString (or specific ID validator)
Without @IsString, numbers/objects could pass if non-empty, risking downstream failures.
@ApiProperty()
+ @IsString({ message: 'orgId must be a string' })
@IsNotEmpty({ message: 'please provide orgId' })
orgId: string;Optional (if UUID):
- @IsString({ message: 'orgId must be a string' })
+ @IsUUID('4', { message: 'orgId must be a valid UUID v4' })Remember to import IsUUID if used.
📝 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.
| @ApiProperty() | |
| @IsNotEmpty({ message: 'please provide orgId' }) | |
| orgId: string; | |
| @ApiProperty() | |
| @IsString({ message: 'orgId must be a string' }) | |
| @IsNotEmpty({ message: 'please provide orgId' }) | |
| orgId: string; |
🤖 Prompt for AI Agents
In apps/api-gateway/src/agent-service/dto/create-schema.dto.ts around lines 27
to 29, the orgId property currently only has @IsNotEmpty and lacks a type
validator; add @IsString() to the decorators (or @IsUUID() if orgId should be a
UUID) and import the chosen validator from class-validator so non-string values
are rejected and downstream type assumptions are safe.
| fetch: 'OID4VC template(s) fetched successfully.', | ||
| getById: 'OID4VC template details fetched successfully.' | ||
| getById: 'OID4VC template details fetched successfully.', | ||
| deleteTemplate: '[createTemplate] compensating delete succeeded for templateId=${templateId}' |
There was a problem hiding this comment.
🧩 Analysis chain
Fix template string interpolation in static message constant.
The message uses ${templateId} syntax within a regular string literal, which won't interpolate at runtime. This will output the literal text ${templateId} instead of the actual template ID value.
Consider one of these approaches:
Option 1: Use placeholder pattern (if the codebase has a message formatting utility):
- deleteTemplate: '[createTemplate] compensating delete succeeded for templateId=${templateId}'
+ deleteTemplate: '[createTemplate] compensating delete succeeded for templateId={templateId}'Option 2: Document that this requires runtime formatting and ensure consuming code uses template literals:
// Usage example at call site:
const message = `${ResponseMessages.oidcTemplate.success.deleteTemplate}${templateId}`;Verify how other dynamic messages are handled in the codebase to maintain consistency.
🏁 Script executed:
#!/bin/bash
# Search for other message patterns with placeholders to determine the convention
rg -n -C2 'ResponseMessages\.' --type=ts | rg -A2 -B2 '\{[a-zA-Z]+\}|\$\{[a-zA-Z]+\}'Length of output: 4685
🏁 Script executed:
#!/bin/bash
# Search for where deleteTemplate message is used
rg -n 'deleteTemplate' --type=ts -C2Length of output: 6616
🏁 Script executed:
#!/bin/bash
# Also check for other compensating messages in the response-messages file
rg -n 'compensating' libs/common/src/response-messages/index.ts -C2Length of output: 521
🏁 Script executed:
#!/bin/bash
# Check the actual response-messages file around lines 528-556 to see all new additions
sed -n '520,560p' libs/common/src/response-messages/index.tsLength of output: 1817
Fix malformed template string interpolation in compensating delete messages.
The success message at line 529 uses ${templateId} in a static string, which won't interpolate at runtime. The call site (oid4vc-issuance.service.ts:340) concatenates the ID directly, producing a malformed log: ...templateId=${templateId}actual-id. Additionally, the related error message at line 541 is incomplete—it ends with templateId= but lacks the placeholder.
Both messages need correction for consistency and proper formatting:
- deleteTemplate: '[createTemplate] compensating delete succeeded for templateId=${templateId}'
+ deleteTemplate: '[createTemplate] compensating delete succeeded for templateId='- failedDeleteTemplate: '[createTemplate] compensating delete FAILED for templateId='
+ failedDeleteTemplate: '[createTemplate] compensating delete FAILED for templateId='Update the call site (oid4vc-issuance.service.ts:340) to append the ID with proper formatting:
-this.logger.log(`${ResponseMessages.oidcTemplate.success.deleteTemplate}${createdTemplate.id}`);
+this.logger.log(`${ResponseMessages.oidcTemplate.success.deleteTemplate}${createdTemplate.id}`)📝 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.
| deleteTemplate: '[createTemplate] compensating delete succeeded for templateId=${templateId}' | |
| deleteTemplate: '[createTemplate] compensating delete succeeded for templateId=' |
🤖 Prompt for AI Agents
In libs/common/src/response-messages/index.ts around lines 529 and 541, the
success message currently contains a literal "${templateId}" which won't
interpolate when the call site appends the id (producing
"...templateId=${templateId}actual-id"), and the related error message ends with
"templateId=" but lacks a placeholder; remove the literal "${templateId}" from
the success constant so it ends with a clear separator (e.g., "templateId=" or
"templateId: ") that the caller can safely append to, and update the error
constant to either include a proper placeholder if it should be formatted here
or also end with the same separator for caller appending; then update the call
site (oid4vc-issuance.service.ts:340) to append the id using that separator
(e.g., `${MSG.deleteTemplate}${templateId}` or
`${MSG.deleteTemplate}${templateId}`) so logs are consistently formatted.
| templateNameAlreadyExist: 'Template name already exists for this issuer.', | ||
| deleteTemplate: 'Error while deleting template.' | ||
| deleteTemplate: 'Error while deleting template.', | ||
| failedDeleteTemplate: '[createTemplate] compensating delete FAILED for templateId=' |
There was a problem hiding this comment.
Fix incomplete error message format.
The message ends with = without completing the templateId reference, creating an inconsistent format with the success message on line 529.
Apply this diff to align with the success message pattern:
- failedDeleteTemplate: '[createTemplate] compensating delete FAILED for templateId='
+ failedDeleteTemplate: '[createTemplate] compensating delete FAILED for templateId=${templateId}'Note: This still requires the same interpolation fix mentioned in the previous comment (line 529). Both messages should use a consistent placeholder pattern.
📝 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.
| failedDeleteTemplate: '[createTemplate] compensating delete FAILED for templateId=' | |
| failedDeleteTemplate: '[createTemplate] compensating delete FAILED for templateId=${templateId}' |
🤖 Prompt for AI Agents
In libs/common/src/response-messages/index.ts around line 541, the
failedDeleteTemplate string ends with an incomplete '=' and must be aligned with
the success message pattern on line 529; replace the value so it includes the
same templateId placeholder format used by the success message (i.e. complete
the trailing '=' by appending the placeholder) and ensure both success and
failure messages use the identical placeholder/interpolation pattern.
| /* | ||
| Warnings: | ||
| - Added the required column `authorizationServerUrl` to the `oidc_issuer` table without a default value. This is not possible if the table is not empty. | ||
| */ | ||
| -- AlterTable | ||
| ALTER TABLE "oidc_issuer" ADD COLUMN "authorizationServerUrl" TEXT NOT NULL; |
There was a problem hiding this comment.
🧩 Analysis chain
NOT NULL on existing table can fail; use two-step/backfill.
Adding a required column without a default will fail if oidc_issuer has rows. Use a safe pattern.
Recommended migration sequence:
-- Step 1: add as nullable
ALTER TABLE "oidc_issuer" ADD COLUMN "authorizationServerUrl" TEXT;
-- Step 2: backfill (set appropriate values based on your data source)
-- UPDATE "oidc_issuer" SET "authorizationServerUrl" = '<auth-server-url>';
-- Step 3: enforce NOT NULL
ALTER TABLE "oidc_issuer" ALTER COLUMN "authorizationServerUrl" SET NOT NULL;Alternatively:
ALTER TABLE "oidc_issuer" ADD COLUMN "authorizationServerUrl" TEXT NOT NULL DEFAULT '';
ALTER TABLE "oidc_issuer" ALTER COLUMN "authorizationServerUrl" DROP DEFAULT;Please confirm which backfill source you’ll use.
🏁 Script executed:
#!/bin/bash
# Find all Prisma migrations related to oidc_issuer table
find . -type f -name "*.sql" -path "*/migrations/*" | sort | head -20Length of output: 1828
🏁 Script executed:
#!/bin/bash
# Search for all migrations that mention oidc_issuer
rg -n "oidc_issuer" --type sql -g "*/migrations/*"Length of output: 148
🏁 Script executed:
#!/bin/bash
# Check for seed files or test fixtures that populate oidc_issuer
rg -n "oidc_issuer" --type ts --type js -g "*/seed*" -g "*/fixture*" -C 2Length of output: 148
🏁 Script executed:
#!/bin/bash
# Look for the initial oidc_issuer table creation migration
rg -n "CREATE TABLE.*oidc_issuer" --type sqlLength of output: 118
🏁 Script executed:
#!/bin/bash
# Find the schema.prisma to understand the data model
find . -name "schema.prisma" -type fLength of output: 42
🏁 Script executed:
#!/bin/bash
# Examine the initial oidc_issuer table creation migration
cat ./libs/prisma-service/prisma/migrations/20250822145801_fix_issuer_relation/migration.sqlLength of output: 1320
🏁 Script executed:
#!/bin/bash
# Search for oidc_issuer without restrictive filters
rg -n "oidc_issuer" -C 3 | head -50Length of output: 4384
🏁 Script executed:
#!/bin/bash
# Check schema.prisma for oidc_issuer model definition
cat ./libs/prisma-service/prisma/schema.prisma | grep -A 20 "model oidc_issuer"Length of output: 761
🏁 Script executed:
#!/bin/bash
# List all migrations in order to see which ones touch oidc_issuer
find ./libs/prisma-service/prisma/migrations -name "migration.sql" -type f | sort | tail -30Length of output: 3090
🏁 Script executed:
#!/bin/bash
# Search for seed or initialization files
find . -type f \( -name "*seed*" -o -name "*init*" -o -name "*fixture*" \) | grep -E "\.(ts|js|sql)$" | grep -v node_modules | head -20Length of output: 994
🏁 Script executed:
#!/bin/bash
# Check the Sept 4 migration that adds fields to oidc_issuer
cat ./libs/prisma-service/prisma/migrations/20250904181430_added_fields_in_oidc_issuer/migration.sqlLength of output: 314
🏁 Script executed:
#!/bin/bash
# Check the seed file for oidc_issuer data
cat ./libs/prisma-service/prisma/seed.tsLength of output: 14056
🏁 Script executed:
#!/bin/bash
# Also check the Sept 2 migration
cat ./libs/prisma-service/prisma/migrations/20250902082321_add_org_agent_id_to_oidc_issuer/migration.sqlLength of output: 955
Add NOT NULL column safely with backfill; current migration will fail on non-empty table.
The table has been in production for 2+ months with runtime-created records (application code calls oidcIssuerCreate). Adding a NOT NULL column without a default will fail during migration execution.
Use a safe two-step approach:
-- Step 1: add as nullable
ALTER TABLE "oidc_issuer" ADD COLUMN "authorizationServerUrl" TEXT;
-- Step 2: backfill with appropriate value(s)
UPDATE "oidc_issuer" SET "authorizationServerUrl" = '<value>';
-- Step 3: enforce NOT NULL
ALTER TABLE "oidc_issuer" ALTER COLUMN "authorizationServerUrl" SET NOT NULL;Or use temporary default:
ALTER TABLE "oidc_issuer" ADD COLUMN "authorizationServerUrl" TEXT NOT NULL DEFAULT '';
ALTER TABLE "oidc_issuer" ALTER COLUMN "authorizationServerUrl" DROP DEFAULT;Confirm the appropriate backfill value for existing records.
🤖 Prompt for AI Agents
In
libs/prisma-service/prisma/migrations/20251023120134_added_authorization_server_url/migration.sql
around lines 1 to 8 the migration adds a NOT NULL column directly which will
fail on non-empty production tables; change to a safe three-step migration:
first add the column as nullable, then backfill existing rows with the correct
authorizationServerUrl value for each record, then alter the column to SET NOT
NULL (alternatively add with a temporary DEFAULT, backfill, then DROP DEFAULT
and SET NOT NULL); ensure you choose and apply the correct backfill value based
on existing records before enforcing NOT NULL.
Signed-off-by: Rinkal Bhojani <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/api-gateway/src/agent-service/dto/create-schema.dto.ts (2)
20-27: Apply the fix you've already identified in the TODO comments.The TODO comments on lines 23-25 contain the correct solution, but they remain commented out while the incorrect
@IsNotEmptyon line 26 stays active. Empty arrays will still pass validation.Uncomment lines 24-25 and remove line 26, then update imports:
-import { IsString, IsNotEmpty, IsArray } from 'class-validator'; +import { IsString, IsNotEmpty, IsArray, ArrayNotEmpty } from 'class-validator';@ApiProperty() @IsArray({ message: 'attributes must be an array' }) @IsString({ each: true }) - // TODO: IsNotEmpty won't work for array. Must use @ArrayNotEmpty() instead - // @ArrayNotEmpty({ message: 'please provide at least one attribute' }) - // @IsNotEmpty({ each: true, message: 'attribute must not be empty' }) - @IsNotEmpty({ message: 'please provide valid attributes' }) + @ArrayNotEmpty({ message: 'please provide at least one attribute' }) + @IsNotEmpty({ each: true, message: 'attribute must not be empty' }) attributes: string[];
29-31: Add type validation for orgId.Without
@IsString, non-string values can pass validation if they're non-empty, risking downstream type failures.@ApiProperty() + @IsString({ message: 'orgId must be a string' }) @IsNotEmpty({ message: 'please provide orgId' }) orgId: string;
🧹 Nitpick comments (3)
libs/enum/src/enum.ts (1)
336-340: Remove or document the commented enum.The commented
SignerOptionenum should either be removed if no longer needed or restored with a clear TODO explaining why it's temporarily disabled.Apply this diff if the enum is truly unused:
- -// export enum SignerOption { -// DID, -// X509_P256, -// X509_ED25519 -// }apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (1)
312-313: Add parentheses for operator precedence clarity.ESLint flags the mixed
+and*operators. While mathematically correct, explicit parentheses improve readability.Apply this diff:
- exp: dateToSeconds(new Date(Date.now() + 5 * 365 * 24 * 60 * 60 * 1000)) + exp: dateToSeconds(new Date(Date.now() + (5 * 365 * 24 * 60 * 60 * 1000)))Alternatively, use the constants from
@credebl/common/date-only:+import { oneYearInMilliseconds } from '@credebl/common/date-only'; ... - exp: dateToSeconds(new Date(Date.now() + 5 * 365 * 24 * 60 * 60 * 1000)) + exp: dateToSeconds(new Date(Date.now() + 5 * oneYearInMilliseconds))apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (1)
41-45: Clean up TODO comment forvalue_type.The
value_typeproperty is correctly typed asAttributeType(as suggested in the past review). The TODO on line 44 can be removed since the change is already implemented.Apply this diff:
- // TODO: Check how do we handle claims with only path rpoperty like email, etc. @ApiProperty({ enum: AttributeType, description: 'Type of the attribute value (string, number, date, etc.)' }) @IsEnum(AttributeType) - // TODO: changes value_type: AttributeType; value_type: AttributeType;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api-gateway/src/agent-service/dto/create-schema.dto.ts(1 hunks)apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts(5 hunks)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts(4 hunks)apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts(1 hunks)libs/common/src/date-only.ts(1 hunks)libs/enum/src/enum.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts
[error] 313-313: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
[error] 313-313: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
🔇 Additional comments (17)
libs/enum/src/enum.ts (1)
326-334: LGTM! AttributeType enum is well-defined.The new
AttributeTypeenum provides comprehensive coverage of primitive and complex data types needed for credential attributes, including the specializedIMAGEtype for binary data.libs/common/src/date-only.ts (3)
38-40: Consider timezone implications oftoISOString().The internal
Dateobject retains timezone information, buttoISOString()always returns UTC. If a user constructsDateOnlywith a local date string (e.g.,"2025-10-29"), it will be interpreted in the local timezone, but serialization may shift the date. Consider documenting this behavior or normalizing to UTC midnight in the constructor.
47-50: LGTM! Time constants are correctly defined.The constants are correctly calculated. Note that
oneYearInMillisecondsassumes 365 days, which is appropriate for approximate calculations.
52-58: LGTM! Validation correctly added.The function now properly validates input before conversion to seconds, addressing the concern from the past review.
apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (2)
122-140: LGTM! ValidityInfo correctly uses Date transformation.The class properly uses
@Type(() => Date)with@IsDate()validators, which is one of the recommended approaches for handling date validation. This addresses the concern from the past review.
189-196: LGTM! Authorization type discriminator simplifies validation.The explicit
authorizationTypefield is a cleaner design than the previous XOR constraint approach, making the API more intuitive.apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts (4)
3-14: LGTM! Template interfaces are well-structured.The
SdJwtTemplateandMdocTemplateinterfaces cleanly model the distinct requirements of each credential format, with MDOC's namespace-based structure properly represented.
16-25: LGTM! Template union provides type-safe format handling.The
templatefield's union type (SdJwtTemplate | MdocTemplate) enables format-specific structure while maintaining type safety.
34-38: Verify ifClaiminterface needs recursive structure.The past review suggested adding a
childrenproperty toClaimfor nested claim trees. Currently, onlyCredentialAttribute(lines 40-47) haschildren. IfClaimis used for metadata presentation whileCredentialAttributehandles template definitions, the current design may be intentional. However, ifClaimshould also support nested structures, consider adding:export interface Claim { path?: string[]; display?: ClaimDisplay[]; mandatory?: boolean; children?: Record<string, Claim>; // For nested claims }Please confirm whether
ClaimandCredentialAttributeserve distinct purposes or if they should be unified.
40-47: LGTM! CredentialAttribute supports recursive nesting.The
childrenproperty enables proper representation of nested credential attributes (objects and arrays), addressing the structural requirements for complex credentials.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (7)
20-30: LGTM! Display DTO is well-structured.The
CredentialAttributeDisplayDtoproperly models localized attribute display information with appropriate validation.
58-67: LGTM! Nested attribute validation is correctly implemented.The
childrenarray properly uses@ValidateNested({ each: true })and@Type()decorators to ensure recursive validation of nested attributes, addressing the concern from the past review.
111-126: LGTM! Display branding fields are properly defined.The additional branding properties (
background_color,text_color,background_image) enhance credential appearance customization with appropriate validation.
158-169: LGTM! MDOC namespace structure is correctly modeled.The
MdocNamespaceDtoproperly enforces non-empty attributes with@ArrayNotEmpty()and applies nested validation for each attribute.
170-185: Consider uncommenting conditional validation.Line 175 has a commented
@ValidateIfthat would enforcedoctypeonly when format is'mso_mdoc'. SinceMdocTemplateDtois only used when the format is MDOC (via the Type discriminator on line 246), the conditional validation may not be necessary. However, the commented code suggests this was considered—verify whether it should be removed or uncommented.
197-205: LGTM! SD-JWT attributes have proper nested validation.The attributes array correctly includes
@ValidateNested({ each: true })and@Type(() => CredentialAttributeDto), addressing the concern from the past review.
227-252: LGTM! Polymorphic template pattern is well-implemented.The combination of:
- Format-based guards (
_doctypeAbsentGuard,_vctAbsentGuard) preventing field mixing- Type discriminator selecting the correct DTO class
- Proper nested validation
...provides robust, type-safe template validation that correctly handles both SD-JWT and MDOC formats.
| @ApiPropertyOptional({ | ||
| description: 'Selective disclosure: claim -> boolean (or nested map)', | ||
| example: { name: true, DOB: true, additionalProp3: false }, | ||
| example: { validFrom: '2025-04-23T14:34:09.188Z', validUntil: '2026-05-03T14:34:09.188Z' }, | ||
| required: false | ||
| }) | ||
| @IsOptional() | ||
| @IsDisclosureFrame() | ||
| disclosureFrame?: Record<string, boolean | Record<string, boolean>>; | ||
| validityInfo?: ValidityInfo; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add nested validation for validityInfo.
The optional validityInfo field should use @ValidateNested() and @Type(() => ValidityInfo) to ensure its properties are validated when present.
Apply this diff:
@ApiPropertyOptional({
example: { validFrom: '2025-04-23T14:34:09.188Z', validUntil: '2026-05-03T14:34:09.188Z' },
required: false
})
@IsOptional()
+ @ValidateNested()
+ @Type(() => ValidityInfo)
validityInfo?: ValidityInfo;📝 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.
| @ApiPropertyOptional({ | |
| description: 'Selective disclosure: claim -> boolean (or nested map)', | |
| example: { name: true, DOB: true, additionalProp3: false }, | |
| example: { validFrom: '2025-04-23T14:34:09.188Z', validUntil: '2026-05-03T14:34:09.188Z' }, | |
| required: false | |
| }) | |
| @IsOptional() | |
| @IsDisclosureFrame() | |
| disclosureFrame?: Record<string, boolean | Record<string, boolean>>; | |
| validityInfo?: ValidityInfo; | |
| @ApiPropertyOptional({ | |
| example: { validFrom: '2025-04-23T14:34:09.188Z', validUntil: '2026-05-03T14:34:09.188Z' }, | |
| required: false | |
| }) | |
| @IsOptional() | |
| @ValidateNested() | |
| @Type(() => ValidityInfo) | |
| validityInfo?: ValidityInfo; |
🤖 Prompt for AI Agents
In apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts around lines
161 to 166, the optional validityInfo property lacks nested validation
annotations; add @ValidateNested() and @Type(() => ValidityInfo) above
validityInfo so that when provided its inner properties are validated, keeping
@IsOptional() and the existing @ApiPropertyOptional intact.
| public constructor(date?: string | Date) { | ||
| if (date instanceof Date) { | ||
| if (isNaN(date.getTime())) { | ||
| throw new TypeError('Invalid Date'); | ||
| } | ||
| this.date = date; | ||
| return; | ||
| } | ||
| if (!date) { | ||
| this.date = new Date(); | ||
| return; | ||
| } | ||
| // Accept only YYYY-MM-DD or full ISO strings | ||
| const iso = /^\d{4}-\d{2}-\d{2}(T.*Z)?$/.test(date) ? date : ''; | ||
| const d = new Date(iso || date); | ||
| if (isNaN(d.getTime())) { | ||
| throw new TypeError('Invalid date string'); | ||
| } | ||
| this.date = d; | ||
| } |
There was a problem hiding this comment.
Tighten the date string regex validation.
The current regex /^\d{4}-\d{2}-\d{2}(T.*Z)?$/ is too permissive: it accepts invalid ISO strings like "2025-01-01TXXXXXZ". Additionally, line 19's fallback new Date(iso || date) allows malformed strings through when the regex fails.
Apply this diff to use a stricter pattern:
- // Accept only YYYY-MM-DD or full ISO strings
- const iso = /^\d{4}-\d{2}-\d{2}(T.*Z)?$/.test(date) ? date : '';
- const d = new Date(iso || date);
+ // Accept only YYYY-MM-DD or strict ISO 8601
+ const iso = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d{3})?Z)?$/.test(date);
+ if (!iso) {
+ throw new TypeError('Date string must be YYYY-MM-DD or ISO 8601 format');
+ }
+ const d = new Date(date);
if (isNaN(d.getTime())) {
throw new TypeError('Invalid date string');
}📝 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.
| public constructor(date?: string | Date) { | |
| if (date instanceof Date) { | |
| if (isNaN(date.getTime())) { | |
| throw new TypeError('Invalid Date'); | |
| } | |
| this.date = date; | |
| return; | |
| } | |
| if (!date) { | |
| this.date = new Date(); | |
| return; | |
| } | |
| // Accept only YYYY-MM-DD or full ISO strings | |
| const iso = /^\d{4}-\d{2}-\d{2}(T.*Z)?$/.test(date) ? date : ''; | |
| const d = new Date(iso || date); | |
| if (isNaN(d.getTime())) { | |
| throw new TypeError('Invalid date string'); | |
| } | |
| this.date = d; | |
| } | |
| public constructor(date?: string | Date) { | |
| if (date instanceof Date) { | |
| if (isNaN(date.getTime())) { | |
| throw new TypeError('Invalid Date'); | |
| } | |
| this.date = date; | |
| return; | |
| } | |
| if (!date) { | |
| this.date = new Date(); | |
| return; | |
| } | |
| // Accept only YYYY-MM-DD or strict ISO 8601 | |
| const iso = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d{3})?Z)?$/.test(date); | |
| if (!iso) { | |
| throw new TypeError('Date string must be YYYY-MM-DD or ISO 8601 format'); | |
| } | |
| const d = new Date(date); | |
| if (isNaN(d.getTime())) { | |
| throw new TypeError('Invalid date string'); | |
| } | |
| this.date = d; | |
| } |
🤖 Prompt for AI Agents
In libs/common/src/date-only.ts around lines 5 to 24, the regex and fallback
logic are too permissive and allow malformed date-time strings to be passed to
new Date; replace the loose pattern with a stricter regex that validates
year-month-day with valid month/day ranges and an optional properly-formed time
portion and timezone (e.g. require HH:MM:SS with optional fractional seconds and
either Z or ±HH:MM), only construct a Date when that stricter regex matches, and
if it doesn't match (and the input wasn't a valid Date object) throw a TypeError
instead of falling back to new Date(date); ensure invalid Date instances still
throw as before.
Signed-off-by: Tipu_Singh <[email protected]>
|
Summary by CodeRabbit
New Features
Chores