Skip to content

Commit

Permalink
Shared albums should not consider the require_link property. (Lyche…
Browse files Browse the repository at this point in the history 
…eOrg#1480)

* clarify policies using named arguments (php8)
* add album shared require link tests
* also fix subqueries
* fix order of upload thus recent thumb changed
  • Loading branch information
@ildyria
ildyria committed Sep 11, 2022
1 parent b5652ee commit ed4240e
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 53 deletions.
28 changes: 8 additions & 20 deletions app/Policies/AlbumQueryPolicy.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public function __construct(AlbumFactory $albumFactory, AlbumPolicy $albumPolicy
*
* - the user is an admin
* - the user is the owner of the album
* - the album is shared with the user and the album does not require a direct link
* - the album is shared with the user
* - the album is public and the album does not require a direct link
*
* @param AlbumBuilder|TagAlbumBuilder $query
Expand Down Expand Up @@ -77,11 +77,7 @@ public function applyVisibilityFilter(AlbumBuilder|FixedQueryBuilder $query): Al
if ($userID !== null) {
$query2
->orWhere('base_albums.owner_id', '=', $userID)
->orWhere(
fn (AlbumBuilder|TagAlbumBuilder $q) => $q
->where('base_albums.requires_link', '=', false)
->where('user_base_album.user_id', '=', $userID)
);
->orWhere('user_base_album.user_id', '=', $userID);
}
};

Expand Down Expand Up @@ -153,7 +149,7 @@ public function appendAccessibilityConditions(BaseBuilder $query): BaseBuilder
*
* - the user is the admin, or
* - the user is the owner, or
* - the album does not require a direct link and is shared with the user, or
* - the album is shared with the user, or
* - the album does not require a direct link, is public and has no password set, or
* - the album does not require a direct link, is public and has been unlocked
*
Expand Down Expand Up @@ -197,11 +193,7 @@ public function applyReachabilityFilter(AlbumBuilder $query): AlbumBuilder
if ($userID !== null) {
$query2
->orWhere('base_albums.owner_id', '=', $userID)
->orWhere(
fn (Builder $q) => $q
->where('base_albums.requires_link', '=', false)
->where('user_base_album.user_id', '=', $userID)
);
->orWhere('user_base_album.user_id', '=', $userID);
}
};

Expand Down Expand Up @@ -355,15 +347,11 @@ public function appendUnreachableAlbumsCondition(BaseBuilder $builder, int|strin
if ($userID !== null) {
$builder
->where('inner_base_albums.owner_id', '<>', $userID)
->where(
->whereNotExists(
fn (BaseBuilder $q) => $q
->where('inner_base_albums.requires_link', '=', true)
->orWhereNotExists(
fn (BaseBuilder $q2) => $q2
->from('user_base_album', 'user_inner_base_album')
->whereColumn('user_inner_base_album.base_album_id', '=', 'inner_base_albums.id')
->where('user_inner_base_album.user_id', '=', $userID)
)
->from('user_base_album', 'user_inner_base_album')
->whereColumn('user_inner_base_album.base_album_id', '=', 'inner_base_albums.id')
->where('user_inner_base_album.user_id', '=', $userID)
);
}

Expand Down
4 changes: 2 additions & 2 deletions tests/Feature/AlbumTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,8 @@ public function testTrueNegative(): void

$this->albums_tests->set_description('-1', 'new description', 422);
$this->albums_tests->set_description('abcdefghijklmnopqrstuvwx', 'new description', 404);
$this->albums_tests->set_protection_policy('-1', true, true, false, false, true, true, null, 422);
$this->albums_tests->set_protection_policy('abcdefghijklmnopqrstuvwx', true, true, false, false, true, true, null, 404);
$this->albums_tests->set_protection_policy(id: '-1', expectedStatusCode: 422);
$this->albums_tests->set_protection_policy(id: 'abcdefghijklmnopqrstuvwx', expectedStatusCode: 404);

Auth::logout();
Session::flush();
Expand Down
2 changes: 1 addition & 1 deletion tests/Feature/GeoDataTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ public function testThumbnailsInsideHiddenAlbum(): void
TestCase::createUploadedFile(TestCase::SAMPLE_FILE_MONGOLIA_IMAGE), $albumID13
)->offsetGet('id');

$this->albums_tests->set_protection_policy($albumID1, true, true, true);
$this->albums_tests->set_protection_policy(id: $albumID1, full_photo: true, public: true, requiresLink: true);
// Sic! We do not make album 1.1 public to ensure that the
// search filter does not include too much
$this->albums_tests->set_protection_policy($albumID12);
Expand Down
2 changes: 1 addition & 1 deletion tests/Feature/PhotosOperationsTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ public function testThumbnailsInsideHiddenAlbum(): void
TestCase::createUploadedFile(TestCase::SAMPLE_FILE_SUNSET_IMAGE), $albumID121
)->offsetGet('id');

$this->albums_tests->set_protection_policy($albumID1, true, true, true);
$this->albums_tests->set_protection_policy(id: $albumID1, full_photo: true, public: true, requiresLink: true);
$this->albums_tests->set_protection_policy($albumID11);
$this->albums_tests->set_protection_policy($albumID12);
$this->albums_tests->set_protection_policy($albumID121);
Expand Down
12 changes: 6 additions & 6 deletions tests/Feature/SharingSpecialTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,12 @@ protected function prepareSixAlbumsWithDifferentProtectionSettings(): void
$this->photos_tests->set_title($this->photoID5, 'Abenddämmerung'); // we rename the duplicated images, such that we can ensure
$this->photos_tests->set_title($this->photoID6, 'Zug'); // a deterministic, alphabetic order which makes testing easier

$this->albums_tests->set_protection_policy($this->albumID1, true, true, false, false, true, true, self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy($this->albumID2);
$this->albums_tests->set_protection_policy($this->albumID3, true, true, false, false, true, true, self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy($this->albumID4, true, true, true, false, true, true, self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy($this->albumID5, true, true, false, false, true, true, self::ALBUM_PWD_2);
$this->albums_tests->set_protection_policy($this->albumID6, true, true, true, false, true, true, self::ALBUM_PWD_2);
$this->albums_tests->set_protection_policy(id: $this->albumID1, password: self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy(id: $this->albumID2);
$this->albums_tests->set_protection_policy(id: $this->albumID3, password: self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy(id: $this->albumID4, requiresLink: true, password: self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy(id: $this->albumID5, password: self::ALBUM_PWD_2);
$this->albums_tests->set_protection_policy(id: $this->albumID6, requiresLink: true, password: self::ALBUM_PWD_2);

Auth::logout();
Session::flush();
Expand Down
45 changes: 27 additions & 18 deletions tests/Feature/SharingTestScenariosAbstract.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,16 @@ public function tearDown(): void
parent::tearDown();
}

abstract protected function generateExpectedRootJson(
?string $unsortedAlbumThumbID = null,
?string $starredAlbumThumbID = null,
?string $publicAlbumThumbID = null,
?string $recentAlbumThumbID = null,
array $expectedAlbumJson = []
): array;

abstract protected function generateExpectedTreeJson(array $expectedAlbums = []): array;

/**
* Uploads an unsorted, private photo and logs out.
*
Expand Down Expand Up @@ -312,8 +322,8 @@ protected function preparePublicAlbumAndPasswordProtectedAlbum(): void
$this->albumID2 = $this->albums_tests->add(null, self::ALBUM_TITLE_2)->offsetGet('id');
$this->photoID1 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_MONGOLIA_IMAGE), $this->albumID1)->offsetGet('id');
$this->photoID2 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_TRAIN_IMAGE), $this->albumID2)->offsetGet('id');
$this->albums_tests->set_protection_policy($this->albumID1, true, true, false, false, true, true, self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy($this->albumID2);
$this->albums_tests->set_protection_policy(id: $this->albumID1, password: self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy(id: $this->albumID2);
Auth::logout();
Session::flush();
$this->clearCachedSmartAlbums();
Expand Down Expand Up @@ -446,7 +456,7 @@ protected function preparePublicAlbumAndPasswordProtectedAlbumWithStarredPhoto()
$this->albumID2 = $this->albums_tests->add(null, self::ALBUM_TITLE_2)->offsetGet('id');
$this->photoID1 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_MONGOLIA_IMAGE), $this->albumID1)->offsetGet('id');
$this->photoID2 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_TRAIN_IMAGE), $this->albumID2)->offsetGet('id');
$this->albums_tests->set_protection_policy($this->albumID1, true, true, false, false, true, true, self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy(id: $this->albumID1, password: self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy($this->albumID2);
$this->photos_tests->set_star([$this->photoID1], true);
Auth::logout();
Expand Down Expand Up @@ -586,8 +596,8 @@ protected function preparePublicAlbumAndHiddenAlbum(): void
$this->albumID2 = $this->albums_tests->add(null, self::ALBUM_TITLE_2)->offsetGet('id');
$this->photoID1 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_MONGOLIA_IMAGE), $this->albumID1)->offsetGet('id');
$this->photoID2 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_TRAIN_IMAGE), $this->albumID2)->offsetGet('id');
$this->albums_tests->set_protection_policy($this->albumID1);
$this->albums_tests->set_protection_policy($this->albumID2, true, true, true);
$this->albums_tests->set_protection_policy(id: $this->albumID1);
$this->albums_tests->set_protection_policy(id: $this->albumID2, requiresLink: true);
$this->photos_tests->set_star([$this->photoID2], true);
Auth::logout();
Session::flush();
Expand Down Expand Up @@ -668,8 +678,8 @@ protected function preparePublicAlbumAndHiddenPasswordProtectedAlbum(): void
$this->albumID2 = $this->albums_tests->add(null, self::ALBUM_TITLE_2)->offsetGet('id');
$this->photoID1 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_MONGOLIA_IMAGE), $this->albumID1)->offsetGet('id');
$this->photoID2 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_TRAIN_IMAGE), $this->albumID2)->offsetGet('id');
$this->albums_tests->set_protection_policy($this->albumID1);
$this->albums_tests->set_protection_policy($this->albumID2, true, true, true, false, true, true, self::ALBUM_PWD_2);
$this->albums_tests->set_protection_policy(id: $this->albumID1);
$this->albums_tests->set_protection_policy(id: $this->albumID2, requiresLink: true, password: self::ALBUM_PWD_2);
$this->photos_tests->set_star([$this->photoID2], true);
Auth::logout();
Session::flush();
Expand Down Expand Up @@ -792,18 +802,21 @@ public function testPublicAlbumAndHiddenPasswordProtectedUnlockedAlbum(): void
}

/**
* Creates two albums, puts a single photo in each, shares one
* album with a user and logs out.
* Creates three albums, puts a single photo in each, shares two
* album with a user, mark one as public with requireLink and logs out.
*
* @return void
*/
protected function preparePhotosInSharedAndPrivateAlbum(): void
protected function preparePhotosInSharedAndPrivateAndRequireLinkAlbum(): void
{
$this->albumID1 = $this->albums_tests->add(null, self::ALBUM_TITLE_1)->offsetGet('id');
$this->albumID2 = $this->albums_tests->add(null, self::ALBUM_TITLE_2)->offsetGet('id');
$this->albumID3 = $this->albums_tests->add(null, self::ALBUM_TITLE_3)->offsetGet('id');
$this->photoID1 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_MONGOLIA_IMAGE), $this->albumID1)->offsetGet('id');
$this->photoID2 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_TRAIN_IMAGE), $this->albumID2)->offsetGet('id');
$this->sharing_tests->add([$this->albumID1], [$this->userID]);
$this->photoID3 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_SUNSET_IMAGE), $this->albumID3)->offsetGet('id');
$this->sharing_tests->add([$this->albumID1, $this->albumID3], [$this->userID]);
$this->albums_tests->set_protection_policy(id: $this->albumID3, requiresLink: true);
Auth::logout();
Session::flush();
$this->clearCachedSmartAlbums();
Expand All @@ -825,7 +838,7 @@ protected function preparePhotoInSharedPublicPasswordProtectedAlbum(): void
{
$this->albumID1 = $this->albums_tests->add(null, self::ALBUM_TITLE_1)->offsetGet('id');
$this->photoID1 = $this->photos_tests->upload(static::createUploadedFile(static::SAMPLE_FILE_MONGOLIA_IMAGE), $this->albumID1)->offsetGet('id');
$this->albums_tests->set_protection_policy($this->albumID1, true, true, false, false, true, true, self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy(id: $this->albumID1, password: self::ALBUM_PWD_1);
$this->sharing_tests->add([$this->albumID1], [$this->userID]);
Auth::logout();
Session::flush();
Expand Down Expand Up @@ -902,12 +915,8 @@ protected function prepareThreeAlbumsWithMixedSharingAndPasswordProtection(): vo
// Sic! We use the same password for both albums here, because we want
// to ensure that incidentally "unlocking" an album which is also
// shared has no negative side effect.
$this->albums_tests->set_protection_policy(
$this->albumID2, true, true, false, true, true, true, self::ALBUM_PWD_1
);
$this->albums_tests->set_protection_policy(
$this->albumID3, true, true, false, true, true, true, self::ALBUM_PWD_1
);
$this->albums_tests->set_protection_policy(id: $this->albumID2, nsfw: true, password: self::ALBUM_PWD_1);
$this->albums_tests->set_protection_policy(id: $this->albumID3, nsfw: true, password: self::ALBUM_PWD_1);

Auth::logout();
Session::flush();
Expand Down
10 changes: 9 additions & 1 deletion tests/Feature/SharingWithAnonUserAbstract.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,35 +26,43 @@ abstract class SharingWithAnonUserAbstract extends SharingTestScenariosAbstract
{
public function testPhotosInSharedAndPrivateAlbum(): void
{
$this->preparePhotosInSharedAndPrivateAlbum();
$this->preparePhotosInSharedAndPrivateAndRequireLinkAlbum();

$responseForRoot = $this->root_album_tests->get();
$responseForRoot->assertJson($this->generateExpectedRootJson());
$responseForRoot->assertJsonMissing(['id' => $this->albumID1]);
$responseForRoot->assertJsonMissing(['id' => $this->photoID1]);
$responseForRoot->assertJsonMissing(['id' => $this->albumID2]);
$responseForRoot->assertJsonMissing(['id' => $this->photoID2]);
$responseForRoot->assertJsonMissing(['id' => $this->albumID3]);
$responseForRoot->assertJsonMissing(['id' => $this->photoID3]);

$responseForStarred = $this->albums_tests->get(StarredAlbum::ID);
$responseForStarred->assertJson($this->generateExpectedSmartAlbumJson(true));
$responseForStarred->assertJsonMissing(['id' => $this->albumID1]);
$responseForStarred->assertJsonMissing(['id' => $this->photoID1]);
$responseForStarred->assertJsonMissing(['id' => $this->albumID2]);
$responseForStarred->assertJsonMissing(['id' => $this->photoID2]);
$responseForStarred->assertJsonMissing(['id' => $this->albumID3]);
$responseForStarred->assertJsonMissing(['id' => $this->photoID3]);

$responseForRecent = $this->albums_tests->get(RecentAlbum::ID);
$responseForRecent->assertJson($this->generateExpectedSmartAlbumJson(true));
$responseForRecent->assertJsonMissing(['id' => $this->albumID1]);
$responseForRecent->assertJsonMissing(['id' => $this->photoID1]);
$responseForRecent->assertJsonMissing(['id' => $this->albumID2]);
$responseForRecent->assertJsonMissing(['id' => $this->photoID2]);
$responseForRecent->assertJsonMissing(['id' => $this->albumID3]);
$responseForRecent->assertJsonMissing(['id' => $this->photoID3]);

$responseForTree = $this->root_album_tests->getTree();
$responseForTree->assertJson($this->generateExpectedTreeJson());
$responseForTree->assertJsonMissing(['id' => $this->albumID1]);
$responseForTree->assertJsonMissing(['id' => $this->photoID1]);
$responseForTree->assertJsonMissing(['id' => $this->albumID2]);
$responseForTree->assertJsonMissing(['id' => $this->photoID2]);
$responseForTree->assertJsonMissing(['id' => $this->albumID3]);
$responseForTree->assertJsonMissing(['id' => $this->photoID3]);

$this->albums_tests->get($this->albumID1, $this->getExpectedInaccessibleHttpStatusCode(), $this->getExpectedDefaultInaccessibleMessage(), self::EXPECTED_PASSWORD_REQUIRED_MSG);
$this->photos_tests->get($this->photoID1, $this->getExpectedInaccessibleHttpStatusCode());
Expand Down
20 changes: 16 additions & 4 deletions tests/Feature/SharingWithNonAdminUserAbstract.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,16 @@ public function testTwoPhotosInPublicAlbum(): void

public function testPhotosInSharedAndPrivateAlbum(): void
{
$this->preparePhotosInSharedAndPrivateAlbum();
$this->preparePhotosInSharedAndPrivateAndRequireLinkAlbum();

$responseForRoot = $this->root_album_tests->get();
$responseForRoot->assertJson($this->generateExpectedRootJson(
null,
null,
null,
$this->photoID1, [
$this->photoID3, [
self::generateExpectedAlbumJson($this->albumID1, self::ALBUM_TITLE_1, null, $this->photoID1),
self::generateExpectedAlbumJson($this->albumID3, self::ALBUM_TITLE_3, null, $this->photoID3),
]
));
$responseForRoot->assertJsonMissing(['id' => $this->albumID2]);
Expand All @@ -70,11 +71,14 @@ public function testPhotosInSharedAndPrivateAlbum(): void
$responseForStarred->assertJson($this->generateExpectedSmartAlbumJson(true));
$responseForStarred->assertJsonMissing(['id' => $this->albumID1]);
$responseForStarred->assertJsonMissing(['id' => $this->photoID1]);
$responseForStarred->assertJsonMissing(['id' => $this->albumID3]);
$responseForStarred->assertJsonMissing(['id' => $this->photoID3]);

$responseForRecent = $this->albums_tests->get(RecentAlbum::ID);
$responseForRecent->assertJson($this->generateExpectedSmartAlbumJson(
true,
$this->photoID1, [
$this->photoID3, [
$this->generateExpectedPhotoJson(self::SAMPLE_FILE_SUNSET_IMAGE, $this->photoID3, $this->albumID3),
$this->generateExpectedPhotoJson(self::SAMPLE_FILE_MONGOLIA_IMAGE, $this->photoID1, $this->albumID1),
]
));
Expand All @@ -84,6 +88,7 @@ public function testPhotosInSharedAndPrivateAlbum(): void
$responseForTree = $this->root_album_tests->getTree();
$responseForTree->assertJson($this->generateExpectedTreeJson([
self::generateExpectedAlbumJson($this->albumID1, self::ALBUM_TITLE_1, null, $this->photoID1),
self::generateExpectedAlbumJson($this->albumID3, self::ALBUM_TITLE_3, null, $this->photoID3),
]));
$responseForTree->assertJsonMissing(['id' => $this->albumID2]);
$responseForTree->assertJsonMissing(['id' => $this->photoID2]);
Expand All @@ -96,6 +101,12 @@ public function testPhotosInSharedAndPrivateAlbum(): void

$this->albums_tests->get($this->albumID2, $this->getExpectedInaccessibleHttpStatusCode(), $this->getExpectedDefaultInaccessibleMessage(), self::EXPECTED_PASSWORD_REQUIRED_MSG);
$this->photos_tests->get($this->photoID2, $this->getExpectedInaccessibleHttpStatusCode());

$responseForAlbum3 = $this->albums_tests->get($this->albumID3);
$responseForAlbum3->assertJson(self::generateExpectedAlbumJson(
$this->albumID3, self::ALBUM_TITLE_3, null, $this->photoID3
));
$this->photos_tests->get($this->photoID3);
}

public function testPhotoInSharedPublicPasswordProtectedAlbum(): void
Expand Down Expand Up @@ -146,7 +157,8 @@ public function testThreeAlbumsWithMixedSharingAndPasswordProtection(): void
null,
null,
null,
$this->photoID1, [ // photo 1 is alphabetically first, as photo 3 is locked
$this->photoID1, // photo 1 is alphabetically first, as photo 3 is locked
[
$this->generateExpectedAlbumJson($this->albumID1, self::ALBUM_TITLE_1, null, $this->photoID1),
$this->generateExpectedAlbumJson($this->albumID2, self::ALBUM_TITLE_2, null, $this->photoID2),
$this->generateExpectedAlbumJson($this->albumID3, self::ALBUM_TITLE_3), // album 3 is locked, hence no thumb
Expand Down

0 comments on commit ed4240e

Please sign in to comment.