-
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 23 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,11 @@ | ||
| --- | ||
| "@milaboratories/pl-client": minor | ||
| "@milaboratories/pl-drivers": minor | ||
| "@milaboratories/pl-tree": minor | ||
| --- | ||
|
|
||
| Add resource signature propagation for server-side access control. | ||
|
|
||
| - `pl-client`: cross-transaction `SignatureCache`, automatic signature tracking in `PlTransaction` (store/retrieve signatures for all resource and field operations), `setDefaultColor` for color proof on resource creation, `PermissionDeniedError` error type | ||
| - `pl-drivers`: pass `resourceSignature` through proxied APIs (download, upload, logs, progress, ls), encode signatures in remote blob and log handles | ||
| - `pl-tree`: propagate `resourceSignature` in `ResourceInfo`, `ResourceSnapshot`, and `PlTreeResource` state |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,15 +4,16 @@ import { LLPlClient } from "./ll_client"; | |
| import type { AnyResourceRef } from "./transaction"; | ||
| import { PlTransaction, toGlobalResourceId, TxCommitConflict } from "./transaction"; | ||
| import { createHash } from "node:crypto"; | ||
| import type { OptionalResourceId, ResourceId } from "./types"; | ||
| import type { OptionalResourceId, ResourceId, ResourceSignature } from "./types"; | ||
| import { | ||
| bigintToResourceId, | ||
| ensureResourceIdNotNull, | ||
| isNullResourceId, | ||
| NullResourceId, | ||
| toResourceSignature, | ||
| } from "./types"; | ||
| import { ClientRoot } from "../helpers/pl"; | ||
| import { isUnimplementedError } from "./errors"; | ||
| import { isPermissionDenied, isUnimplementedError } from "./errors"; | ||
| import type { MiLogger, RetryOptions } from "@milaboratories/ts-helpers"; | ||
| import { assertNever, createRetryState, nextRetryStateOrError } from "@milaboratories/ts-helpers"; | ||
| import type { PlDriver, PlDriverDefinition } from "./driver"; | ||
|
|
@@ -32,6 +33,7 @@ import { addStat, initialTxStat } from "./stat"; | |
| import type { WireConnection } from "./wire"; | ||
| import { advisoryLock } from "./advisory_locks"; | ||
| import { plAddressToConfig } from "./config"; | ||
| import { SignatureCache } from "./signature_cache"; | ||
|
|
||
| export type TxOps = PlCallOps & { | ||
| sync?: boolean; | ||
|
|
@@ -96,6 +98,9 @@ export class PlClient { | |
| /** Resource data cache, to minimize redundant data rereading from remote db */ | ||
| private readonly resourceDataCache: LRUCache<ResourceId, ResourceDataCacheRecord>; | ||
|
|
||
| /** Cross-transaction signature cache */ | ||
| private readonly _signatureCache = new SignatureCache(); | ||
|
|
||
| private constructor( | ||
| configOrAddress: PlClientConfig | string, | ||
| auth: AuthOps, | ||
|
|
@@ -209,6 +214,12 @@ export class PlClient { | |
| return this._serverInfo!; | ||
| } | ||
|
|
||
| /** Shared signature cache, persists across transactions. | ||
| * Call clear() on auth errors to invalidate stale signatures. */ | ||
| public get signatureCache(): SignatureCache { | ||
| return this._signatureCache; | ||
| } | ||
|
|
||
| /** Discovers or creates the user's root resource. | ||
| * Tries ListUserResources RPC first (new backend), falls back to | ||
| * legacy named-resource lookup for older backends. */ | ||
|
|
@@ -232,11 +243,13 @@ export class PlClient { | |
|
|
||
| // Try ListUserResources first (new backend, gRPC only) | ||
| let rootFromServer: ResourceId | undefined; | ||
| let rootSignature: ResourceSignature | undefined; | ||
| try { | ||
| const responses = await this._ll.listUserResources({ limit: 1 }); | ||
| for (const msg of responses) { | ||
| if (msg.entry.oneofKind === "userRoot") { | ||
| rootFromServer = bigintToResourceId(msg.entry.userRoot.resourceId); | ||
| rootSignature = toResourceSignature(msg.entry.userRoot.resourceSignature); | ||
| break; | ||
| } | ||
| } | ||
|
|
@@ -246,6 +259,12 @@ export class PlClient { | |
| } | ||
|
|
||
| if (rootFromServer !== undefined) { | ||
| // Store root signature in cross-transaction cache so subsequent | ||
| // transactions can attach it to requests and use it as color proof. | ||
| if (rootSignature && rootSignature.length > 0) { | ||
| this._signatureCache.set(rootFromServer, rootSignature); | ||
| } | ||
|
|
||
| // New path: server created/returned the root | ||
| if (this.conf.alternativeRoot === undefined) { | ||
| this._clientRoot = rootFromServer; | ||
|
|
@@ -341,14 +360,20 @@ export class PlClient { | |
| // opening low-level tx | ||
| const llTx = this.ll.createTx(writable, ops); | ||
| // wrapping it into high-level tx (this also asynchronously sends initialization message) | ||
| const tx = new PlTransaction( | ||
| llTx, | ||
| name, | ||
| writable, | ||
| clientRoot, | ||
| this.finalPredicate, | ||
| this.resourceDataCache, | ||
| ); | ||
| const tx = new PlTransaction(llTx, name, writable, clientRoot, { | ||
| finalPredicate: this.finalPredicate, | ||
| resourceDataCache: this.resourceDataCache, | ||
| signatureCache: this._signatureCache, | ||
| }); | ||
|
|
||
| // Auto-set default color proof so that resource creation (write TXs) | ||
| // and name lookups (read TXs) carry the correct access color. | ||
| if (!isNullResourceId(clientRoot)) { | ||
| const rootSig = this._signatureCache.get(clientRoot); | ||
| if (rootSig) { | ||
| tx.setDefaultColor(rootSig); | ||
| } | ||
| } | ||
|
|
||
| let ok = false; | ||
| let result: T | undefined = undefined; | ||
|
|
@@ -369,6 +394,14 @@ export class PlClient { | |
| } else { | ||
| // collecting stat | ||
| this._txErrorStat = addStat(this._txErrorStat, tx.stat); | ||
| // Invalidate all cached signatures on permission denied. | ||
| // Targeted invalidation is impractical here because the failing | ||
| // resource id is not reliably available from the error. A full | ||
| // clear is safe: signatures are re-populated lazily from server | ||
| // responses in subsequent transactions. | ||
| if (isPermissionDenied(e)) { | ||
| this._signatureCache.clear(); | ||
|
Member
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. This is a significant event, and clearing the cache seems to be not enough and on the contrary creates an irrecoverable state for the whole system. As this is a rare thing we can afford doing something that can interrupt user flow, like log-out form the server, but we must be sure that there is a clear recovery route from this situation. And we must make sure that this "reset" actually propagates everywhere and cleans all preserved signatures.
Member
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. Two potential problems here:
ML stores resource signatures along with resources info.
Current tests in monorepo do not cover this scenario, as switching from project to project having the same client never was a problem, but it seems it becomes now, because global client-level cache of signatures does not sync with trees switched by ML when user does actions in UI. |
||
| } | ||
| throw e; | ||
| } | ||
| } finally { | ||
|
|
||
| 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,44 @@ | ||
| import { LRUCache } from "lru-cache"; | ||
| import type { ResourceId, ResourceSignature, SignedResourceId } from "./types"; | ||
|
|
||
| const DEFAULT_MAX_ENTRIES = 100_000; | ||
|
|
||
| /** Cross-transaction cache for resource signatures. | ||
| * Keyed by ResourceId, stores opaque ResourceSignature bytes. | ||
| * Uses LRU eviction to prevent unbounded memory growth in long-running clients. */ | ||
| export class SignatureCache { | ||
| private readonly store: LRUCache<ResourceId, ResourceSignature>; | ||
|
Member
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.
|
||
|
|
||
| constructor(maxEntries: number = DEFAULT_MAX_ENTRIES) { | ||
| this.store = new LRUCache({ max: maxEntries }); | ||
| } | ||
|
|
||
| set(id: ResourceId, sig: ResourceSignature): void { | ||
| this.store.set(id, sig); | ||
| } | ||
|
|
||
| get(id: ResourceId): ResourceSignature | undefined { | ||
| return this.store.get(id); | ||
| } | ||
|
|
||
| /** Return a SignedResourceId by looking up the cached signature. */ | ||
| sign(id: ResourceId): SignedResourceId { | ||
| return { resourceId: id, resourceSignature: this.get(id) }; | ||
| } | ||
|
|
||
| delete(id: ResourceId): boolean { | ||
| return this.store.delete(id); | ||
| } | ||
|
|
||
| clear(): void { | ||
| this.store.clear(); | ||
| } | ||
|
|
||
| has(id: ResourceId): 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