Sign and verify raw data with the provided key#273
Conversation
Signed-off-by: Ankita Patidar <ankita.patidar@ayanworks.com>
Signed-off-by: Ankita Patidar <ankita.patidar@ayanworks.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughNew cryptographic and credential management capabilities have been added to the multi-tenancy controller. These include endpoints and logic for signing and verifying arbitrary data and W3C credentials, as well as new type definitions supporting these operations. Existing logic remains unchanged, with all additions scoped to tenant-specific contexts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MultiTenancyController
participant TenantAgent
Client->>MultiTenancyController: sign(tenantId, signOptions)
MultiTenancyController->>TenantAgent: signData(signOptions)
TenantAgent-->>MultiTenancyController: signature
MultiTenancyController-->>Client: signature
Client->>MultiTenancyController: verify(tenantId, verifyOptions)
MultiTenancyController->>TenantAgent: verifyData(verifyOptions)
TenantAgent-->>MultiTenancyController: isValid
MultiTenancyController-->>Client: isValid
Client->>MultiTenancyController: signCredential(tenantId, store, credentialOptions)
MultiTenancyController->>TenantAgent: signCredential(credentialOptions)
TenantAgent-->>MultiTenancyController: signedCredential
MultiTenancyController-->>Client: signedCredential
Client->>MultiTenancyController: verifyCredential(tenantId, credentialOptions)
MultiTenancyController->>TenantAgent: verifyCredential(credentialOptions)
TenantAgent-->>MultiTenancyController: verifiedCredential
MultiTenancyController-->>Client: verifiedCredential
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/controllers/multi-tenancy/MultiTenancyController.ts (1)
2019-2024: Same unreachableinstanceofin credential verificationSame issue as signing path – the verification route will always throw.
Apply the same structural guard fix here.
🧹 Nitpick comments (3)
src/controllers/types.ts (2)
389-401: Explicitly document base64 pre-conditionBoth
data(andsignature) are required to be base-64 strings, but the type gives no hint.
Consider tightening the contract to prevent accidental raw-string usage:-export type SignDataOptions = { - data: string +export type SignDataOptions = { + /** base64-encoded payload */ + data: string keyType: KeyType publicKeyBase58: string } -export type VerifyDataOptions = { - data: string +export type VerifyDataOptions = { + /** base64-encoded payload */ + data: string keyType: KeyType publicKeyBase58: string signature: string }
403-414: Follow conventional PascalCase for type names
jsonLdCredentialOptionsandcredentialPayloadToSignbreak the otherwise-consistent PascalCase naming used in this file (DidCreate,CreateSchemaInput, …).
Renaming avoids cognitive friction and improves discoverability.-export interface jsonLdCredentialOptions { +export interface JsonLdCredentialOptions { -export interface credentialPayloadToSign { +export interface CredentialPayloadToSign {src/controllers/multi-tenancy/MultiTenancyController.ts (1)
1965-1976: Minor: duplicate assignment
const isValidSignature = ...is declared twice – outer and inner scopes.
Rename inner constant to avoid shadowing (or simplyreturn):- const isValidSignature = await tenantAgent.context.wallet.verify({ + const valid = await tenantAgent.context.wallet.verify({ ... - return isValidSignature + return valid
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/controllers/multi-tenancy/MultiTenancyController.ts(5 hunks)src/controllers/types.ts(2 hunks)
| JsonObject, | ||
| } from '@credo-ts/core' | ||
| import type { SingleOrArray } from '@credo-ts/core/build/utils' | ||
| import type { DIDDocument } from 'did-resolver' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid importing from private build paths
@credo-ts/core/build/utils is a private bundle path – it is not part of the public API and can break after every upstream release.
Import SingleOrArray from the public root package to keep the dependency stable.
-import type { SingleOrArray } from '@credo-ts/core/build/utils'
+import type { SingleOrArray } from '@credo-ts/core'📝 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.
| JsonObject, | |
| } from '@credo-ts/core' | |
| import type { SingleOrArray } from '@credo-ts/core/build/utils' | |
| import type { DIDDocument } from 'did-resolver' | |
| JsonObject, | |
| } from '@credo-ts/core' | |
| import type { SingleOrArray } from '@credo-ts/core' | |
| import type { DIDDocument } from 'did-resolver' |
🤖 Prompt for AI Agents
In src/controllers/types.ts around lines 28 to 31, the import of SingleOrArray
is done from the private path '@credo-ts/core/build/utils', which is unstable
and can break with upstream changes. Change the import statement to import
SingleOrArray directly from the public root package '@credo-ts/core' instead,
ensuring the dependency remains stable and supported.
| public async signCredential( | ||
| @Path('tenantId') tenantId: string, | ||
| @Query('storeCredential') storeCredential: boolean, | ||
| @Body() credentialToSign: W3cJsonLdSignCredentialOptions | W3cJwtSignCredentialOptions | ||
| ) { | ||
| let storedCredential | ||
| let formattedCredential | ||
| try { | ||
| await this.agent.modules.tenants.withTenantAgent({ tenantId }, async (tenantAgent) => { | ||
| if(credentialToSign.credential instanceof W3cCredential) { | ||
| const signedCred = await tenantAgent.w3cCredentials.signCredential(credentialToSign) | ||
| formattedCredential = JsonTransformer.fromJSON(signedCred, W3cJsonLdVerifiableCredential) //TODO: add support for JWT format as well | ||
| if(storeCredential) { | ||
| storedCredential = await tenantAgent.w3cCredentials.storeCredential({ credential: formattedCredential }) | ||
| } | ||
| } else { | ||
| throw new Error('Credential Type not supported') | ||
| } | ||
| }) | ||
| return storedCredential ?? formattedCredential |
There was a problem hiding this comment.
🛠️ Refactor suggestion
instanceof W3cCredential check can never be true
W3cCredential is a TypeScript type, erased at runtime.
credentialToSign.credential instanceof W3cCredential will therefore always
evaluate to false, and valid credentials will be rejected.
Replace the runtime check with a structural guard, e.g.:
if (!credentialToSign || !('credentialSubject' in credentialToSign.credential)) {
throw new BadRequestError('Unsupported credential payload')
}Also note that JsonTransformer.fromJSON is unnecessary – signCredential already returns a strongly-typed object.
🤖 Prompt for AI Agents
In src/controllers/multi-tenancy/MultiTenancyController.ts around lines 1985 to
2004, replace the runtime check using 'instanceof W3cCredential' with a
structural type guard that verifies the presence of expected properties like
'credentialSubject' on credentialToSign.credential to correctly identify valid
credentials. Remove the unnecessary call to JsonTransformer.fromJSON since
signCredential already returns a strongly-typed object. Adjust the logic
accordingly to throw a BadRequestError if the credential structure is
unsupported.
| import { assertAskarWallet } from '@credo-ts/askar/build/utils/assertAskarWallet' | ||
| import { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Importing assertAskarWallet from internal build path is brittle
@credo-ts/askar/build/utils/assertAskarWallet is an internal bundle path.
Use the public re-export to avoid future breakage:
-import { assertAskarWallet } from '@credo-ts/askar/build/utils/assertAskarWallet'
+import { assertAskarWallet } from '@credo-ts/askar'📝 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.
| import { assertAskarWallet } from '@credo-ts/askar/build/utils/assertAskarWallet' | |
| import { | |
| import { assertAskarWallet } from '@credo-ts/askar' | |
| import { |
🤖 Prompt for AI Agents
In src/controllers/multi-tenancy/MultiTenancyController.ts at lines 32-33, the
import of assertAskarWallet is done from an internal build path
'@credo-ts/askar/build/utils/assertAskarWallet', which is brittle. Change the
import to use the public re-export path provided by the '@credo-ts/askar'
package to ensure stability and avoid breakage in future updates.
| @Post('/sign/:tenantId') | ||
| public async sign(@Path('tenantId') tenantId: string, @Body() request: SignDataOptions) { | ||
| try { | ||
| const signature = await this.agent.modules.tenants.withTenantAgent({ tenantId }, async (tenantAgent) => { | ||
| assertAskarWallet(tenantAgent.context.wallet) | ||
|
|
||
| tenantAgent.w3cCredentials | ||
|
|
||
| const signature = await tenantAgent.context.wallet.sign({ | ||
| data: TypedArrayEncoder.fromBase64(request.data), | ||
| key: Key.fromPublicKeyBase58(request.publicKeyBase58, request.keyType), | ||
| }) | ||
| return TypedArrayEncoder.toBase64(signature) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
sign endpoint will fail – private key never loaded
wallet.sign requires the secret part of the key stored in the wallet.
Key.fromPublicKeyBase58 constructs an in-memory handle without the secret, resulting in
AskarError: secret bytes not available.
Replace the ad-hoc key construction with a lookup inside the wallet and drop the stray tenantAgent.w3cCredentials line:
- tenantAgent.w3cCredentials
-
- const signature = await tenantAgent.context.wallet.sign({
- data: TypedArrayEncoder.fromBase64(request.data),
- key: Key.fromPublicKeyBase58(request.publicKeyBase58, request.keyType),
- })
+ const key = await tenantAgent.context.wallet.getKey({
+ publicKeyBase58: request.publicKeyBase58,
+ })
+ if (!key) throw new NotFoundError('Keypair not found in wallet')
+
+ const signatureBytes = await tenantAgent.context.wallet.sign({
+ data: TypedArrayEncoder.fromBase64(request.data),
+ key,
+ })
+ const signature = TypedArrayEncoder.toBase64(signatureBytes)📝 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.
| @Post('/sign/:tenantId') | |
| public async sign(@Path('tenantId') tenantId: string, @Body() request: SignDataOptions) { | |
| try { | |
| const signature = await this.agent.modules.tenants.withTenantAgent({ tenantId }, async (tenantAgent) => { | |
| assertAskarWallet(tenantAgent.context.wallet) | |
| tenantAgent.w3cCredentials | |
| const signature = await tenantAgent.context.wallet.sign({ | |
| data: TypedArrayEncoder.fromBase64(request.data), | |
| key: Key.fromPublicKeyBase58(request.publicKeyBase58, request.keyType), | |
| }) | |
| return TypedArrayEncoder.toBase64(signature) | |
| }) | |
| @Post('/sign/:tenantId') | |
| public async sign( | |
| @Path('tenantId') tenantId: string, | |
| @Body() request: SignDataOptions | |
| ) { | |
| try { | |
| const signature = await this.agent.modules.tenants.withTenantAgent( | |
| { tenantId }, | |
| async (tenantAgent) => { | |
| assertAskarWallet(tenantAgent.context.wallet) | |
| // Look up the full keypair (with secret) instead of constructing one in-memory | |
| const key = await tenantAgent.context.wallet.getKey({ | |
| publicKeyBase58: request.publicKeyBase58, | |
| }) | |
| if (!key) { | |
| throw new NotFoundError('Keypair not found in wallet') | |
| } | |
| // Sign using the real keypair | |
| const signatureBytes = await tenantAgent.context.wallet.sign({ | |
| data: TypedArrayEncoder.fromBase64(request.data), | |
| key, | |
| }) | |
| const signature = TypedArrayEncoder.toBase64(signatureBytes) | |
| return signature | |
| } | |
| ) | |
| // …rest of your controller logic… | |
| } catch (err) { | |
| // …error handling… | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/controllers/multi-tenancy/MultiTenancyController.ts around lines 1934 to
1947, the sign method incorrectly uses Key.fromPublicKeyBase58 to create a key
without the secret part, causing wallet.sign to fail. To fix this, remove the
stray tenantAgent.w3cCredentials line and replace the key construction with a
proper lookup of the key inside the wallet that includes the secret key material
before calling wallet.sign.
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/controllers/multi-tenancy/MultiTenancyController.ts (2)
29-30: Use public re-export forassertAskarWalletThe import still targets the internal build path (
@credo-ts/askar/build/utils/assertAskarWallet).
Please switch to the public export to avoid breakage on package updates.-import { assertAskarWallet } from '@credo-ts/askar/build/utils/assertAskarWallet' +import { assertAskarWallet } from '@credo-ts/askar'
1930-1942:⚠️ Potential issue
sign()will throw – secret key never loaded
Key.fromPublicKeyBase58creates an in-memory key handle that contains no secret bytes.
wallet.sign()therefore fails withAskarError: secret bytes not available.Fix: look the key up inside the wallet (which holds the secret part) and keep the
assertAskarWalletguard that is currently commented out.- // assertAskarWallet(tenantAgent.context.wallet) - - const signature = await tenantAgent.context.wallet.sign({ - data: TypedArrayEncoder.fromBase64(request.data), - key: Key.fromPublicKeyBase58(request.publicKeyBase58, request.keyType), - }) + assertAskarWallet(tenantAgent.context.wallet) + + const key = await tenantAgent.context.wallet.getKey({ + publicKeyBase58: request.publicKeyBase58, + }) + if (!key) throw new NotFoundError('Keypair not found in wallet') + + const sigBytes = await tenantAgent.context.wallet.sign({ + data: TypedArrayEncoder.fromBase64(request.data), + key, + }) + const signature = TypedArrayEncoder.toBase64(sigBytes)
🧹 Nitpick comments (5)
src/controllers/multi-tenancy/MultiTenancyController.ts (1)
1960-1971: Minor: keep symmetry betweensignandverify
verify()correctly asserts an Askar wallet, whilesign()has the guard commented out.
Oncesign()is fixed (see above) consider restoring the guard for consistency.src/routes/swagger.json (4)
1628-1646: Enforce immutability on SignDataOptions schema
To keep schema definitions strict and prevent unexpected properties, add"additionalProperties": falseunder theSignDataOptionsobject."SignDataOptions": { "properties": { "publicKeyBase58": { "type": "string" }, "keyType": { "$ref": "#/components/schemas/KeyType" }, "data": { "type": "string" } }, "required": ["publicKeyBase58", "keyType", "data"], "type": "object" + ,"additionalProperties": false }
1647-1669: Enforce immutability on VerifyDataOptions schema
Mirror other schemas by disallowing extra fields. Add"additionalProperties": falseunder theVerifyDataOptionsobject."VerifyDataOptions": { "properties": { "signature": { "type": "string" }, "publicKeyBase58": { "type": "string" }, "keyType": { "$ref": "#/components/schemas/KeyType" }, "data": { "type": "string" } }, "required": ["signature", "publicKeyBase58", "keyType", "data"], "type": "object" + ,"additionalProperties": false }
6640-6688: Refine/multi-tenancy/sign/{tenantId}operation
- The
operationId:"Sign"is very generic and may collide with other operations.- The description should live under
requestBodyrather than inside the schema.
6689-6737: Refine/multi-tenancy/verify/{tenantId}operation
- Consider a more specific
operationId(e.g.,VerifyData).- Move the request description from the schema to the
requestBodylevel for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/controllers/multi-tenancy/MultiTenancyController.ts(5 hunks)src/routes/routes.ts(2 hunks)src/routes/swagger.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/routes/routes.ts (2)
src/utils/TsyringeAdapter.ts (1)
iocContainer(5-9)src/utils/tsyringeTsoaIocContainer.ts (1)
iocContainer(5-9)
🪛 ESLint
src/controllers/multi-tenancy/MultiTenancyController.ts
[error] 1989-1989: 'credential' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (4)
src/routes/swagger.json (3)
1670-1709: LinkedDataProof schema looks correct
Properties are well-defined and immutability is enforced. No changes needed.
1710-1755: DataIntegrityProof schema looks correct
Required fields and immutability match the rest of the definitions.
1756-1782: SingleOrArray_LinkedDataProof-or-DataIntegrityProof union looks correct
The anyOf array covers both singular and array cases appropriately.src/routes/routes.ts (1)
1-5192: Auto-generated file changes align with PR objectives.The additions to this auto-generated tsoa routes file correctly implement the new signing and verification functionality described in the PR objectives. The new model definitions and route handlers follow the established patterns and include proper authentication middleware.
Key additions:
- Models for
SignDataOptions,VerifyDataOptions, and W3C credential structures- Routes for
/multi-tenancy/sign/:tenantId,/multi-tenancy/verify/:tenantId,/multi-tenancy/credential/sign/:tenantId, and/multi-tenancy/credential/verify/:tenantIdSince this is an auto-generated file, ensure that the source controllers and type definitions that generate these routes are properly implemented.
| await this.agent.modules.tenants.withTenantAgent({ tenantId }, async (tenantAgent) => { | ||
| const {credential, ...credentialObject} = credentialToSign | ||
| const transformedCredential = JsonTransformer.fromJSON(credentialToSign.credential, W3cCredential) | ||
| const signedCred = await tenantAgent.w3cCredentials.signCredential({credential: transformedCredential, ...credentialObject}) as W3cJsonLdVerifiableCredential | ||
| formattedCredential = signedCred.toJson() | ||
| if (storeCredential) { | ||
| storedCredential = await tenantAgent.w3cCredentials.storeCredential({ credential: signedCred }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused variable, wrong property reference & wrong casing
credentialis extracted but never used – ESLint flags this.JsonTransformer.fromJSON(credentialToSign.credential, …)should use the
freshly-destructuredcredentialvariable.signedCred.toJson()is inconsistent with the rest of the codebase
(toJSON()is the canonical name and the one provided byclass JsonObject).
- const {credential, ...credentialObject} = credentialToSign
- const transformedCredential = JsonTransformer.fromJSON(credentialToSign.credential, W3cCredential)
- const signedCred = await tenantAgent.w3cCredentials.signCredential({credential: transformedCredential, ...credentialObject}) as W3cJsonLdVerifiableCredential
- formattedCredential = signedCred.toJson()
+ const { credential, ...credentialObject } = credentialToSign
+ const transformedCredential = JsonTransformer.fromJSON(credential, W3cCredential)
+ const signedCred = await tenantAgent.w3cCredentials.signCredential({
+ credential: transformedCredential,
+ ...credentialObject,
+ }) as W3cJsonLdVerifiableCredential
+ formattedCredential = signedCred.toJSON()📝 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.
| await this.agent.modules.tenants.withTenantAgent({ tenantId }, async (tenantAgent) => { | |
| const {credential, ...credentialObject} = credentialToSign | |
| const transformedCredential = JsonTransformer.fromJSON(credentialToSign.credential, W3cCredential) | |
| const signedCred = await tenantAgent.w3cCredentials.signCredential({credential: transformedCredential, ...credentialObject}) as W3cJsonLdVerifiableCredential | |
| formattedCredential = signedCred.toJson() | |
| if (storeCredential) { | |
| storedCredential = await tenantAgent.w3cCredentials.storeCredential({ credential: signedCred }) | |
| } | |
| }) | |
| await this.agent.modules.tenants.withTenantAgent({ tenantId }, async (tenantAgent) => { | |
| const { credential, ...credentialObject } = credentialToSign | |
| const transformedCredential = JsonTransformer.fromJSON(credential, W3cCredential) | |
| const signedCred = await tenantAgent.w3cCredentials.signCredential({ | |
| credential: transformedCredential, | |
| ...credentialObject, | |
| }) as W3cJsonLdVerifiableCredential | |
| formattedCredential = signedCred.toJSON() | |
| if (storeCredential) { | |
| storedCredential = await tenantAgent.w3cCredentials.storeCredential({ credential: signedCred }) | |
| } | |
| }) |
🧰 Tools
🪛 ESLint
[error] 1989-1989: 'credential' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In src/controllers/multi-tenancy/MultiTenancyController.ts around lines 1988 to
1996, fix the unused variable by removing the destructured 'credential' since
it's not used directly. Change the call to JsonTransformer.fromJSON to use the
destructured 'credential' variable instead of accessing
credentialToSign.credential. Also, update the method call from
signedCred.toJson() to signedCred.toJSON() to match the canonical casing used in
the codebase.
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
GHkrishna
added
the
enhancement
* feat:sign and verify raw data with the provided key Signed-off-by: Ankita Patidar <ankita.patidar@ayanworks.com> * sign and verify credential payload Signed-off-by: Ankita Patidar <ankita.patidar@ayanworks.com> * fix: update sign API Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: verification logic Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: final touches Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: verify credential even with credential status object present in it Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: final touches Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> --------- Signed-off-by: Ankita Patidar <ankita.patidar@ayanworks.com> Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> Co-authored-by: Krishna Waske <krishna.waske@ayanworks.com> Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
4 participants
Capability to sign and verify provided data with they keypair associated with the provided publicKey.
If the keypair is present in the wallet, it will sign the data.
Summary by CodeRabbit
New Features
Documentation