From 36ea4a9a8084c10bb77f027a00b96d3ff7c9836a Mon Sep 17 00:00:00 2001 From: johnnyq Date: Mon, 31 Aug 2026 18:30:09 -0400 Subject: [PATCH] API: allow reads to filter by client_id on top of RBAC client scoping The RBAC rework dropped the client_id read filter along with per-key client scope, so an all-clients key could no longer narrow to one client. apiClientScopeSql() now appends the filter after the scope fragment, so it can only narrow, never widen. Reads only - writes take client_id as their target client. --- api/v1/enforce_api_rbac.php | 46 +++++++++++++++++++++++++++------- api/v1/invoice_items/read.php | 8 +++--- api/v1/technicians/time.php | 1 + api/v1/ticket_replies/read.php | 1 + 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/api/v1/enforce_api_rbac.php b/api/v1/enforce_api_rbac.php index ef2b5b01f..46c000a05 100644 --- a/api/v1/enforce_api_rbac.php +++ b/api/v1/enforce_api_rbac.php @@ -12,6 +12,10 @@ * the user. Reads are scoped with apiClientScopeSql(); writes act on a single * target client supplied in the request and validated against the user's access. * + * Reads may also pass client_id as an optional filter (e.g. every asset belonging to + * client 5). It is applied inside apiClientScopeSql() on top of the user's scope, so it + * can only narrow the result set, never widen it. + * * Reuses the existing RBAC machinery so there is no parallel permission model. * Included by validate_api_key.php - runs in global scope with $mysqli, * $api_key_user_id and $return_arr available. @@ -44,11 +48,32 @@ function apiUserCanAccessClient($client_id) { return empty($client_access_array) || in_array($client_id, $client_access_array, true); } -// Client-scope SQL fragment for a read query, from the user's allow / deny lists. -// Thin wrapper over clientScopeSql() in functions/auth.php so the API and the UI share one -// implementation. Kept under the api* name because every endpoint already calls it. +// Client-scope SQL fragment for a read query: the user's allow / deny lists, plus the +// optional client_id filter the caller asked for. +// +// The scope half is a thin wrapper over clientScopeSql() in functions/auth.php so the API +// and the UI share one implementation. Kept under the api* name because every endpoint +// already calls it. +// +// The filter half is what lets a key that can see every client ask for just one of them - +// GET assets/read.php?api_key=...&client_id=5. It is appended AFTER the scope fragment, so +// it can only narrow: a user who cannot see client 5 still gets nothing back. +// +// Reads only. Writes take client_id as the target client they act on and validate it +// against the user's access further down, so filtering a write query by it would be wrong. function apiClientScopeSql($column) { - return clientScopeSql($column); + global $client_id, $client_id_supplied, $is_write; + + $sql = clientScopeSql($column); + + // Tested on $client_id_supplied, not on the value: client_id = 0 is a real request + // (records with no client), and require_get_method.php turns an absent client_id + // into "%", which would silently compare as 0 against an integer column. + if (!empty($client_id_supplied) && empty($is_write)) { + $sql .= " AND $column = " . intval($client_id); + } + + return $sql; } // --- Every key must be tied to a user (legacy keys were removed in the 2.4.7 migration) --- @@ -145,11 +170,13 @@ if (lookupUserPermission($resource_module[$resource]) < $required_level) { } // --- 3) Target client for writes: taken from the request, validated against the user --- -// Reads ignore this and use apiClientScopeSql(). Create/update/delete act on a single -// client the caller names (client_id in the body/query); it must be within the user's -// access. Callers that omit it get $client_id = 0 (creates that require a client fail -// their own !empty($client_id) guard, which is the intended "must name a client"). +// Create/update/delete act on a single client the caller names (client_id in the +// body/query); it must be within the user's access. Callers that omit it get +// $client_id = 0 (creates that require a client fail their own !empty($client_id) guard, +// which is the intended "must name a client"). On a read the same parameter is not a +// permission at all, just an optional filter applied inside apiClientScopeSql(). $client_id = intval($_POST['client_id'] ?? $_GET['client_id'] ?? 0); +$client_id_supplied = isset($_POST['client_id']) || isset($_GET['client_id']); $is_write = in_array($operation_file, ['create.php', 'update.php', 'delete.php'], true); if ($is_write && !apiUserCanAccessClient($client_id)) { // Writes act on a single client the caller names (client_id 0 = a global record). @@ -157,4 +184,5 @@ if ($is_write && !apiUserCanAccessClient($client_id)) { // writing global (client_id 0) records, which their access does not include. apiDeny("The user linked to this API key does not have access to the target client for this write."); } -// Reads ignore $client_id entirely and are scoped by apiClientScopeSql(). +// Reads never treat $client_id as a permission - apiClientScopeSql() enforces the user's +// scope first and only then narrows to $client_id if the caller supplied one. diff --git a/api/v1/invoice_items/read.php b/api/v1/invoice_items/read.php index 670116147..c8acec549 100644 --- a/api/v1/invoice_items/read.php +++ b/api/v1/invoice_items/read.php @@ -10,17 +10,17 @@ * invoice_id required* - Return items for a single invoice * item_id required* - Return a single line item by its own ID * * One of invoice_id or item_id must be provided + * client_id optional - Only return items on invoices for this client * limit optional - Max rows to return (default 50) * offset optional - Offset for pagination (default 0) * * Security: * - invoice_items are always joined to invoices so that invoice_client_id - * is checked against the API key's client scope. A scoped key can never + * is checked against the key user's client scope. A restricted key can never * read items belonging to another client, even when item_id is supplied * directly. - * - $client_id is set to "%" by validate_api_key.php for All-Clients keys, - * which causes the LIKE to match every client — consistent with other - * endpoints in this API. + * - An optional client_id narrows the result further, within that scope. It is + * a filter, not a grant - it can never widen what the key can see. */ require_once '../validate_api_key.php'; require_once '../require_get_method.php'; diff --git a/api/v1/technicians/time.php b/api/v1/technicians/time.php index e9e771a19..a180e790d 100644 --- a/api/v1/technicians/time.php +++ b/api/v1/technicians/time.php @@ -9,6 +9,7 @@ * year (optional) - Filter by year (default: current year) * month (optional) - Filter by month 1-12 (default: current month) * technician_id (optional) - Filter by specific technician user ID + * client_id (optional) - Only count time on tickets for this client * limit (optional) - Number of results to return (default: 50) * offset (optional) - Offset for pagination (default: 0) */ diff --git a/api/v1/ticket_replies/read.php b/api/v1/ticket_replies/read.php index 1819d9974..3ed6ec8fe 100644 --- a/api/v1/ticket_replies/read.php +++ b/api/v1/ticket_replies/read.php @@ -10,6 +10,7 @@ * ticket_reply_id optional - Return a single reply by its own ID * ticket_id optional - Return all replies on a single ticket * type optional - Filter by reply type: Internal, Public or Client + * client_id optional - Only return replies on tickets for this client * include_archived optional - Set to 1 to include archived replies (default: excluded) * limit optional - Max rows to return (default 50) * offset optional - Offset for pagination (default 0)