diff --git a/internal/api/photos_search.go b/internal/api/photos_search.go index c678cec1b..758106033 100644 --- a/internal/api/photos_search.go +++ b/internal/api/photos_search.go @@ -39,11 +39,12 @@ func searchPhotosForm(c *gin.Context) (frm form.SearchPhotos, s *entity.Session, frm.Public = false } - // Ignore private flag if feature is disabled. + // Force Review off for non manager/admin users via Quality if frm.Scope == "" && settings.Features.Review && acl.Rules.Deny(acl.ResourcePhotos, s.GetUserRole(), acl.ActionManage) { frm.Quality = 3 + frm.NonManager = true // Note that a non manager was encountered to enable restricted users to see all their Private including review level photos. } return frm, s, nil diff --git a/internal/api/photos_search_test.go b/internal/api/photos_search_test.go index dc472ac4f..fc528ed8d 100644 --- a/internal/api/photos_search_test.go +++ b/internal/api/photos_search_test.go @@ -36,6 +36,21 @@ func TestSearchPhotos(t *testing.T) { result := PerformRequest(app, "GET", "/api/v1/photos?xxx=10") assert.Equal(t, http.StatusBadRequest, result.Code) }) + t.Run("PrivateQualityQueryTo4", func(t *testing.T) { + app, router, _ := NewApiTest() + SearchPhotos(router) + r := PerformRequest(app, "GET", "/api/v1/photos?count=156&offset=0&merged=true&country=&camera=0&lens=0&label=&latlng=&year=0&month=0&color=&order=added&reverse=false&q=quality:4&public=&quality=3&private=true") + body := r.Body.String() + + count := gjson.Get(body, "#") + assert.LessOrEqual(t, int64(1), count.Int()) + assert.Equal(t, http.StatusOK, r.Code) + qualities := gjson.Get(body, "#.Quality") + assert.GreaterOrEqual(t, len(qualities.Array()), 1) + for _, quality := range qualities.Array() { + assert.GreaterOrEqual(t, quality.Int(), int64(4)) + } + }) } func TestSearchPhotosView(t *testing.T) { diff --git a/internal/entity/photo_fixtures.go b/internal/entity/photo_fixtures.go index 6f34d55de..19a089f26 100644 --- a/internal/entity/photo_fixtures.go +++ b/internal/entity/photo_fixtures.go @@ -3779,7 +3779,7 @@ var PhotoFixtures = PhotoMap{ PhotoExposure: "1/40", PhotoFocalLength: 105, PhotoFNumber: 3.5, - PhotoQuality: 3, + PhotoQuality: 4, // Update to 4 to support quality tests of private photos PhotoResolution: 16, Camera: CameraFixtures.Pointer("apple-iphone-se"), CameraID: CameraFixtures.Pointer("apple-iphone-se").ID, diff --git a/internal/entity/search/photos.go b/internal/entity/search/photos.go index ff1a5639f..71edf24b3 100644 --- a/internal/entity/search/photos.go +++ b/internal/entity/search/photos.go @@ -486,15 +486,15 @@ func searchPhotos(frm form.SearchPhotos, sess *entity.Session, resultCols string if frm.Review { s = s.Where("photos.photo_quality < 3") - } else if frm.Quality != 0 && frm.Private == false { + } else if frm.Quality != 0 && ((frm.NonManager && !frm.Private) || !frm.NonManager) { s = s.Where("photos.photo_quality >= ?", frm.Quality) } } // Filter private pictures. - if frm.Public { + if frm.Public && !frm.Private { s = s.Where("photos.photo_private = 0") - } else if frm.Private { + } else if !frm.Public && frm.Private { s = s.Where("photos.photo_private = 1") } diff --git a/internal/entity/search/photos_geo.go b/internal/entity/search/photos_geo.go index 183d6d30c..c11ac93ce 100644 --- a/internal/entity/search/photos_geo.go +++ b/internal/entity/search/photos_geo.go @@ -620,15 +620,15 @@ func UserPhotosGeo(frm form.SearchPhotosGeo, sess *entity.Session) (results GeoR if frm.Review { s = s.Where("photos.photo_quality < 3") - } else if frm.Quality != 0 && frm.Private == false { + } else if frm.Quality != 0 { s = s.Where("photos.photo_quality >= ?", frm.Quality) } } // Filter private pictures. - if frm.Public { + if frm.Public && !frm.Private { s = s.Where("photos.photo_private = 0") - } else if frm.Private { + } else if !frm.Public && frm.Private { s = s.Where("photos.photo_private = 1") } diff --git a/internal/entity/search/photos_geo_test.go b/internal/entity/search/photos_geo_test.go index 7d0e16d83..ac193c9b3 100644 --- a/internal/entity/search/photos_geo_test.go +++ b/internal/entity/search/photos_geo_test.go @@ -1266,4 +1266,61 @@ func TestGeo(t *testing.T) { assert.True(t, foundRight) }) + t.Run("SearchForPublicAndPrivate", func(t *testing.T) { + var f form.SearchPhotosGeo + + f.Query = "" + f.Count = 5000 + f.Offset = 0 + f.Public = true + f.Private = true + + photos, err := PhotosGeo(f) + + if err != nil { + t.Fatal(err) + } + + assert.LessOrEqual(t, 3, len(photos)) + count := 0 + for _, r := range photos { + assert.IsType(t, GeoResult{}, r) + p := entity.FindPhoto(entity.Photo{PhotoUID: r.PhotoUID}) + if p.PhotoPrivate { + count++ + } + } + assert.LessOrEqual(t, 1, count, "No private photos returned") + }) + t.Run("SearchForPrivateQuality", func(t *testing.T) { + var f form.SearchPhotosGeo + + f.Query = "" + f.Count = 5000 + f.Offset = 0 + f.Quality = 3 + f.Private = true + + photos, err := PhotosGeo(f) + + if err != nil { + t.Fatal(err) + } + + assert.LessOrEqual(t, 1, len(photos)) + privateCount := 0 + publicCount := 0 + for _, r := range photos { + assert.IsType(t, GeoResult{}, r) + p := entity.FindPhoto(entity.Photo{PhotoUID: r.PhotoUID}) + assert.LessOrEqual(t, 3, p.PhotoQuality, "PhotoUID "+p.PhotoUID) + if p.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + } + assert.LessOrEqual(t, 1, privateCount, "No private photos returned") + assert.Equal(t, 0, publicCount, "Public photos returned") + }) } diff --git a/internal/entity/search/photos_test.go b/internal/entity/search/photos_test.go index 656be1363..25113a378 100644 --- a/internal/entity/search/photos_test.go +++ b/internal/entity/search/photos_test.go @@ -283,6 +283,18 @@ func TestPhotos(t *testing.T) { t.Fatal(err) } assert.LessOrEqual(t, 1, len(photos)) + privateCount := 0 + publicCount := 0 + for _, photo := range photos { + assert.LessOrEqual(t, 3, photo.PhotoQuality, "PhotoUID "+photo.PhotoUID) + if photo.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + } + assert.LessOrEqual(t, 1, privateCount, "No private photos returned") + assert.Equal(t, 0, publicCount, "Public photos returned") }) t.Run("SearchForPublic", func(t *testing.T) { var f form.SearchPhotos @@ -298,6 +310,81 @@ func TestPhotos(t *testing.T) { t.Fatal(err) } assert.LessOrEqual(t, 3, len(photos)) + privateCount := 0 + publicCount := 0 + for _, photo := range photos { + if photo.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + } + assert.Equal(t, 0, privateCount, "Private photos returned") + assert.LessOrEqual(t, 3, publicCount, "No public photos returned") + }) + t.Run("SearchForPublicAndPrivateNonReview", func(t *testing.T) { + // This test is to confirm that it is possible to retrieve all the non-review photos in a single request + var f form.SearchPhotos + + f.Query = "" + f.Count = 5000 + f.Offset = 0 + f.Public = true + f.Private = true + f.Quality = 3 + + photos, _, err := Photos(f) + + if err != nil { + t.Fatal(err) + } + assert.LessOrEqual(t, 3, len(photos)) + privateCount, publicCount, qualityLessThan3 := 0, 0, 0 + for _, photo := range photos { + if photo.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + if photo.PhotoQuality < 3 { + qualityLessThan3++ + } + } + assert.LessOrEqual(t, 1, privateCount, "No private photos returned") + assert.LessOrEqual(t, 1, publicCount, "No public photos returned") + assert.Equal(t, 0, qualityLessThan3) + }) + t.Run("SearchForPublicAndPrivateReview", func(t *testing.T) { + // This test is to confirm that it is possible to retrieve all the photos in a single request (review or otherwise) + var f form.SearchPhotos + + f.Query = "" + f.Count = 5000 + f.Offset = 0 + f.Public = true + f.Private = true + f.Quality = 0 + + photos, _, err := Photos(f) + + if err != nil { + t.Fatal(err) + } + assert.LessOrEqual(t, 3, len(photos)) + privateCount, publicCount, qualityLessThan3 := 0, 0, 0 + for _, photo := range photos { + if photo.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + if photo.PhotoQuality < 3 { + qualityLessThan3++ + } + } + assert.LessOrEqual(t, 1, privateCount, "No private photos returned") + assert.LessOrEqual(t, 1, publicCount, "No public photos returned") + assert.LessOrEqual(t, 1, qualityLessThan3, "No review photos returned") }) t.Run("SearchForReview", func(t *testing.T) { var f form.SearchPhotos @@ -329,6 +416,116 @@ func TestPhotos(t *testing.T) { t.Fatal(err) } assert.LessOrEqual(t, 1, len(photos)) + for _, photo := range photos { + assert.LessOrEqual(t, 3, photo.PhotoQuality, "PhotoUID "+photo.PhotoUID) + } + }) + t.Run("SearchForPrivateQuality", func(t *testing.T) { + var f form.SearchPhotos + + f.Query = "quality:4" + f.Count = 5000 + f.Offset = 0 + f.Quality = 3 + f.Private = true + + photos, _, err := Photos(f) + + if err != nil { + t.Fatal(err) + } + assert.LessOrEqual(t, 1, len(photos)) + privateCount := 0 + publicCount := 0 + for _, photo := range photos { + assert.GreaterOrEqual(t, photo.PhotoQuality, 4, "PhotoUID "+photo.PhotoUID) + if photo.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + } + assert.LessOrEqual(t, 1, privateCount, "No private photos returned") + assert.Equal(t, 0, publicCount, "Public photos returned") + }) + t.Run("NonManagerPrivateQuality", func(t *testing.T) { + // Can not test this via API as there is no ACL that supports Private that is not Admin. + // This tests if a User who is not a Manager or Admin level role + // can request a list of private photos with a Quality score. + // This is not permitted, as it prevents non manager/admins from + // seeing their private non reviewed photos. + // See https://github.com/photoprism/photoprism/pull/5651#issuecomment-4705854230 + var f form.SearchPhotos + + f.Query = "quality:4" + f.Count = 5000 + f.Offset = 0 + f.Quality = 3 // pushed in from API via searchPhotosForm + f.Private = true + f.Public = false + f.NonManager = true // pushed in from API via searchPhotosForm + + photos, _, err := Photos(f) + + if err != nil { + t.Fatal(err) + } + assert.LessOrEqual(t, 1, len(photos)) + privateCount := 0 + publicCount := 0 + qualityLessThan4 := 0 + for _, photo := range photos { + if photo.PhotoQuality < 4 { + qualityLessThan4++ + } + if photo.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + } + assert.GreaterOrEqual(t, qualityLessThan4, 1) // Make sure that the Quality is IGNORED + assert.LessOrEqual(t, 1, privateCount, "No private photos returned") + assert.Equal(t, 0, publicCount, "Public photos returned") + }) + t.Run("NonManagerPublicQuality", func(t *testing.T) { + // Can not test this via API as there is no ACL that supports Private that is not Admin. + // This tests if a User who is not a Manager or Admin level role + // can request a list of public photos with a Quality score. + // This is permitted. + // See https://github.com/photoprism/photoprism/pull/5651#issuecomment-4705854230 + var f form.SearchPhotos + + f.Query = "quality:4" // Should override the Parameter + f.Count = 5000 + f.Offset = 0 + f.Quality = 3 // pushed in from API via searchPhotosForm + f.Private = false + f.Public = true + f.NonManager = true // pushed in from API via searchPhotosForm + + photos, _, err := Photos(f) + + if err != nil { + t.Fatal(err) + } + assert.LessOrEqual(t, 1, len(photos)) + privateCount := 0 + publicCount := 0 + qualityLessThan4 := 0 + for _, photo := range photos { + if photo.PhotoQuality < 4 { + qualityLessThan4++ + } + if photo.PhotoPrivate { + privateCount++ + } else { + publicCount++ + } + } + assert.Equal(t, 0, qualityLessThan4) // Make sure that the Quality is NOT ignored + assert.Equal(t, 0, privateCount, "Private photos returned") + assert.LessOrEqual(t, 30, publicCount, "Public photos returned") }) t.Run("SearchForFileError", func(t *testing.T) { var f form.SearchPhotos @@ -2416,7 +2613,7 @@ func TestPhotos(t *testing.T) { assert.NotEmpty(t, p.PhotoName) } }) - t.Run("NamePhotoNum41", func(t *testing.T) { + t.Run("QueryNamePhotoNum41", func(t *testing.T) { var f form.SearchPhotos f.Query = "name:photo\\|41" diff --git a/internal/form/search_photos.go b/internal/form/search_photos.go index 779f748fd..2a914d2a2 100644 --- a/internal/form/search_photos.go +++ b/internal/form/search_photos.go @@ -101,6 +101,7 @@ type SearchPhotos struct { Reverse bool `form:"reverse" serialize:"-"` // Merge FILES in response Merged bool `form:"merged" serialize:"-"` // Merge FILES in response Details bool `form:"-" serialize:"-"` // Include additional information from details table + NonManager bool `form:"-" serialize:"-"` // Indicates that this search needs special handling of Quality } // GetQuery returns the current search query string.