Merge 8b2737942a into 0cecb5287f

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

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) {

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,

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")
 	}
 

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")
 	}
 

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")
+	})
 }

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"

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.