Skip to content

Commit b0b267e

Browse files
fogelitofogelito
committed
fix
1 parent baedc92 commit b0b267e

4 files changed

Lines changed: 147 additions & 27 deletions

File tree

‎src/Database/Adapter/Postgres.php‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2226,13 +2226,20 @@ public function prepareColumnPermissions(Document $collection): bool
22262226
*/
22272227
protected function hasColumnPermissionsIndex(string $index): bool
22282228
{
2229+
// Filtered by schema. pg_indexes spans every schema the connection can see,
2230+
// and index names are unique only within one -- two projects in their own
2231+
// schemas generate the same name for a collection with the same id. Without
2232+
// this, one project already migrated would make another look migrated too,
2233+
// and its index would silently stay on the narrow shape.
22292234
$stmt = $this->getPDO()->prepare("
22302235
SELECT 1
22312236
FROM pg_indexes
2232-
WHERE indexname = :index
2237+
WHERE schemaname = :schema
2238+
AND indexname = :index
22332239
AND indexdef LIKE '%_column%'
22342240
LIMIT 1
22352241
");
2242+
$stmt->bindValue(':schema', $this->getDatabase());
22362243
$stmt->bindValue(':index', $index);
22372244
$stmt->execute();
22382245

‎src/Database/Database.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7322,7 +7322,11 @@ public function updateDocument(string $collection, string $id, Document $documen
73227322

73237323
$this->trigger(self::EVENT_DOCUMENT_UPDATE, $document);
73247324

7325-
return $document;
7325+
// Write scopes and read scopes are independent, so what the caller was
7326+
// allowed to change says nothing about what they may see. The merged document
7327+
// carries every stored column, and handing it back would let an update on one
7328+
// column return the rest.
7329+
return $this->maskUnreadableColumns($collection, $document);
73267330
}
73277331

73287332
/**
@@ -7555,7 +7559,10 @@ public function updateDocuments(
75557559
$doc = $this->decode($collection, $doc);
75567560
}
75577561
try {
7558-
$onNext && $onNext($doc, $old[$index]);
7562+
$onNext && $onNext(
7563+
$this->maskUnreadableColumns($collection, $doc),
7564+
$this->maskUnreadableColumns($collection, $old[$index])
7565+
);
75597566
} catch (Throwable $th) {
75607567
$onError ? $onError($th) : throw $th;
75617568
}
@@ -8456,7 +8463,10 @@ public function upsertDocumentsWithIncrease(
84568463
}
84578464

84588465
try {
8459-
$onNext && $onNext($doc, $old->isEmpty() ? null : $old);
8466+
$onNext && $onNext(
8467+
$this->maskUnreadableColumns($collection, $doc),
8468+
$old->isEmpty() ? null : $this->maskUnreadableColumns($collection, $old)
8469+
);
84608470
} catch (\Throwable $th) {
84618471
$onError ? $onError($th) : throw $th;
84628472
}

‎tests/unit/ColumnPermissionEnforcementTest.php‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use Utopia\Database\Exception\Authorization as AuthorizationException;
1212
use Utopia\Database\Helpers\Permission;
1313
use Utopia\Database\Helpers\Role;
14+
use Utopia\Database\Query;
1415
use Utopia\Database\Validator\Authorization;
1516

1617
/**
@@ -308,6 +309,89 @@ public function testUnscopedUpdaterMayRewritePermissions(): void
308309
$this->assertContains('read("team:audit", "salary")', $stored->getPermissions());
309310
}
310311

312+
/**
313+
* Write scopes and read scopes are independent, so being allowed to change a
314+
* column says nothing about being allowed to see the rest of the row. The merged
315+
* document a write returns carries every stored column, so it has to go through
316+
* the same read masking a get would.
317+
*/
318+
public function testUpdateResponseIsMaskedByReadPermissions(): void
319+
{
320+
$this->authorization->skip(function () {
321+
$this->database->createDocument('employees', new Document([
322+
'$id' => 'w1',
323+
'$permissions' => [
324+
Permission::update(Role::user('ed'), 'name'), // may write name
325+
Permission::read(Role::user('ed'), 'email'), // may read email
326+
],
327+
'name' => 'Bob',
328+
'email' => 'bob@example.com',
329+
'salary' => '100000',
330+
]));
331+
});
332+
333+
$this->authorization->cleanRoles();
334+
$this->authorization->addRole('user:ed');
335+
336+
$returned = $this->database->updateDocument('employees', 'w1', new Document([
337+
'name' => 'Robert',
338+
]));
339+
340+
$this->assertSame('bob@example.com', $returned->getAttribute('email'));
341+
$this->assertNull($returned->getAttribute('name'), 'a writable column is not thereby readable');
342+
$this->assertNull($returned->getAttribute('salary'), 'update response leaked a hidden column');
343+
344+
// the write itself still landed
345+
$stored = $this->authorization->skip(
346+
fn () => $this->database->getDocument('employees', 'w1')
347+
);
348+
$this->assertSame('Robert', $stored->getAttribute('name'));
349+
$this->assertSame('100000', $stored->getAttribute('salary'));
350+
}
351+
352+
public function testBulkUpdateCallbackPayloadIsMasked(): void
353+
{
354+
$this->authorization->skip(function () {
355+
$this->database->createDocument('employees', new Document([
356+
'$id' => 'w2',
357+
'$permissions' => [
358+
Permission::update(Role::user('ed'), 'name'),
359+
Permission::read(Role::user('ed'), 'email'),
360+
],
361+
'name' => 'Bob',
362+
'email' => 'bob@example.com',
363+
'salary' => '100000',
364+
]));
365+
});
366+
367+
$this->authorization->cleanRoles();
368+
$this->authorization->addRole('user:ed');
369+
370+
$seen = [];
371+
372+
$this->database->updateDocuments(
373+
'employees',
374+
new Document(['name' => 'Bobby']),
375+
[Query::equal('$id', ['w2'])],
376+
100,
377+
onNext: function (Document $document) use (&$seen) {
378+
$seen[] = \array_keys(\array_filter(
379+
$document->getArrayCopy(),
380+
fn (string $key) => !\str_starts_with($key, '$'),
381+
ARRAY_FILTER_USE_KEY
382+
));
383+
}
384+
);
385+
386+
$this->assertSame([['email']], $seen, 'bulk callback leaked hidden columns');
387+
388+
$stored = $this->authorization->skip(
389+
fn () => $this->database->getDocument('employees', 'w2')
390+
);
391+
$this->assertSame('Bobby', $stored->getAttribute('name'));
392+
$this->assertSame('100000', $stored->getAttribute('salary'));
393+
}
394+
311395
public function testUnscopedRoleMayUpdateAnyColumn(): void
312396
{
313397
$this->authorization->cleanRoles();

‎tests/unit/ColumnPermissionTest.php‎

Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,15 @@
33
namespace Tests\Unit;
44

55
use PHPUnit\Framework\TestCase;
6+
use Utopia\Cache\Adapter\None as NoCache;
7+
use Utopia\Cache\Cache;
8+
use Utopia\Database\Adapter\Memory;
9+
use Utopia\Database\Database;
610
use Utopia\Database\Document;
711
use Utopia\Database\Exception as DatabaseException;
812
use Utopia\Database\Helpers\Permission;
913
use Utopia\Database\Helpers\Role;
14+
use Utopia\Database\Validator\Authorization;
1015
use Utopia\Database\Validator\Permissions;
1116

1217
class ColumnPermissionTest extends TestCase
@@ -95,34 +100,48 @@ public function testWildcardColumnIsRejected(): void
95100
}
96101

97102
/**
98-
* A column-scoped permission must still resolve to a bare role, or every
99-
* existing document-level authorization check silently breaks.
103+
* A column-scoped permission still grants its role ordinary row-level access --
104+
* the column narrows what is returned, it does not withhold the row. Asserted
105+
* through a read rather than through the shape of the extracted permission list.
100106
*/
101-
public function testDocumentPermissionsByTypeReturnsRolesOnly(): void
107+
public function testColumnScopedGrantStillGrantsTheRowToThatRole(): void
102108
{
103-
$document = new Document(['$permissions' => [
104-
'read("any")',
105-
'read("user:1", "salary")',
106-
'update("user:1", "name")',
107-
'delete("user:1")',
108-
]]);
109+
$authorization = new Authorization();
109110

110-
$this->assertSame(['any', 'user:1'], \array_values($document->getRead()));
111-
$this->assertSame(['user:1'], \array_values($document->getUpdate()));
112-
$this->assertSame(['user:1'], \array_values($document->getDelete()));
113-
}
111+
$database = new Database(new Memory(), new Cache(new NoCache()));
112+
$database
113+
->setAuthorization($authorization)
114+
->setDatabase('columnPermissions')
115+
->setNamespace('cpt_' . \uniqid());
114116

115-
public function testDocumentPermissionsByTypeWithColumns(): void
116-
{
117-
$document = new Document(['$permissions' => [
118-
'read("any")',
119-
'read("user:1", "salary")',
120-
]]);
117+
$database->create();
118+
119+
$authorization->skip(function () use ($database) {
120+
$database->createCollection('employees', documentSecurity: true, columnSecurity: true, permissions: []);
121+
$database->createAttribute('employees', 'name', Database::VAR_STRING, 128, false);
122+
$database->createAttribute('employees', 'salary', Database::VAR_INTEGER, 8, false);
123+
124+
$database->createDocument('employees', new Document([
125+
'$id' => 'e1',
126+
'$permissions' => [Permission::read(Role::user('hr'), 'salary')],
127+
'name' => 'Bob',
128+
'salary' => 100000,
129+
]));
130+
});
131+
132+
$authorization->cleanRoles();
133+
$authorization->addRole('user:hr');
134+
135+
$document = $database->getDocument('employees', 'e1');
136+
137+
$this->assertFalse($document->isEmpty(), 'a column-scoped grant must still make the row visible');
138+
$this->assertSame(100000, $document->getAttribute('salary'));
139+
$this->assertNull($document->getAttribute('name'));
140+
141+
$authorization->cleanRoles();
142+
$authorization->addRole('user:other');
121143

122-
$this->assertSame([
123-
['role' => 'any', 'column' => Permission::COLUMN_ALL],
124-
['role' => 'user:1', 'column' => 'salary'],
125-
], $document->getPermissionsByTypeWithColumns('read'));
144+
$this->assertTrue($database->getDocument('employees', 'e1')->isEmpty());
126145
}
127146

128147
public function testValidatorAcceptsColumnScopedReadCreateUpdate(): void

0 commit comments

Comments
 (0)