feat: readonly album sharing (#8720)

* rename albums_shared_users_users to album_permissions and add readonly column

* disable synchronize on the original join table

* remove unnecessary FK names

* set readonly=true as default for new album shares

* separate and implement album READ and WRITE permission

* expose albumPermissions on the API, deprecate sharedUsers

* generate openapi

* create readonly view on frontend

* ??? move slideshow button out from ellipsis menu so that non-owners can have access too

* correct sharedUsers joins

* add album permission repository

* remove a log

* fix assetCount getting reset when adding users

* fix lint

* add set permission endpoint and UI

* sort users

* remove log

* Revert "??? move slideshow button out from ellipsis menu so that non-owners can have access too"

This reverts commit 1343bfa311.

* rename stuff

* fix db schema annotations

* sql generate

* change readonly default to follow migration

* fix deprecation notice

* change readonly boolean to role enum

* fix joincolumn as primary key

* rename albumUserRepository in album service

* clean up userId and albumId

* add write access to shared link

* fix existing tests

* switch to vitest

* format and fix tests on web

* add new test

* fix one e2e test

* rename new API field to albumUsers

* capitalize serverside enum

* remove unused ReadWrite type

* missed rename from previous commit

* rename to albumUsers in album entity as well

* remove outdated Equals calls

* unnecessary relation

* rename to updateUser in album service

* minor renamery

* move sorting to backend

* rename and separate ALBUM_WRITE as ADD_ASSET and REMOVE_ASSET

* fix tests

* fix "should migrate single moving picture" test failing on European system timezone

* generated changes after merge

* lint fix

* fix correct page to open after removing user from album

* fix e2e tests and some bugs

* rename updateAlbumUser rest endpoint

* add new e2e tests for updateAlbumUser endpoint

* small optimizations

* refactor album e2e test, add new album shared with viewer

* add new test to check if viewer can see the album

* add new e2e tests for readonly share

* failing test: User delete doesn't cascade to UserAlbum entity

* fix: handle deleted users

* use lodash for sort

* add role to addUsersToAlbum endpoint

* add UI for adding editors

* lint fixes

* change role back to editor as DB default

* fix server tests

* redesign user selection modal editor selector

* style tweaks

* fix type error

* Revert "style tweaks"

This reverts commit ab604f4c8f.

* Revert "redesign user selection modal editor selector"

This reverts commit e6f344856c.

* chore: cleanup and improve add user modal

* chore: open api

* small styling

---------

Co-authored-by: mgabor <>
Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
Co-authored-by: Alex Tran <alex.tran1502@gmail.com>

server/src/services/album.service.spec.ts

@@ -1,6 +1,8 @@
 import { BadRequestException } from '@nestjs/common';
 import _ from 'lodash';
 import { BulkIdErrorReason } from 'src/dtos/asset-ids.response.dto';
+import { AlbumUserRole } from 'src/entities/album-user.entity';
+import { IAlbumUserRepository } from 'src/interfaces/album-user.interface';
 import { IAlbumRepository } from 'src/interfaces/album.interface';
 import { IAssetRepository } from 'src/interfaces/asset.interface';
 import { IUserRepository } from 'src/interfaces/user.interface';
@@ -9,6 +11,7 @@ import { albumStub } from 'test/fixtures/album.stub';
 import { authStub } from 'test/fixtures/auth.stub';
 import { userStub } from 'test/fixtures/user.stub';
 import { IAccessRepositoryMock, newAccessRepositoryMock } from 'test/repositories/access.repository.mock';
+import { newAlbumUserRepositoryMock } from 'test/repositories/album-user.repository.mock';
 import { newAlbumRepositoryMock } from 'test/repositories/album.repository.mock';
 import { newAssetRepositoryMock } from 'test/repositories/asset.repository.mock';
 import { newUserRepositoryMock } from 'test/repositories/user.repository.mock';
@@ -20,14 +23,16 @@ describe(AlbumService.name, () => {
   let albumMock: Mocked<IAlbumRepository>;
   let assetMock: Mocked<IAssetRepository>;
   let userMock: Mocked<IUserRepository>;
+  let albumUserMock: Mocked<IAlbumUserRepository>;
 
   beforeEach(() => {
     accessMock = newAccessRepositoryMock();
     albumMock = newAlbumRepositoryMock();
     assetMock = newAssetRepositoryMock();
     userMock = newUserRepositoryMock();
+    albumUserMock = newAlbumUserRepositoryMock();
 
-    sut = new AlbumService(accessMock, albumMock, assetMock, userMock);
+    sut = new AlbumService(accessMock, albumMock, assetMock, userMock, albumUserMock);
   });
 
   it('should work', () => {
@@ -189,7 +194,7 @@ describe(AlbumService.name, () => {
         ownerId: authStub.admin.user.id,
         albumName: albumStub.empty.albumName,
         description: albumStub.empty.description,
-        sharedUsers: [{ id: 'user-id' }],
+        albumUsers: [{ user: { id: 'user-id' } }],
         assets: [{ id: '123' }],
         albumThumbnailAssetId: '123',
       });
@@ -225,7 +230,7 @@ describe(AlbumService.name, () => {
         ownerId: authStub.admin.user.id,
         albumName: 'Test album',
         description: '',
-        sharedUsers: [],
+        albumUsers: [],
         assets: [{ id: 'asset-1' }],
         albumThumbnailAssetId: 'asset-1',
       });
@@ -327,7 +332,7 @@ describe(AlbumService.name, () => {
   describe('addUsers', () => {
     it('should throw an error if the auth user is not the owner', async () => {
       await expect(
-        sut.addUsers(authStub.admin, albumStub.sharedWithAdmin.id, { sharedUserIds: ['user-1'] }),
+        sut.addUsers(authStub.admin, albumStub.sharedWithAdmin.id, { albumUsers: [{ userId: 'user-1' }] }),
       ).rejects.toBeInstanceOf(BadRequestException);
       expect(albumMock.update).not.toHaveBeenCalled();
     });
@@ -336,7 +341,9 @@ describe(AlbumService.name, () => {
       accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.sharedWithAdmin.id]));
       albumMock.getById.mockResolvedValue(albumStub.sharedWithAdmin);
       await expect(
-        sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: [authStub.admin.user.id] }),
+        sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, {
+          albumUsers: [{ userId: authStub.admin.user.id }],
+        }),
       ).rejects.toBeInstanceOf(BadRequestException);
       expect(albumMock.update).not.toHaveBeenCalled();
     });
@@ -346,7 +353,7 @@ describe(AlbumService.name, () => {
       albumMock.getById.mockResolvedValue(albumStub.sharedWithAdmin);
       userMock.get.mockResolvedValue(null);
       await expect(
-        sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: ['user-3'] }),
+        sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { albumUsers: [{ userId: 'user-3' }] }),
       ).rejects.toBeInstanceOf(BadRequestException);
       expect(albumMock.update).not.toHaveBeenCalled();
     });
@@ -356,11 +363,19 @@ describe(AlbumService.name, () => {
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.sharedWithAdmin));
       albumMock.update.mockResolvedValue(albumStub.sharedWithAdmin);
       userMock.get.mockResolvedValue(userStub.user2);
-      await sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: [authStub.user2.user.id] });
-      expect(albumMock.update).toHaveBeenCalledWith({
-        id: albumStub.sharedWithAdmin.id,
-        updatedAt: expect.any(Date),
-        sharedUsers: [userStub.admin, { id: authStub.user2.user.id }],
+      albumUserMock.create.mockResolvedValue({
+        userId: userStub.user2.id,
+        user: userStub.user2,
+        albumId: albumStub.sharedWithAdmin.id,
+        album: albumStub.sharedWithAdmin,
+        role: AlbumUserRole.EDITOR,
+      });
+      await sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, {
+        albumUsers: [{ userId: authStub.user2.user.id }],
+      });
+      expect(albumUserMock.create).toHaveBeenCalledWith({
+        userId: authStub.user2.user.id,
+        albumId: albumStub.sharedWithAdmin.id,
       });
     });
   });
@@ -381,11 +396,10 @@ describe(AlbumService.name, () => {
         sut.removeUser(authStub.admin, albumStub.sharedWithUser.id, userStub.user1.id),
       ).resolves.toBeUndefined();
 
-      expect(albumMock.update).toHaveBeenCalledTimes(1);
-      expect(albumMock.update).toHaveBeenCalledWith({
-        id: albumStub.sharedWithUser.id,
-        updatedAt: expect.any(Date),
-        sharedUsers: [],
+      expect(albumUserMock.delete).toHaveBeenCalledTimes(1);
+      expect(albumUserMock.delete).toHaveBeenCalledWith({
+        albumId: albumStub.sharedWithUser.id,
+        userId: userStub.user1.id,
       });
       expect(albumMock.getById).toHaveBeenCalledWith(albumStub.sharedWithUser.id, { withAssets: false });
     });
@@ -397,7 +411,7 @@ describe(AlbumService.name, () => {
         sut.removeUser(authStub.user1, albumStub.sharedWithMultiple.id, authStub.user2.user.id),
       ).rejects.toBeInstanceOf(BadRequestException);
 
-      expect(albumMock.update).not.toHaveBeenCalled();
+      expect(albumUserMock.delete).not.toHaveBeenCalled();
       expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith(
         authStub.user1.user.id,
         new Set([albumStub.sharedWithMultiple.id]),
@@ -409,11 +423,10 @@ describe(AlbumService.name, () => {
 
       await sut.removeUser(authStub.user1, albumStub.sharedWithUser.id, authStub.user1.user.id);
 
-      expect(albumMock.update).toHaveBeenCalledTimes(1);
-      expect(albumMock.update).toHaveBeenCalledWith({
-        id: albumStub.sharedWithUser.id,
-        updatedAt: expect.any(Date),
-        sharedUsers: [],
+      expect(albumUserMock.delete).toHaveBeenCalledTimes(1);
+      expect(albumUserMock.delete).toHaveBeenCalledWith({
+        albumId: albumStub.sharedWithUser.id,
+        userId: authStub.user1.user.id,
       });
     });
 
@@ -422,11 +435,10 @@ describe(AlbumService.name, () => {
 
       await sut.removeUser(authStub.user1, albumStub.sharedWithUser.id, 'me');
 
-      expect(albumMock.update).toHaveBeenCalledTimes(1);
-      expect(albumMock.update).toHaveBeenCalledWith({
-        id: albumStub.sharedWithUser.id,
-        updatedAt: expect.any(Date),
-        sharedUsers: [],
+      expect(albumUserMock.delete).toHaveBeenCalledTimes(1);
+      expect(albumUserMock.delete).toHaveBeenCalledWith({
+        albumId: albumStub.sharedWithUser.id,
+        userId: authStub.user1.user.id,
       });
     });
 
@@ -512,6 +524,7 @@ describe(AlbumService.name, () => {
       expect(accessMock.album.checkSharedAlbumAccess).toHaveBeenCalledWith(
         authStub.user1.user.id,
         new Set(['album-123']),
+        AlbumUserRole.VIEWER,
       );
     });
 
@@ -522,6 +535,7 @@ describe(AlbumService.name, () => {
       expect(accessMock.album.checkSharedAlbumAccess).toHaveBeenCalledWith(
         authStub.admin.user.id,
         new Set(['album-123']),
+        AlbumUserRole.VIEWER,
       );
     });
   });
@@ -589,6 +603,17 @@ describe(AlbumService.name, () => {
       expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']);
     });
 
+    it('should not allow a shared user with viewer access to add assets', async () => {
+      accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set([]));
+      albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.sharedWithUser));
+
+      await expect(
+        sut.addAssets(authStub.user2, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }),
+      ).rejects.toBeInstanceOf(BadRequestException);
+
+      expect(albumMock.update).not.toHaveBeenCalled();
+    });
+
     it('should allow a shared link user to add assets', async () => {
       accessMock.album.checkSharedLinkAccess.mockResolvedValue(new Set(['album-123']));
       accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
@@ -709,7 +734,7 @@ describe(AlbumService.name, () => {
       expect(albumMock.update).not.toHaveBeenCalled();
     });
 
-    it('should skip assets without user permission to remove', async () => {
+    it('should skip assets when user has remove permission on album but not on asset', async () => {
       accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123']));
       albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
       albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id']));

server/src/services/album.service.ts

@@ -14,10 +14,11 @@ import {
 } from 'src/dtos/album.dto';
 import { BulkIdResponseDto, BulkIdsDto } from 'src/dtos/asset-ids.response.dto';
 import { AuthDto } from 'src/dtos/auth.dto';
+import { AlbumUserEntity, AlbumUserRole } from 'src/entities/album-user.entity';
 import { AlbumEntity } from 'src/entities/album.entity';
 import { AssetEntity } from 'src/entities/asset.entity';
-import { UserEntity } from 'src/entities/user.entity';
 import { IAccessRepository } from 'src/interfaces/access.interface';
+import { IAlbumUserRepository } from 'src/interfaces/album-user.interface';
 import { AlbumAssetCount, AlbumInfoOptions, IAlbumRepository } from 'src/interfaces/album.interface';
 import { IAssetRepository } from 'src/interfaces/asset.interface';
 import { IUserRepository } from 'src/interfaces/user.interface';
@@ -31,6 +32,7 @@ export class AlbumService {
     @Inject(IAlbumRepository) private albumRepository: IAlbumRepository,
     @Inject(IAssetRepository) private assetRepository: IAssetRepository,
     @Inject(IUserRepository) private userRepository: IUserRepository,
+    @Inject(IAlbumUserRepository) private albumUserRepository: IAlbumUserRepository,
   ) {
     this.access = AccessCore.create(accessRepository);
   }
@@ -126,7 +128,7 @@ export class AlbumService {
       ownerId: auth.user.id,
       albumName: dto.albumName,
       description: dto.description,
-      sharedUsers: dto.sharedWithUserIds?.map((value) => ({ id: value }) as UserEntity) ?? [],
+      albumUsers: dto.sharedWithUserIds?.map((userId) => ({ user: { id: userId } }) as AlbumUserEntity) ?? [],
       assets,
       albumThumbnailAssetId: assets[0]?.id || null,
     });
@@ -167,7 +169,7 @@ export class AlbumService {
 
   async addAssets(auth: AuthDto, id: string, dto: BulkIdsDto): Promise<BulkIdResponseDto[]> {
     const album = await this.findOrFail(id, { withAssets: false });
-    await this.access.requirePermission(auth, Permission.ALBUM_READ, id);
+    await this.access.requirePermission(auth, Permission.ALBUM_ADD_ASSET, id);
 
     const results = await addAssets(
       auth,
@@ -190,7 +192,7 @@ export class AlbumService {
   async removeAssets(auth: AuthDto, id: string, dto: BulkIdsDto): Promise<BulkIdResponseDto[]> {
     const album = await this.findOrFail(id, { withAssets: false });
 
-    await this.access.requirePermission(auth, Permission.ALBUM_READ, id);
+    await this.access.requirePermission(auth, Permission.ALBUM_REMOVE_ASSET, id);
 
     const results = await removeAssets(
       auth,
@@ -209,17 +211,25 @@ export class AlbumService {
     return results;
   }
 
-  async addUsers(auth: AuthDto, id: string, dto: AddUsersDto): Promise<AlbumResponseDto> {
+  async addUsers(auth: AuthDto, id: string, { albumUsers, sharedUserIds }: AddUsersDto): Promise<AlbumResponseDto> {
+    // Remove once deprecated sharedUserIds is removed
+    if (!albumUsers) {
+      if (!sharedUserIds) {
+        throw new BadRequestException('No users provided');
+      }
+      albumUsers = sharedUserIds.map((userId) => ({ userId, role: AlbumUserRole.EDITOR }));
+    }
+
     await this.access.requirePermission(auth, Permission.ALBUM_SHARE, id);
 
     const album = await this.findOrFail(id, { withAssets: false });
 
-    for (const userId of dto.sharedUserIds) {
+    for (const { userId, role } of albumUsers) {
       if (album.ownerId === userId) {
         throw new BadRequestException('Cannot be shared with owner');
       }
 
-      const exists = album.sharedUsers.find((user) => user.id === userId);
+      const exists = album.albumUsers.find(({ user: { id } }) => id === userId);
       if (exists) {
         throw new BadRequestException('User already added');
       }
@@ -229,16 +239,10 @@ export class AlbumService {
         throw new BadRequestException('User not found');
       }
 
-      album.sharedUsers.push({ id: userId } as UserEntity);
+      await this.albumUserRepository.create({ userId: userId, albumId: id, role });
     }
 
-    return this.albumRepository
-      .update({
-        id: album.id,
-        updatedAt: new Date(),
-        sharedUsers: album.sharedUsers,
-      })
-      .then(mapAlbumWithoutAssets);
+    return this.findOrFail(id, { withAssets: true }).then(mapAlbumWithoutAssets);
   }
 
   async removeUser(auth: AuthDto, id: string, userId: string | 'me'): Promise<void> {
@@ -252,7 +256,7 @@ export class AlbumService {
       throw new BadRequestException('Cannot remove album owner');
     }
 
-    const exists = album.sharedUsers.find((user) => user.id === userId);
+    const exists = album.albumUsers.find(({ user: { id } }) => id === userId);
     if (!exists) {
       throw new BadRequestException('Album not shared with user');
     }
@@ -262,11 +266,13 @@ export class AlbumService {
       await this.access.requirePermission(auth, Permission.ALBUM_SHARE, id);
     }
 
-    await this.albumRepository.update({
-      id: album.id,
-      updatedAt: new Date(),
-      sharedUsers: album.sharedUsers.filter((user) => user.id !== userId),
-    });
+    await this.albumUserRepository.delete({ albumId: id, userId });
+  }
+
+  async updateUser(auth: AuthDto, id: string, userId: string, dto: Partial<AlbumUserEntity>): Promise<void> {
+    await this.access.requirePermission(auth, Permission.ALBUM_SHARE, id);
+
+    await this.albumUserRepository.update({ albumId: id, userId }, { role: dto.role });
   }
 
   private async findOrFail(id: string, options: AlbumInfoOptions) {