Skip to content

Commit adc5894

Browse files
ildyriaildyria
authored
Fix propagation error (#4481)
1 parent 9c13c3f commit adc5894

4 files changed

Lines changed: 180 additions & 2 deletions

File tree

‎app/Actions/Sharing/Propagate.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,10 @@ private function applyUpdate(Album $album): void
4949
// for each descendant, create a new permission if it does not exist.
5050
// or update the existing permission.
5151
$descendants = $album->descendants()->getQuery()->select('id')->pluck('id');
52-
$permissions = $album->access_permissions()->whereNotNull(APC::USER_ID)->orWhereNotNull(APC::USER_GROUP_ID)->get();
52+
$permissions = $album->access_permissions()->where(fn ($q) => $q
53+
->whereNotNull(APC::USER_ID)
54+
->orWhereNotNull(APC::USER_GROUP_ID)
55+
)->get();
5356

5457
// This is super inefficient.
5558
// It would be better to do it in a single query...
@@ -126,7 +129,10 @@ private function applyOverwrite(Album $album): void
126129
->where('_rgt', '<', $album->_rgt)
127130
->pluck('id');
128131

129-
$access_permissions = $album->access_permissions()->whereNotNull('user_id')->orWhereNotNull('user_group_id')->get();
132+
$access_permissions = $album->access_permissions()->where(fn ($q) => $q
133+
->whereNotNull(APC::USER_ID)
134+
->orWhereNotNull(APC::USER_GROUP_ID)
135+
)->get();
130136

131137
$new_perm = $access_permissions->reduce(
132138
fn (?array $acc, AccessPermission $permission) => array_merge(

‎database/migrations/2026_07_01_120000_deduplicate_and_constrain_access_permissions.php‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
<?php
2+
3+
/**
4+
* SPDX-License-Identifier: MIT
5+
* Copyright (c) 2017-2018 Tobias Reich
6+
* Copyright (c) 2018-2026 LycheeOrg.
7+
*/
8+
9+
use Illuminate\Database\Migrations\Migration;
10+
use Illuminate\Database\Query\Builder;
11+
use Illuminate\Database\Schema\Blueprint;
12+
use Illuminate\Support\Facades\App;
13+
use Illuminate\Support\Facades\DB;
14+
use Illuminate\Support\Facades\Schema;
15+
16+
return new class() extends Migration {
17+
private const TABLE_NAME = 'access_permissions';
18+
19+
private const BASE_ALBUM_ID = 'base_album_id';
20+
private const USER_ID = 'user_id';
21+
private const USER_GROUP_ID = 'user_group_id';
22+
23+
// Generated columns coalescing the nullable USER_ID / USER_GROUP_ID so that
24+
// a plain unique index can actually enforce uniqueness for both of them.
25+
// (A unique index ignores NULL, so [base_album_id, user_id, user_group_id]
26+
// alone would never catch duplicate group permissions, since user_id is
27+
// always NULL for those rows.)
28+
private const USER_ID_UNIQUE_KEY = 'user_id_unique_key';
29+
private const USER_GROUP_ID_UNIQUE_KEY = 'user_group_id_unique_key';
30+
31+
// Explicit name because Laravel's auto-generated name (concatenating all
32+
// four column names) exceeds MySQL's 64-character identifier limit.
33+
private const UNIQUE_INDEX_NAME = 'access_permissions_dedup_unique';
34+
35+
/**
36+
* Run the migrations.
37+
*/
38+
public function up(): void
39+
{
40+
if (!App::runningUnitTests()) {
41+
// @codeCoverageIgnoreStart
42+
DB::transaction(fn () => $this->deduplicate());
43+
// @codeCoverageIgnoreEnd
44+
} else {
45+
$this->deduplicate();
46+
}
47+
48+
Schema::table(self::TABLE_NAME, function (Blueprint $table) {
49+
// MySQL/MariaDB refuses to add a STORED generated column to a table
50+
// that still has an active foreign key: doing so requires an
51+
// in-place table rebuild, which InnoDB rejects with error 1215
52+
// ("Cannot add foreign key constraint"). Drop it here and
53+
// recreate it once the new columns are in place.
54+
$table->dropForeign([self::USER_ID]);
55+
$table->dropUnique([self::BASE_ALBUM_ID, self::USER_ID]);
56+
$table->unsignedInteger(self::USER_ID_UNIQUE_KEY)->nullable(false)->storedAs('COALESCE(' . self::USER_ID . ', 0)');
57+
$table->unsignedInteger(self::USER_GROUP_ID_UNIQUE_KEY)->nullable(false)->storedAs('COALESCE(' . self::USER_GROUP_ID . ', 0)');
58+
});
59+
60+
Schema::table(self::TABLE_NAME, function (Blueprint $table) {
61+
$table->unique([self::BASE_ALBUM_ID, self::USER_ID_UNIQUE_KEY, self::USER_GROUP_ID_UNIQUE_KEY], self::UNIQUE_INDEX_NAME);
62+
// MySQL prohibits CASCADE/SET NULL referential actions on a column
63+
// that is a base column of a generated column (user_id_unique_key
64+
// depends on user_id here), so this can no longer cascade at the
65+
// DB level. This is not a behavior change in practice: User::delete()
66+
// already deletes the user's AccessPermission rows explicitly before
67+
// removing the user.
68+
$table->foreign(self::USER_ID)->references('id')->on('users')->restrictOnUpdate()->restrictOnDelete();
69+
});
70+
}
71+
72+
/**
73+
* Reverse the migrations.
74+
*/
75+
public function down(): void
76+
{
77+
Schema::table(self::TABLE_NAME, function (Blueprint $table) {
78+
$table->dropForeign([self::USER_ID]);
79+
$table->dropUnique(self::UNIQUE_INDEX_NAME);
80+
$table->dropColumn([self::USER_ID_UNIQUE_KEY, self::USER_GROUP_ID_UNIQUE_KEY]);
81+
});
82+
83+
Schema::table(self::TABLE_NAME, function (Blueprint $table) {
84+
$table->unique([self::BASE_ALBUM_ID, self::USER_ID]);
85+
// The generated columns are already gone at this point, so the
86+
// original CASCADE actions are legal again here.
87+
$table->foreign(self::USER_ID)->references('id')->on('users')->cascadeOnUpdate()->cascadeOnDelete();
88+
});
89+
}
90+
91+
/**
92+
* Remove duplicate (base_album_id, user_id, user_group_id) rows, e.g. those
93+
* created by the Propagate action bug where an unscoped query leaked
94+
* unrelated albums' group permissions and re-inserted them for every
95+
* descendant on every click. One row per group is kept, the rest deleted.
96+
*/
97+
private function deduplicate(): void
98+
{
99+
$duplicate_groups = DB::table(self::TABLE_NAME)
100+
->select(self::BASE_ALBUM_ID, self::USER_ID, self::USER_GROUP_ID)
101+
->groupBy(self::BASE_ALBUM_ID, self::USER_ID, self::USER_GROUP_ID)
102+
->havingRaw('count(*) > 1')
103+
->get();
104+
105+
foreach ($duplicate_groups as $group) {
106+
$scoped = $this->matchNullable(
107+
$this->matchNullable(
108+
$this->matchNullable(
109+
DB::table(self::TABLE_NAME),
110+
self::BASE_ALBUM_ID,
111+
$group->base_album_id
112+
),
113+
self::USER_ID,
114+
$group->user_id
115+
),
116+
self::USER_GROUP_ID,
117+
$group->user_group_id
118+
);
119+
120+
// Keep the most recently written row: if duplicates ended up with
121+
// different grants (e.g. leaked from different source albums),
122+
// the newest one best reflects the last-applied intended state.
123+
$keep_id = $scoped->max('id');
124+
$scoped->where('id', '!=', $keep_id)->delete();
125+
}
126+
}
127+
128+
/**
129+
* Constrain a query builder on a column that may legitimately be NULL.
130+
*/
131+
private function matchNullable(Builder $query, string $column, string|int|null $value): Builder
132+
{
133+
return $value === null ? $query->whereNull($column) : $query->where($column, '=', $value);
134+
}
135+
};

‎tests/Feature_v2/Album/SharingTest.php‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,40 @@ public function testUpdate(): void
182182
self::assertTrue($perm->grants_upload);
183183
}
184184

185+
public function testUpdateDoesNotLeakUnrelatedAlbumGroupPermissions(): void
186+
{
187+
// Give an *unrelated* album (not in album1's tree) a group permission.
188+
// A buggy query scoping in Propagate::applyUpdate would leak this into
189+
// album1's descendants because it is not actually restricted to album1.
190+
$response = $this->actingAs($this->userMayUpload2)->postJson('Sharing', [
191+
'user_ids' => [],
192+
'group_ids' => [$this->group2->id],
193+
'album_ids' => [$this->album2->id],
194+
'grants_edit' => true,
195+
'grants_delete' => true,
196+
'grants_download' => true,
197+
'grants_full_photo_access' => true,
198+
'grants_upload' => true,
199+
]);
200+
$this->assertOk($response);
201+
202+
// Propagate album1 (unrelated to album2) to its descendants.
203+
$response = $this->actingAs($this->userMayUpload1)->putJson('Sharing', [
204+
'album_id' => $this->album1->id,
205+
'shall_override' => false,
206+
]);
207+
$this->assertNoContent($response);
208+
209+
// subAlbum1 should only inherit album1's own permissions (perm1, perm11),
210+
// not the unrelated group2 permission that lives on album2.
211+
self::assertEquals(2, AccessPermission::where(APC::BASE_ALBUM_ID, '=', $this->subAlbum1->id)->count());
212+
self::assertEquals(0,
213+
AccessPermission::query()
214+
->where(APC::BASE_ALBUM_ID, '=', $this->subAlbum1->id)
215+
->where(APC::USER_GROUP_ID, '=', $this->group2->id)
216+
->count());
217+
}
218+
185219
public function testOverride(): void
186220
{
187221
// Set up the permission in subSlbum

‎tests/Unit/Http/Requests/Album/GetAlbumPhotosRequestTest.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,15 @@
1515
use App\Http\Requests\Album\GetAlbumPhotosRequest;
1616
use App\Models\Tag;
1717
use App\Policies\AlbumPolicy;
18+
use Illuminate\Foundation\Testing\DatabaseTransactions;
1819
use Illuminate\Support\Facades\Gate;
1920
use Illuminate\Validation\ValidationException;
2021
use Tests\Unit\Http\Requests\Base\BaseRequestTest;
2122

2223
class GetAlbumPhotosRequestTest extends BaseRequestTest
2324
{
25+
use DatabaseTransactions;
26+
2427
public function testAuthorization(): void
2528
{
2629
Gate::shouldReceive('check')

0 commit comments

Comments
 (0)