-
Notifications
You must be signed in to change notification settings - Fork 1
MILAB-5815: resource signature support #1553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 16 commits
2c8cdd4
2cd0a79
110a697
385217b
828479a
b05b7cd
8b1b471
d47af3a
de52353
16bdf1e
9eb7a35
c01dc4b
4e6f65f
156422b
b8abbbe
c05891e
9913614
0702bdc
8ac66a7
84698f3
392677f
5322110
1cd84e1
5aa69af
8bd1835
0a2e14a
7a37723
2660124
6db30bb
1a8c22c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| --- | ||
| "@milaboratories/pl-client": major | ||
| --- | ||
|
|
||
| Sync pl proto API: resource signing, access control, locks, auth, field renames. | ||
|
|
||
| **Breaking changes:** | ||
|
|
||
| Proto field renames: | ||
| - `Resource.id` → `Resource.resource_id` | ||
| - `ResourceAPI.Remove.Request.id` → `ResourceAPI.Remove.Request.resource_id` | ||
| - `FieldAPI.SetError.Request.err_resource_id` → `FieldAPI.SetError.Request.error_resource_id` | ||
|
|
||
| Proto field removals: | ||
| - `MaintenanceAPI.Ping.Response.server_info` removed (field 3) | ||
|
|
||
| Proto deprecations: | ||
| - `CacheAPI.DeleteExpiredRecords` replaced with `Util.Deprecated` placeholder | ||
|
|
||
| Lease endpoint URL changes: | ||
| - `/v1/locks/lease` → `/v1/locks/lease/create` | ||
| - `PUT /v1/locks/lease` → `POST /v1/locks/lease/update` | ||
| - `DELETE /v1/locks/lease` → `POST /v1/locks/lease/release` | ||
|
|
||
| **New proto fields — resource signing:** | ||
|
|
||
| Core messages: | ||
| - `Resource.resource_signature` (bytes) — opaque signature for resource ID | ||
| - `Field.value_signature` (bytes) — signature for field value resource, inheriting parent color | ||
| - `Field.error_signature` (bytes) — signature for error resource, inheriting parent color | ||
| - `FieldRef.resource_signature` (bytes) — signature for the referenced resource | ||
|
|
||
| Transaction operations: | ||
| - `TxAPI.SetDefaultColor` — set default color for resource creation via `color_proof` | ||
|
|
||
| Resource creation — `color_proof` added to: | ||
| - `CreateStruct.Request`, `CreateEphemeral.Request`, `CreateValue.Request`, `CreateSingleton.Request` | ||
|
|
||
| Resource creation — `resource_signature` added to responses: | ||
| - `CreateStruct`, `CreateEphemeral`, `CreateValue`, `CreateSingleton`, `CreateRoot`, `GetValueID` | ||
|
|
||
| Resource access — `resource_signature` added to requests: | ||
| - `Remove`, `Get`, `LockInputs`, `LockOutputs`, `Exists`, `SetError` (+ `error_resource_signature`), `Tree`, `TreeSize`, `Name.Set` | ||
|
|
||
| Resource access — `resource_signature` added to responses: | ||
| - `Name.Get` | ||
|
|
||
| Field operations — `resource_signature` added to: | ||
| - `FieldAPI.List.Request`, `FieldAPI.SetError.Request` (`error_resource_signature`) | ||
|
|
||
| KV operations — `resource_signature` added to all `ResourceKVAPI.*.Request`: | ||
| - `Set`, `Get`, `GetIfExists`, `Delete`, `SetFlag`, `GetFlag`, `List` | ||
|
|
||
| Lease operations — `resource_signature` added to: | ||
| - `Lease.Create.Request`, `Lease.Update.Request`, `Lease.Release.Request` | ||
|
|
||
| **New API — access control:** | ||
|
|
||
| RPCs: | ||
| - `GrantAccess` — grant resource access to another user | ||
| - `RevokeGrant` — revoke previously granted access | ||
| - `ListGrants` — server-side streaming, list grants for a resource | ||
| - `ListUserResources` — server-side streaming, user root + shared resources with pagination | ||
|
|
||
| Messages: | ||
| - `AuthAPI.Grant` — grant record (user, resource_id, permissions, granted_by, granted_at) | ||
| - `AuthAPI.Grant.Permissions` — access bitmask (writable) | ||
| - `AuthAPI.ListUserResources.UserRoot` — signed user root | ||
| - `AuthAPI.ListUserResources.SharedResource` — signed shared resource with type and permissions | ||
|
|
||
| **New API — auth:** | ||
|
|
||
| - `AuthAPI.GetJWTToken.Role` enum — `ROLE_UNSPECIFIED`, `USER`, `CONTROLLER`, `WORKFLOW` | ||
| - `AuthAPI.GetJWTToken.Request.requested_role` — request JWT with specific role | ||
| - `AuthAPI.GetJWTToken.Response.session_id` — 128-bit session ID | ||
|
|
||
| **New API — locks:** | ||
|
|
||
| - `LocksAPI.LockFieldValues` — optimistic locking on resolved field values | ||
|
|
||
| **New API — schema:** | ||
|
|
||
| - `ResourceSchema.AccessFlags` — per-type access restrictions for non-controller roles (create_resource, read_fields, write_fields, read_kv, write_kv, per-field-type overrides via read_by_field_type/write_by_field_type maps) | ||
| - `ResourceSchema.free_inputs` / `free_outputs` — skip automatic locking on creation | ||
|
|
||
| **New API — notifications:** | ||
|
|
||
| - `Notification.Events.resource_recovered` — new event type | ||
|
|
||
| **New utility:** | ||
|
|
||
| - `Util.Deprecated` — empty message for deprecated oneOf slots |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,18 @@ import { test, expect } from "vitest"; | |||||||||||
|
|
||||||||||||
| import { isTimeoutOrCancelError } from "./errors"; | ||||||||||||
| import { Aborted } from "@milaboratories/ts-helpers"; | ||||||||||||
| import type { LLPlClient } from "./ll_client"; | ||||||||||||
|
|
||||||||||||
| /** Get root resource signature from ListUserResources for use as color proof in tests. */ | ||||||||||||
| async function getRootSignature(client: LLPlClient): Promise<Uint8Array> { | ||||||||||||
| const responses = await client.listUserResources({ limit: 1 }); | ||||||||||||
| for (const msg of responses) { | ||||||||||||
| if (msg.entry.oneofKind === "userRoot" && msg.entry.userRoot.resourceSignature) { | ||||||||||||
| return msg.entry.userRoot.resourceSignature; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| return new Uint8Array(0); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| test("check successful transaction", async () => { | ||||||||||||
| const client = await getTestLLClient(); | ||||||||||||
|
|
@@ -80,6 +92,7 @@ test("check timeout error type (passive)", async () => { | |||||||||||
|
|
||||||||||||
| test("check timeout error type (active)", async () => { | ||||||||||||
| const client = await getTestLLClient(); | ||||||||||||
| const rootSig = await getRootSignature(client); | ||||||||||||
| const tx = client.createTx(true, { timeout: 500 }); | ||||||||||||
|
|
||||||||||||
| try { | ||||||||||||
|
|
@@ -96,6 +109,12 @@ test("check timeout error type (active)", async () => { | |||||||||||
| ); | ||||||||||||
| expect(openResponse.txOpen.tx?.isValid).toBeTruthy(); | ||||||||||||
|
|
||||||||||||
| // Set default color so resource creation succeeds in strict mode | ||||||||||||
| await tx.send( | ||||||||||||
| { oneofKind: "setDefaultColor", setDefaultColor: { colorProof: rootSig } }, | ||||||||||||
| false, | ||||||||||||
| ); | ||||||||||||
|
|
||||||||||||
| const rData = Uint8Array.from([ | ||||||||||||
| (Math.random() * 256) & 0xff, | ||||||||||||
| (Math.random() * 256) & 0xff, | ||||||||||||
|
|
@@ -115,12 +134,13 @@ test("check timeout error type (active)", async () => { | |||||||||||
| type: { name: "TestValue", version: "1" }, | ||||||||||||
| data: rData, | ||||||||||||
| errorIfExists: false, | ||||||||||||
| colorProof: new Uint8Array(0), | ||||||||||||
| }, | ||||||||||||
| }, | ||||||||||||
| false, | ||||||||||||
| ); | ||||||||||||
| const id = (await createResponse).resourceCreateValue.resourceId; | ||||||||||||
| const createResp = (await createResponse).resourceCreateValue; | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: lib/node/pl-client/src/core/ll_transaction.test.ts
Line: 141
Comment:
**Redundant `await` on already-resolved value**
`createResponse` is obtained via `const createResponse = await tx.send(...)` (line 129), so it is already the resolved `ServerMessageResponse` value, not a `Promise`. The inner `await createResponse` on this line re-awaits a non-Promise, which works but is misleading. The same pattern appears in the "check is abort error (active)" test at line 213.
```suggestion
const createResp = createResponse.resourceCreateValue;
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||
| const id = createResp.resourceId; | ||||||||||||
| const resourceSignature = createResp.resourceSignature ?? new Uint8Array(0); | ||||||||||||
|
Comment on lines
+141
to
+143
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||||||||
|
|
||||||||||||
| while (true) { | ||||||||||||
| const vr = await tx.send( | ||||||||||||
|
|
@@ -129,7 +149,7 @@ test("check timeout error type (active)", async () => { | |||||||||||
| resourceGet: { | ||||||||||||
| resourceId: id, | ||||||||||||
| loadFields: false, | ||||||||||||
| resourceSignature: new Uint8Array(0), | ||||||||||||
| resourceSignature, | ||||||||||||
| }, | ||||||||||||
| }, | ||||||||||||
| false, | ||||||||||||
|
|
@@ -144,6 +164,7 @@ test("check timeout error type (active)", async () => { | |||||||||||
|
|
||||||||||||
| test("check is abort error (active)", async () => { | ||||||||||||
| const client = await getTestLLClient(); | ||||||||||||
| const rootSig = await getRootSignature(client); | ||||||||||||
| const tx = client.createTx(true, { abortSignal: AbortSignal.timeout(100) }); | ||||||||||||
|
|
||||||||||||
| try { | ||||||||||||
|
|
@@ -160,6 +181,12 @@ test("check is abort error (active)", async () => { | |||||||||||
| ); | ||||||||||||
| expect(openResponse.txOpen.tx?.isValid).toBeTruthy(); | ||||||||||||
|
|
||||||||||||
| // Set default color so resource creation succeeds in strict mode | ||||||||||||
| await tx.send( | ||||||||||||
| { oneofKind: "setDefaultColor", setDefaultColor: { colorProof: rootSig } }, | ||||||||||||
| false, | ||||||||||||
| ); | ||||||||||||
|
|
||||||||||||
| const rData = Uint8Array.from([ | ||||||||||||
| Math.random() & 0xff, | ||||||||||||
| Math.random() & 0xff, | ||||||||||||
|
|
@@ -179,12 +206,13 @@ test("check is abort error (active)", async () => { | |||||||||||
| type: { name: "TestValue", version: "1" }, | ||||||||||||
| data: rData, | ||||||||||||
| errorIfExists: false, | ||||||||||||
| colorProof: new Uint8Array(0), | ||||||||||||
| }, | ||||||||||||
| }, | ||||||||||||
| false, | ||||||||||||
| ); | ||||||||||||
| const id = (await createResponse).resourceCreateValue.resourceId; | ||||||||||||
| const createResp = (await createResponse).resourceCreateValue; | ||||||||||||
| const id = createResp.resourceId; | ||||||||||||
| const resourceSignature = createResp.resourceSignature ?? new Uint8Array(0); | ||||||||||||
|
Comment on lines
+213
to
+215
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the previous comment, the
Suggested change
|
||||||||||||
|
|
||||||||||||
| while (true) { | ||||||||||||
| const vr = await tx.send( | ||||||||||||
|
|
@@ -193,7 +221,7 @@ test("check is abort error (active)", async () => { | |||||||||||
| resourceGet: { | ||||||||||||
| resourceId: id, | ||||||||||||
| loadFields: false, | ||||||||||||
| resourceSignature: new Uint8Array(0), | ||||||||||||
| resourceSignature, | ||||||||||||
| }, | ||||||||||||
| }, | ||||||||||||
| false, | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /** Cross-transaction cache for resource signatures. | ||
| * Keyed by resource ID (bigint), stores opaque signature bytes (Uint8Array). */ | ||
| export class SignatureCache { | ||
| private readonly store = new Map<bigint, Uint8Array>(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: lib/node/pl-client/src/core/signature_cache.ts
Line: 3-4
Comment:
**Unbounded cache with no eviction policy**
`SignatureCache` uses a plain `Map` keyed by resource ID with no size cap or TTL. In a long-running process where many resources are created or observed, the store will grow without bound. Consider capping it with an `LRUCache` (already a dependency via `lru-cache`) or adding a `maxSize` option, consistent with the existing `resourceDataCache` in `PlClient`.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| set(id: bigint, sig: Uint8Array): void { | ||
| this.store.set(id, sig); | ||
| } | ||
|
|
||
| get(id: bigint): Uint8Array | undefined { | ||
| return this.store.get(id); | ||
| } | ||
|
|
||
| delete(id: bigint): boolean { | ||
| return this.store.delete(id); | ||
| } | ||
|
|
||
| clear(): void { | ||
| this.store.clear(); | ||
| } | ||
|
|
||
| has(id: bigint): boolean { | ||
| return this.store.has(id); | ||
| } | ||
|
|
||
| get size(): number { | ||
| return this.store.size; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear()on auth errors is documented but not implementedThe JSDoc says "Call clear() on auth errors to invalidate stale signatures", but there is no call site in
PlClient(or the auth refresh path) that actually invokesthis._signatureCache.clear(). Callers who rely on this cache through the publicsignatureCachegetter would need to know to call it themselves. Either wire it up automatically in the auth-error handling path, or make the documentation more explicit that this is entirely the caller's responsibility.Prompt To Fix With AI