feat: Refactor AppModule and Add Credential Management with Revocation Support #64

lotharking · 2024-12-16T20:46:22Z

Summary

Flow:

Create a Credential
- The user can request to create a credential.
- The library validates whether the credential can be created and ensures there isn't already one associated with the same hash.
  - If a credential with the same hash exists, the credential will be revoked and associated with the new connection (from where the request is sent). A new credential request will then be initiated, ensuring the proper threadId is sent as part of the process
  - If valid, the credential creation request is sent.
Rejection
- Associates the thread and marks the credential as revoked (revoked only in terms of database nomenclature).
Acceptance
- Associates the thread with the credential.
Revocation
- When a credential is revoked, it is marked as revoked in the database.

Additional Constraints:

In all cases, a new record is created, ensuring indices do not overlap and atomicity is guaranteed during queries.
Records are expected to be assigned from 0 to the maximum value minus 1 (-1).
- Validation has confirmed that assignments from 1 to the maximum value fail.
Two revocation records will always remain available.

Changes

A refactoring was implemented in the AppModule of the nestjs-client module. This was necessary because the original main module was not optimized for the recurrent use of variables, requiring multiple instantiations of the same variables. Modifying the AppModule and the configuration approach avoided redundancy when the module is instantiated. Compatibility with individual configurations was maintained.
Support for credential management and revocation was implemented when configuring the nestjs-client module. The methods are controlled through the library, allowing for handling rejections and revocations as well.
A demo was created to showcase how to use the demo-dts. As observed, the configuration required for complete credential management is minimal.

Related Issues

Support revocable credentials #58

Testing

Checklist

I have commented on my code, especially in areas that are hard to understand.
I have added only changes relevant to the issue in question.
I have added tests to confirm that my fix is effective or that my feature works as intended.
I have modified existing tests as needed to accommodate my changes.
I have flagged specific areas for further review, if necessary.
I have updated the documentation where necessary.

genaris · 2025-01-16T20:26:31Z

packages/nestjs-client/src/credentials/credential.service.ts

+    }
+    const { id: credentialDefinitionId, revocationSupported } = credentialType
+
+    const cred = await this.credentialRepository.findOne({


Note that there is a chance that the user issued multiple credentials with the same refId (i.e. they didn't set revokeIfAlreadyIssued) and then they attempt to issue another credential with the same refId but using revokeIfAlreadyIssued = true, only the first credential found here will be revoked.

I think it would be safer to find all non-revoked issued credentials that match the refId and revoke/save them accordingly.

fix: implement revocation for all non-revoked credentials

genaris · 2025-01-16T20:57:12Z

packages/nestjs-client/src/credentials/credential.service.ts

+  }
+
+  // private methods
+  private async saveCredentialType(


I don't understand the meaning of this method: why does it save a credential type in the credential repository?

Why don't you simply name it createRevocationRegistry and call it only when appropriate (i.e. supportRevocation is true when creating credential type or the current revocation registry is full)?

fix: updated to createRevocationRegistry

genaris · 2025-01-16T20:58:06Z

packages/nestjs-client/src/credentials/credential.service.ts

+    return revocationRegistry
+  }
+
+  private refIdHash(refId: string): string {


The method itself seems to be a simple hash operation. What about making it general and simply call it hash(value: string)?

fix: change name refIdHash by hash

genaris · 2025-01-16T21:59:59Z

packages/nestjs-client/src/credentials/credential.service.ts

+   * @param threadId - The thread ID to link with the credential.
+   * @throws Error if no credential is found with the specified connection ID.
+   */
+  async accept(connectionId: string, threadId: string): Promise<void> {


Now I understand why you called this service an events service in the beginning: accept and reject are user actions, so they are called by event handlers coming from the agent. Since they are not actions performed by us (such as credential issuance and revocation), they can be called `processAcceptance/processRejection' or 'handleAcceptance/handleRejection', showing that they are supposed to act as a result from an event coming from the DIDComm layer.

fix: change name to handleAcceptance/handleRejection

genaris · 2025-01-16T22:07:03Z

packages/nestjs-client/src/credentials/credential.service.ts

+   * @throws Error if no credential is found with the specified connection ID.
+   */
+  async accept(connectionId: string, threadId: string): Promise<void> {
+    const cred = await this.credentialRepository.findOne({


A problem I see here is that you are using connectionId to identify the credential the event is related to. This is not correct, since you can issue multiple credentials to a single DIDComm connection.

The proper way you should identify a credential is by its threadId. Therefore, these accept and reject methods should only receive the threadId.

You can find the threadId in the response of the Message endpoint (the id correspond to the threadId).

fix: acept and reject only by thread

genaris · 2025-01-16T22:09:34Z

packages/nestjs-client/src/credentials/credential.service.ts

+    if (!cred) throw new Error(`Credencial with connectionId ${connectionId} not found.`)
+
+    cred.threadId = threadId
+    cred.revoked = true


A rejection by the user does not mean that a credential has been revoked. Maybe you can create a status field for the credential entity that covers the entire life cycle of a credential: probably offered, accepted, rejected and revoked may be enough.

fix: implement to CredentialStatus

genaris · 2025-01-16T22:10:24Z

packages/nestjs-client/src/credentials/credential.service.ts

+      maximumCredentialNumber?: number
+    } = {},
+  ) {
+    const { name = 'Chatbot', version = '1.0', supportRevocation, maximumCredentialNumber } = options


I don't think it is correct to leave name and version as optional fields.

fix: remove name and version from optionals fields

genaris · 2025-01-16T22:16:36Z

packages/nestjs-client/src/credentials/credential.service.ts

+            revocationRegistryIndex: undefined,
+          }
+        }
+        const invalidRegistries = await transaction.find(CredentialEntity, {


This logic is quite obscure to me. I don't think it will work.

I guess that somewhere (maybe another table/entity) you should keep track of the current revocation registry and index for each credential definition, and use that as the source of truth to assign the revocation registry/index of each issued credential, and tell if it is needed to create a new revocation registry or not.

feat: Creation of the RevocationRegistryEntity for managing revocation records.

Please review this last change with special care. The RevocationRegistryEntity was created, and during its implementation, it was detected that the threadId returned by the CredentialIssuanceMessage was not being properly handled by the SA. As a solution, GenericRecords was used to retrieve it. Additionally, as you mentioned, the handling of credentials has been improved by dividing the entities.

packages/nestjs-client/src/credentials/credential.service.ts

Co-authored-by: Ariel Gentile <gentilester@gmail.com>

…n records

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris · 2025-01-21T17:56:44Z

packages/nestjs-client/src/credentials/revocation-registry.entity.ts

+import { Entity, Column, PrimaryGeneratedColumn, CreateDateColumn, UpdateDateColumn } from 'typeorm'
+
+@Entity('revocation_registries')
+export class RevocationRegistryEntity {


Add credentialDefinitionId

genaris · 2025-01-21T17:57:18Z

packages/nestjs-client/src/credentials/revocation-registry.entity.ts

+  revocationDefinitionId!: string
+
+  @Column({ type: 'integer', nullable: false, default: 0 })
+  revocationRegistryIndex!: number


Suggested change

revocationRegistryIndex!: number

currentIndex!: number

fix: implemented currentIndex

genaris · 2025-01-21T17:57:36Z

packages/nestjs-client/src/credentials/credential.entity.ts

+  id!: string
+
+  @Column({ type: 'varchar', nullable: false })
+  credentialDefinitionId!: string


add revocationRegistryIndex

fix: add credentialDefinitionId to revocation registry entity

genaris · 2025-01-21T17:58:11Z

packages/nestjs-client/src/credentials/credential.service.ts

+        })
+      }
+      let lastCred = await transaction
+        .createQueryBuilder(RevocationRegistryEntity, 'registry')


add credentialDefinitionId to the filter

lotharking added 30 commits December 10, 2024 17:18

feat: update model supporting revocation

4dd777c

feat: create FullTailsFileService and add server for tails

f576635

fix: create FullTailsFileService and add server for tails

da3ade9

feat: update controller credential type

bb061dc

fix: test

42736a0

fix: test

1ac0989

fix: test

efc7e83

Merge branch 'main' into feat/add-revocation-registry

7640997

feat: add axios to tales service

a66bdcc

feat: add revocation to get credential

a6a327d

feat: update did server for revcredential

a1277e0

feat: improve credential request message

206ab4c

feat: add update revocation status list

e9d70d9

fix: add update revocation status list

a83ec16

fix: add update revocation status list

27b9a33

fix: add update revocation status list

4dcf175

feat: update index

a0363f2

feat: update credential generation

ce1fc60

fix: update

aa74e16

fix: update

709567b

docs: update

69b2465

fix: update

04b345f

fix: return

37c335a

fix: return

a9aff6b

feat: add CredentialRevocationMessage

0815f41

Merge branch 'main' into feat/add-revocation-registry

30ddd25

feat: add CredentialRevocationMessage

8e3ac1f

add: docs

44e3be7

docs: update

6a559e1

feat: update chatbot demo

9912183

fix: add CredentialTypeResult

c0743e3

lotharking requested a review from genaris January 16, 2025 17:45

genaris requested changes Jan 16, 2025

View reviewed changes

lotharking and others added 13 commits January 16, 2025 18:01

fix: Update packages/nestjs-client/src/credentials/credential.service.ts

e44a5c4

Co-authored-by: Ariel Gentile <gentilester@gmail.com>

Merge branch 'main' into feat/create-revocation-nestjs-client

dae1bb7

fix: change name refIdHash by hash

cedf3f0

fix: remove name and version from optionals fields

43d85ad

fix: implement revocation for all non-revoked credentials

8012af1

fix: updated to createRevocationRegistry

53ec35d

fix: change name to handleAcceptance/handleRejection

9831c46

fix: acept and reject only by thread

696e285

fix: implement to CredentialStatus

87e7586

fix: acept and reject only by thread

4425e97

fix: acept and reject only by thread

692f640

feat: Creation of the RevocationRegistryEntity for managing revocatio…

5763e9f

…n records

feat: Creation of the RevocationRegistryEntity for managing revocatio…

0641fe2

…n records

lotharking requested a review from genaris January 20, 2025 13:45

fix: change Service Agent message threadId

a10c1e4

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris requested changes Jan 21, 2025

View reviewed changes

lotharking added 7 commits January 21, 2025 16:52

fix: add credentialDefinitionId to revocation registry entity

96876b4

fix: add credentialDefinitionId to where

bc2386e

fix: implemented currentIndex

27e8562

fix: update revocationRegistryIndex current index

061115e

fix: add flow record

6b1c4de

docs: update

55497d5

fix: CredentialStateChangedEvent

be2a2e4

lotharking requested a review from genaris January 22, 2025 13:22

genaris approved these changes Jan 22, 2025

View reviewed changes

genaris merged commit 9758805 into main Jan 22, 2025
2 checks passed

genaris deleted the feat/create-revocation-nestjs-client branch January 22, 2025 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor AppModule and Add Credential Management with Revocation Support #64

feat: Refactor AppModule and Add Credential Management with Revocation Support #64

lotharking commented Dec 16, 2024 •

edited

Loading

genaris Jan 16, 2025

lotharking Jan 17, 2025

genaris Jan 16, 2025

lotharking Jan 17, 2025

genaris Jan 16, 2025

lotharking Jan 16, 2025

genaris Jan 16, 2025

lotharking Jan 17, 2025

genaris Jan 16, 2025

lotharking Jan 17, 2025

genaris Jan 16, 2025

lotharking Jan 17, 2025

genaris Jan 16, 2025

lotharking Jan 16, 2025

genaris Jan 16, 2025

lotharking Jan 18, 2025

genaris Jan 21, 2025

lotharking Jan 22, 2025

genaris Jan 21, 2025

lotharking Jan 22, 2025

genaris Jan 21, 2025

lotharking Jan 21, 2025

genaris Jan 21, 2025

lotharking Jan 22, 2025

feat: Refactor AppModule and Add Credential Management with Revocation Support #64

feat: Refactor AppModule and Add Credential Management with Revocation Support #64

Conversation

lotharking commented Dec 16, 2024 • edited Loading

Summary

Flow:

Additional Constraints:

Changes

Related Issues

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lotharking commented Dec 16, 2024 •

edited

Loading