Added validator for folder permissions #1824
Conversation
| if !strings.HasPrefix(b.Config().Workspace.ResourcePath, rootPath) && | ||
| !libraries.IsVolumesPath(b.Config().Workspace.ResourcePath) { | ||
| paths = append(paths, b.Config().Workspace.ResourcePath) | ||
| } |
There was a problem hiding this comment.
With the logic expanded like this, it can be a loop on the different paths.
| for i, p := range paths { | ||
| g.Go(func() error { | ||
| diags, err, _ := syncGroup.Do(p, func() (any, error) { | ||
| diags := checkFolderPermission(ctx, b, p) |
There was a problem hiding this comment.
These paths are different by design so there won't be reuse at this level. I figured you could initialize the sync group here and pass it to checkFolderPermissions. Then inside of getClosestExistingObject, you can use syncGroup.Do() to make sure you're only doing a single call for any given path at any level.
Those calls will be done multiple times in the current approach.
There was a problem hiding this comment.
On second thought, this will apply only to cases where the different paths are not a child of the root. This should be infrequent enough that we can skip caching altogether.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestValidateFolderPermissions(t *testing.T) { |
There was a problem hiding this comment.
Please add suffix to capture the intent; I believe it is to test what happens if the root path doesn't exist yet.
| require.Empty(t, diags) | ||
| } | ||
|
|
||
| func TestValidateFolderPermissionsDifferentCount(t *testing.T) { |
There was a problem hiding this comment.
This is no longer about a different count.
| require.Equal(t, diag.Warning, diags[1].Severity) | ||
| } | ||
|
|
||
| func TestNoRootFolder(t *testing.T) { |
There was a problem hiding this comment.
Missing prefix: TestValidateFolderPermissions
| if perm.UserName != "" { | ||
| sb.WriteString(fmt.Sprintf("- level: %s\n user_name: %s\n", perm.Level, perm.UserName)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Shouldn't this use p.String() defined in permission.go?
| } | ||
|
|
||
| return fmt.Sprintf("level: %s", p.Level) | ||
| } |
There was a problem hiding this comment.
Also see comment below; if this is not used it can be removed.
Co-authored-by: Pieter Noordhuis <[email protected]>
Co-authored-by: Pieter Noordhuis <[email protected]>
Changes
This validator checks permissions defined in top-level bundle config and permissions set in workspace and raises the warning if they don't match
Tests