Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(server): Check album permissions in bulk #5290

Conversation

adamantike
Copy link
Contributor

Modify Access repository, to evaluate album permissions in bulk. Queries have been validated to match what they currently generate for single ids.

Queries:

  • Owner access:
-- Before
SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (
  SELECT 1
  FROM "albums" "AlbumEntity"
  WHERE
    "AlbumEntity"."id" = $1
    AND "AlbumEntity"."ownerId" = $2
    AND "AlbumEntity"."deletedAt" IS NULL
)
LIMIT 1

-- After
SELECT
  "AlbumEntity"."id" AS "AlbumEntity_id"
FROM "albums" "AlbumEntity"
WHERE
  "AlbumEntity"."id" IN ($1, $2)
  AND "AlbumEntity"."ownerId" = $3
  AND "AlbumEntity"."deletedAt" IS NULL
  • Shared link access:
-- Before
SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (
  SELECT 1
  FROM "shared_links" "SharedLinkEntity"
  WHERE
    "SharedLinkEntity"."id" = $1
    AND "SharedLinkEntity"."albumId" = $2
)
LIMIT 1

-- After
SELECT
  "SharedLinkEntity"."albumId" AS "SharedLinkEntity_albumId",
  "SharedLinkEntity"."id" AS "SharedLinkEntity_id"
FROM "shared_links" "SharedLinkEntity"
WHERE
  "SharedLinkEntity"."id" = $1
  AND "SharedLinkEntity"."albumId" IN ($2, $3)
  • Shared album access:
-- Before
SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (
  SELECT 1
  FROM "albums" "AlbumEntity"
    LEFT JOIN "albums_shared_users_users" "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"
      ON "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."albumsId"="AlbumEntity"."id"
    LEFT JOIN "users" "AlbumEntity__AlbumEntity_sharedUsers"
      ON "AlbumEntity__AlbumEntity_sharedUsers"."id"="AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."usersId"
      AND "AlbumEntity__AlbumEntity_sharedUsers"."deletedAt" IS NULL
  WHERE
    "AlbumEntity"."id" = $1
    AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $2
    AND "AlbumEntity"."deletedAt" IS NULL
)
LIMIT 1

-- After
SELECT
  "AlbumEntity"."id" AS "AlbumEntity_id"
FROM "albums" "AlbumEntity"
  LEFT JOIN "albums_shared_users_users" "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"
    ON "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."albumsId"="AlbumEntity"."id"
  LEFT JOIN "users" "AlbumEntity__AlbumEntity_sharedUsers"
    ON "AlbumEntity__AlbumEntity_sharedUsers"."id"="AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."usersId"
    AND "AlbumEntity__AlbumEntity_sharedUsers"."deletedAt" IS NULL
WHERE
  "AlbumEntity"."id" IN ($1, $2)
  AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $3
  AND "AlbumEntity"."deletedAt" IS NULL

Modify Access repository, to evaluate `album` permissions in bulk.
Queries have been validated to match what they currently generate for
single ids.

Queries:

* Owner access:

```sql
-- Before
SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (
  SELECT 1
  FROM "albums" "AlbumEntity"
  WHERE
    "AlbumEntity"."id" = $1
    AND "AlbumEntity"."ownerId" = $2
    AND "AlbumEntity"."deletedAt" IS NULL
)
LIMIT 1

-- After
SELECT
  "AlbumEntity"."id" AS "AlbumEntity_id"
FROM "albums" "AlbumEntity"
WHERE
  "AlbumEntity"."id" IN ($1, $2)
  AND "AlbumEntity"."ownerId" = $3
  AND "AlbumEntity"."deletedAt" IS NULL
```

* Shared link access:

```sql
-- Before
SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (
  SELECT 1
  FROM "shared_links" "SharedLinkEntity"
  WHERE
    "SharedLinkEntity"."id" = $1
    AND "SharedLinkEntity"."albumId" = $2
)
LIMIT 1

-- After
SELECT
  "SharedLinkEntity"."albumId" AS "SharedLinkEntity_albumId",
  "SharedLinkEntity"."id" AS "SharedLinkEntity_id"
FROM "shared_links" "SharedLinkEntity"
WHERE
  "SharedLinkEntity"."id" = $1
  AND "SharedLinkEntity"."albumId" IN ($2, $3)
```

* Shared album access:

```sql
-- Before
SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS (
  SELECT 1
  FROM "albums" "AlbumEntity"
    LEFT JOIN "albums_shared_users_users" "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"
      ON "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."albumsId"="AlbumEntity"."id"
    LEFT JOIN "users" "AlbumEntity__AlbumEntity_sharedUsers"
      ON "AlbumEntity__AlbumEntity_sharedUsers"."id"="AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."usersId"
      AND "AlbumEntity__AlbumEntity_sharedUsers"."deletedAt" IS NULL
  WHERE
    "AlbumEntity"."id" = $1
    AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $2
    AND "AlbumEntity"."deletedAt" IS NULL
)
LIMIT 1

-- After
SELECT
  "AlbumEntity"."id" AS "AlbumEntity_id"
FROM "albums" "AlbumEntity"
  LEFT JOIN "albums_shared_users_users" "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"
    ON "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."albumsId"="AlbumEntity"."id"
  LEFT JOIN "users" "AlbumEntity__AlbumEntity_sharedUsers"
    ON "AlbumEntity__AlbumEntity_sharedUsers"."id"="AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."usersId"
    AND "AlbumEntity__AlbumEntity_sharedUsers"."deletedAt" IS NULL
WHERE
  "AlbumEntity"."id" IN ($1, $2)
  AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $3
  AND "AlbumEntity"."deletedAt" IS NULL
```
Comment on lines 159 to 160
const isOwner = await this.repository.album.checkOwnerAccess(authUser.id, ids);
const isShared = await this.repository.album.checkSharedAlbumAccess(authUser.id, ids);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always does two calls even if all the ids are valid from the first request. Since most calls are for a single, owned album, that's the primary scenario.

Can we either abstract this so it can return early if all the IDs are found sooner or update the database query to combine the checks into a single SQL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I prefer to maintain this simpler API instead of starting to add more complex queries. That's the main reason I'm splitting these changes in multiple PRs, to validate that the queries match and we don't introduce any security issues.

I have added some set utils, and we now call the second check with the ids that weren't already approved by the first one :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh perfect!

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, we're still exercising two queries for albums I think, even if there are zero records. Other than that it looks good.

server/src/domain/access/access.core.ts Outdated Show resolved Hide resolved
server/src/domain/album/album.service.ts Outdated Show resolved Hide resolved
server/src/domain/domain.util.ts Outdated Show resolved Hide resolved
server/src/infra/repositories/access.repository.ts Outdated Show resolved Hide resolved
server/src/domain/access/access.core.ts Show resolved Hide resolved
@jrasm91 jrasm91 merged commit 6d1b325 into immich-app:main Nov 25, 2023
18 checks passed
@adamantike adamantike deleted the chore/server/check-album-permissions-in-bulk branch November 26, 2023 00:59
@bo0tzz bo0tzz mentioned this pull request Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants