diff --git a/server/src/dtos/album.dto.ts b/server/src/dtos/album.dto.ts index b270125b36..fb209bdca9 100644 --- a/server/src/dtos/album.dto.ts +++ b/server/src/dtos/album.dto.ts @@ -230,7 +230,7 @@ export const mapAlbum = ( const assets = entity.assets || []; const hasSharedLink = !!entity.sharedLinks && entity.sharedLinks.length > 0; - const hasSharedUser = albumUsers.length > 0; + const hasSharedUser = albumUsers.some(({ role }) => role !== AlbumUserRole.Owner); let startDate = assets.at(0)?.localDateTime; let endDate = assets.at(-1)?.localDateTime; diff --git a/server/src/enum.ts b/server/src/enum.ts index 1277a39036..637ac1d0e5 100644 --- a/server/src/enum.ts +++ b/server/src/enum.ts @@ -56,6 +56,7 @@ export enum AssetFileType { export enum AlbumUserRole { Editor = 'editor', Viewer = 'viewer', + Owner = 'owner', } export enum AssetOrder { diff --git a/server/src/repositories/access.repository.ts b/server/src/repositories/access.repository.ts index 1661e42c14..8159e7d25f 100644 --- a/server/src/repositories/access.repository.ts +++ b/server/src/repositories/access.repository.ts @@ -91,7 +91,9 @@ class AlbumAccess { } const accessRole = - access === AlbumUserRole.Editor ? [AlbumUserRole.Editor] : [AlbumUserRole.Editor, AlbumUserRole.Viewer]; + access === AlbumUserRole.Editor + ? [AlbumUserRole.Editor, AlbumUserRole.Owner] + : [AlbumUserRole.Editor, AlbumUserRole.Viewer, AlbumUserRole.Owner]; return this.db .selectFrom('album') diff --git a/server/src/repositories/album.repository.ts b/server/src/repositories/album.repository.ts index e4d802b93c..193913ef8e 100644 --- a/server/src/repositories/album.repository.ts +++ b/server/src/repositories/album.repository.ts @@ -14,6 +14,7 @@ import { InjectKysely } from 'nestjs-kysely'; import { columns } from 'src/database'; import { Chunked, ChunkedArray, ChunkedSet, DummyValue, GenerateSql } from 'src/decorators'; import { AlbumUserCreateDto } from 'src/dtos/album.dto'; +import { AlbumUserRole } from 'src/enum'; import { DB } from 'src/schema'; import { AlbumTable } from 'src/schema/tables/album.table'; import { AssetExifTable } from 'src/schema/tables/asset-exif.table'; @@ -213,12 +214,25 @@ export class AlbumRepository { .selectAll('album') .where((eb) => eb.or([ + // Albums I own that have non-owner shared users + eb.and([ + eb('album.ownerId', '=', ownerId), + eb.exists( + eb + .selectFrom('album_user') + .whereRef('album_user.albumId', '=', 'album.id') + .where('album_user.role', '!=', AlbumUserRole.Owner), + ), + ]), + // Albums shared with me (I'm in album_user but not the owner) eb.exists( eb .selectFrom('album_user') .whereRef('album_user.albumId', '=', 'album.id') - .where((eb) => eb.or([eb('album.ownerId', '=', ownerId), eb('album_user.userId', '=', ownerId)])), + .where('album_user.userId', '=', ownerId) + .where('album_user.role', '!=', AlbumUserRole.Owner), ), + // Albums with shared links eb.exists( eb .selectFrom('shared_link') @@ -245,7 +259,16 @@ export class AlbumRepository { .selectAll('album') .where('album.ownerId', '=', ownerId) .where('album.deletedAt', 'is', null) - .where((eb) => eb.not(eb.exists(eb.selectFrom('album_user').whereRef('album_user.albumId', '=', 'album.id')))) + .where((eb) => + eb.not( + eb.exists( + eb + .selectFrom('album_user') + .whereRef('album_user.albumId', '=', 'album.id') + .where('album_user.role', '!=', AlbumUserRole.Owner), + ), + ), + ) .where((eb) => eb.not(eb.exists(eb.selectFrom('shared_link').whereRef('shared_link.albumId', '=', 'album.id')))) .select(withOwner) .orderBy('album.createdAt', 'desc') @@ -322,14 +345,13 @@ export class AlbumRepository { await this.addAssets(tx, newAlbum.id, assetIds); } - if (albumUsers.length > 0) { - await tx - .insertInto('album_user') - .values( - albumUsers.map((albumUser) => ({ albumId: newAlbum.id, userId: albumUser.userId, role: albumUser.role })), - ) - .execute(); - } + // Always insert the owner with Owner role in album_user + const allAlbumUsers = [ + { albumId: newAlbum.id, userId: album.ownerId, role: AlbumUserRole.Owner }, + ...albumUsers.map((albumUser) => ({ albumId: newAlbum.id, userId: albumUser.userId, role: albumUser.role })), + ]; + + await tx.insertInto('album_user').values(allAlbumUsers).execute(); return tx .selectFrom('album') diff --git a/server/src/schema/functions.ts b/server/src/schema/functions.ts index 6dbbd28b1a..fb7d1ae96d 100644 --- a/server/src/schema/functions.ts +++ b/server/src/schema/functions.ts @@ -125,9 +125,6 @@ export const album_delete_audit = registerFunction({ language: 'PLPGSQL', body: ` BEGIN - INSERT INTO album_audit ("albumId", "userId") - SELECT "id", "ownerId" - FROM OLD; RETURN NULL; END`, }); diff --git a/server/src/schema/migrations/1774923680849-AddOwnerToAlbumUser.ts b/server/src/schema/migrations/1774923680849-AddOwnerToAlbumUser.ts new file mode 100644 index 0000000000..8c161dadca --- /dev/null +++ b/server/src/schema/migrations/1774923680849-AddOwnerToAlbumUser.ts @@ -0,0 +1,44 @@ +import { Kysely, sql } from 'kysely'; + +export async function up(db: Kysely): Promise { + // Backfill: insert album owner into album_user with 'owner' role for all existing albums + await sql` + INSERT INTO "album_user" ("albumId", "userId", "role") + SELECT "id", "ownerId", 'owner' FROM "album" + ON CONFLICT ("albumId", "userId") DO NOTHING + `.execute(db); + + // Make album_delete_audit a no-op since the owner is now in album_user + // and the CASCADE delete will trigger album_user_delete_audit instead + await sql` + CREATE OR REPLACE FUNCTION album_delete_audit() + RETURNS TRIGGER + LANGUAGE PLPGSQL AS $$ + BEGIN + RETURN NULL; + END $$ + `.execute(db); +} + +export async function down(db: Kysely): Promise { + // Restore album_delete_audit to insert owner audit entries + await sql` + CREATE OR REPLACE FUNCTION album_delete_audit() + RETURNS TRIGGER + LANGUAGE PLPGSQL AS $$ + BEGIN + INSERT INTO album_audit ("albumId", "userId") + SELECT "id", "ownerId" + FROM OLD; + RETURN NULL; + END $$ + `.execute(db); + + // Remove owner entries from album_user + await sql` + DELETE FROM "album_user" + WHERE ("albumId", "userId") IN ( + SELECT "id", "ownerId" FROM "album" + ) + `.execute(db); +} diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 47646d0c6d..472df3c483 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -283,16 +283,19 @@ describe(AlbumService.name, () => { ); }); - it('should throw an error if the userId is the ownerId', async () => { + it('should silently filter the owner from albumUsers', async () => { const album = AlbumFactory.create(); - mocks.user.get.mockResolvedValue(album.owner); - await expect( - sut.create(AuthFactory.create(album.owner), { - albumName: 'Empty album', - albumUsers: [{ userId: album.owner.id, role: AlbumUserRole.Editor }], - }), - ).rejects.toBeInstanceOf(BadRequestException); - expect(mocks.album.create).not.toHaveBeenCalled(); + mocks.album.create.mockResolvedValue(getForAlbum(album)); + mocks.user.getMetadata.mockResolvedValue([]); + await sut.create(AuthFactory.create(album.owner), { + albumName: 'Empty album', + albumUsers: [{ userId: album.owner.id, role: AlbumUserRole.Editor }], + }); + expect(mocks.album.create).toHaveBeenCalledWith( + expect.objectContaining({ ownerId: album.owner.id }), + [], + [], + ); }); }); @@ -420,7 +423,7 @@ describe(AlbumService.name, () => { sut.addUsers(AuthFactory.create(album.owner), album.id, { albumUsers: [{ userId: album.owner.id }], }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toThrow('User already added'); expect(mocks.album.update).not.toHaveBeenCalled(); expect(mocks.user.get).not.toHaveBeenCalled(); }); @@ -537,6 +540,7 @@ describe(AlbumService.name, () => { const user = UserFactory.create(); const album = AlbumFactory.from().albumUser({ userId: user.id }).build(); mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set([album.id])); + mocks.album.getById.mockResolvedValue(getForAlbum(album)); mocks.albumUser.update.mockResolvedValue(); await sut.updateUser(AuthFactory.create(album.owner), album.id, user.id, { role: AlbumUserRole.Viewer }); @@ -546,6 +550,16 @@ describe(AlbumService.name, () => { { role: AlbumUserRole.Viewer }, ); }); + + it('should not allow changing the album owner role', async () => { + const album = AlbumFactory.create(); + mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set([album.id])); + mocks.album.getById.mockResolvedValue(getForAlbum(album)); + + await expect( + sut.updateUser(AuthFactory.create(album.owner), album.id, album.owner.id, { role: AlbumUserRole.Viewer }), + ).rejects.toThrow('Cannot change album owner role'); + }); }); describe('getAlbumInfo', () => { diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index 547ec63bf8..f7ead0adc0 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -17,7 +17,7 @@ import { } from 'src/dtos/album.dto'; import { BulkIdErrorReason, BulkIdResponseDto, BulkIdsDto } from 'src/dtos/asset-ids.response.dto'; import { AuthDto } from 'src/dtos/auth.dto'; -import { Permission } from 'src/enum'; +import { AlbumUserRole, Permission } from 'src/enum'; import { AlbumAssetCount, AlbumInfoOptions } from 'src/repositories/album.repository'; import { BaseService } from 'src/services/base.service'; import { addAssets, removeAssets } from 'src/utils/asset.util'; @@ -80,7 +80,8 @@ export class AlbumService extends BaseService { const album = await this.findOrFail(id, { withAssets }); const [albumMetadataForIds] = await this.albumRepository.getMetadataForIds([album.id]); - const hasSharedUsers = album.albumUsers && album.albumUsers.length > 0; + const hasSharedUsers = + album.albumUsers && album.albumUsers.some(({ role }) => role !== AlbumUserRole.Owner); const hasSharedLink = album.sharedLinks && album.sharedLinks.length > 0; const isShared = hasSharedUsers || hasSharedLink; @@ -98,16 +99,19 @@ export class AlbumService extends BaseService { const albumUsers = dto.albumUsers || []; for (const { userId } of albumUsers) { + if (userId == auth.user.id) { + continue; + } + const exists = await this.userRepository.get(userId, {}); if (!exists) { throw new BadRequestException('User not found'); } - - if (userId == auth.user.id) { - throw new BadRequestException('Cannot share album with owner'); - } } + // Filter out the owner from albumUsers since they are always added by the repository + const filteredAlbumUsers = albumUsers.filter(({ userId }) => userId !== auth.user.id); + const allowedAssetIdsSet = await this.checkAccess({ auth, permission: Permission.AssetShare, @@ -126,10 +130,10 @@ export class AlbumService extends BaseService { order: getPreferences(userMetadata).albums.defaultAssetOrder, }, assetIds, - albumUsers, + filteredAlbumUsers, ); - for (const { userId } of albumUsers) { + for (const { userId } of filteredAlbumUsers) { await this.eventRepository.emit('AlbumInvite', { id: album.id, userId }); } @@ -188,9 +192,9 @@ export class AlbumService extends BaseService { albumThumbnailAssetId: album.albumThumbnailAssetId ?? firstNewAssetId, }); - const allUsersExceptUs = [...album.albumUsers.map(({ user }) => user.id), album.owner.id].filter( - (userId) => userId !== auth.user.id, - ); + const allUsersExceptUs = album.albumUsers + .map(({ user }) => user.id) + .filter((userId) => userId !== auth.user.id); for (const recipientId of allUsersExceptUs) { await this.eventRepository.emit('AlbumUpdate', { id, recipientId }); @@ -248,9 +252,9 @@ export class AlbumService extends BaseService { updatedAt: new Date(), albumThumbnailAssetId: album.albumThumbnailAssetId ?? notPresentAssetIds[0], }); - const allUsersExceptUs = [...album.albumUsers.map(({ user }) => user.id), album.owner.id].filter( - (userId) => userId !== auth.user.id, - ); + const allUsersExceptUs = album.albumUsers + .map(({ user }) => user.id) + .filter((userId) => userId !== auth.user.id); events.push({ id: albumId, recipients: allUsersExceptUs }); } @@ -288,10 +292,6 @@ export class AlbumService extends BaseService { const album = await this.findOrFail(id, { withAssets: false }); for (const { userId, role } of albumUsers) { - if (album.ownerId === userId) { - throw new BadRequestException('Cannot be shared with owner'); - } - const exists = album.albumUsers.find(({ user: { id } }) => id === userId); if (exists) { throw new BadRequestException('User already added'); @@ -316,13 +316,13 @@ export class AlbumService extends BaseService { const album = await this.findOrFail(id, { withAssets: false }); - if (album.ownerId === userId) { - throw new BadRequestException('Cannot remove album owner'); + const albumUser = album.albumUsers.find(({ user: { id } }) => id === userId); + if (!albumUser) { + throw new BadRequestException('Album not shared with user'); } - const exists = album.albumUsers.find(({ user: { id } }) => id === userId); - if (!exists) { - throw new BadRequestException('Album not shared with user'); + if (albumUser.role === AlbumUserRole.Owner) { + throw new BadRequestException('Cannot remove album owner'); } // non-admin can remove themselves @@ -335,6 +335,13 @@ export class AlbumService extends BaseService { async updateUser(auth: AuthDto, id: string, userId: string, dto: UpdateAlbumUserDto): Promise { await this.requireAccess({ auth, permission: Permission.AlbumShare, ids: [id] }); + + const album = await this.findOrFail(id, { withAssets: false }); + const albumUser = album.albumUsers.find(({ user: { id } }) => id === userId); + if (albumUser?.role === AlbumUserRole.Owner) { + throw new BadRequestException('Cannot change album owner role'); + } + await this.albumUserRepository.update({ albumId: id, userId }, { role: dto.role }); } diff --git a/server/test/factories/album.factory.ts b/server/test/factories/album.factory.ts index f401cd343d..d3e4ac70a2 100644 --- a/server/test/factories/album.factory.ts +++ b/server/test/factories/album.factory.ts @@ -1,5 +1,5 @@ import { Selectable } from 'kysely'; -import { AssetOrder } from 'src/enum'; +import { AlbumUserRole, AssetOrder } from 'src/enum'; import { AlbumTable } from 'src/schema/tables/album.table'; import { SharedLinkTable } from 'src/schema/tables/shared-link.table'; import { AlbumUserFactory } from 'test/factories/album-user.factory'; @@ -76,11 +76,20 @@ export class AlbumFactory { } build() { + const owner = this.#owner.build(); + const ownerAlbumUser = AlbumUserFactory.from({ + albumId: this.value.id, + userId: owner.id, + role: AlbumUserRole.Owner, + }) + .user(owner) + .build(); + return { ...this.value, - owner: this.#owner.build(), + owner, assets: this.#assets.map((asset) => asset.build()), - albumUsers: this.#albumUsers.map((albumUser) => albumUser.build()), + albumUsers: [ownerAlbumUser, ...this.#albumUsers.map((albumUser) => albumUser.build())], sharedLinks: this.#sharedLinks, }; } diff --git a/server/test/medium/specs/sync/sync-album-user.spec.ts b/server/test/medium/specs/sync/sync-album-user.spec.ts index 4970995d28..22b70c9908 100644 --- a/server/test/medium/specs/sync/sync-album-user.spec.ts +++ b/server/test/medium/specs/sync/sync-album-user.spec.ts @@ -25,6 +25,15 @@ describe(SyncRequestType.AlbumUsersV1, () => { const { albumUser } = await ctx.newAlbumUser({ albumId: album.id, userId: user.id, role: AlbumUserRole.Editor }); await expect(ctx.syncStream(auth, [SyncRequestType.AlbumUsersV1])).resolves.toEqual([ + { + ack: expect.any(String), + data: expect.objectContaining({ + albumId: album.id, + role: AlbumUserRole.Owner, + userId: auth.user.id, + }), + type: SyncEntityType.AlbumUserV1, + }, { ack: expect.any(String), data: expect.objectContaining({ @@ -47,6 +56,10 @@ describe(SyncRequestType.AlbumUsersV1, () => { const response = await ctx.syncStream(auth, [SyncRequestType.AlbumUsersV1]); expect(response).toEqual([ + expect.objectContaining({ + data: expect.objectContaining({ albumId: album.id, userId: auth.user.id, role: AlbumUserRole.Owner }), + type: SyncEntityType.AlbumUserV1, + }), { ack: expect.any(String), data: expect.objectContaining({ @@ -136,6 +149,10 @@ describe(SyncRequestType.AlbumUsersV1, () => { const response = await ctx.syncStream(auth, [SyncRequestType.AlbumUsersV1]); expect(response).toEqual([ + expect.objectContaining({ + data: expect.objectContaining({ albumId: album.id, userId: user1.id, role: AlbumUserRole.Owner }), + type: SyncEntityType.AlbumUserV1, + }), { ack: expect.any(String), data: expect.objectContaining({ @@ -163,6 +180,7 @@ describe(SyncRequestType.AlbumUsersV1, () => { const response = await ctx.syncStream(auth, [SyncRequestType.AlbumUsersV1]); expect(response).toEqual([ + expect.objectContaining({ type: SyncEntityType.AlbumUserV1 }), expect.objectContaining({ type: SyncEntityType.AlbumUserV1 }), expect.objectContaining({ type: SyncEntityType.AlbumUserV1 }), expect.objectContaining({ type: SyncEntityType.SyncCompleteV1 }), @@ -201,6 +219,7 @@ describe(SyncRequestType.AlbumUsersV1, () => { const response = await ctx.syncStream(auth, [SyncRequestType.AlbumUsersV1]); expect(response).toEqual([ + expect.objectContaining({ type: SyncEntityType.AlbumUserV1 }), expect.objectContaining({ type: SyncEntityType.AlbumUserV1 }), expect.objectContaining({ type: SyncEntityType.AlbumUserV1 }), expect.objectContaining({ type: SyncEntityType.SyncCompleteV1 }), @@ -233,8 +252,7 @@ describe(SyncRequestType.AlbumUsersV1, () => { const { user: user2 } = await ctx.newUser(); const { album: album1 } = await ctx.newAlbum({ ownerId: user1.id }); const { album: album2 } = await ctx.newAlbum({ ownerId: user1.id }); - // backfill album user - await ctx.newAlbumUser({ albumId: album1.id, userId: user1.id, role: AlbumUserRole.Editor }); + // owner (user1) is already in album_user from album creation await wait(2); // initial album user await ctx.newAlbumUser({ albumId: album2.id, userId: auth.user.id, role: AlbumUserRole.Editor }); @@ -244,6 +262,10 @@ describe(SyncRequestType.AlbumUsersV1, () => { const response = await ctx.syncStream(auth, [SyncRequestType.AlbumUsersV1]); expect(response).toEqual([ + expect.objectContaining({ + data: expect.objectContaining({ albumId: album2.id, userId: user1.id, role: AlbumUserRole.Owner }), + type: SyncEntityType.AlbumUserV1, + }), { ack: expect.any(String), data: expect.objectContaining({ @@ -261,14 +283,14 @@ describe(SyncRequestType.AlbumUsersV1, () => { // get access to the backfill album user await ctx.newAlbumUser({ albumId: album1.id, userId: auth.user.id, role: AlbumUserRole.Editor }); - // should backfill the album user + // should backfill the album user (owner user1 is already there from album creation) const newResponse = await ctx.syncStream(auth, [SyncRequestType.AlbumUsersV1]); expect(newResponse).toEqual([ { ack: expect.any(String), data: expect.objectContaining({ albumId: album1.id, - role: AlbumUserRole.Editor, + role: AlbumUserRole.Owner, userId: user1.id, }), type: SyncEntityType.AlbumUserBackfillV1,