Skip to content

Commit b076ee2

Browse files
spencerugbospencerugbo
committed
refactor permission reset logic
1 parent 52ae9f5 commit b076ee2

File tree

4 files changed

+76
-99
lines changed

4 files changed

+76
-99
lines changed

‎internal/file/file_manager_service.go‎

Lines changed: 14 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616
"path"
1717
"path/filepath"
1818
"strconv"
19-
"strings"
2019
"sync"
2120

2221
"google.golang.org/grpc"
@@ -161,7 +160,7 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,
161160

162161
permissionErr := fms.validateAndFixPermissions(ctx, fileOverview.GetFiles())
163162
if permissionErr != nil {
164-
return model.PermissionChange, permissionErr
163+
return model.RollbackRequired, permissionErr
165164
}
166165

167166
diffFiles, fileContent, compareErr := fms.DetermineFileActions(
@@ -604,32 +603,24 @@ func (fms *FileManagerService) checkAllowedDirectory(checkFiles []*mpi.File) err
604603
}
605604

606605
func (fms *FileManagerService) validateAndFixPermissions(ctx context.Context, fileList []*mpi.File) error {
607-
var permissionIssues []string
608-
609606
for _, file := range fileList {
610-
if err := fms.checkFilePermissions(file); err != nil {
611-
permissionIssues = append(permissionIssues, file.GetFileMeta().GetName())
612-
613-
if resetErr := fms.resetFilePermissions(file); resetErr != nil {
607+
err := fms.checkFilePermissions(file)
608+
if err != nil {
609+
resetErr := fms.resetFilePermissions(ctx, file)
610+
if resetErr != nil {
614611
return fmt.Errorf("failed to reset permissions for %s: %w", file.GetFileMeta().GetName(), resetErr)
615612
}
616-
617-
slog.InfoContext(ctx, "Reset execute permissions", "file", file.GetFileMeta().GetName())
618613
}
619614
}
620615

621-
if len(permissionIssues) > 0 {
622-
return fmt.Errorf("reset execute permissions for files: %s", strings.Join(permissionIssues, ", "))
623-
}
624-
625616
return nil
626617
}
627618

628619
func (fms *FileManagerService) checkFilePermissions(file *mpi.File) error {
629620
filePermission := file.GetFileMeta().GetPermissions()
630621

631622
permissionOctal, err := strconv.ParseUint(filePermission, 8, 32)
632-
if err != nil {
623+
if err != nil || len(filePermission) != 4 {
633624
return fmt.Errorf("file %s has malformed permissions", file.GetFileMeta().GetName())
634625
}
635626

@@ -640,25 +631,21 @@ func (fms *FileManagerService) checkFilePermissions(file *mpi.File) error {
640631
return nil
641632
}
642633

643-
func (fms *FileManagerService) resetFilePermissions(file *mpi.File) error {
634+
func (fms *FileManagerService) resetFilePermissions(ctx context.Context, file *mpi.File) error {
644635
filePermission := file.GetFileMeta().GetPermissions()
645636

646637
permissionOctal, err := strconv.ParseUint(filePermission, 8, 32)
647638
if err != nil {
648-
chmodErr := os.Chmod(file.GetFileMeta().GetName(), os.FileMode(filePerm))
649-
if chmodErr != nil {
650-
return fmt.Errorf("failed to set file permissions: %w", err)
651-
}
652-
653-
return fmt.Errorf("falied to parse file permissions, permissions set to 0o644: %w", err)
639+
return fmt.Errorf("falied to parse file permissions: %w", err)
654640
}
655641

656-
newPermission := permissionOctal &^ executePerm
642+
permissionOctal &^= executePerm
657643

658-
err = os.Chmod(file.GetFileMeta().GetName(), os.FileMode(newPermission))
659-
if err != nil {
660-
return fmt.Errorf("failed to set file permissions: %w", err)
661-
}
644+
newPermission := "0" + strconv.FormatUint(permissionOctal, 8)
645+
file.FileMeta.Permissions = newPermission
646+
647+
slog.DebugContext(ctx, "Permissions have been changed", "file", file.GetFileMeta().GetName(),
648+
"old permissions", filePermission, "new permissions", newPermission)
662649

663650
return nil
664651
}

‎internal/file/file_manager_service_test.go‎

Lines changed: 62 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"os"
1414
"path"
1515
"path/filepath"
16-
"strconv"
1716
"sync"
1817
"testing"
1918

@@ -327,46 +326,62 @@ func TestFileManagerService_validateAndFixPermissions(t *testing.T) {
327326
ctx := context.Background()
328327
fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
329328

330-
tempDir := t.TempDir()
331-
execFile := helpers.CreateFileWithErrorCheck(t, tempDir, "exec.conf")
332-
defer helpers.RemoveFileWithErrorCheck(t, execFile.Name())
333-
334-
normalFile := helpers.CreateFileWithErrorCheck(t, tempDir, "normal.conf")
335-
defer helpers.RemoveFileWithErrorCheck(t, normalFile.Name())
336-
337-
err := os.Chmod(execFile.Name(), 0o700)
338-
require.NoError(t, err)
339-
err = os.Chmod(normalFile.Name(), 0o620)
340-
require.NoError(t, err)
341-
342-
fileList := []*mpi.File{
329+
tests := []struct {
330+
name string
331+
errorMsg string
332+
files []*mpi.File
333+
expectError bool
334+
}{
343335
{
344-
FileMeta: &mpi.FileMeta{
345-
Name: execFile.Name(),
346-
Permissions: "0700",
336+
name: "Test 1: File list with valid permissions",
337+
expectError: false,
338+
files: []*mpi.File{
339+
{
340+
FileMeta: &mpi.FileMeta{
341+
Name: "exec.conf",
342+
Permissions: "0700",
343+
},
344+
},
345+
{
346+
FileMeta: &mpi.FileMeta{
347+
Name: "normal.conf",
348+
Permissions: "0620",
349+
},
350+
},
347351
},
348352
},
349353
{
350-
FileMeta: &mpi.FileMeta{
351-
Name: normalFile.Name(),
352-
Permissions: "0620",
354+
name: "Test 2: File list with invalid permissions",
355+
expectError: true,
356+
errorMsg: "failed to reset permissions",
357+
files: []*mpi.File{
358+
{
359+
FileMeta: &mpi.FileMeta{
360+
Name: "malformed.conf",
361+
Permissions: "07000",
362+
},
363+
},
364+
{
365+
FileMeta: &mpi.FileMeta{
366+
Name: "invalid.conf",
367+
Permissions: "abcde",
368+
},
369+
},
353370
},
354371
},
355372
}
356373

357-
err = fileManagerService.validateAndFixPermissions(ctx, fileList)
358-
359-
require.Error(t, err)
360-
assert.Contains(t, err.Error(), "reset execute permissions for files")
361-
assert.Contains(t, err.Error(), execFile.Name())
362-
363-
info, err := os.Stat(execFile.Name())
364-
require.NoError(t, err)
365-
assert.Equal(t, os.FileMode(0o600), info.Mode().Perm())
366-
367-
info, err = os.Stat(normalFile.Name())
368-
require.NoError(t, err)
369-
assert.Equal(t, os.FileMode(0o620), info.Mode().Perm())
374+
for _, test := range tests {
375+
t.Run(test.name, func(t *testing.T) {
376+
err := fileManagerService.validateAndFixPermissions(ctx, test.files)
377+
if test.expectError {
378+
require.Error(t, err)
379+
assert.Contains(t, err.Error(), test.errorMsg)
380+
} else {
381+
require.NoError(t, err)
382+
}
383+
})
384+
}
370385
}
371386

372387
func TestFileManagerService_checkFilePermissions(t *testing.T) {
@@ -379,22 +394,28 @@ func TestFileManagerService_checkFilePermissions(t *testing.T) {
379394
expectError bool
380395
}{
381396
{
382-
name: "File with read and write permissions for owner",
397+
name: "Test 1: File with read and write permissions for owner",
383398
permissions: "0600",
384399
expectError: false,
385400
},
386401
{
387-
name: "File with read/write and execute permissions for owner",
402+
name: "Test 2: File with read/write and execute permissions for owner",
388403
permissions: "0700",
389404
expectError: true,
390405
errorMsg: "has execute permissions",
391406
},
392407
{
393-
name: "File with malformed permissions",
408+
name: "Test 3: File with malformed permissions",
394409
permissions: "abcde",
395410
expectError: true,
396411
errorMsg: "has malformed permissions",
397412
},
413+
{
414+
name: "Test 4: File with invalid permissions",
415+
permissions: "000070",
416+
expectError: true,
417+
errorMsg: "has malformed permissions",
418+
},
398419
}
399420

400421
for _, test := range tests {
@@ -420,7 +441,6 @@ func TestFileManagerService_checkFilePermissions(t *testing.T) {
420441

421442
func TestFileManagerService_resetFilePermissions(t *testing.T) {
422443
fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
423-
tempDir := t.TempDir()
424444

425445
tests := []struct {
426446
name string
@@ -430,13 +450,13 @@ func TestFileManagerService_resetFilePermissions(t *testing.T) {
430450
expectError bool
431451
}{
432452
{
433-
name: "File with execute permissions for owner and others",
453+
name: "Test 1: File with execute permissions for owner and others",
434454
permissions: "0703",
435455
expectError: false,
436456
expectPermissions: "0602",
437457
},
438458
{
439-
name: "File with malformed permissions",
459+
name: "Test 2: File with malformed permissions",
440460
permissions: "abcde",
441461
expectError: true,
442462
expectPermissions: "0600",
@@ -446,30 +466,21 @@ func TestFileManagerService_resetFilePermissions(t *testing.T) {
446466

447467
for _, test := range tests {
448468
t.Run(test.name, func(t *testing.T) {
449-
tempFile := helpers.CreateFileWithErrorCheck(t, tempDir, "test.conf")
450-
defer helpers.RemoveFileWithErrorCheck(t, tempFile.Name())
451-
452469
file := &mpi.File{
453470
FileMeta: &mpi.FileMeta{
454-
Name: tempFile.Name(),
471+
Name: "test.conf",
455472
Permissions: test.permissions,
456473
},
457474
}
458475

459-
parseErr := fileManagerService.resetFilePermissions(file)
460-
461-
info, err := os.Stat(tempFile.Name())
462-
require.NoError(t, err)
463-
464-
actual := "0" + strconv.FormatUint(uint64(info.Mode().Perm()), 8)
476+
parseErr := fileManagerService.resetFilePermissions(t.Context(), file)
465477

466478
if test.expectError {
467479
require.Error(t, parseErr)
468-
assert.Equal(t, test.expectPermissions, actual)
469480
assert.Contains(t, parseErr.Error(), test.errorMsg)
470481
} else {
471482
require.NoError(t, parseErr)
472-
assert.Equal(t, test.expectPermissions, actual)
483+
assert.Equal(t, test.expectPermissions, file.GetFileMeta().GetPermissions())
473484
}
474485
})
475486
}

‎internal/file/file_plugin.go‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -347,26 +347,6 @@ func (fp *FilePlugin) handleConfigApplyRequest(ctx context.Context, msg *bus.Mes
347347
}
348348

349349
fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.WriteConfigSuccessfulTopic, Data: data})
350-
case model.PermissionChange:
351-
slog.WarnContext(ctx, "Files with execute permissions found and reset",
352-
"details", err.Error())
353-
dpResponse := fp.createDataPlaneResponse(
354-
correlationID,
355-
mpi.CommandResponse_COMMAND_STATUS_OK,
356-
"Config apply successful, files updated with permission changes",
357-
instanceID,
358-
"",
359-
)
360-
361-
successMessage := &model.ConfigApplySuccess{
362-
ConfigContext: &model.NginxConfigContext{},
363-
DataPlaneResponse: dpResponse,
364-
}
365-
366-
fp.fileManagerService.ClearCache()
367-
fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.ConfigApplySuccessfulTopic, Data: successMessage})
368-
369-
return
370350
}
371351
}
372352

‎internal/model/config.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@ const (
7575
NoChange
7676
Error
7777
OK
78-
PermissionChange = 5
7978
)
8079

8180
type ConfigApplySuccess struct {

0 commit comments

Comments
 (0)