account-service: added graph key endpoints #488

aramikm · 2024-09-06T21:02:20Z

Problem

To be able to easily add graph keys we need endpoints which facilitates this operation.

Solution

implemented two new endpoints which

Creates an encoded payload which is ready for signing
Publishes a new graph key on chain
closes applyItemActions (for graph encryption key management) #414

Screenshots (optional):

new endpoints
endpoint that prepared the payload to add a new graph key
endpoint that submits the graph key creation

worker logs
blockchain submission

apps/account-worker/src/transaction_notifier/notifier.service.ts

mattheworris

Read through changes, looks good!
Do we want to add tests for these endpoints?

🚢 it!

JoeCap08055 · 2024-09-09T14:19:28Z

apps/account-api/src/controllers/v1/graphs-v1.controller.ts

+  @Get('addKey/:msaId')
+  @HttpCode(HttpStatus.OK)
+  @ApiQuery({
+    name: 'newKey',
+    description: 'New public key to be added in hex format',
+    type: 'string',
+    required: true,
+  })
+  @ApiOperation({ summary: 'Get a properly encoded StatefulStorageItemizedSignaturePayloadV2 that can be signed.' })
+  @ApiOkResponse({ description: 'Returned an encoded StatefulStorageItemizedSignaturePayloadV2 for signing' })
+  /**
+   * Using the provided query parameters, creates a new payload that can be signed to add new graph keys.
+   * @param queryParams - The query parameters for adding a new key
+   * @returns Payload is included for convenience. Encoded payload to be used when signing the transaction.
+   * @throws An error if the key already exists or the payload creation fails.
+   */
+  async getAddKeyPayload(
+    @Param('msaId') msaId: string,
+    @Query('newKey') newKey: HexString,
+  ): Promise<AddNewGraphKeyPayloadRequest> {
+    // this is temporary until we find a better way to enforce data validation. the validation decorator didn't work
+    if (!isHexString(newKey)) {
+      throw new BadRequestException('Not a valid Hex value!');
+    }
+    return this.graphsService.getAddingNewKeyPayload(msaId, newKey);
+  }


While I agree that this should be a GET operation, we need to be careful because the hex-encoded public key may be too long, depending on the web server configuration. We should either:

confirm that the max hex-encoded key length will not break under any valid web server configuration, or

change to a POST request and put the key in the request body (at least until there is more widespread support for the new HTTP QUERY method) (this is a common workaround for essentially "GET with a body" (see Refactor GET endpoints that use POST method to use new HTTP QUERY #219 )

The public key is limited to 32 byes currently. We have new issues to enforce better verifications for apis and we have another issue to limit the request size so I think that would solve the server overload.

Query is nice but I don't think a lot of clients/proxies and ... support it out of the box right now.

apps/account-api/src/controllers/v1/graphs-v1.controller.ts

apps/account-api/src/services/graphs.service.ts

apps/account-worker/src/transaction_publisher/publisher.service.ts

jest.config.json

JoeCap08055 · 2024-09-09T14:56:14Z

openapi-specs/account.openapi.json

I think this also should be in .pretttierignore

It already is but I think this is dependent on the code generator.

JoeCap08055 · 2024-09-09T14:57:01Z

openapi-specs/content-watcher.openapi.json

.prettierignore

It already is but I think this is dependent on the code generator.

JoeCap08055 · 2024-09-09T14:57:15Z

openapi-specs/graph.openapi.json

.prettierignore

JoeCap08055 · 2024-09-09T15:07:23Z

Overall, looks pretty good. Left some comments inline. My biggest comment is for naming REST API endpoints. I've been trying to follow this approach. Going by that, perhaps the following endpoint names would be better, or something similar:

GET <path>/graphKeys/addKeyPayload/{key} // see my inline comment about this one; it may need to be a POST
// or
POST <path>/graphKeys/getAddKeyPayload
// (aware that getAddKeyPayload breaks the naming convention referenced, but it's a necessary evil because we have to use a POST operation to perform a GET in this scenario)

POST <path>/graphKeys // no need for any other specifier; the HTTP method & resource name tell us what the endpoint does

GET <path>/graphKeys/{msaId} // return list of all graph keys for MSA
GET <path>/graphKeys/{msaId}/{index} // get specific graph key from MSA's list of graph keys

claireolmstead · 2024-09-09T16:29:51Z

apps/account-api/src/metadata.ts

@@ -1,14 +1,16 @@
 /* eslint-disable */
 export default async () => {
    const t = {


make sure to run format. this looks unformatted.

This is generated every time we change anything in the api and I'm not sure if we need formatting on generated code. Open to know why that might be useful.

claireolmstead · 2024-09-09T16:42:28Z

libs/account-lib/src/blockchain/blockchain.service.ts

+  public handlePublishGraphKeyTxResult(event: Event): ItemizedPageUpdated {
+    const graphKeyValues: Partial<ItemizedPageUpdated> = {};
+
+    // Grab the event data
+    if (event && this.api.events.statefulStorage.ItemizedPageUpdated.is(event)) {
+      graphKeyValues.msaId = event.data.msaId.toString();
+      graphKeyValues.schemaId = event.data.schemaId.toString();
+      graphKeyValues.prevContentHash = event.data.prevContentHash.toString();
+      graphKeyValues.currContentHash = event.data.currContentHash.toString();
+      graphKeyValues.debugMsg = `New Graph Public Key Added for msaId: ${graphKeyValues.msaId} and schemaId: ${graphKeyValues.schemaId}`;
+    }
+
+    return graphKeyValues as ItemizedPageUpdated;
+  }


Suggested change

public handlePublishGraphKeyTxResult(event: Event): ItemizedPageUpdated {

const graphKeyValues: Partial<ItemizedPageUpdated> = {};

// Grab the event data

if (event && this.api.events.statefulStorage.ItemizedPageUpdated.is(event)) {

graphKeyValues.msaId = event.data.msaId.toString();

graphKeyValues.schemaId = event.data.schemaId.toString();

graphKeyValues.prevContentHash = event.data.prevContentHash.toString();

graphKeyValues.currContentHash = event.data.currContentHash.toString();

graphKeyValues.debugMsg = `New Graph Public Key Added for msaId: ${graphKeyValues.msaId} and schemaId: ${graphKeyValues.schemaId}`;

}

return graphKeyValues as ItemizedPageUpdated;

}

public handlePublishGraphKeyTxResult(event: Event): ItemizedPageUpdated {

// Grab the event data

if (event && this.api.events.statefulStorage.ItemizedPageUpdated.is(event)) {

return {

msaId: event.data.msaId.toString(),

schemaId: event.data.schemaId.toString(),

prevContentHash: event.data.prevContentHash.toString(),

currContentHash: event.data.currContentHash.toString(),

debugMsg: `New Graph Public Key Added for msaId: ${event.data.msaId} and schemaId: ${event.data.schemaId}`,

};

}

throw new Error('Graph Key Tx Result Event invalid.');

}

I don't think we are handling the errors correctly for these events in general. Unfortunately throwing an error alone here will not solve that issue. I opened a new issue for this #500

apps/account-api/src/controllers/v1/keys-v1.controller.ts

JoeCap08055 · 2024-09-10T17:57:16Z

libs/account-lib/src/utils/custom.decorator.ts

@@ -0,0 +1,25 @@
+import { registerDecorator, ValidationArguments, ValidationOptions } from 'class-validator';
+
+export function IsHexPublicKey(validationOptions?: ValidationOptions) {


might be nice to add some validationOptions in here, such as:

prefixRequired: boolean

minByteLen: number

maxByteLen: number

Would make it easy to support different key algorithms in the future.

I'll add this in the validation specific ticket.

As an unrelated note I think going forward we have to be more careful about adding new features and functionality in general. We have a lot of existing features that needs to be hardened and adding more would only make our life harder.

One question I usually like to think about is that how hard would it be for a future developer to change this to support that extra feature or data. If it is easy enough and the probability of this being changed in the near future is low enough. Usually keeping it simple and dumb might be preferable.

The best code is no code at all!

probably from somebody wise.

aramikm added 2 commits September 6, 2024 13:59

account-service: added graph key endpoints

fdb171b

Merge branch 'main' into add_graph_key

6157c4f

aramikm requested review from JoeCap08055, claireolmstead and mattheworris September 6, 2024 21:19

aramikm marked this pull request as ready for review September 6, 2024 21:19

aramikm commented Sep 6, 2024

View reviewed changes

apps/account-worker/src/transaction_notifier/notifier.service.ts Outdated Show resolved Hide resolved

mattheworris approved these changes Sep 6, 2024

View reviewed changes