diff --git a/pkg/cli/codemod_permissions.go b/pkg/cli/codemod_permissions.go index 7c722a527dc..493a74169a0 100644 --- a/pkg/cli/codemod_permissions.go +++ b/pkg/cli/codemod_permissions.go @@ -7,6 +7,84 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) +var permissionsReadCodemodLog = logger.New("cli:codemod_permissions_read") + +// getPermissionsReadCodemod creates a codemod for converting invalid "read" and "write" shorthands +func getPermissionsReadCodemod() Codemod { + return Codemod{ + ID: "permissions-read-to-read-all", + Name: "Convert invalid permissions shorthand", + Description: "Converts 'permissions: read' to 'permissions: read-all' and 'permissions: write' to 'permissions: write-all' as per GitHub Actions spec", + IntroducedIn: "0.5.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + // Check if permissions exist + permissionsValue, hasPermissions := frontmatter["permissions"] + if !hasPermissions { + return content, false, nil + } + + // Check if permissions uses invalid shorthand (read or write) + hasInvalidShorthand := false + if strValue, ok := permissionsValue.(string); ok { + if strValue == "read" || strValue == "write" { + hasInvalidShorthand = true + } + } + + if !hasInvalidShorthand { + return content, false, nil + } + + // Parse frontmatter to get raw lines + frontmatterLines, markdown, err := parseFrontmatterLines(content) + if err != nil { + return content, false, err + } + + // Find and replace invalid shorthand permissions + var modified bool + result := make([]string, len(frontmatterLines)) + + for i, line := range frontmatterLines { + trimmedLine := strings.TrimSpace(line) + + // Check for permissions line with shorthand + if strings.HasPrefix(trimmedLine, "permissions:") { + // Handle shorthand on same line: "permissions: read" or "permissions: write" + if strings.Contains(trimmedLine, ": read") && !strings.Contains(trimmedLine, "read-all") && !strings.Contains(trimmedLine, ": read\n") { + // Make sure it's "permissions: read" and not "contents: read" + if strings.TrimSpace(strings.Split(line, ":")[0]) == "permissions" { + result[i] = strings.Replace(line, ": read", ": read-all", 1) + modified = true + permissionsReadCodemodLog.Printf("Replaced 'permissions: read' with 'permissions: read-all' on line %d", i+1) + continue + } + } else if strings.Contains(trimmedLine, ": write") && !strings.Contains(trimmedLine, "write-all") { + // Make sure it's "permissions: write" and not "contents: write" + if strings.TrimSpace(strings.Split(line, ":")[0]) == "permissions" { + result[i] = strings.Replace(line, ": write", ": write-all", 1) + modified = true + permissionsReadCodemodLog.Printf("Replaced 'permissions: write' with 'permissions: write-all' on line %d", i+1) + continue + } + } + } + + result[i] = line + } + + if !modified { + return content, false, nil + } + + // Reconstruct the content + newContent := reconstructContent(result, markdown) + permissionsReadCodemodLog.Print("Applied permissions read/write to read-all/write-all migration") + return newContent, true, nil + }, + } +} + var writePermissionsCodemodLog = logger.New("cli:codemod_permissions") // getWritePermissionsCodemod creates a codemod for converting write permissions to read diff --git a/pkg/cli/codemod_permissions_test.go b/pkg/cli/codemod_permissions_test.go index 7b6b8a7d9eb..d1ca10969a9 100644 --- a/pkg/cli/codemod_permissions_test.go +++ b/pkg/cli/codemod_permissions_test.go @@ -7,6 +7,180 @@ import ( "github.com/stretchr/testify/require" ) +func TestGetPermissionsReadCodemod(t *testing.T) { + codemod := getPermissionsReadCodemod() + + assert.Equal(t, "permissions-read-to-read-all", codemod.ID) + assert.Equal(t, "Convert invalid permissions shorthand", codemod.Name) + assert.NotEmpty(t, codemod.Description) + assert.Equal(t, "0.5.0", codemod.IntroducedIn) + require.NotNil(t, codemod.Apply) +} + +func TestPermissionsReadCodemod_Read(t *testing.T) { + codemod := getPermissionsReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: read +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": "read", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "permissions: read-all") + assert.NotContains(t, result, "permissions: read\n") +} + +func TestPermissionsReadCodemod_Write(t *testing.T) { + codemod := getPermissionsReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": "write", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "permissions: write-all") + assert.NotContains(t, result, "permissions: write\n") +} + +func TestPermissionsReadCodemod_NoChange_ReadAll(t *testing.T) { + codemod := getPermissionsReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: read-all +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": "read-all", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestPermissionsReadCodemod_NoChange_WriteAll(t *testing.T) { + codemod := getPermissionsReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: write-all +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": "write-all", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestPermissionsReadCodemod_NoChange_MapFormat(t *testing.T) { + codemod := getPermissionsReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read + issues: read +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "read", + "issues": "read", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestPermissionsReadCodemod_NoPermissions(t *testing.T) { + codemod := getPermissionsReadCodemod() + + content := `--- +on: workflow_dispatch +timeout-minutes: 30 +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "timeout-minutes": 30, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestPermissionsReadCodemod_PreservesMarkdown(t *testing.T) { + codemod := getPermissionsReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: read +--- + +# Test Workflow + +This workflow needs permissions.` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": "read", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "# Test Workflow") + assert.Contains(t, result, "This workflow needs permissions.") +} + func TestGetWritePermissionsCodemod(t *testing.T) { codemod := getWritePermissionsCodemod() diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 7dc6ff19500..e568c5759e6 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -24,6 +24,7 @@ func GetAllCodemods() []Codemod { getSafeInputsModeCodemod(), getUploadAssetsCodemod(), getWritePermissionsCodemod(), + getPermissionsReadCodemod(), // Fix permissions: read -> permissions: read-all getAgentTaskToAgentSessionCodemod(), getSandboxAgentFalseRemovalCodemod(), getScheduleAtToAroundCodemod(), diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 03168524e63..4165fb84760 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -41,7 +41,7 @@ func TestGetAllCodemods_ReturnsAllCodemods(t *testing.T) { codemods := GetAllCodemods() // Verify we have the expected number of codemods - expectedCount := 11 + expectedCount := 12 assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount) // Verify all codemods have required fields @@ -71,6 +71,7 @@ func TestGetAllCodemods_ContainsExpectedCodemods(t *testing.T) { "safe-inputs-mode-removal", "upload-assets-to-upload-asset-migration", "write-permissions-to-read-migration", + "permissions-read-to-read-all", "agent-task-to-agent-session-migration", "sandbox-agent-false-removal", "schedule-at-to-around-migration", @@ -106,6 +107,7 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) { "safe-inputs-mode-removal", "upload-assets-to-upload-asset-migration", "write-permissions-to-read-migration", + "permissions-read-to-read-all", "agent-task-to-agent-session-migration", "sandbox-agent-false-removal", "schedule-at-to-around-migration", diff --git a/pkg/cli/update_check_integration_test.go b/pkg/cli/update_check_integration_test.go index 965c011010f..c897a7ac2c8 100644 --- a/pkg/cli/update_check_integration_test.go +++ b/pkg/cli/update_check_integration_test.go @@ -51,7 +51,7 @@ func TestUpdateCheckIntegration(t *testing.T) { workflowContent := `--- name: Test Workflow on: workflow_dispatch -permissions: read +permissions: read-all engine: copilot --- @@ -94,7 +94,7 @@ Test workflow content. workflowContent := `--- name: Test Workflow on: workflow_dispatch -permissions: read +permissions: read-all engine: copilot --- @@ -139,7 +139,7 @@ Test workflow content. workflowContent := `--- name: Test Workflow on: workflow_dispatch -permissions: read +permissions: read-all engine: copilot --- @@ -203,7 +203,7 @@ Test workflow content. workflowContent := `--- name: Test Workflow on: workflow_dispatch -permissions: read +permissions: read-all engine: copilot --- diff --git a/pkg/parser/frontmatter_fuzz_test.go b/pkg/parser/frontmatter_fuzz_test.go index 188e5f14ad0..d8f14d0e538 100644 --- a/pkg/parser/frontmatter_fuzz_test.go +++ b/pkg/parser/frontmatter_fuzz_test.go @@ -29,7 +29,7 @@ title: Simple Workflow on: push: branches: [main] -permissions: read +permissions: read-all --- # Simple markdown`) diff --git a/pkg/parser/frontmatter_syntax_errors_test.go b/pkg/parser/frontmatter_syntax_errors_test.go index 30a104b0e84..052cea17eb6 100644 --- a/pkg/parser/frontmatter_syntax_errors_test.go +++ b/pkg/parser/frontmatter_syntax_errors_test.go @@ -27,7 +27,7 @@ func TestFrontmatterSyntaxErrors(t *testing.T) { frontmatterContent: `--- name: Test Workflow on push -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This is a test workflow.`, @@ -45,7 +45,7 @@ on: push: branches: - main -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has invalid indentation.`, @@ -61,7 +61,7 @@ This workflow has invalid indentation.`, name: Test Workflow on: push name: Duplicate Name -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has duplicate keys.`, @@ -78,7 +78,7 @@ name: Test Workflow on: push: branches: [main, dev -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has unclosed brackets.`, @@ -94,7 +94,7 @@ This workflow has unclosed brackets.`, name: Test Workflow on: push: {branches: [main], types: [opened -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has unclosed braces.`, @@ -109,7 +109,7 @@ This workflow has unclosed braces.`, frontmatterContent: `--- name: Test Workflow on: @invalid_character -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has invalid YAML characters.`, @@ -124,7 +124,7 @@ This workflow has invalid YAML characters.`, frontmatterContent: `--- name: "Test Workflow on: push -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has malformed string quotes.`, @@ -140,7 +140,7 @@ This workflow has malformed string quotes.`, name: Test Workflow on: push enabled: yes_please -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has invalid boolean value.`, @@ -155,7 +155,7 @@ This workflow has invalid boolean value.`, frontmatterContent: `--- name: Test Workflow on: -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has missing value after colon.`, @@ -174,7 +174,7 @@ on: branches: main - dev -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has invalid list structure.`, @@ -206,7 +206,7 @@ This workflow has unexpected end of stream.`, name: Test Workflow description: "Invalid escape: \z" on: push -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has invalid escape sequence.`, @@ -224,7 +224,7 @@ on: push: branches: - main -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has mixed tab and space indentation.`, @@ -242,7 +242,7 @@ defaults: &default_settings timeout: 30 on: push job1: *missing_anchor -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has anchor without alias.`, @@ -270,7 +270,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has complex nested structure error.`, @@ -289,7 +289,7 @@ description: | description that has invalid_key: value on: push -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow has invalid multiline string.`, @@ -306,7 +306,7 @@ name: Test Workflow on: push unknown_field: value invalid_permissions: write -permissions: read +permissions: read-all ---`, markdownContent: `# Test Workflow This workflow may have schema validation errors.`, @@ -527,7 +527,7 @@ on: branches: [main, dev pull_request: types: [opened] -permissions: read +permissions: read-all jobs: test: runs-on: ubuntu-latest @@ -616,7 +616,7 @@ func TestFrontmatterSyntaxErrorBoundaryConditions(t *testing.T) { content: `--- name: Test very_long_line_with_error: ` + strings.Repeat("a", 1000) + ` invalid: syntax -permissions: read +permissions: read-all --- # Content`, @@ -630,7 +630,7 @@ name: "测试工作流 🚀" description: "这是一个测试" invalid_syntax_here on: push -permissions: read +permissions: read-all --- # 测试内容 @@ -657,7 +657,7 @@ jobs: - os: windows node: 14 invalid syntax here -permissions: read +permissions: read-all --- # Content`, diff --git a/pkg/parser/schema_location_integration_test.go b/pkg/parser/schema_location_integration_test.go index e9397c5acb4..23ff99aab93 100644 --- a/pkg/parser/schema_location_integration_test.go +++ b/pkg/parser/schema_location_integration_test.go @@ -13,7 +13,7 @@ func TestValidateWithSchemaAndLocation_PreciseLocation(t *testing.T) { // Create a test file with invalid frontmatter testContent := `--- on: push -permissions: read +permissions: read-all age: "not-a-number" invalid_property: value tools: @@ -89,7 +89,7 @@ timeout_minutes: 30 func TestLocateJSONPathInYAML_RealExample(t *testing.T) { // Test with a real frontmatter example yamlContent := `on: push -permissions: read +permissions: read-all engine: claude tools: - name: github diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index 17464a7c764..459d3d3c4f4 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -22,7 +22,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { }, "stop-after": "2024-12-31", }, - "permissions": "read", + "permissions": "read-all", "run-name": "Test Run", "runs-on": "ubuntu-latest", "timeout_minutes": 30, @@ -217,7 +217,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"created", "edited", "answered"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -229,7 +229,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"created", "edited"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -249,7 +249,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"created", "deleted"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -261,7 +261,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"completed", "rerequested"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -273,7 +273,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"completed"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -317,7 +317,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"created", "deleted"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -329,7 +329,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"checks_requested"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -341,7 +341,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"opened", "closed"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -370,7 +370,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "branches": []string{"main"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -382,7 +382,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"submitted", "edited"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -394,7 +394,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"published", "updated"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -406,7 +406,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"custom-event", "deploy"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -426,7 +426,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"started"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -446,7 +446,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"opened", "typed", "untyped"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -459,7 +459,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "lock-for-agent": true, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -472,7 +472,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "lock-for-agent": false, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -485,7 +485,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "lock-for-agent": true, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -498,7 +498,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "lock-for-agent": false, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -510,7 +510,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "types": []string{"opened", "milestoned", "demilestoned", "ready_for_review", "auto_merge_enabled"}, }, }, - "permissions": "read", + "permissions": "read-all", }, wantErr: false, }, @@ -1284,7 +1284,7 @@ func TestValidateIncludedFileFrontmatterWithSchema(t *testing.T) { name: "invalid frontmatter with multiple unexpected keys", frontmatter: map[string]any{ "on": "push", - "permissions": "read", + "permissions": "read-all", "tools": map[string]any{"github": "test"}, }, wantErr: true, @@ -1294,7 +1294,7 @@ func TestValidateIncludedFileFrontmatterWithSchema(t *testing.T) { name: "invalid frontmatter with only unexpected keys", frontmatter: map[string]any{ "on": "push", - "permissions": "read", + "permissions": "read-all", }, wantErr: true, errContains: "cannot be used in shared workflows", diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 7d73ff0c163..58122830fb7 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -1415,8 +1415,8 @@ "oneOf": [ { "type": "string", - "enum": ["read-all", "write-all", "read", "write"], - "description": "Simple permissions string: 'read-all' (all read permissions), 'write-all' (all write permissions), 'read' or 'write' (basic level)" + "enum": ["read-all", "write-all"], + "description": "Simple permissions string: 'read-all' (all read permissions) or 'write-all' (all write permissions)" }, { "type": "object", diff --git a/pkg/workflow/permissions.go b/pkg/workflow/permissions.go index e4e0ecb37b3..07e1d6dcebe 100644 --- a/pkg/workflow/permissions.go +++ b/pkg/workflow/permissions.go @@ -101,8 +101,9 @@ func (p *PermissionsParser) parse() { return } - // Check if it's a shorthand permission (read-all, write-all, read, write, none) - shorthandPerms := []string{"read-all", "write-all", "read", "write", "none"} + // Check if it's a shorthand permission (read-all, write-all, none) + // Note: "read" and "write" are no longer valid shorthands as they create invalid GitHub Actions YAML + shorthandPerms := []string{"read-all", "write-all", "none"} for _, shorthand := range shorthandPerms { if yamlContent == shorthand { p.isShorthand = true @@ -160,7 +161,7 @@ func (p *PermissionsParser) HasContentsReadAccess() bool { // Handle shorthand permissions if p.isShorthand { switch p.shorthandValue { - case "read-all", "write-all", "read", "write": + case "read-all", "write-all": permissionsLog.Printf("Shorthand permissions grant contents read: %s", p.shorthandValue) return true case "none": @@ -202,10 +203,6 @@ func (p *PermissionsParser) IsAllowed(scope, level string) bool { return level == "read" case "write-all": return level == "read" || level == "write" - case "read": - return level == "read" - case "write": - return level == "read" || level == "write" case "none": return false default: @@ -309,10 +306,6 @@ func (p *PermissionsParser) ToPermissions() *Permissions { return NewPermissionsReadAll() case "write-all": return NewPermissionsWriteAll() - case "read": - return NewPermissionsRead() - case "write": - return NewPermissionsWrite() case "none": return NewPermissionsNone() default: @@ -501,20 +494,6 @@ func NewPermissionsWriteAll() *Permissions { } } -// NewPermissionsRead creates a Permissions with read shorthand -func NewPermissionsRead() *Permissions { - return &Permissions{ - shorthand: "read", - } -} - -// NewPermissionsWrite creates a Permissions with write shorthand -func NewPermissionsWrite() *Permissions { - return &Permissions{ - shorthand: "write", - } -} - // NewPermissionsNone creates a Permissions with none shorthand func NewPermissionsNone() *Permissions { return &Permissions{ @@ -578,9 +557,9 @@ func (p *Permissions) Get(scope PermissionScope) (PermissionLevel, bool) { if p.shorthand != "" { // Shorthand permissions apply to all scopes switch p.shorthand { - case "read-all", "read": + case "read-all": return PermissionRead, true - case "write-all", "write": + case "write-all": return PermissionWrite, true case "none": return PermissionNone, true @@ -687,12 +666,8 @@ func (p *Permissions) Merge(other *Permissions) { // Promote to the higher permission level if other.shorthand == "write-all" || p.shorthand == "write-all" { p.shorthand = "write-all" - } else if other.shorthand == "write" || p.shorthand == "write" { - p.shorthand = "write" } else if other.shorthand == "read-all" || p.shorthand == "read-all" { p.shorthand = "read-all" - } else if other.shorthand == "read" || p.shorthand == "read" { - p.shorthand = "read" } // none is lowest, so only keep if both are none return @@ -701,9 +676,9 @@ func (p *Permissions) Merge(other *Permissions) { // Apply other's shorthand as baseline, then our specific permissions override otherLevel := PermissionNone switch other.shorthand { - case "read-all", "read": + case "read-all": otherLevel = PermissionRead - case "write-all", "write": + case "write-all": otherLevel = PermissionWrite } diff --git a/pkg/workflow/permissions_enum_test.go b/pkg/workflow/permissions_enum_test.go index e5ac9066fb1..97de598ef1f 100644 --- a/pkg/workflow/permissions_enum_test.go +++ b/pkg/workflow/permissions_enum_test.go @@ -27,16 +27,16 @@ func TestPermissionsEnumValidation(t *testing.T) { description: "write-all is a valid shorthand permission", }, { - name: "valid: read", + name: "invalid: read (without -all)", permissions: "permissions: read", - expectValid: true, - description: "read is a valid shorthand permission", + expectValid: false, + description: "read without -all is not a valid shorthand (use read-all)", }, { - name: "valid: write", + name: "invalid: write (without -all)", permissions: "permissions: write", - expectValid: true, - description: "write is a valid shorthand permission", + expectValid: false, + description: "write without -all is not a valid shorthand (use write-all)", }, { name: "valid: none", diff --git a/pkg/workflow/permissions_shortcut_included_test.go b/pkg/workflow/permissions_shortcut_included_test.go index b5118341854..3bf8b9b70e6 100644 --- a/pkg/workflow/permissions_shortcut_included_test.go +++ b/pkg/workflow/permissions_shortcut_included_test.go @@ -9,7 +9,7 @@ import ( "github.com/githubnext/gh-aw/pkg/testutil" ) -// TestPermissionsShortcutInIncludedFiles tests that permissions shortcuts (read-all, write-all, read, write) +// TestPermissionsShortcutInIncludedFiles tests that permissions shortcuts (read-all, write-all, none) // work correctly in included files, matching the UX of main workflows. func TestPermissionsShortcutInIncludedFiles(t *testing.T) { tests := []struct { @@ -33,20 +33,6 @@ func TestPermissionsShortcutInIncludedFiles(t *testing.T) { expectCompilationError: false, expectLockFileContains: "permissions: write-all", }, - { - name: "read shortcut in included file", - includedPermissions: "permissions: read", - mainPermissions: "permissions: read", - expectCompilationError: false, - expectLockFileContains: "permissions: read", - }, - { - name: "write shortcut in included file", - includedPermissions: "permissions: write", - mainPermissions: "permissions: write\nfeatures:\n dangerous-permissions-write: true", - expectCompilationError: false, - expectLockFileContains: "permissions: write", - }, { name: "object form still works in included file", includedPermissions: `permissions: diff --git a/pkg/workflow/permissions_test.go b/pkg/workflow/permissions_test.go index 53b36a561b8..caf4b7bf64a 100644 --- a/pkg/workflow/permissions_test.go +++ b/pkg/workflow/permissions_test.go @@ -22,14 +22,14 @@ func TestPermissionsParser_HasContentsReadAccess(t *testing.T) { expected: true, }, { - name: "shorthand read grants contents access", + name: "invalid shorthand read does not grant contents access", permissions: "permissions: read", - expected: true, + expected: false, // "read" is no longer a valid shorthand }, { - name: "shorthand write grants contents access", + name: "invalid shorthand write does not grant contents access", permissions: "permissions: write", - expected: true, + expected: false, // "write" is no longer a valid shorthand }, { name: "shorthand none denies contents access", @@ -504,8 +504,6 @@ func TestNewPermissionsShorthand(t *testing.T) { }{ {"read-all", NewPermissionsReadAll, "read-all"}, {"write-all", NewPermissionsWriteAll, "write-all"}, - {"read", NewPermissionsRead, "read"}, - {"write", NewPermissionsWrite, "write"}, {"none", NewPermissionsNone, "none"}, } @@ -737,13 +735,13 @@ func TestPermissionsMerge(t *testing.T) { }, { name: "merge shorthand - write-all wins over read", - base: NewPermissionsRead(), + base: NewPermissionsReadAll(), merge: NewPermissionsWriteAll(), wantSH: "write-all", }, { name: "merge shorthand - write-all wins over write", - base: NewPermissionsWrite(), + base: NewPermissionsWriteAll(), merge: NewPermissionsWriteAll(), wantSH: "write-all", }, @@ -754,26 +752,26 @@ func TestPermissionsMerge(t *testing.T) { wantSH: "write-all", }, { - name: "merge shorthand - write wins over read-all", + name: "merge shorthand - write-all wins over read-all", base: NewPermissionsReadAll(), - merge: NewPermissionsWrite(), - wantSH: "write", + merge: NewPermissionsWriteAll(), + wantSH: "write-all", }, { - name: "merge shorthand - write wins over read", - base: NewPermissionsRead(), - merge: NewPermissionsWrite(), - wantSH: "write", + name: "merge shorthand - write-all wins over read-all (duplicate for coverage)", + base: NewPermissionsReadAll(), + merge: NewPermissionsWriteAll(), + wantSH: "write-all", }, { - name: "merge shorthand - write wins over none", + name: "merge shorthand - write-all wins over none", base: NewPermissionsNone(), - merge: NewPermissionsWrite(), - wantSH: "write", + merge: NewPermissionsWriteAll(), + wantSH: "write-all", }, { - name: "merge shorthand - read-all wins over read", - base: NewPermissionsRead(), + name: "merge shorthand - read-all wins over read-all", + base: NewPermissionsReadAll(), merge: NewPermissionsReadAll(), wantSH: "read-all", }, @@ -784,21 +782,21 @@ func TestPermissionsMerge(t *testing.T) { wantSH: "read-all", }, { - name: "merge shorthand - read wins over none", + name: "merge shorthand - read-all wins over none (duplicate for coverage)", base: NewPermissionsNone(), - merge: NewPermissionsRead(), - wantSH: "read", + merge: NewPermissionsReadAll(), + wantSH: "read-all", }, { name: "merge shorthand - read-all preserved when merging read", base: NewPermissionsReadAll(), - merge: NewPermissionsRead(), + merge: NewPermissionsReadAll(), wantSH: "read-all", }, { name: "merge shorthand - write-all preserved when merging write", base: NewPermissionsWriteAll(), - merge: NewPermissionsWrite(), + merge: NewPermissionsWriteAll(), wantSH: "write-all", }, { @@ -870,7 +868,7 @@ func TestPermissionsMerge(t *testing.T) { { name: "merge read shorthand into map - adds all missing scopes as read", base: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{PermissionContents: PermissionWrite}), - merge: NewPermissionsRead(), + merge: NewPermissionsReadAll(), want: map[PermissionScope]PermissionLevel{ PermissionContents: PermissionWrite, PermissionActions: PermissionRead, @@ -893,7 +891,7 @@ func TestPermissionsMerge(t *testing.T) { { name: "merge write shorthand into map - adds all missing scopes as write", base: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{PermissionIssues: PermissionRead}), - merge: NewPermissionsWrite(), + merge: NewPermissionsWriteAll(), want: map[PermissionScope]PermissionLevel{ PermissionIssues: PermissionRead, PermissionActions: PermissionWrite, @@ -941,7 +939,7 @@ func TestPermissionsMerge(t *testing.T) { }, { name: "merge complex map into read shorthand", - base: NewPermissionsRead(), + base: NewPermissionsReadAll(), merge: NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ PermissionContents: PermissionWrite, PermissionIssues: PermissionRead,