Updated fix to use permissions based 403 instead of 404

Author: bhanurp

artifactory_test.go

@@ -2801,146 +2801,89 @@ func TestArtifactoryDeleteByProps(t *testing.T) {
 	cleanArtifactoryTest()
 }
 
-// TestArtifactoryDeleteCountsNonExistentFiles tests that delete correctly handles
-// files that don't exist. The CLI searches first, so non-existent files return
-// All 3 files should return 404 on second delete attempt.
-// TestArtifactoryDeleteCountsNonExistentFiles tests that delete correctly counts
-// 404 failures when all files have already been deleted.
-// Files are placed in separate subfolders to prevent path reduction optimization.
-func TestArtifactoryDeleteCountsNonExistentFiles(t *testing.T) {
-	initArtifactoryTest(t, "")
-
-	// Step 1: Upload 3 specific test files in SEPARATE subfolders to prevent path reduction
-	// Each file goes in its own subfolder so delete service can't optimize to folder delete
-	testFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in"}
-	sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in"}
-	for i, f := range testFiles {
-		runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-nonexistent/"+f)
-	}
-	t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles)
-
-	// Step 2: Delete each file using direct DELETE API and verify success (HTTP 204)
-	for _, f := range testFiles {
-		artifactPath := tests.RtRepo1 + "/delete-nonexistent/" + f
-		success, err := tests.DeleteFileDirect(serverDetails, artifactPath)
-		assert.NoError(t, err, "First delete of %s should not return error", f)
-		assert.True(t, success, "First delete of %s should succeed (HTTP 204)", f)
-	}
-	t.Log("First delete: all files deleted successfully (HTTP 204)")
-
-	// Step 3: Verify all files are now gone via search
-	searchSpec := spec.NewBuilder().
-		Pattern(tests.RtRepo1 + "/delete-nonexistent/*").
-		Recursive(true).
-		BuildSpec()
-	searchCmd := generic.NewSearchCommand()
-	searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec)
-	reader, err := searchCmd.Search()
-	assert.NoError(t, err)
-	remainingFiles, err := reader.Length()
-	assert.NoError(t, err)
-	readerCloseAndAssert(t, reader)
-	assert.Equal(t, 0, remainingFiles, "All files should be deleted")
-	t.Log("Verified all files are deleted")
+// TestArtifactoryDeleteCountsPartialSuccess tests that delete correctly handles
+// a mix of successful and failed deletions using CLI's DeleteFiles API.
+// Uses permission-based failures (403) to reliably test partial success scenarios.
+// - Files in RtRepo1: user has delete permission → success (204)
+// - Files in RtRepo2: user has NO delete permission → failure (403)
+func TestArtifactoryDeleteCountsPartialSuccess(t *testing.T) {
+	initArtifactoryTest(t, "")
 
-	// Step 4: Try to delete ALL files AGAIN using CLI's DeleteFiles API
-	// Build list of artifact paths
-	var artifactPaths []string
-	for _, f := range testFiles {
-		artifactPaths = append(artifactPaths, tests.RtRepo1+"/delete-nonexistent/"+f)
-	}
-	t.Logf("Attempting second delete of paths: %v", artifactPaths)
+	// Test user credentials
+	testUser := "delete-test-user"
+	testPassword := "DeleteTest123!"
 
-	// Use CLI's DeleteFiles API which should properly count 404s as failures
-	// Error IS expected because all files return 404
-	totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(serverDetails, artifactPaths)
-	// Error is expected when files don't exist (404 responses)
-	t.Logf("Second delete result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err)
+	// Step 1: Create a test user
+	err := tests.CreateUserWithPassword(serverDetails, testUser, testPassword)
+	assert.NoError(t, err, "Failed to create test user")
+	t.Logf("Created test user: %s", testUser)
 
-	// Step 5: Verify - all 3 files should return 404 (failedCount=3)
-	expectedSuccess := 0
-	expectedFail := len(testFiles)
-	t.Logf("Expected: successCount=%d, failCount=%d", expectedSuccess, expectedFail)
-	t.Logf("Actual:   successCount=%d, failCount=%d", totalSuccess, totalFail)
-	assert.Equal(t, expectedSuccess, totalSuccess,
-		"Second delete should have 0 successes (all files already deleted), got %d", totalSuccess)
-	assert.Equal(t, expectedFail, totalFail,
-		"Second delete should have %d failures (all 404), got %d", expectedFail, totalFail)
+	// Cleanup user at the end
+	defer func() {
+		_ = tests.DeleteUser(serverDetails, testUser)
+		t.Logf("Cleaned up test user: %s", testUser)
+	}()
 
-	// Cleanup
-	cleanArtifactoryTest()
-}
+	// Step 2: Create permission target - user can delete from RtRepo1 but NOT from RtRepo2
+	permissionName := "delete-partial-test-perm"
+	err = tests.CreatePermissionTarget(serverDetails, permissionName, tests.RtRepo1, testUser, []string{"read", "write", "delete"})
+	assert.NoError(t, err, "Failed to create permission target for RtRepo1")
+	t.Logf("Created permission target: %s (allows delete on %s)", permissionName, tests.RtRepo1)
 
-// TestArtifactoryDeleteCountsPartiallyDeleted tests that delete correctly handles
-// a mix of existing and non-existing files using CLI's DeleteFiles API.
-// - 3 existing files should return success
-// - 2 pre-deleted files should return failure (404)
-// Files are placed in separate subfolders to prevent path reduction optimization.
-func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) {
-	initArtifactoryTest(t, "")
+	// Cleanup permission at the end
+	defer func() {
+		_ = tests.DeletePermissionTarget(serverDetails, permissionName)
+		t.Logf("Cleaned up permission target: %s", permissionName)
+	}()
 
-	// Step 1: Upload 5 specific test files in SEPARATE subfolders to prevent path reduction
-	// Each file goes in its own subfolder so delete service can't optimize to folder delete
-	testFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in", "f4/b1.in", "f5/b2.in"}
-	sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in", "testdata/a/b/b1.in", "testdata/a/b/b2.in"}
-	for i, f := range testFiles {
-		runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-partial/"+f)
+	// Step 3: Upload files as admin to both repos
+	// 3 files to RtRepo1 (will succeed with delete)
+	allowedFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in"}
+	sourceFilesAllowed := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in"}
+	for i, f := range allowedFiles {
+		runRt(t, "upload", sourceFilesAllowed[i], tests.RtRepo1+"/delete-partial/"+f)
 	}
-	t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles)
+	t.Logf("Uploaded %d files to %s (delete allowed)", len(allowedFiles), tests.RtRepo1)
 
-	// Step 2: Pre-delete 2 files (f1/a1.in and f2/a2.in) using direct DELETE API
-	filesToPreDelete := []string{"f1/a1.in", "f2/a2.in"}
-	for _, f := range filesToPreDelete {
-		artifactPath := tests.RtRepo1 + "/delete-partial/" + f
-		success, err := tests.DeleteFileDirect(serverDetails, artifactPath)
-		assert.NoError(t, err, "Pre-delete of %s should not return error", f)
-		assert.True(t, success, "Pre-delete of %s should succeed (HTTP 204)", f)
-		t.Logf("Pre-deleted %s successfully (HTTP 204)", f)
+	// 2 files to RtRepo2 (will fail with 403)
+	deniedFiles := []string{"f4/b1.in", "f5/b2.in"}
+	sourceFilesDenied := []string{"testdata/a/b/b1.in", "testdata/a/b/b2.in"}
+	for i, f := range deniedFiles {
+		runRt(t, "upload", sourceFilesDenied[i], tests.RtRepo2+"/delete-partial/"+f)
 	}
+	t.Logf("Uploaded %d files to %s (delete denied)", len(deniedFiles), tests.RtRepo2)
 
-	// Step 3: Verify pre-deleted files are gone via search
-	for _, f := range filesToPreDelete {
-		searchSpec := spec.NewBuilder().
-			Pattern(tests.RtRepo1 + "/delete-partial/" + f).
-			BuildSpec()
-		searchCmd := generic.NewSearchCommand()
-		searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec)
-		reader, err := searchCmd.Search()
-		assert.NoError(t, err)
-		count, err := reader.Length()
-		assert.NoError(t, err)
-		readerCloseAndAssert(t, reader)
-		assert.Equal(t, 0, count, "File %s should be deleted", f)
-	}
-	t.Log("Verified pre-deleted files are gone")
+	// Step 4: Create server details for the test user
+	testUserServerDetails := tests.CreateServerDetailsWithCredentials(serverDetails, testUser, testPassword)
 
-	// Step 4: Now try to delete ALL 5 files using CLI's DeleteFiles API
-	// - 3 files exist (f3/a3, f4/b1, f5/b2) → should return success
-	// - 2 files don't exist (f1/a1, f2/a2) → should return failure (404)
+	// Step 5: Build list of all artifact paths (from both repos)
 	var artifactPaths []string
-	for _, f := range testFiles {
+	for _, f := range allowedFiles {
 		artifactPaths = append(artifactPaths, tests.RtRepo1+"/delete-partial/"+f)
 	}
-	t.Logf("Attempting to delete paths: %v", artifactPaths)
+	for _, f := range deniedFiles {
+		artifactPaths = append(artifactPaths, tests.RtRepo2+"/delete-partial/"+f)
+	}
+	t.Logf("Attempting to delete %d paths with limited user: %v", len(artifactPaths), artifactPaths)
 
-	// Error IS expected because some files return 404
-	totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(serverDetails, artifactPaths)
-	// Error is expected when some files don't exist (404 responses)
-	t.Logf("Delete all 5 files result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err)
+	// Step 6: Delete using the test user's credentials
+	// - 3 files from RtRepo1 → should succeed (204)
+	// - 2 files from RtRepo2 → should fail (403)
+	totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(testUserServerDetails, artifactPaths)
+	// Error IS expected because some files return 403
+	t.Logf("Delete result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err)
 
-	// Step 5: Verify counts
-	// 3 files existed (f3/a3, f4/b1, f5/b2) = 3 success
-	// 2 files didn't exist (f1/a1, f2/a2) = 2 fail (404)
-	expectedSuccess := len(testFiles) - len(filesToPreDelete)
-	expectedFail := len(filesToPreDelete)
+	// Step 7: Verify counts
+	expectedSuccess := len(allowedFiles)
+	expectedFail := len(deniedFiles)
 	t.Logf("Expected: successCount=%d, failCount=%d", expectedSuccess, expectedFail)
 	t.Logf("Actual:   successCount=%d, failCount=%d", totalSuccess, totalFail)
 	assert.Equal(t, expectedSuccess, totalSuccess,
-		"Should have %d successes (f3/a3, f4/b1, f5/b2 still existed), got %d", expectedSuccess, totalSuccess)
+		"Should have %d successes (files from %s), got %d", expectedSuccess, tests.RtRepo1, totalSuccess)
 	assert.Equal(t, expectedFail, totalFail,
-		"Should have %d failures (f1/a1, f2/a2 already deleted = 404), got %d", expectedFail, totalFail)
+		"Should have %d failures (files from %s - 403), got %d", expectedFail, tests.RtRepo2, totalFail)
 
-	// Step 6: Verify all files are now gone
+	// Step 8: Verify files from RtRepo1 are deleted
 	searchSpec := spec.NewBuilder().
 		Pattern(tests.RtRepo1 + "/delete-partial/*").
 		Recursive(true).
@@ -2949,10 +2892,24 @@ func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) {
 	searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec)
 	reader, err := searchCmd.Search()
 	assert.NoError(t, err)
-	finalCount, err := reader.Length()
+	repo1Count, err := reader.Length()
 	assert.NoError(t, err)
 	readerCloseAndAssert(t, reader)
-	assert.Equal(t, 0, finalCount, "All files should be deleted")
+	assert.Equal(t, 0, repo1Count, "All files from %s should be deleted", tests.RtRepo1)
+
+	// Step 9: Verify files from RtRepo2 still exist (delete failed)
+	searchSpec2 := spec.NewBuilder().
+		Pattern(tests.RtRepo2 + "/delete-partial/*").
+		Recursive(true).
+		BuildSpec()
+	searchCmd2 := generic.NewSearchCommand()
+	searchCmd2.SetServerDetails(serverDetails).SetSpec(searchSpec2)
+	reader2, err := searchCmd2.Search()
+	assert.NoError(t, err)
+	repo2Count, err := reader2.Length()
+	assert.NoError(t, err)
+	readerCloseAndAssert(t, reader2)
+	assert.Equal(t, len(deniedFiles), repo2Count, "Files from %s should still exist (delete was denied)", tests.RtRepo2)
 
 	// Cleanup
 	cleanArtifactoryTest()

utils/tests/utils.go

@@ -347,6 +347,68 @@ func DeleteFilesByPathsUsingCli(serverDetails *config.ServerDetails, artifactory
 	return deleteCmd.DeleteFiles(reader)
 }
 
+// CreateUserWithPassword creates a new user with the specified password.
+func CreateUserWithPassword(serverDetails *config.ServerDetails, username, password string) error {
+	servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false)
+	if err != nil {
+		return err
+	}
+	adminFalse := false
+	userParams := services.NewUserParams()
+	userParams.UserDetails.Name = username
+	userParams.UserDetails.Email = username + "@test.com"
+	userParams.UserDetails.Password = password
+	userParams.UserDetails.Admin = &adminFalse
+	return servicesManager.CreateUser(userParams)
+}
+
+// DeleteUser deletes a user.
+func DeleteUser(serverDetails *config.ServerDetails, username string) error {
+	servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false)
+	if err != nil {
+		return err
+	}
+	return servicesManager.DeleteUser(username)
+}
+
+// CreatePermissionTarget creates a permission target giving the specified user permissions on the specified repo.
+func CreatePermissionTarget(serverDetails *config.ServerDetails, permName, repoKey, username string, actions []string) error {
+	servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false)
+	if err != nil {
+		return err
+	}
+	params := services.NewPermissionTargetParams()
+	params.Name = permName
+	params.Repo = &services.PermissionTargetSection{
+		Repositories: []string{repoKey},
+		Actions: &services.Actions{
+			Users: map[string][]string{
+				username: actions,
+			},
+		},
+	}
+	return servicesManager.CreatePermissionTarget(params)
+}
+
+// DeletePermissionTarget deletes a permission target.
+func DeletePermissionTarget(serverDetails *config.ServerDetails, permName string) error {
+	servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false)
+	if err != nil {
+		return err
+	}
+	return servicesManager.DeletePermissionTarget(permName)
+}
+
+// CreateServerDetailsWithCredentials creates a copy of server details with different credentials.
+func CreateServerDetailsWithCredentials(original *config.ServerDetails, username, password string) *config.ServerDetails {
+	newDetails := *original
+	newDetails.SetUser(username)
+	newDetails.SetPassword(password)
+	// Clear access token if using username/password
+	newDetails.SetAccessToken("")
+	return &newDetails
+}
+
 // This function makes no assertion, caller is responsible to assert as needed.
 func GetBuildInfo(serverDetails *config.ServerDetails, buildName, buildNumber string) (pbi *buildinfo.PublishedBuildInfo, found bool, err error) {
 	servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false)