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.

packages/main/src/controllers/credentials/CredentialTypeController.ts

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

genaris · 2025-01-16T12:42:25Z

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

+      await this.saveCredential(credential.id, supportRevocation)
+      await this.saveCredential(credential.id, supportRevocation)


Same comment as before: these methods are saving credential types

fix: name credentialType

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

genaris · 2025-01-16T12:43:13Z

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

+   * @param {object} [options] - Additional options for credential issuance.
+   *
+   * ### Options
+   * - `refId` (optional, `string`): A unique identifier for the credential. If provided:


This is a great explanation! But it makes the reader think that the only requirement for credential revocation at issuance is to have the same refId , while currently we also need to set autoRevocationEnabled to do so.

If autoRevocationEnabled is false (default), multiple credentials with the same refId will be created. This is not necessarily wrong, but what happens if afterwards I want to issue a credential with the same refId and turn autoRevocationEnabled on? Will it revoke ALL previous credentials with the same refId?

docs: update

genaris · 2025-01-16T13:52:55Z

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

+   *   - Encrypted in the database for security.
+   * - `credentialDefinitionId` (optional, `string`): Specifies the ID of the credential definition to use.
+   *   - If not provided, the first available credential definition is used.
+   * - `autoRevocationEnabled` (optional, `boolean`): Whether automatic revocation is enabled (default false)


I think this should be more self-explanatory. Maybe something like: revokeIfAlreadyIssued

fix: add revokeIfAlreadyIssued

genaris · 2025-01-16T13:53:50Z

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

+  threadId?: string
+
+  @Column({ type: 'varchar', nullable: true })
+  hashIdentifier?: string


I think this field should be renamed. refIdHash?

fix: name refIdHash

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

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

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

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.

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)?

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.

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).

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.

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.

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

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

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

lotharking added 9 commits January 15, 2025 16:36

docs: update

bfb8cf4

fix: implement selection credential by name and version

bdb02a7

fix: updated name method create

c03f1cc

fix: updated for many credentials

b5a8510

fix: updated for many credentials

70b3b62

fix: update on demo dts

275de0d

fix: update on demo dts

f5c0fba

fix: remove name hashRefId

96c88e1

fix: add supportRevocation by get credential types

e230f48

lotharking requested a review from genaris January 16, 2025 06:34

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

88a5d3f

genaris requested changes Jan 16, 2025

View reviewed changes

lotharking and others added 12 commits January 16, 2025 10:34

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

53b47de

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

6e62c40

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

fix: Update packages/main/src/controllers/credentials/CredentialTypeC…

c75b92f

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

fix: add revocationSupported

4cf2e30

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

9231f63

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

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

107f250

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

fix: name credType

e6139e7

fix: name refIdHash

9018b1d

fix: add revokeIfAlreadyIssued

41f18af

docs: update

a68003e

fix: linter

37cec5b

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 4 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

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 16, 2025

genaris Jan 16, 2025

lotharking Jan 16, 2025

genaris Jan 16, 2025

lotharking Jan 16, 2025

genaris Jan 16, 2025

lotharking Jan 16, 2025

genaris Jan 16, 2025

genaris Jan 16, 2025

genaris Jan 16, 2025

lotharking Jan 16, 2025

genaris Jan 16, 2025

genaris Jan 16, 2025

genaris Jan 16, 2025

genaris Jan 16, 2025

lotharking Jan 16, 2025

genaris Jan 16, 2025

		await this.saveCredential(credential.id, supportRevocation)
		await this.saveCredential(credential.id, supportRevocation)

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

Are you sure you want to change the base?

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

lotharking commented Dec 16, 2024 •

edited

Loading