Skip to content

fix(tasks): admins can fetch any task by id (unblocks customer-prompt-task) #689

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.

Sign up for GitHub

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

Merged
sweetmantech merged 1 commit into from
Jun 19, 2026
Merged

fix(tasks): admins can fetch any task by id (unblocks customer-prompt-task) #689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions lib/tasks/__tests__/validateGetTasksQuery.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,41 @@ describe("validateGetTasksQuery", () => {
});
});

it("lets an admin fetch any task by id alone, dropping account scope", async () => {
vi.mocked(validateAuthContext).mockResolvedValue({
accountId: "worker_admin_acc",
orgId: null,
authToken: "token",
});
vi.mocked(checkIsAdmin).mockResolvedValue(true);

const result = await validateGetTasksQuery(
createMockRequest("http://localhost:3000/api/tasks?id=task_owned_by_someone_else"),
);

expect(checkIsAdmin).toHaveBeenCalledWith("worker_admin_acc");
expect(validateAccountIdOverride).not.toHaveBeenCalled();
// No account_id key -> selectScheduledActions filters by id only (cross-account read).
expect(result).toEqual({ id: "task_owned_by_someone_else" });
});

it("keeps a non-admin id lookup scoped to the authenticated account", async () => {
vi.mocked(validateAuthContext).mockResolvedValue({
accountId: "regular_acc",
orgId: null,
authToken: "token",
});
vi.mocked(checkIsAdmin).mockResolvedValue(false);

const result = await validateGetTasksQuery(
createMockRequest("http://localhost:3000/api/tasks?id=task_1"),
);

expect(checkIsAdmin).toHaveBeenCalledWith("regular_acc");
expect(validateAccountIdOverride).not.toHaveBeenCalled();
expect(result).toEqual({ id: "task_1", account_id: "regular_acc" });
});

it("returns 403 when non-admin override is denied", async () => {
vi.mocked(validateAuthContext).mockResolvedValue({
accountId: "org_owner_acc",
Expand Down
16 changes: 13 additions & 3 deletions lib/tasks/validateGetTasksQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ export const getTasksQuerySchema = z.object({
});

export type GetTasksQuery = z.infer<typeof getTasksQuerySchema>;
export type ValidatedGetTasksQuery = Omit<GetTasksQuery, "account_id"> & { account_id: string };
// account_id is optional: admin callers fetching a single task by `id` are not
// scoped to their own account, so the resolved query may omit account_id entirely.
export type ValidatedGetTasksQuery = Omit<GetTasksQuery, "account_id"> & { account_id?: string };

/**
* Validates get tasks query parameters from a NextRequest.
Expand Down Expand Up @@ -55,7 +57,7 @@ export async function validateGetTasksQuery(
);
}

let targetAccountId = authResult.accountId;
let targetAccountId: string | undefined = authResult.accountId;

if (
validationResult.data.account_id &&
Expand All @@ -76,10 +78,18 @@ export async function validateGetTasksQuery(

targetAccountId = overrideResult.accountId;
}
} else if (validationResult.data.id && !validationResult.data.account_id) {
// Single-task lookup by id: admins may read any task regardless of owner
// (e.g. the background worker fetching a customer's scheduled task config).
// Non-admins stay scoped to their own account.
const isAdmin = await checkIsAdmin(authResult.accountId);
if (isAdmin) {
targetAccountId = undefined;
}
Comment on lines +81 to +88

@sweetmantech sweetmantech Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRY - do we already have a shared lib to check for org + admin account gating? If so, we should use the existing libs instead of directly inlining checkIsAdmin. Check neighboring validate functions.

@sweetmantech sweetmantech Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good check — I looked at the neighbors. A few findings:

The change already calls the shared helper, not inlined logic. checkIsAdmin(authResult.accountId) is the shared lib/admins/checkIsAdmin.ts (→ validateOrganizationAccess(accountId, RECOUP_ORG_ID)). No org-membership query is inlined here.

It's the established neighbor pattern. The closest sibling — lib/tasks/validateGetTaskRunQuery.ts (GET /api/tasks/runs) — uses the identical checkIsAdmin admin-bypass shape, and this same file's existing account_id-override branch already imports and uses checkIsAdmin. I reused that import rather than introducing a new gating call.

The other org helpers don't fit better here:

  • lib/organizations/canAccessAccount.ts needs a targetAccountId (caller-vs-target). My case has no target account — an admin reads any task by id, owner unknown until fetched. Using it would mean restructuring to fetch-then-authorize, and it also grants shared-org access (org-mates, not just Recoup admins) — broader than this bugfix's intended scope (issue chat#1810 deliberately scopes to admin-only).
  • lib/organizations/isRecoupAdmin.ts is functionally equivalent to checkIsAdmin but via a different query; switching to it would make this validator inconsistent with its lib/tasks siblings.

So I kept checkIsAdmin for neighbor-consistency. Happy to switch to the org-aware canAccessAccount (fetch-then-authorize) if you want org-mates — not just Recoup admins — to read each other's tasks by id; that's a scope decision, so flagging rather than assuming.

}

return {
...validationResult.data,
account_id: targetAccountId,
...(targetAccountId ? { account_id: targetAccountId } : {}),
};
}
Loading