refactor validator functions

Author: spencerugbo

internal/file/file_manager_service.go

@@ -158,7 +158,7 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,
 		return model.Error, allowedErr
 	}
 
-	permissionErr := fms.validateAndFixPermissions(ctx, fileOverview.GetFiles())
+	permissionErr := fms.validateAndUpdateFilePermissions(ctx, fileOverview.GetFiles())
 	if permissionErr != nil {
 		return model.RollbackRequired, permissionErr
 	}
@@ -602,11 +602,10 @@ func (fms *FileManagerService) checkAllowedDirectory(checkFiles []*mpi.File) err
 	return nil
 }
 
-func (fms *FileManagerService) validateAndFixPermissions(ctx context.Context, fileList []*mpi.File) error {
+func (fms *FileManagerService) validateAndUpdateFilePermissions(ctx context.Context, fileList []*mpi.File) error {
 	for _, file := range fileList {
-		err := fms.checkFilePermissions(file)
-		if err != nil {
-			resetErr := fms.resetFilePermissions(ctx, file)
+		if fms.areExecuteFilePermissionsSet(file) {
+			resetErr := fms.removeExecuteFilePermissions(ctx, file)
 			if resetErr != nil {
 				return fmt.Errorf("failed to reset permissions for %s: %w", file.GetFileMeta().GetName(), resetErr)
 			}
@@ -616,22 +615,18 @@ func (fms *FileManagerService) validateAndFixPermissions(ctx context.Context, fi
 	return nil
 }
 
-func (fms *FileManagerService) checkFilePermissions(file *mpi.File) error {
+func (fms *FileManagerService) areExecuteFilePermissionsSet(file *mpi.File) bool {
 	filePermission := file.GetFileMeta().GetPermissions()
 
 	permissionOctal, err := strconv.ParseUint(filePermission, 8, 32)
 	if err != nil || len(filePermission) != 4 {
-		return fmt.Errorf("file %s has malformed permissions", file.GetFileMeta().GetName())
-	}
-
-	if permissionOctal&executePerm > 0 {
-		return fmt.Errorf("file %s has execute permissions", file.GetFileMeta().GetName())
+		return false
 	}
 
-	return nil
+	return permissionOctal&executePerm > 0
 }
 
-func (fms *FileManagerService) resetFilePermissions(ctx context.Context, file *mpi.File) error {
+func (fms *FileManagerService) removeExecuteFilePermissions(ctx context.Context, file *mpi.File) error {
 	filePermission := file.GetFileMeta().GetPermissions()
 
 	permissionOctal, err := strconv.ParseUint(filePermission, 8, 32)
@@ -645,7 +640,7 @@ func (fms *FileManagerService) resetFilePermissions(ctx context.Context, file *m
 	file.FileMeta.Permissions = newPermission
 
 	slog.DebugContext(ctx, "Permissions have been changed", "file", file.GetFileMeta().GetName(),
-		"old permissions", filePermission, "new permissions", newPermission)
+		"old_permissions", filePermission, "new_permissions", newPermission)
 
 	return nil
 }

internal/file/file_manager_service_test.go

@@ -322,7 +322,7 @@ func TestFileManagerService_checkAllowedDirectory(t *testing.T) {
 	require.Error(t, err)
 }
 
-func TestFileManagerService_validateAndFixPermissions(t *testing.T) {
+func TestFileManagerService_validateAndUpdateFilePermissions(t *testing.T) {
 	ctx := context.Background()
 	fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
 
@@ -373,7 +373,7 @@ func TestFileManagerService_validateAndFixPermissions(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			err := fileManagerService.validateAndFixPermissions(ctx, test.files)
+			err := fileManagerService.validateAndUpdateFilePermissions(ctx, test.files)
 			if test.expectError {
 				require.Error(t, err)
 				assert.Contains(t, err.Error(), test.errorMsg)
@@ -384,49 +384,43 @@ func TestFileManagerService_validateAndFixPermissions(t *testing.T) {
 	}
 }
 
-func TestFileManagerService_checkFilePermissions(t *testing.T) {
+func TestFileManagerService_areExecuteFilePermissionsSet(t *testing.T) {
 	fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
 
 	tests := []struct {
 		name        string
 		permissions string
-		errorMsg    string
-		expectError bool
+		expectBool  bool
 	}{
 		{
 			name:        "Test 1: File with read and write permissions for owner",
 			permissions: "0600",
-			expectError: false,
+			expectBool:  false,
 		},
 		{
 			name:        "Test 2: File with read/write and execute permissions for owner",
 			permissions: "0700",
-			expectError: true,
-			errorMsg:    "has execute permissions",
+			expectBool:  true,
 		},
 		{
 			name:        "Test 3: File with read/write and execute permissions for owner and group",
 			permissions: "0770",
-			expectError: true,
-			errorMsg:    "has execute permissions",
+			expectBool:  true,
 		},
 		{
 			name:        "Test 4: File with read and execute permissions for everyone",
 			permissions: "0555",
-			expectError: true,
-			errorMsg:    "has execute permissions",
+			expectBool:  true,
 		},
 		{
 			name:        "Test 5: File with malformed permissions",
 			permissions: "abcde",
-			expectError: true,
-			errorMsg:    "has malformed permissions",
+			expectBool:  false,
 		},
 		{
 			name:        "Test 6: File with invalid permissions",
 			permissions: "000070",
-			expectError: true,
-			errorMsg:    "has malformed permissions",
+			expectBool:  false,
 		},
 	}
 
@@ -439,19 +433,13 @@ func TestFileManagerService_checkFilePermissions(t *testing.T) {
 				},
 			}
 
-			err := fileManagerService.checkFilePermissions(file)
-
-			if test.expectError {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), test.errorMsg)
-			} else {
-				assert.NoError(t, err)
-			}
+			got := fileManagerService.areExecuteFilePermissionsSet(file)
+			assert.Equal(t, test.expectBool, got)
 		})
 	}
 }
 
-func TestFileManagerService_resetFilePermissions(t *testing.T) {
+func TestFileManagerService_removeExecuteFilePermissions(t *testing.T) {
 	fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
 
 	tests := []struct {
@@ -484,7 +472,7 @@ func TestFileManagerService_resetFilePermissions(t *testing.T) {
 				},
 			}
 
-			parseErr := fileManagerService.resetFilePermissions(t.Context(), file)
+			parseErr := fileManagerService.removeExecuteFilePermissions(t.Context(), file)
 
 			if test.expectError {
 				require.Error(t, parseErr)