diff --git a/apps/dev-playground/server/index.ts b/apps/dev-playground/server/index.ts index af05b11f..7868ccce 100644 --- a/apps/dev-playground/server/index.ts +++ b/apps/dev-playground/server/index.ts @@ -32,7 +32,7 @@ createApp({ spaces: { demo: process.env.DATABRICKS_GENIE_SPACE_ID ?? "placeholder" }, }), lakebaseExamples(), - files(), + files({ volumes: { default: { policy: files.policy.allowAll() } } }), serving(), ], ...(process.env.APPKIT_E2E_TEST && { client: createMockClient() }), diff --git a/docs/docs/api/appkit/Class.PolicyDeniedError.md b/docs/docs/api/appkit/Class.PolicyDeniedError.md new file mode 100644 index 00000000..63a46a64 --- /dev/null +++ b/docs/docs/api/appkit/Class.PolicyDeniedError.md @@ -0,0 +1,48 @@ +# Class: PolicyDeniedError + +Thrown when a policy denies an action. + +## Extends + +- `Error` + +## Constructors + +### Constructor + +```ts +new PolicyDeniedError(action: FileAction, volumeKey: string): PolicyDeniedError; +``` + +#### Parameters + +| Parameter | Type | +| ------ | ------ | +| `action` | [`FileAction`](TypeAlias.FileAction.md) | +| `volumeKey` | `string` | + +#### Returns + +`PolicyDeniedError` + +#### Overrides + +```ts +Error.constructor +``` + +## Properties + +### action + +```ts +readonly action: FileAction; +``` + +*** + +### volumeKey + +```ts +readonly volumeKey: string; +``` diff --git a/docs/docs/api/appkit/Interface.FilePolicyUser.md b/docs/docs/api/appkit/Interface.FilePolicyUser.md new file mode 100644 index 00000000..0c4bae2f --- /dev/null +++ b/docs/docs/api/appkit/Interface.FilePolicyUser.md @@ -0,0 +1,21 @@ +# Interface: FilePolicyUser + +Minimal user identity passed to the policy function. + +## Properties + +### id + +```ts +id: string; +``` + +*** + +### isServicePrincipal? + +```ts +optional isServicePrincipal: boolean; +``` + +`true` when the caller is the service principal (direct SDK call, not `asUser`). diff --git a/docs/docs/api/appkit/Interface.FileResource.md b/docs/docs/api/appkit/Interface.FileResource.md new file mode 100644 index 00000000..44aa15cd --- /dev/null +++ b/docs/docs/api/appkit/Interface.FileResource.md @@ -0,0 +1,33 @@ +# Interface: FileResource + +Describes the file or directory being acted upon. + +## Properties + +### path + +```ts +path: string; +``` + +Relative path within the volume. + +*** + +### size? + +```ts +optional size: number; +``` + +Content length in bytes — only present for uploads. + +*** + +### volume + +```ts +volume: string; +``` + +The volume key (e.g. `"uploads"`). diff --git a/docs/docs/api/appkit/TypeAlias.FileAction.md b/docs/docs/api/appkit/TypeAlias.FileAction.md new file mode 100644 index 00000000..3223cc6a --- /dev/null +++ b/docs/docs/api/appkit/TypeAlias.FileAction.md @@ -0,0 +1,17 @@ +# Type Alias: FileAction + +```ts +type FileAction = + | "list" + | "read" + | "download" + | "raw" + | "exists" + | "metadata" + | "preview" + | "upload" + | "mkdir" + | "delete"; +``` + +Every action the files plugin can perform. diff --git a/docs/docs/api/appkit/TypeAlias.FilePolicy.md b/docs/docs/api/appkit/TypeAlias.FilePolicy.md new file mode 100644 index 00000000..1d818560 --- /dev/null +++ b/docs/docs/api/appkit/TypeAlias.FilePolicy.md @@ -0,0 +1,20 @@ +# Type Alias: FilePolicy() + +```ts +type FilePolicy = (action: FileAction, resource: FileResource, user: FilePolicyUser) => boolean | Promise; +``` + +A policy function that decides whether `user` may perform `action` on +`resource`. Return `true` to allow, `false` to deny. + +## Parameters + +| Parameter | Type | +| ------ | ------ | +| `action` | [`FileAction`](TypeAlias.FileAction.md) | +| `resource` | [`FileResource`](Interface.FileResource.md) | +| `user` | [`FilePolicyUser`](Interface.FilePolicyUser.md) | + +## Returns + +`boolean` \| `Promise`\<`boolean`\> diff --git a/docs/docs/api/appkit/Variable.READ_ACTIONS.md b/docs/docs/api/appkit/Variable.READ_ACTIONS.md new file mode 100644 index 00000000..bcbca838 --- /dev/null +++ b/docs/docs/api/appkit/Variable.READ_ACTIONS.md @@ -0,0 +1,7 @@ +# Variable: READ\_ACTIONS + +```ts +const READ_ACTIONS: ReadonlySet; +``` + +Actions that only read data. diff --git a/docs/docs/api/appkit/Variable.WRITE_ACTIONS.md b/docs/docs/api/appkit/Variable.WRITE_ACTIONS.md new file mode 100644 index 00000000..39c90dff --- /dev/null +++ b/docs/docs/api/appkit/Variable.WRITE_ACTIONS.md @@ -0,0 +1,7 @@ +# Variable: WRITE\_ACTIONS + +```ts +const WRITE_ACTIONS: ReadonlySet; +``` + +Actions that mutate data. diff --git a/docs/docs/api/appkit/index.md b/docs/docs/api/appkit/index.md index f5163db4..c0d06a9b 100644 --- a/docs/docs/api/appkit/index.md +++ b/docs/docs/api/appkit/index.md @@ -21,6 +21,7 @@ plugin architecture, and React integration. | [ExecutionError](Class.ExecutionError.md) | Error thrown when an operation execution fails. Use for statement failures, canceled operations, or unexpected states. | | [InitializationError](Class.InitializationError.md) | Error thrown when a service or component is not properly initialized. Use when accessing services before they are ready. | | [Plugin](Class.Plugin.md) | Base abstract class for creating AppKit plugins. | +| [PolicyDeniedError](Class.PolicyDeniedError.md) | Thrown when a policy denies an action. | | [ResourceRegistry](Class.ResourceRegistry.md) | Central registry for tracking plugin resource requirements. Deduplication uses type + resourceKey (machine-stable); alias is for display only. | | [ServerError](Class.ServerError.md) | Error thrown when server lifecycle operations fail. Use for server start/stop issues, configuration conflicts, etc. | | [TunnelError](Class.TunnelError.md) | Error thrown when remote tunnel operations fail. Use for tunnel connection issues, message parsing failures, etc. | @@ -34,6 +35,8 @@ plugin architecture, and React integration. | [CacheConfig](Interface.CacheConfig.md) | Configuration for the CacheInterceptor. Controls TTL, size limits, storage backend, and probabilistic cleanup. | | [DatabaseCredential](Interface.DatabaseCredential.md) | Database credentials with OAuth token for Postgres connection | | [EndpointConfig](Interface.EndpointConfig.md) | - | +| [FilePolicyUser](Interface.FilePolicyUser.md) | Minimal user identity passed to the policy function. | +| [FileResource](Interface.FileResource.md) | Describes the file or directory being acted upon. | | [GenerateDatabaseCredentialRequest](Interface.GenerateDatabaseCredentialRequest.md) | Request parameters for generating database OAuth credentials | | [ITelemetry](Interface.ITelemetry.md) | Plugin-facing interface for OpenTelemetry instrumentation. Provides a thin abstraction over OpenTelemetry APIs for plugins. | | [LakebasePoolConfig](Interface.LakebasePoolConfig.md) | Configuration for creating a Lakebase connection pool | @@ -55,6 +58,8 @@ plugin architecture, and React integration. | ------ | ------ | | [ConfigSchema](TypeAlias.ConfigSchema.md) | Configuration schema definition for plugin config. Re-exported from the standard JSON Schema Draft 7 types. | | [ExecutionResult](TypeAlias.ExecutionResult.md) | Discriminated union for plugin execution results. | +| [FileAction](TypeAlias.FileAction.md) | Every action the files plugin can perform. | +| [FilePolicy](TypeAlias.FilePolicy.md) | A policy function that decides whether `user` may perform `action` on `resource`. Return `true` to allow, `false` to deny. | | [IAppRouter](TypeAlias.IAppRouter.md) | Express router type for plugin route registration | | [PluginData](TypeAlias.PluginData.md) | Tuple of plugin class, config, and name. Created by `toPlugin()` and passed to `createApp()`. | | [ResourcePermission](TypeAlias.ResourcePermission.md) | Union of all possible permission levels across all resource types. | @@ -65,7 +70,9 @@ plugin architecture, and React integration. | Variable | Description | | ------ | ------ | +| [READ\_ACTIONS](Variable.READ_ACTIONS.md) | Actions that only read data. | | [sql](Variable.sql.md) | SQL helper namespace | +| [WRITE\_ACTIONS](Variable.WRITE_ACTIONS.md) | Actions that mutate data. | ## Functions diff --git a/docs/docs/api/appkit/typedoc-sidebar.ts b/docs/docs/api/appkit/typedoc-sidebar.ts index 720c78ea..1df5652a 100644 --- a/docs/docs/api/appkit/typedoc-sidebar.ts +++ b/docs/docs/api/appkit/typedoc-sidebar.ts @@ -56,6 +56,11 @@ const typedocSidebar: SidebarsConfig = { id: "api/appkit/Class.Plugin", label: "Plugin" }, + { + type: "doc", + id: "api/appkit/Class.PolicyDeniedError", + label: "PolicyDeniedError" + }, { type: "doc", id: "api/appkit/Class.ResourceRegistry", @@ -102,6 +107,16 @@ const typedocSidebar: SidebarsConfig = { id: "api/appkit/Interface.EndpointConfig", label: "EndpointConfig" }, + { + type: "doc", + id: "api/appkit/Interface.FilePolicyUser", + label: "FilePolicyUser" + }, + { + type: "doc", + id: "api/appkit/Interface.FileResource", + label: "FileResource" + }, { type: "doc", id: "api/appkit/Interface.GenerateDatabaseCredentialRequest", @@ -188,6 +203,16 @@ const typedocSidebar: SidebarsConfig = { id: "api/appkit/TypeAlias.ExecutionResult", label: "ExecutionResult" }, + { + type: "doc", + id: "api/appkit/TypeAlias.FileAction", + label: "FileAction" + }, + { + type: "doc", + id: "api/appkit/TypeAlias.FilePolicy", + label: "FilePolicy" + }, { type: "doc", id: "api/appkit/TypeAlias.IAppRouter", @@ -219,10 +244,20 @@ const typedocSidebar: SidebarsConfig = { type: "category", label: "Variables", items: [ + { + type: "doc", + id: "api/appkit/Variable.READ_ACTIONS", + label: "READ_ACTIONS" + }, { type: "doc", id: "api/appkit/Variable.sql", label: "sql" + }, + { + type: "doc", + id: "api/appkit/Variable.WRITE_ACTIONS", + label: "WRITE_ACTIONS" } ] }, diff --git a/docs/docs/plugins/execution-context.md b/docs/docs/plugins/execution-context.md index 6c7f8960..68dbb795 100644 --- a/docs/docs/plugins/execution-context.md +++ b/docs/docs/plugins/execution-context.md @@ -41,7 +41,6 @@ Exported from `@databricks/appkit`: - `getWorkspaceClient()`: Returns the appropriate WorkspaceClient for current context - `getWarehouseId()`: `Promise` (from `DATABRICKS_WAREHOUSE_ID` or auto-selected in dev) - `getWorkspaceId()`: `Promise` (from `DATABRICKS_WORKSPACE_ID` or fetched) -- `isInUserContext()`: Returns `true` if currently executing in user context ## Development mode behavior diff --git a/docs/docs/plugins/files.md b/docs/docs/plugins/files.md index ccc91265..c823a581 100644 --- a/docs/docs/plugins/files.md +++ b/docs/docs/plugins/files.md @@ -15,6 +15,7 @@ File operations against Databricks Unity Catalog Volumes. Supports listing, read - Automatic cache invalidation on write operations - Custom content type mappings - Per-user execution context (OBO) +- **Access policies**: Per-volume policy functions that gate read and write operations ## Basic usage @@ -75,6 +76,8 @@ interface IFilesConfig { } interface VolumeConfig { + /** Access policy for this volume. */ + policy?: FilePolicy; /** Maximum upload size in bytes for this volume. Overrides plugin-level default. */ maxUploadSize?: number; /** Map of file extensions to MIME types for this volume. Overrides plugin-level default. */ @@ -97,6 +100,121 @@ files({ }); ``` +### Permission model + +There are three layers of access control in the files plugin. Understanding how they interact is critical for securing your app: + +``` +┌─────────────────────────────────────────────────┐ +│ Unity Catalog grants │ +│ (WRITE_VOLUME on the SP — set at deploy time) │ +├─────────────────────────────────────────────────┤ +│ Execution identity │ +│ HTTP routes → always service principal │ +│ Programmatic → SP by default, asUser() for OBO │ +├─────────────────────────────────────────────────┤ +│ File policies │ +│ Per-volume (action, resource, user) → boolean │ +│ Only app-level gate for HTTP routes │ +└─────────────────────────────────────────────────┘ +``` + +- **UC grants** control what the service principal can do at the Databricks level. These are set at deploy time via `app.yaml` resource bindings. The SP needs `WRITE_VOLUME` — the plugin declares this via resource requirements. +- **Execution identity** determines whose credentials are used for the actual API call. HTTP routes always use the SP. The programmatic API uses SP by default but supports `asUser(req)` for OBO. +- **File policies** are application-level checks evaluated **before** the API call. They receive the requesting user's identity (from the `x-forwarded-user` header) and decide allow/deny. This is the only gate that distinguishes between users on HTTP routes. + +:::warning + +Since HTTP routes always execute as the service principal, removing a user's UC `WRITE_VOLUME` grant has **no effect** on HTTP access — the SP's grant is what's used. Policies are how you restrict what individual users can do through your app. + +::: + +:::info New in v0.21.0 + +File policies are new. Volumes without an explicit policy now default to `publicRead()`, which **denies all write operations** (`upload`, `mkdir`, `delete`). If your app relies on write access, set an explicit policy — for example `files.policy.allowAll()` — on each volume that needs it. + +::: + +#### Access policies + +Attach a policy to a volume to control which actions are allowed: + +```ts +import { files } from "@databricks/appkit"; + +files({ + volumes: { + uploads: { policy: files.policy.publicRead() }, + }, +}); +``` + +#### Actions + +Policies receive an action string. The full list, split by category: + +| Category | Actions | +|----------|---------| +| Read | `list`, `read`, `download`, `raw`, `exists`, `metadata`, `preview` | +| Write | `upload`, `mkdir`, `delete` | + +#### Built-in policies + +| Helper | Allows | Denies | +|--------|--------|--------| +| `files.policy.publicRead()` | all read actions | all write actions | +| `files.policy.allowAll()` | everything | nothing | +| `files.policy.denyAll()` | nothing | everything | + +#### Composing policies + +Combine built-in and custom policies with three combinators: + +- **`files.policy.all(a, b)`** — AND: all policies must allow. Short-circuits on first denial. +- **`files.policy.any(a, b)`** — OR: at least one policy must allow. Short-circuits on first allow. +- **`files.policy.not(p)`** — Inverts a policy. For example, `not(publicRead())` yields a write-only policy (useful for ingestion/drop-box volumes). + +```ts +// Read-only for regular users, full access for the service principal +files({ + volumes: { + shared: { + policy: files.policy.any( + (_action, _resource, user) => !!user.isServicePrincipal, + files.policy.publicRead(), + ), + }, + }, +}); +``` + +#### Custom policies + +`FilePolicy` is a function `(action, resource, user) → boolean | Promise`, so you can inline arbitrary logic: + +```ts +import { type FilePolicy, WRITE_ACTIONS } from "@databricks/appkit"; + +const ADMIN_IDS = ["admin-sp-id", "lead-user-id"]; + +const adminOnly: FilePolicy = (action, _resource, user) => { + if (WRITE_ACTIONS.has(action)) { + return ADMIN_IDS.includes(user.id); + } + return true; // reads allowed for everyone +}; + +files({ + volumes: { reports: { policy: adminOnly } }, +}); +``` + +#### Enforcement + +- **HTTP routes**: Policy checked before every operation. Denied → `403` JSON response with `Policy denied "{action}" on volume "{volumeKey}"`. +- **Programmatic API**: Policy checked on both `appkit.files("vol").list()` (SP identity, `isServicePrincipal: true`) and `appkit.files("vol").asUser(req).list()` (user identity). Denied → throws `PolicyDeniedError`. +- **No policy configured**: Defaults to `files.policy.publicRead()` — read actions are allowed, write actions are denied. A startup warning is logged encouraging you to set an explicit policy. + ### Custom content types Override or extend the built-in extension → MIME map: @@ -115,7 +233,7 @@ Dangerous MIME types (`text/html`, `text/javascript`, `application/javascript`, ## HTTP routes -Routes are mounted at `/api/files/*`. All routes except `/volumes` execute in user context via `asUser(req)`. +Routes are mounted at `/api/files/*`. All routes execute as the service principal. Policy enforcement checks user identity (from the `x-forwarded-user` header) before allowing operations — see [Access policies](#access-policies). | Method | Path | Query / Body | Response | | ------ | -------------------------- | ---------------------------- | ------------------------------------------------- | @@ -236,7 +354,36 @@ interface FilePreview extends FileMetadata { isImage: boolean; } +type FileAction = + | "list" | "read" | "download" | "raw" + | "exists" | "metadata" | "preview" + | "upload" | "mkdir" | "delete"; + +interface FileResource { + /** Relative path within the volume. */ + path: string; + /** The volume key (e.g. `"uploads"`). */ + volume: string; + /** Content length in bytes — only present for uploads. */ + size?: number; +} + +interface FilePolicyUser { + /** User ID from the `x-forwarded-user` header. */ + id: string; + /** `true` when the caller is the service principal (direct SDK call, not `asUser`). */ + isServicePrincipal?: boolean; +} + +type FilePolicy = ( + action: FileAction, + resource: FileResource, + user: FilePolicyUser, +) => boolean | Promise; + interface VolumeConfig { + /** Access policy for this volume. */ + policy?: FilePolicy; /** Maximum upload size in bytes for this volume. */ maxUploadSize?: number; /** Map of file extensions to MIME types for this volume. */ @@ -273,7 +420,7 @@ Built-in extensions: `.png`, `.jpg`, `.jpeg`, `.gif`, `.webp`, `.svg`, `.bmp`, ` ## User context -Routes use `this.asUser(req)` so operations execute with the requesting user's Databricks credentials (on-behalf-of / OBO). The `/volumes` route is the only exception since it only reads plugin config. +HTTP routes always execute as the **service principal** — the SP's Databricks credentials are used for all API calls. User identity is extracted from the `x-forwarded-user` header and passed to the volume's [access policy](#access-policies) for authorization. This means UC grants on the SP (not individual users) determine what operations are possible, while policies control what each user is allowed to do through the app. The programmatic API returns a `VolumeHandle` that exposes all `VolumeAPI` methods directly (service principal) and an `asUser(req)` method for OBO access. Calling any method without `asUser()` logs a warning encouraging OBO usage but does not throw. OBO access is strongly recommended for production use. @@ -297,6 +444,7 @@ All errors return JSON: | Status | Description | | ------ | -------------------------------------------------------------- | | 400 | Missing or invalid `path` parameter | +| 403 | Policy denied "`{action}`" on volume "`{volumeKey}`" | | 404 | Unknown volume key | | 413 | Upload exceeds `maxUploadSize` | | 500 | Operation failed (SDK, network, upstream, or unhandled error) | diff --git a/packages/appkit/src/context/execution-context.ts b/packages/appkit/src/context/execution-context.ts index d707f52d..9588558e 100644 --- a/packages/appkit/src/context/execution-context.ts +++ b/packages/appkit/src/context/execution-context.ts @@ -82,10 +82,7 @@ export function getWorkspaceId(): Promise { return getExecutionContext().workspaceId; } -/** - * Check if currently running in a user context. - */ -export function isInUserContext(): boolean { +function isInUserContext(): boolean { const ctx = executionContextStorage.getStore(); return ctx !== undefined; } diff --git a/packages/appkit/src/context/index.ts b/packages/appkit/src/context/index.ts index 6a5a738b..d306d359 100644 --- a/packages/appkit/src/context/index.ts +++ b/packages/appkit/src/context/index.ts @@ -4,7 +4,6 @@ export { getWarehouseId, getWorkspaceClient, getWorkspaceId, - isInUserContext, runInUserContext, } from "./execution-context"; export { ServiceContext } from "./service-context"; diff --git a/packages/appkit/src/context/user-context.ts b/packages/appkit/src/context/user-context.ts index 20746c91..12548103 100644 --- a/packages/appkit/src/context/user-context.ts +++ b/packages/appkit/src/context/user-context.ts @@ -20,13 +20,13 @@ export interface UserContext { } /** - * Execution context can be either service or user context. - */ -export type ExecutionContext = ServiceContextState | UserContext; - -/** - * Check if an execution context is a user context. + * Type guard to check if a context is a UserContext. */ export function isUserContext(ctx: ExecutionContext): ctx is UserContext { return "isUserContext" in ctx && ctx.isUserContext === true; } + +/** + * Execution context can be either service or user context. + */ +export type ExecutionContext = ServiceContextState | UserContext; diff --git a/packages/appkit/src/index.ts b/packages/appkit/src/index.ts index 955bfde6..77ae4785 100644 --- a/packages/appkit/src/index.ts +++ b/packages/appkit/src/index.ts @@ -54,6 +54,18 @@ export { toPlugin, } from "./plugin"; export { analytics, files, genie, lakebase, server, serving } from "./plugins"; +// Files plugin types (for custom policy authoring) +export type { + FileAction, + FilePolicy, + FilePolicyUser, + FileResource, +} from "./plugins/files"; +export { + PolicyDeniedError, + READ_ACTIONS, + WRITE_ACTIONS, +} from "./plugins/files"; export type { EndpointConfig, ServingEndpointEntry, diff --git a/packages/appkit/src/plugins/files/index.ts b/packages/appkit/src/plugins/files/index.ts index 386c47ef..64a38714 100644 --- a/packages/appkit/src/plugins/files/index.ts +++ b/packages/appkit/src/plugins/files/index.ts @@ -1,3 +1,4 @@ export * from "./defaults"; export * from "./plugin"; +export * from "./policy"; export * from "./types"; diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 9344af85..6ef3b2a3 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -9,7 +9,7 @@ import { isSafeInlineContentType, validateCustomContentTypes, } from "../../connectors/files"; -import { getWorkspaceClient, isInUserContext } from "../../context"; +import { getCurrentUserId, getWorkspaceClient } from "../../context"; import { AuthenticationError } from "../../errors"; import { createLogger } from "../../logging/logger"; import { Plugin, toPlugin } from "../../plugin"; @@ -23,6 +23,13 @@ import { } from "./defaults"; import { parentDirectory, sanitizeFilename } from "./helpers"; import manifest from "./manifest.json"; +import { + type FileAction, + type FilePolicyUser, + type FileResource, + PolicyDeniedError, + policy, +} from "./policy"; import type { DownloadResponse, FilesExport, @@ -93,30 +100,99 @@ export class FilesPlugin extends Plugin { } /** - * Warns when a method is called without a user context (i.e. as service principal). - * OBO access via `asUser(req)` is strongly recommended. + * Extract user identity from the request. + * Falls back to `getCurrentUserId()` in development mode. */ - private warnIfNoUserContext(volumeKey: string, method: string): void { - if (!isInUserContext()) { + private _extractUser(req: express.Request): FilePolicyUser { + const userId = req.header("x-forwarded-user")?.trim(); + if (userId) return { id: userId }; + if (process.env.NODE_ENV === "development") { logger.warn( - `app.files("${volumeKey}").${method}() called without user context (service principal). ` + - `Please use OBO instead: app.files("${volumeKey}").asUser(req).${method}()`, + "No x-forwarded-user header — falling back to service principal identity for policy checks. " + + "Ensure your proxy forwards user headers to test per-user policies.", ); + return { id: getCurrentUserId() }; } + throw AuthenticationError.missingToken( + "Missing x-forwarded-user header. Cannot resolve user ID.", + ); } /** - * Throws when a method is called without a user context (i.e. as service principal). - * OBO access via `asUser(req)` is enforced for now. + * Check the policy for a volume. No-op if no policy is configured. + * Throws `PolicyDeniedError` if denied. */ - private throwIfNoUserContext(volumeKey: string, method: string): void { - if (!isInUserContext()) { - throw new Error( - `app.files("${volumeKey}").${method}() called without user context (service principal). Use OBO instead: app.files("${volumeKey}").asUser(req).${method}()`, + private async _checkPolicy( + volumeKey: string, + action: FileAction, + path: string, + user: FilePolicyUser, + resourceOverrides?: Partial, + ): Promise { + const policyFn = this.volumeConfigs[volumeKey]?.policy; + if (typeof policyFn !== "function") return; + + const resource: FileResource = { + path, + volume: volumeKey, + ...resourceOverrides, + }; + const allowed = await policyFn(action, resource, user); + if (!allowed) { + const userId = user.isServicePrincipal ? "" : user.id; + logger.warn( + 'Policy denied "%s" on volume "%s" for user "%s"', + action, + volumeKey, + userId, ); + throw new PolicyDeniedError(action, volumeKey); } } + /** + * HTTP-level wrapper around `_checkPolicy`. + * Extracts user (401 on failure), runs policy (403 on denial). + * Returns `true` if the request may proceed, `false` if a response was sent. + */ + private async _enforcePolicy( + req: express.Request, + res: express.Response, + volumeKey: string, + action: FileAction, + path: string, + resourceOverrides?: Partial, + ): Promise { + let user: FilePolicyUser; + try { + user = this._extractUser(req); + } catch (error) { + if (error instanceof AuthenticationError) { + res.status(401).json({ error: error.message, plugin: this.name }); + return false; + } + throw error; + } + + try { + await this._checkPolicy(volumeKey, action, path, user, resourceOverrides); + } catch (error) { + if (error instanceof PolicyDeniedError) { + res.status(403).json({ error: error.message, plugin: this.name }); + return false; + } + // A crashing policy is treated as a server error (fail closed). + logger.error("Policy function threw on volume %s: %O", volumeKey, error); + res.status(500).json({ + error: "Policy evaluation failed", + plugin: this.name, + }); + return false; + } + + return true; + } + constructor(config: IFilesConfig) { super(config); this.config = config; @@ -138,6 +214,7 @@ export class FilesPlugin extends Plugin { maxUploadSize: volumeCfg.maxUploadSize ?? config.maxUploadSize, customContentTypes: volumeCfg.customContentTypes ?? config.customContentTypes, + policy: volumeCfg.policy ?? policy.publicRead(), }; this.volumeConfigs[key] = mergedConfig; @@ -148,60 +225,61 @@ export class FilesPlugin extends Plugin { customContentTypes: mergedConfig.customContentTypes, }); } + + // Warn at startup for volumes without an explicit policy + for (const key of this.volumeKeys) { + if (!volumes[key].policy) { + logger.warn( + 'Volume "%s" has no explicit policy — defaulting to publicRead(). ' + + "Set a policy in files({ volumes: { %s: { policy: ... } } }) to silence this warning.", + key, + key, + ); + } + } } /** * Creates a VolumeAPI for a specific volume key. - * Each method warns if called outside a user context (service principal). + * All operations execute as the service principal without policy checks. + * + * Not used internally — `_createPolicyWrappedAPI` handles all current + * call sites. Kept as a `protected` extension point so subclasses can + * override `exports()` or build custom APIs with raw connector access, + * e.g. background jobs or migrations that should bypass user-facing policies. + * + * @security This method skips all policy enforcement. Do not expose its + * return value to HTTP routes or end-user-facing code paths — use + * `_createPolicyWrappedAPI` for anything that serves user requests. */ protected createVolumeAPI(volumeKey: string): VolumeAPI { + logger.debug( + 'createVolumeAPI("%s") — bypassing policy enforcement', + volumeKey, + ); const connector = this.volumeConnectors[volumeKey]; return { - list: (directoryPath?: string) => { - this.throwIfNoUserContext(volumeKey, `list`); - return connector.list(getWorkspaceClient(), directoryPath); - }, - read: (filePath: string, options?: { maxSize?: number }) => { - this.throwIfNoUserContext(volumeKey, `read`); - return connector.read(getWorkspaceClient(), filePath, options); - }, - download: (filePath: string): Promise => { - this.throwIfNoUserContext(volumeKey, `download`); - return connector.download(getWorkspaceClient(), filePath); - }, - exists: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `exists`); - return connector.exists(getWorkspaceClient(), filePath); - }, - metadata: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `metadata`); - return connector.metadata(getWorkspaceClient(), filePath); - }, + list: (directoryPath?: string) => + connector.list(getWorkspaceClient(), directoryPath), + read: (filePath: string, options?: { maxSize?: number }) => + connector.read(getWorkspaceClient(), filePath, options), + download: (filePath: string): Promise => + connector.download(getWorkspaceClient(), filePath), + exists: (filePath: string) => + connector.exists(getWorkspaceClient(), filePath), + metadata: (filePath: string) => + connector.metadata(getWorkspaceClient(), filePath), upload: ( filePath: string, contents: ReadableStream | Buffer | string, options?: { overwrite?: boolean }, - ) => { - this.throwIfNoUserContext(volumeKey, `upload`); - return connector.upload( - getWorkspaceClient(), - filePath, - contents, - options, - ); - }, - createDirectory: (directoryPath: string) => { - this.throwIfNoUserContext(volumeKey, `createDirectory`); - return connector.createDirectory(getWorkspaceClient(), directoryPath); - }, - delete: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `delete`); - return connector.delete(getWorkspaceClient(), filePath); - }, - preview: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `preview`); - return connector.preview(getWorkspaceClient(), filePath); - }, + ) => connector.upload(getWorkspaceClient(), filePath, contents, options), + createDirectory: (directoryPath: string) => + connector.createDirectory(getWorkspaceClient(), directoryPath), + delete: (filePath: string) => + connector.delete(getWorkspaceClient(), filePath), + preview: (filePath: string) => + connector.preview(getWorkspaceClient(), filePath), }; } @@ -381,7 +459,6 @@ export class FilesPlugin extends Plugin { private _invalidateListCache( volumeKey: string, parentPath: string, - userId: string, connector: FilesConnector, ): void { const parent = parentDirectory(parentPath); @@ -390,7 +467,7 @@ export class FilesPlugin extends Plugin { : "__root__"; const listKey = this.cache.generateKey( [`files:${volumeKey}:list`, cachePathSegment], - userId, + getCurrentUserId(), ); this.cache.delete(listKey); } @@ -400,6 +477,13 @@ export class FilesPlugin extends Plugin { error: unknown, fallbackMessage: string, ): void { + if (error instanceof PolicyDeniedError) { + res.status(403).json({ + error: error.message, + plugin: this.name, + }); + return; + } if (error instanceof AuthenticationError) { res.status(401).json({ error: error.message, @@ -440,13 +524,12 @@ export class FilesPlugin extends Plugin { ): Promise { const path = req.query.path as string | undefined; + if (!(await this._enforcePolicy(req, res, volumeKey, "list", path ?? "/"))) + return; + try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `list`); - return connector.list(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.list(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:list`, path ? connector.resolvePath(path) : "__root__", @@ -470,19 +553,18 @@ export class FilesPlugin extends Plugin { volumeKey: string, ): Promise { const path = req.query.path as string; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "read", path))) return; + try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `read`); - return connector.read(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.read(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:read`, connector.resolvePath(path), @@ -534,24 +616,27 @@ export class FilesPlugin extends Plugin { opts: { mode: "download" | "raw" }, ): Promise { const path = req.query.path as string; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, opts.mode, path))) + return; + const label = opts.mode === "download" ? "Download" : "Raw fetch"; const volumeCfg = this.volumeConfigs[volumeKey]; try { - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_DOWNLOAD_DEFAULTS, }; - const response = await userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `download`); - return connector.download(getWorkspaceClient(), path); - }, settings); + const response = await this.execute( + async () => connector.download(getWorkspaceClient(), path), + settings, + ); if (!response.ok) { this._sendStatusError(res, response.status); @@ -611,19 +696,19 @@ export class FilesPlugin extends Plugin { volumeKey: string, ): Promise { const path = req.query.path as string; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "exists", path))) + return; + try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `exists`); - return connector.exists(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.exists(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:exists`, connector.resolvePath(path), @@ -647,19 +732,19 @@ export class FilesPlugin extends Plugin { volumeKey: string, ): Promise { const path = req.query.path as string; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "metadata", path))) + return; + try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `metadata`); - return connector.metadata(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.metadata(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:metadata`, connector.resolvePath(path), @@ -683,19 +768,19 @@ export class FilesPlugin extends Plugin { volumeKey: string, ): Promise { const path = req.query.path as string; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "preview", path))) + return; + try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `preview`); - return connector.preview(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.preview(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:preview`, connector.resolvePath(path), @@ -732,6 +817,13 @@ export class FilesPlugin extends Plugin { ? parseInt(rawContentLength, 10) : undefined; + if ( + !(await this._enforcePolicy(req, res, volumeKey, "upload", path, { + size: contentLength, + })) + ) + return; + if ( contentLength !== undefined && !Number.isNaN(contentLength) && @@ -774,24 +866,17 @@ export class FilesPlugin extends Plugin { path, contentLength ?? 0, ); - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS, }; const result = await this.trackWrite(() => - userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `upload`); + this.execute(async () => { await connector.upload(getWorkspaceClient(), path, webStream); return { success: true as const }; }, settings), ); - this._invalidateListCache( - volumeKey, - path, - this.resolveUserId(req), - connector, - ); + this._invalidateListCache(volumeKey, path, connector); if (!result.ok) { logger.error( @@ -827,31 +912,28 @@ export class FilesPlugin extends Plugin { ): Promise { const dirPath = typeof req.body?.path === "string" ? req.body.path : undefined; + const valid = this._isValidPath(dirPath); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "mkdir", dirPath))) + return; + try { - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS, }; const result = await this.trackWrite(() => - userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `createDirectory`); + this.execute(async () => { await connector.createDirectory(getWorkspaceClient(), dirPath); return { success: true as const }; }, settings), ); - this._invalidateListCache( - volumeKey, - dirPath, - this.resolveUserId(req), - connector, - ); + this._invalidateListCache(volumeKey, dirPath, connector); if (!result.ok) { this._sendStatusError(res, result.status); @@ -871,6 +953,7 @@ export class FilesPlugin extends Plugin { volumeKey: string, ): Promise { const rawPath = req.query.path as string | undefined; + const valid = this._isValidPath(rawPath); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -878,25 +961,21 @@ export class FilesPlugin extends Plugin { } const path = rawPath as string; + if (!(await this._enforcePolicy(req, res, volumeKey, "delete", path))) + return; + try { - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS, }; const result = await this.trackWrite(() => - userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `delete`); + this.execute(async () => { await connector.delete(getWorkspaceClient(), path); return { success: true as const }; }, settings), ); - this._invalidateListCache( - volumeKey, - path, - this.resolveUserId(req), - connector, - ); + this._invalidateListCache(volumeKey, path, connector); if (!result.ok) { this._sendStatusError(res, result.status); @@ -909,6 +988,70 @@ export class FilesPlugin extends Plugin { } } + /** + * Creates a VolumeAPI that enforces the volume's policy and then delegates + * to the connector as the service principal. + */ + private _createPolicyWrappedAPI( + volumeKey: string, + user: FilePolicyUser, + ): VolumeAPI { + const connector = this.volumeConnectors[volumeKey]; + const check = ( + action: FileAction, + path: string, + overrides?: Partial, + ) => this._checkPolicy(volumeKey, action, path, user, overrides); + + return { + list: async (directoryPath?: string) => { + await check("list", directoryPath ?? "/"); + return connector.list(getWorkspaceClient(), directoryPath); + }, + read: async (filePath: string, options?: { maxSize?: number }) => { + await check("read", filePath); + return connector.read(getWorkspaceClient(), filePath, options); + }, + download: async (filePath: string) => { + await check("download", filePath); + return connector.download(getWorkspaceClient(), filePath); + }, + exists: async (filePath: string) => { + await check("exists", filePath); + return connector.exists(getWorkspaceClient(), filePath); + }, + metadata: async (filePath: string) => { + await check("metadata", filePath); + return connector.metadata(getWorkspaceClient(), filePath); + }, + upload: async ( + filePath: string, + contents: ReadableStream | Buffer | string, + options?: { overwrite?: boolean }, + ) => { + await check("upload", filePath); + return connector.upload( + getWorkspaceClient(), + filePath, + contents, + options, + ); + }, + createDirectory: async (directoryPath: string) => { + await check("mkdir", directoryPath); + return connector.createDirectory(getWorkspaceClient(), directoryPath); + }, + delete: async (filePath: string) => { + await check("delete", filePath); + return connector.delete(getWorkspaceClient(), filePath); + }, + preview: async (filePath: string) => { + await check("preview", filePath); + return connector.preview(getWorkspaceClient(), filePath); + }, + }; + } + private inflightWrites = 0; private trackWrite(fn: () => Promise): Promise { @@ -941,13 +1084,16 @@ export class FilesPlugin extends Plugin { * Returns the programmatic API for the Files plugin. * Callable with a volume key to get a volume-scoped handle. * + * All operations execute as the service principal. + * Use policies to control per-user access. + * * @example * ```ts - * // OBO access (recommended) - * appKit.files("uploads").asUser(req).list() - * - * // Service principal access (logs a warning) + * // Service principal access * appKit.files("uploads").list() + * + * // With policy: pass user identity for access control + * appKit.files("uploads").asUser(req).list() * ``` */ exports(): FilesExport { @@ -958,14 +1104,21 @@ export class FilesPlugin extends Plugin { ); } - // Service principal API — each method logs a warning recommending OBO - const spApi = this.createVolumeAPI(volumeKey); + // Lazy user resolution: getCurrentUserId() is called when a method + // is invoked (policy check), not when exports() is called. + const spUser: FilePolicyUser = { + get id() { + return getCurrentUserId(); + }, + isServicePrincipal: true, + }; + const spApi = this._createPolicyWrappedAPI(volumeKey, spUser); return { ...spApi, - asUser: (req: import("express").Request) => { - const userPlugin = this.asUser(req) as FilesPlugin; - return userPlugin.createVolumeAPI(volumeKey); + asUser: (req: express.Request) => { + const user = this._extractUser(req); + return this._createPolicyWrappedAPI(volumeKey, user); }, }; }; @@ -985,4 +1138,4 @@ export class FilesPlugin extends Plugin { /** * @internal */ -export const files = toPlugin(FilesPlugin); +export const files = Object.assign(toPlugin(FilesPlugin), { policy }); diff --git a/packages/appkit/src/plugins/files/policy.ts b/packages/appkit/src/plugins/files/policy.ts new file mode 100644 index 00000000..87b23f37 --- /dev/null +++ b/packages/appkit/src/plugins/files/policy.ts @@ -0,0 +1,153 @@ +/** + * Per-volume file access policies. + * + * A `FilePolicy` is a function that decides whether a given action on a + * resource is allowed for a specific user. When a policy is attached to a + * volume, the policy controls whether the action is allowed for the requesting user. + */ + +// --------------------------------------------------------------------------- +// Actions +// --------------------------------------------------------------------------- + +/** Every action the files plugin can perform. */ +export type FileAction = + | "list" + | "read" + | "download" + | "raw" + | "exists" + | "metadata" + | "preview" + | "upload" + | "mkdir" + | "delete"; + +/** Actions that only read data. */ +export const READ_ACTIONS: ReadonlySet = new Set([ + "list", + "read", + "download", + "raw", + "exists", + "metadata", + "preview", +]); + +/** Actions that mutate data. */ +export const WRITE_ACTIONS: ReadonlySet = new Set([ + "upload", + "mkdir", + "delete", +]); + +// --------------------------------------------------------------------------- +// Resource & User +// --------------------------------------------------------------------------- + +/** Describes the file or directory being acted upon. */ +export interface FileResource { + /** Relative path within the volume. */ + path: string; + /** The volume key (e.g. `"uploads"`). */ + volume: string; + /** Content length in bytes — only present for uploads. */ + size?: number; +} + +/** Minimal user identity passed to the policy function. */ +export interface FilePolicyUser { + id: string; + /** `true` when the caller is the service principal (direct SDK call, not `asUser`). */ + isServicePrincipal?: boolean; +} + +// --------------------------------------------------------------------------- +// Policy function type +// --------------------------------------------------------------------------- + +/** + * A policy function that decides whether `user` may perform `action` on + * `resource`. Return `true` to allow, `false` to deny. + */ +export type FilePolicy = ( + action: FileAction, + resource: FileResource, + user: FilePolicyUser, +) => boolean | Promise; + +// --------------------------------------------------------------------------- +// PolicyDeniedError +// --------------------------------------------------------------------------- + +/** + * Thrown when a policy denies an action. + */ +export class PolicyDeniedError extends Error { + readonly action: FileAction; + readonly volumeKey: string; + + constructor(action: FileAction, volumeKey: string) { + super(`Policy denied "${action}" on volume "${volumeKey}"`); + this.name = "PolicyDeniedError"; + this.action = action; + this.volumeKey = volumeKey; + } +} + +// --------------------------------------------------------------------------- +// Combinators +// --------------------------------------------------------------------------- + +/** Utility namespace with common policy combinators. */ +export const policy = { + /** + * AND — all policies must allow. Short-circuits on first denial. + */ + all(...policies: FilePolicy[]): FilePolicy { + if (policies.length === 0) { + throw new Error("policy.all() requires at least one policy"); + } + return async (action, resource, user) => { + for (const p of policies) { + if (!(await p(action, resource, user))) return false; + } + return true; + }; + }, + + /** + * OR — at least one policy must allow. Short-circuits on first allow. + */ + any(...policies: FilePolicy[]): FilePolicy { + if (policies.length === 0) { + throw new Error("policy.any() requires at least one policy"); + } + return async (action, resource, user) => { + for (const p of policies) { + if (await p(action, resource, user)) return true; + } + return false; + }; + }, + + /** Negates a policy. */ + not(p: FilePolicy): FilePolicy { + return async (action, resource, user) => !(await p(action, resource, user)); + }, + + /** Allow all read actions (list, read, download, raw, exists, metadata, preview). */ + publicRead(): FilePolicy { + return (action) => READ_ACTIONS.has(action); + }, + + /** Deny every action. */ + denyAll(): FilePolicy { + return () => false; + }, + + /** Allow every action. */ + allowAll(): FilePolicy { + return () => true; + }, +} as const; diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index 55989148..5c97ce96 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -438,17 +438,19 @@ describe("Files Plugin Integration", () => { }); }); - describe("OBO Gateway", () => { - test("production: rejects requests without user token with 401", async () => { - const response = await fetch(`${baseUrl}/api/files/${VOL}/list`); + describe("Service principal execution", () => { + test("requests without user token return 401 (policy requires user identity)", async () => { + // Use a unique path to avoid cached results from earlier tests + const response = await fetch( + `${baseUrl}/api/files/${VOL}/list?path=sp-only`, + ); expect(response.status).toBe(401); - const data = (await response.json()) as { error: string; plugin: string }; - expect(data.plugin).toBe("files"); - expect(data.error).toMatch(/token/i); + const data = (await response.json()) as { error: string }; + expect(data.error).toMatch(/x-forwarded-user/); }); - test("production: allows requests with valid user token (OBO)", async () => { + test("requests with user headers also succeed", async () => { mockFilesApi.listDirectoryContents.mockReturnValue( (async function* () { yield { @@ -466,51 +468,21 @@ describe("Files Plugin Integration", () => { expect(response.status).toBe(200); }); - test("development: falls back to service principal when no user token", async () => { - const originalEnv = process.env.NODE_ENV; - process.env.NODE_ENV = "development"; - - try { - mockFilesApi.listDirectoryContents.mockReturnValue( - (async function* () { - yield { - name: "dev-file.txt", - path: "/Volumes/catalog/schema/vol/dev-file.txt", - is_directory: false, - }; - })(), - ); - - const response = await fetch(`${baseUrl}/api/files/${VOL}/list`); - - expect(response.status).toBe(200); - const data = await response.json(); - expect(data).toEqual([ - { - name: "dev-file.txt", - path: "/Volumes/catalog/schema/vol/dev-file.txt", - is_directory: false, - }, - ]); - } finally { - if (originalEnv === undefined) { - delete process.env.NODE_ENV; - } else { - process.env.NODE_ENV = originalEnv; - } - } - }); + test("write operations without explicit policy are denied by default publicRead()", async () => { + mockFilesApi.createDirectory.mockResolvedValue(undefined); - test("production: rejects write operations without user token", async () => { const response = await fetch(`${baseUrl}/api/files/${VOL}/mkdir`, { method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ path: "/Volumes/catalog/schema/vol/newdir" }), + headers: { + ...MOCK_AUTH_HEADERS, + "Content-Type": "application/json", + }, + body: JSON.stringify({ path: "newdir" }), }); - expect(response.status).toBe(401); - const data = (await response.json()) as { error: string; plugin: string }; - expect(data.plugin).toBe("files"); + expect(response.status).toBe(403); + const data = (await response.json()) as { error: string }; + expect(data.error).toMatch(/Policy denied/); }); }); @@ -538,7 +510,9 @@ describe("Files Plugin Integration", () => { }); } - test(`POST /api/files/${VOL}/upload with content-length over limit returns 413`, async () => { + test(`POST /api/files/${VOL}/upload with content-length over limit returns 403 (policy checked before size)`, async () => { + // Default publicRead() policy denies uploads. Policy enforcement runs + // before the content-length size check, so the response is 403 (not 413). const res = await rawPost( `/api/files/${VOL}/upload?path=/Volumes/catalog/schema/vol/large.bin`, { @@ -547,10 +521,10 @@ describe("Files Plugin Integration", () => { }, ); - expect(res.status).toBe(413); + expect(res.status).toBe(403); const data = JSON.parse(res.body) as { error: string; plugin: string }; expect(data.plugin).toBe("files"); - expect(data.error).toMatch(/exceeds maximum allowed size/); + expect(data.error).toMatch(/Policy denied/); }); }); @@ -611,11 +585,10 @@ describe("Files Plugin Integration", () => { new MockApiError("Conflict", 409), ); - const response = await fetch(`${baseUrl}/api/files/${VOL}/mkdir`, { - method: "POST", - headers: { ...MOCK_AUTH_HEADERS, "Content-Type": "application/json" }, - body: JSON.stringify({ path: "/Volumes/catalog/schema/vol/existing" }), - }); + const response = await fetch( + `${baseUrl}/api/files/${VOL}/metadata?path=/Volumes/catalog/schema/vol/existing`, + { headers: MOCK_AUTH_HEADERS }, + ); expect(response.status).toBe(409); const data = (await response.json()) as { diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 99e08b8c..3edd7a65 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -9,6 +9,7 @@ import { FILES_WRITE_DEFAULTS, } from "../defaults"; import { FilesPlugin, files } from "../plugin"; +import { PolicyDeniedError, policy } from "../policy"; const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { const mockFilesApi = { @@ -60,7 +61,7 @@ vi.mock("../../../context", async (importOriginal) => { return { ...actual, getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), + getCurrentUserId: vi.fn(() => "test-service-principal"), }; }); @@ -72,8 +73,8 @@ vi.mock("../../../cache", () => ({ const VOLUMES_CONFIG = { volumes: { - uploads: { maxUploadSize: 100_000_000 }, - exports: {}, + uploads: { maxUploadSize: 100_000_000, policy: policy.allowAll() }, + exports: { policy: policy.allowAll() }, }, }; @@ -247,7 +248,7 @@ describe("FilesPlugin", () => { }); }); - describe("OBO and service principal access", () => { + describe("Service principal access", () => { const volumeMethods = [ "list", "read", @@ -270,7 +271,7 @@ describe("FilesPlugin", () => { } }); - test("asUser throws AuthenticationError without token in production", () => { + test("asUser without user header in production → throws AuthenticationError", () => { const originalEnv = process.env.NODE_ENV; process.env.NODE_ENV = "production"; @@ -292,7 +293,10 @@ describe("FilesPlugin", () => { try { const plugin = new FilesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); - const mockReq = { header: () => undefined } as any; + const mockReq = { + header: (name: string) => + name === "x-forwarded-user" ? "test-user" : undefined, + } as any; const api = handle.asUser(mockReq); for (const method of volumeMethods) { @@ -303,17 +307,12 @@ describe("FilesPlugin", () => { } }); - test("direct methods on handle throw without user context (OBO enforced)", async () => { - const { isInUserContext } = await import("../../../context"); - (isInUserContext as ReturnType).mockReturnValue(false); - + test("direct methods on handle work as service principal", () => { const plugin = new FilesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); - // Direct call without user context should throw synchronously - expect(() => handle.list()).toThrow( - 'app.files("uploads").list() called without user context (service principal). Use OBO instead: app.files("uploads").asUser(req).list()', - ); + // Direct call executes as service principal (returns a promise, does not throw) + expect(typeof handle.list).toBe("function"); }); }); @@ -446,11 +445,16 @@ describe("FilesPlugin", () => { const res = mockRes(); // uploads has maxUploadSize: 100_000_000 + const headers: Record = { + "content-length": String(200_000_000), + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "uploads" }, query: { path: "/large.bin" }, - headers: { "content-length": String(200_000_000) }, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -470,11 +474,16 @@ describe("FilesPlugin", () => { const res = mockRes(); // exports has no maxUploadSize, uses default 5GB + const headers: Record = { + "content-length": String(6 * 1024 * 1024 * 1024), + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "exports" }, query: { path: "/large.bin" }, - headers: { "content-length": String(6 * 1024 * 1024 * 1024) }, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -493,11 +502,16 @@ describe("FilesPlugin", () => { const handler = getUploadHandler(plugin); const res = mockRes(); + const headers: Record = { + "content-length": String(100_000_000), + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "uploads" }, query: { path: "/file.bin" }, - headers: { "content-length": String(100_000_000) }, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -515,11 +529,15 @@ describe("FilesPlugin", () => { const handler = getUploadHandler(plugin); const res = mockRes(); + const headers: Record = { + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "uploads" }, query: { path: "/file.bin" }, - headers: {}, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -644,6 +662,8 @@ describe("FilesPlugin", () => { const handlerPromise = handler(mockReq("uploads"), res); + // Flush microtasks (policy check) before advancing timers + await vi.advanceTimersByTimeAsync(0); await vi.advanceTimersByTimeAsync(100); await handlerPromise; @@ -869,6 +889,8 @@ describe("FilesPlugin", () => { const handlerPromise = handler(mockReq("uploads"), res); + // Flush microtasks (policy check) before advancing timers + await vi.advanceTimersByTimeAsync(0); // Advance past read-tier timeout (30s) await vi.advanceTimersByTimeAsync(31_000); await handlerPromise; @@ -893,6 +915,640 @@ describe("FilesPlugin", () => { }); }); + describe("Policy enforcement", () => { + const POLICY_CONFIG = { + volumes: { + public: { policy: policy.publicRead() }, + locked: { policy: policy.denyAll() }, + open: { policy: policy.allowAll() }, + writeonly: { policy: policy.not(policy.publicRead()) }, + uploads: {}, + exports: {}, + }, + }; + + function getRouteHandler( + plugin: FilesPlugin, + method: "get" | "post" | "delete", + pathSuffix: string, + ) { + const mockRouter = { + use: vi.fn(), + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + } as any; + plugin.injectRoutes(mockRouter); + const call = mockRouter[method].mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && (c[0] as string).endsWith(pathSuffix), + ); + return call[call.length - 1] as (req: any, res: any) => Promise; + } + + function mockRes() { + const res: any = { headersSent: false }; + res.status = vi.fn().mockReturnValue(res); + res.json = vi.fn().mockReturnValue(res); + res.type = vi.fn().mockReturnValue(res); + res.send = vi.fn().mockReturnValue(res); + res.setHeader = vi.fn().mockReturnValue(res); + res.end = vi.fn(); + return res; + } + + function mockReq(volumeKey: string, overrides: Record = {}) { + const headers: Record = { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + ...(overrides.headers ?? {}), + }; + return { + params: { volumeKey }, + query: {}, + ...overrides, + headers, + header: (name: string) => headers[name.toLowerCase()], + }; + } + + beforeEach(() => { + process.env.DATABRICKS_VOLUME_PUBLIC = "/Volumes/c/s/public"; + process.env.DATABRICKS_VOLUME_LOCKED = "/Volumes/c/s/locked"; + process.env.DATABRICKS_VOLUME_OPEN = "/Volumes/c/s/open"; + process.env.DATABRICKS_VOLUME_WRITEONLY = "/Volumes/c/s/writeonly"; + }); + + afterEach(() => { + delete process.env.DATABRICKS_VOLUME_PUBLIC; + delete process.env.DATABRICKS_VOLUME_LOCKED; + delete process.env.DATABRICKS_VOLUME_OPEN; + delete process.env.DATABRICKS_VOLUME_WRITEONLY; + }); + + test("policy volume + no user header (production) → 401", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + try { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + // Override both headers to undefined so _extractUser has no user + const noUserHeaders: Record = {}; + await handler( + { + params: { volumeKey: "public" }, + query: {}, + headers: noUserHeaders, + header: (name: string) => noUserHeaders[name.toLowerCase()], + }, + res, + ); + + expect(res.status).toHaveBeenCalledWith(401); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); + + test("policy volume + policy returns false → 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("locked"), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("policy volume + policy returns true → 200, runs as SP", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "a.txt", path: "/a.txt", is_directory: false }; + }, + ); + + await handler(mockReq("public", { query: {} }), res); + + expect(res.json).toHaveBeenCalledWith( + expect.arrayContaining([expect.objectContaining({ name: "a.txt" })]), + ); + // Should NOT have received a 401 or 403 + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(401); + expect(statusCodes).not.toContain(403); + }); + + test("policy volume + async policy → works", async () => { + const asyncConfig = { + volumes: { + async_vol: { + policy: async () => true, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_ASYNC_VOL = "/Volumes/c/s/async"; + + try { + const plugin = new FilesPlugin(asyncConfig); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "b.txt", path: "/b.txt", is_directory: false }; + }, + ); + + await handler(mockReq("async_vol"), res); + + expect(res.json).toHaveBeenCalledWith( + expect.arrayContaining([expect.objectContaining({ name: "b.txt" })]), + ); + } finally { + delete process.env.DATABRICKS_VOLUME_ASYNC_VOL; + } + }); + + test("default publicRead() volume → reads succeed", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "c.txt", path: "/c.txt", is_directory: false }; + }, + ); + + await handler(mockReq("uploads"), res); + + // Should succeed (reads allowed by publicRead default) + expect(res.json).toHaveBeenCalledWith( + expect.arrayContaining([expect.objectContaining({ name: "c.txt" })]), + ); + }); + + test("default publicRead() volume → writes denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "post", "/mkdir"); + const res = mockRes(); + + await handler( + mockReq("uploads", { + body: { path: "/newdir" }, + }), + res, + ); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("upload with policy → policy receives size in resource", async () => { + const policySpy = vi.fn().mockReturnValue(true); + const sizeConfig = { + volumes: { + sized: { policy: policySpy }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_SIZED = "/Volumes/c/s/sized"; + + try { + const plugin = new FilesPlugin(sizeConfig); + const handler = getRouteHandler(plugin, "post", "/upload"); + const res = mockRes(); + + await handler( + mockReq("sized", { + query: { path: "/test.bin" }, + headers: { + "content-length": "12345", + "x-forwarded-user": "test-user", + "x-forwarded-access-token": "test-token", + }, + }), + res, + ); + + expect(policySpy).toHaveBeenCalledWith( + "upload", + expect.objectContaining({ size: 12345 }), + expect.objectContaining({ id: "test-user" }), + ); + } finally { + delete process.env.DATABRICKS_VOLUME_SIZED; + } + }); + + test("SDK asUser(req) on policy volume → policy-wrapped API works", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const exported = plugin.exports(); + const handle = exported("public"); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "d.txt", path: "/d.txt", is_directory: false }; + }, + ); + + const mockReqObj = { + header: (name: string) => { + if (name === "x-forwarded-user") return "test-user"; + if (name === "x-forwarded-access-token") return "test-token"; + return undefined; + }, + } as any; + + const api = handle.asUser(mockReqObj); + const result = await api.list(); + expect(result).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "d.txt" })]), + ); + }); + + test("SDK asUser(req) on policy volume + deny → throws PolicyDeniedError", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const exported = plugin.exports(); + const handle = exported("locked"); + + const mockReqObj = { + header: (name: string) => { + if (name === "x-forwarded-user") return "test-user"; + if (name === "x-forwarded-access-token") return "test-token"; + return undefined; + }, + } as any; + + const api = handle.asUser(mockReqObj); + await expect(api.list()).rejects.toThrow(PolicyDeniedError); + }); + + test("direct call on policy volume → enforces policy as SP", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handle = plugin.exports()("open"); + + // Direct call on allowAll() volume succeeds (policy is checked but allows) + const result = await handle.list(); + expect(result).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "d.txt" })]), + ); + }); + + test("direct SP call on denyAll() volume → throws PolicyDeniedError", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handle = plugin.exports()("locked"); + + await expect(handle.list()).rejects.toThrow(PolicyDeniedError); + }); + + test("direct SP call → policy receives { isServicePrincipal: true }", async () => { + const policySpy = vi.fn().mockReturnValue(true); + const spyConfig = { + volumes: { + spied: { policy: policySpy }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_SPIED = "/Volumes/c/s/spied"; + + try { + const plugin = new FilesPlugin(spyConfig); + const handle = plugin.exports()("spied"); + await handle.list(); + + expect(policySpy).toHaveBeenCalledWith( + "list", + expect.objectContaining({ volume: "spied" }), + expect.objectContaining({ isServicePrincipal: true }), + ); + } finally { + delete process.env.DATABRICKS_VOLUME_SPIED; + } + }); + + test("asUser() call → policy receives user without isServicePrincipal", async () => { + const policySpy = vi.fn().mockReturnValue(true); + const spyConfig = { + volumes: { + spied: { policy: policySpy }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_SPIED = "/Volumes/c/s/spied"; + + try { + const plugin = new FilesPlugin(spyConfig); + const handle = plugin.exports()("spied"); + const mockReqObj = { + header: (name: string) => { + if (name === "x-forwarded-user") return "test-user"; + if (name === "x-forwarded-access-token") return "test-token"; + return undefined; + }, + } as any; + + await handle.asUser(mockReqObj).list(); + + expect(policySpy).toHaveBeenCalledWith( + "list", + expect.objectContaining({ volume: "spied" }), + expect.objectContaining({ id: "test-user" }), + ); + // Should NOT have isServicePrincipal set + const userArg = policySpy.mock.calls[0][2]; + expect(userArg.isServicePrincipal).toBeUndefined(); + } finally { + delete process.env.DATABRICKS_VOLUME_SPIED; + } + }); + + test("denyAll() volume → read denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/read"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("publicRead() volume → read allowed", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/read"); + const res = mockRes(); + + mockClient.files.download.mockResolvedValue({ + contents: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("file content")); + controller.close(); + }, + }), + }); + + await handler(mockReq("public", { query: { path: "/test.txt" } }), res); + + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(401); + expect(statusCodes).not.toContain(403); + }); + + test("denyAll() volume → download denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/download"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.bin" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → raw denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/raw"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → exists denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/exists"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → metadata denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/metadata"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → preview denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/preview"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → delete denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "delete", "/:volumeKey"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("not(publicRead()) volume → read denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("writeonly"), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("not(publicRead()) volume → write allowed", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "post", "/mkdir"); + const res = mockRes(); + + mockClient.files.createDirectory.mockResolvedValue(undefined); + + await handler(mockReq("writeonly", { body: { path: "/dropbox" } }), res); + + // Should not receive 403 + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(403); + }); + + test("policy that throws → HTTP route returns 500 (fail closed)", async () => { + const brokenConfig = { + volumes: { + broken: { + policy: () => { + throw new Error("policy crashed"); + }, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; + + try { + const plugin = new FilesPlugin(brokenConfig); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("broken"), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Policy evaluation failed", + plugin: "files", + }), + ); + // Must NOT be 403 — a broken policy is not a denial, it's a server error + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(403); + } finally { + delete process.env.DATABRICKS_VOLUME_BROKEN; + } + }); + + test("policy that throws → programmatic API propagates the error", async () => { + const brokenConfig = { + volumes: { + broken: { + policy: () => { + throw new Error("policy crashed"); + }, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; + + try { + const plugin = new FilesPlugin(brokenConfig); + const handle = plugin.exports()("broken"); + + await expect(handle.list()).rejects.toThrow("policy crashed"); + await expect(handle.list()).rejects.not.toBeInstanceOf( + PolicyDeniedError, + ); + } finally { + delete process.env.DATABRICKS_VOLUME_BROKEN; + } + }); + + test("async policy that rejects → HTTP route returns 500 (fail closed)", async () => { + const brokenConfig = { + volumes: { + broken: { + policy: async () => { + throw new Error("async policy crashed"); + }, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; + + try { + const plugin = new FilesPlugin(brokenConfig); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("broken"), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Policy evaluation failed", + plugin: "files", + }), + ); + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(403); + } finally { + delete process.env.DATABRICKS_VOLUME_BROKEN; + } + }); + }); + describe("Upload Stream Size Limiter", () => { test("stream under limit passes through all chunks", async () => { const maxSize = 100; diff --git a/packages/appkit/src/plugins/files/tests/policy.test.ts b/packages/appkit/src/plugins/files/tests/policy.test.ts new file mode 100644 index 00000000..dc477198 --- /dev/null +++ b/packages/appkit/src/plugins/files/tests/policy.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, test } from "vitest"; +import { + type FileAction, + type FilePolicyUser, + type FileResource, + PolicyDeniedError, + policy, + READ_ACTIONS, + WRITE_ACTIONS, +} from "../policy"; + +const user: FilePolicyUser = { id: "user-1" }; +const resource: FileResource = { path: "/file.txt", volume: "uploads" }; + +describe("FileAction sets", () => { + test("READ_ACTIONS contains all read actions", () => { + for (const a of [ + "list", + "read", + "download", + "raw", + "exists", + "metadata", + "preview", + ] as FileAction[]) { + expect(READ_ACTIONS.has(a)).toBe(true); + } + }); + + test("WRITE_ACTIONS contains all write actions", () => { + for (const a of ["upload", "mkdir", "delete"] as FileAction[]) { + expect(WRITE_ACTIONS.has(a)).toBe(true); + } + }); + + test("READ_ACTIONS and WRITE_ACTIONS are disjoint", () => { + for (const a of READ_ACTIONS) { + expect(WRITE_ACTIONS.has(a)).toBe(false); + } + }); +}); + +describe("policy.publicRead()", () => { + const p = policy.publicRead(); + + test("allows read actions", () => { + for (const a of READ_ACTIONS) { + expect(p(a, resource, user)).toBe(true); + } + }); + + test("denies write actions", () => { + for (const a of WRITE_ACTIONS) { + expect(p(a, resource, user)).toBe(false); + } + }); +}); + +describe("policy.denyAll() / policy.allowAll()", () => { + test("denyAll denies everything", () => { + const p = policy.denyAll(); + expect(p("list", resource, user)).toBe(false); + expect(p("upload", resource, user)).toBe(false); + }); + + test("allowAll allows everything", () => { + const p = policy.allowAll(); + expect(p("list", resource, user)).toBe(true); + expect(p("upload", resource, user)).toBe(true); + }); +}); + +describe("policy.all()", () => { + test("returns true when all policies allow", async () => { + const p = policy.all(policy.allowAll(), policy.allowAll()); + expect(await p("list", resource, user)).toBe(true); + }); + + test("short-circuits on first deny", async () => { + let secondCalled = false; + const p = policy.all(policy.denyAll(), () => { + secondCalled = true; + return true; + }); + expect(await p("list", resource, user)).toBe(false); + expect(secondCalled).toBe(false); + }); + + test("throws when called with no policies", () => { + expect(() => policy.all()).toThrow( + "policy.all() requires at least one policy", + ); + }); +}); + +describe("policy.any()", () => { + test("returns false when all policies deny", async () => { + const p = policy.any(policy.denyAll(), policy.denyAll()); + expect(await p("list", resource, user)).toBe(false); + }); + + test("short-circuits on first allow", async () => { + let secondCalled = false; + const p = policy.any(policy.allowAll(), () => { + secondCalled = true; + return false; + }); + expect(await p("list", resource, user)).toBe(true); + expect(secondCalled).toBe(false); + }); + + test("throws when called with no policies", () => { + expect(() => policy.any()).toThrow( + "policy.any() requires at least one policy", + ); + }); +}); + +describe("policy.not()", () => { + test("inverts allow to deny", async () => { + const p = policy.not(policy.allowAll()); + expect(await p("list", resource, user)).toBe(false); + }); + + test("inverts deny to allow", async () => { + const p = policy.not(policy.denyAll()); + expect(await p("list", resource, user)).toBe(true); + }); +}); + +describe("async policy support", () => { + test("handles async policy that returns Promise", async () => { + const asyncPolicy = async () => true; + const p = policy.all(asyncPolicy); + expect(await p("list", resource, user)).toBe(true); + }); +}); + +describe("PolicyDeniedError", () => { + test("has correct name and message", () => { + const err = new PolicyDeniedError("upload", "images"); + expect(err.name).toBe("PolicyDeniedError"); + expect(err.message).toBe('Policy denied "upload" on volume "images"'); + expect(err.action).toBe("upload"); + expect(err.volumeKey).toBe("images"); + expect(err instanceof Error).toBe(true); + }); +}); diff --git a/packages/appkit/src/plugins/files/types.ts b/packages/appkit/src/plugins/files/types.ts index 9a37a4a1..82b54688 100644 --- a/packages/appkit/src/plugins/files/types.ts +++ b/packages/appkit/src/plugins/files/types.ts @@ -1,5 +1,6 @@ import type { files } from "@databricks/sdk-experimental"; import type { BasePluginConfig, IAppRequest } from "shared"; +import type { FilePolicy } from "./policy"; /** * Per-volume configuration options. @@ -9,11 +10,17 @@ export interface VolumeConfig { maxUploadSize?: number; /** Map of file extensions to MIME types for this volume. Inherits from plugin-level `customContentTypes` if not set. */ customContentTypes?: Record; + /** + * Access-control policy for this volume. When set, operations execute as the + * service principal and the policy decides whether the action is allowed. + */ + policy?: FilePolicy; } /** * User-facing API for a single volume. - * Prefer OBO access via `app.files("volumeKey").asUser(req).list()`. + * All operations execute as the service principal. When a policy is + * configured on the volume, every call is checked against that policy. */ export interface VolumeAPI { list(directoryPath?: string): Promise; @@ -78,8 +85,9 @@ export interface FilePreview extends FileMetadata { /** * Volume handle returned by `app.files("volumeKey")`. * - * - `asUser(req)` — executes on behalf of the user (recommended). - * - Direct methods (e.g. `.list()`) — execute as the service principal (logs a warning encouraging OBO). + * All methods execute as the service principal and enforce the volume's + * policy (if configured) with `{ isServicePrincipal: true }`. + * `asUser(req)` re-wraps with the real user identity for per-user policy checks. */ export type VolumeHandle = VolumeAPI & { asUser: (req: IAppRequest) => VolumeAPI; @@ -91,15 +99,15 @@ export type VolumeHandle = VolumeAPI & { * * @example * ```ts - * // OBO access (recommended) - * appKit.files("uploads").asUser(req).list() - * - * // Service principal access (logs a warning) + * // Service principal access * appKit.files("uploads").list() * + * // With policy: pass user identity for access control + * appKit.files("uploads").asUser(req).list() + * * // Named accessor * const vol = appKit.files.volume("uploads") - * await vol.asUser(req).list() + * await vol.list() * ``` */ export interface FilesExport {