diff --git a/go.mod b/go.mod index c815c07..eaae966 100644 --- a/go.mod +++ b/go.mod @@ -9,8 +9,8 @@ require ( ) require ( - github.com/modelcontextprotocol/go-sdk v1.1.0 github.com/manifoldco/promptui v0.9.0 + github.com/modelcontextprotocol/go-sdk v1.1.0 github.com/stretchr/testify v1.11.1 ) diff --git a/internal/adapter/checkstyle/executor.go b/internal/adapter/checkstyle/executor.go index b33f3f6..8f6d2a6 100644 --- a/internal/adapter/checkstyle/executor.go +++ b/internal/adapter/checkstyle/executor.go @@ -36,7 +36,29 @@ func (a *Adapter) execute(ctx context.Context, config []byte, files []string) (* "-c", configFile, "-f", "xml", // XML output format } - args = append(args, files...) + + // Ensure all file paths are absolute + // Convert relative paths using WorkDir as base + absFiles := make([]string, len(files)) + for i, file := range files { + var absPath string + if filepath.IsAbs(file) { + // Already absolute, use as-is + absPath = file + } else { + // Relative path, resolve against WorkDir + absPath = filepath.Join(a.WorkDir, file) + } + + // Verify file exists + if _, err := os.Stat(absPath); os.IsNotExist(err) { + return nil, fmt.Errorf("file not found: %s", file) + } + + absFiles[i] = absPath + } + + args = append(args, absFiles...) // Execute start := time.Now() diff --git a/internal/adapter/checkstyle/parser.go b/internal/adapter/checkstyle/parser.go index 7e2b3f7..20e5554 100644 --- a/internal/adapter/checkstyle/parser.go +++ b/internal/adapter/checkstyle/parser.go @@ -40,6 +40,14 @@ func parseOutput(output *adapter.ToolOutput) ([]adapter.Violation, error) { return []adapter.Violation{}, nil } + // If no output but non-zero exit code, something went wrong + if output.Stdout == "" { + if output.Stderr != "" { + return nil, fmt.Errorf("checkstyle failed (exit code %d): %s", output.ExitCode, output.Stderr) + } + return nil, fmt.Errorf("checkstyle failed with exit code %d but produced no output", output.ExitCode) + } + // Parse XML output var result CheckstyleOutput if err := xml.Unmarshal([]byte(output.Stdout), &result); err != nil { diff --git a/internal/adapter/eslint/adapter_test.go b/internal/adapter/eslint/adapter_test.go index 2dda823..495b089 100644 --- a/internal/adapter/eslint/adapter_test.go +++ b/internal/adapter/eslint/adapter_test.go @@ -14,6 +14,7 @@ func TestNewAdapter(t *testing.T) { adapter := NewAdapter("", "") if adapter == nil { t.Fatal("NewAdapter() returned nil") + return } if adapter.ToolsDir == "" { diff --git a/internal/adapter/pmd/executor.go b/internal/adapter/pmd/executor.go index 25c14c3..b946aee 100644 --- a/internal/adapter/pmd/executor.go +++ b/internal/adapter/pmd/executor.go @@ -32,10 +32,31 @@ func (a *Adapter) execute(ctx context.Context, config []byte, files []string) (* // Build command pmdPath := a.getPMDPath() + // Ensure all file paths are absolute + // Convert relative paths using WorkDir as base + absFiles := make([]string, len(files)) + for i, file := range files { + var absPath string + if filepath.IsAbs(file) { + // Already absolute, use as-is + absPath = file + } else { + // Relative path, resolve against WorkDir + absPath = filepath.Join(a.WorkDir, file) + } + + // Verify file exists + if _, err := os.Stat(absPath); os.IsNotExist(err) { + return nil, fmt.Errorf("file not found: %s", file) + } + + absFiles[i] = absPath + } + // PMD command format: pmd check -d -R -f json args := []string{ "check", - "-d", strings.Join(files, ","), // Comma-separated file list + "-d", strings.Join(absFiles, ","), // Comma-separated file list "-R", rulesetFile, "-f", "json", // JSON output format "--no-cache", // Disable cache for consistent results diff --git a/internal/adapter/prettier/adapter_test.go b/internal/adapter/prettier/adapter_test.go index a5558f8..5ca6d78 100644 --- a/internal/adapter/prettier/adapter_test.go +++ b/internal/adapter/prettier/adapter_test.go @@ -14,6 +14,7 @@ func TestNewAdapter(t *testing.T) { a := NewAdapter("", "") if a == nil { t.Fatal("NewAdapter() returned nil") + return } if a.ToolsDir == "" { diff --git a/internal/adapter/subprocess_test.go b/internal/adapter/subprocess_test.go index 918b875..6a35ae0 100644 --- a/internal/adapter/subprocess_test.go +++ b/internal/adapter/subprocess_test.go @@ -10,6 +10,7 @@ func TestNewSubprocessExecutor(t *testing.T) { executor := NewSubprocessExecutor() if executor == nil { t.Fatal("NewSubprocessExecutor() returned nil") + return } if executor.Timeout != 2*time.Minute { diff --git a/internal/adapter/tsc/adapter_test.go b/internal/adapter/tsc/adapter_test.go index 2d815e9..c6dbedc 100644 --- a/internal/adapter/tsc/adapter_test.go +++ b/internal/adapter/tsc/adapter_test.go @@ -13,6 +13,7 @@ func TestNewAdapter(t *testing.T) { adapter := NewAdapter("", "") if adapter == nil { t.Fatal("NewAdapter() returned nil") + return } // Should have default ToolsDir diff --git a/internal/git/repo.go b/internal/git/repo.go index ae458b2..a916773 100644 --- a/internal/git/repo.go +++ b/internal/git/repo.go @@ -2,6 +2,7 @@ package git import ( "fmt" + "os" "os/exec" "regexp" "strings" @@ -56,7 +57,15 @@ func IsGitRepo() bool { } // GetCurrentUser returns the current git user name +// Checks GIT_AUTHOR_NAME environment variable first (for testing), +// then falls back to git config user.name func GetCurrentUser() (string, error) { + // Check environment variable first (for testing) + if user := os.Getenv("GIT_AUTHOR_NAME"); user != "" { + return user, nil + } + + // Fallback to git config cmd := exec.Command("git", "config", "--get", "user.name") output, err := cmd.Output() if err != nil { diff --git a/internal/validator/validator.go b/internal/validator/validator.go index 72a9885..d35950d 100644 --- a/internal/validator/validator.go +++ b/internal/validator/validator.go @@ -65,186 +65,6 @@ type Violation struct { Column int } -// Result represents validation result -type Result struct { - Violations []Violation - Passed bool -} - -// Validate validates the given path -func (v *Validator) Validate(path string) (*Result, error) { - if v.policy == nil { - return nil, fmt.Errorf("policy is not loaded") - } - - result := &Result{ - Violations: make([]Violation, 0), - Passed: true, - } - - if v.verbose { - fmt.Printf("šŸ” Validating %s against %d rule(s)...\n", path, len(v.policy.Rules)) - } - - // Check RBAC permissions if enabled - if v.policy.Enforce.RBACConfig != nil && v.policy.Enforce.RBACConfig.Enabled { - // Get current git user - username, err := git.GetCurrentUser() - if err != nil { - if v.verbose { - fmt.Printf("āš ļø RBAC check skipped: %v\n", err) - } - } else { - if v.verbose { - fmt.Printf("šŸ” Checking RBAC permissions for user: %s\n", username) - } - - // Get files to check - fileInfo, err := os.Stat(path) - if err == nil && !fileInfo.IsDir() { - // Validate file permissions - rbacResult, err := roles.ValidateFilePermissions(username, []string{path}) - if err != nil { - if v.verbose { - fmt.Printf("āš ļø RBAC validation failed: %v\n", err) - } - } else if !rbacResult.Allowed { - // Add RBAC violations - for _, deniedFile := range rbacResult.DeniedFiles { - result.Violations = append(result.Violations, Violation{ - RuleID: "rbac-permission-denied", - Severity: "error", - Message: fmt.Sprintf("User '%s' does not have permission to modify this file", username), - File: deniedFile, - Line: 0, - Column: 0, - }) - } - result.Passed = false - - if v.verbose { - fmt.Printf("āŒ RBAC: %d file(s) denied for user %s\n", len(rbacResult.DeniedFiles), username) - } - } else if v.verbose { - fmt.Printf("āœ“ RBAC: User %s has permission to modify all files\n", username) - } - } - } - } - - // For each enabled rule, execute validation - for _, rule := range v.policy.Rules { - if !rule.Enabled { - continue - } - - // Determine which engine to use - engineName := getEngineName(rule) - if engineName == "" { - if v.verbose { - fmt.Printf("āš ļø Rule %s has no engine specified, skipping\n", rule.ID) - } - continue - } - - // Get files that match this rule's selector - files, err := v.selectFilesForRule(path, &rule) - if err != nil { - fmt.Printf("āš ļø Failed to select files for rule %s: %v\n", rule.ID, err) - continue - } - - if len(files) == 0 { - if v.verbose { - fmt.Printf(" Rule %s: no matching files\n", rule.ID) - } - continue - } - - // Get or create engine - engine, err := v.registry.Get(engineName) - if err != nil { - fmt.Printf("āš ļø Engine %s not found for rule %s: %v\n", engineName, rule.ID, err) - continue - } - - // Initialize engine if needed - if err := v.initializeEngine(engine); err != nil { - fmt.Printf("āš ļø Failed to initialize engine %s: %v\n", engineName, err) - continue - } - - // Convert schema.PolicyRule to core.Rule - coreRule := convertToCoreRule(rule) - - // Execute validation - if v.verbose { - fmt.Printf(" Rule %s (%s): checking %d file(s)...\n", rule.ID, engineName, len(files)) - } - - validationResult, err := engine.Validate(v.ctx, coreRule, files) - if err != nil { - fmt.Printf("āš ļø Validation failed for rule %s: %v\n", rule.ID, err) - continue - } - - // Collect violations - if validationResult != nil && len(validationResult.Violations) > 0 { - for _, coreViolation := range validationResult.Violations { - violation := Violation{ - RuleID: rule.ID, - Severity: rule.Severity, - Message: coreViolation.Message, - File: coreViolation.File, - Line: coreViolation.Line, - Column: coreViolation.Column, - } - - // Use custom message if provided - if rule.Message != "" { - violation.Message = rule.Message - } - - result.Violations = append(result.Violations, violation) - } - - if v.verbose { - fmt.Printf(" āŒ Found %d violation(s)\n", len(validationResult.Violations)) - } - } else if v.verbose { - fmt.Printf(" āœ“ Passed\n") - } - } - - // Determine overall pass/fail - result.Passed = len(result.Violations) == 0 - - if v.verbose { - if result.Passed { - fmt.Printf("\nāœ… Validation passed: no violations found\n") - } else { - fmt.Printf("\nāŒ Validation failed: %d violation(s) found\n", len(result.Violations)) - } - } - - return result, nil -} - -// CanAutoFix checks if violations can be auto-fixed -func (v *Result) CanAutoFix() bool { - for _, violation := range v.Violations { - // Check if rule has autofix enabled - _ = violation - } - return false -} - -// AutoFix attempts to automatically fix violations -func (v *Validator) AutoFix(result *Result) error { - // TODO: Implement auto-fix logic - return fmt.Errorf("auto-fix not implemented yet") -} - // Helper functions // getEngineName extracts the engine name from a rule @@ -255,43 +75,6 @@ func getEngineName(rule schema.PolicyRule) string { return "" } -// selectFilesForRule selects files that match the rule's selector -func (v *Validator) selectFilesForRule(basePath string, rule *schema.PolicyRule) ([]string, error) { - // If path is a specific file, check if it matches the selector - fileInfo, err := os.Stat(basePath) - if err != nil { - return nil, err - } - - if !fileInfo.IsDir() { - // Single file - check if it matches selector - if rule.When != nil { - lang := GetLanguageFromFile(basePath) - if len(rule.When.Languages) > 0 { - matched := false - for _, ruleLang := range rule.When.Languages { - if ruleLang == lang { - matched = true - break - } - } - if !matched { - return []string{}, nil - } - } - } - return []string{basePath}, nil - } - - // Directory - use selector to find files - if rule.When == nil { - // No selector, use all files in directory - return v.selector.SelectFiles(nil) - } - - return v.selector.SelectFiles(rule.When) -} - // initializeEngine initializes an engine if not already initialized func (v *Validator) initializeEngine(engine core.Engine) error { // Create engine config @@ -380,10 +163,19 @@ func (v *Validator) ValidateChanges(ctx context.Context, changes []GitChange) (* } // Collect all changed files + // Convert absolute paths to relative paths for RBAC checking changedFiles := make([]string, 0, len(changes)) for _, change := range changes { if change.Status != "D" { // Skip deleted files - changedFiles = append(changedFiles, change.FilePath) + filePath := change.FilePath + // Convert absolute path to relative path from workDir + if filepath.IsAbs(filePath) { + relPath, err := filepath.Rel(v.workDir, filePath) + if err == nil { + filePath = relPath + } + } + changedFiles = append(changedFiles, filePath) } } diff --git a/test_validator.go b/test_validator.go index 8f2ea97..cc612fe 100644 --- a/test_validator.go +++ b/test_validator.go @@ -1,8 +1,10 @@ package main import ( + "context" "fmt" "os" + "os/exec" "github.com/DevSymphony/sym-cli/internal/validator" "github.com/DevSymphony/sym-cli/pkg/schema" @@ -79,16 +81,38 @@ func main() { fmt.Printf("Testing: %s\n", file) fmt.Printf("ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•\n\n") - result, err := v.Validate(file) + // Create GitChange from file using git diff + cmd := exec.Command("git", "diff", "--no-index", "/dev/null", file) + diffOutput, err := cmd.CombinedOutput() + // git diff --no-index returns exit code 1 when there are differences (expected) + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + // Expected - continue + } else { + fmt.Printf("āŒ Failed to generate diff: %v\n\n", err) + continue + } + } + + changes := []validator.GitChange{{ + FilePath: file, + Status: "A", + Diff: string(diffOutput), + }} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) if err != nil { fmt.Printf("āŒ Validation error: %v\n\n", err) continue } - if result.Passed { - fmt.Printf("\nāœ… PASSED: No violations\n\n") + if len(result.Violations) == 0 { + fmt.Printf("\nāœ… PASSED: No violations\n") + fmt.Printf(" Checked: %d, Passed: %d, Failed: %d\n\n", result.Checked, result.Passed, result.Failed) } else { - fmt.Printf("\nāŒ FAILED: %d violation(s) found:\n", len(result.Violations)) + fmt.Printf("\nāŒ FAILED: %d violation(s) found\n", len(result.Violations)) + fmt.Printf(" Checked: %d, Passed: %d, Failed: %d\n", result.Checked, result.Passed, result.Failed) for i, violation := range result.Violations { fmt.Printf("\n%d. [%s] %s\n", i+1, violation.Severity, violation.RuleID) fmt.Printf(" File: %s:%d:%d\n", violation.File, violation.Line, violation.Column) diff --git a/testdata/rbac/code-policy.json b/testdata/rbac/code-policy.json new file mode 100644 index 0000000..627da68 --- /dev/null +++ b/testdata/rbac/code-policy.json @@ -0,0 +1,71 @@ +{ + "version": "1.0.0", + "rbac": { + "roles": { + "admin": { + "permissions": [ + { + "path": "**/*", + "read": true, + "write": true, + "execute": false + } + ] + }, + "developer": { + "permissions": [ + { + "path": "src/**", + "read": true, + "write": true, + "execute": false + }, + { + "path": "tests/**", + "read": true, + "write": true, + "execute": false + }, + { + "path": "src/core/**", + "read": true, + "write": false, + "execute": false + }, + { + "path": "src/api/**", + "read": true, + "write": false, + "execute": false + } + ] + }, + "viewer": { + "permissions": [ + { + "path": "**/*", + "read": true, + "write": false, + "execute": false + } + ] + } + } + }, + "rules": [], + "enforce": { + "stages": [ + "pre-commit" + ], + "fail_on": [ + "error" + ], + "rbac": { + "enabled": true, + "stages": [ + "pre-commit" + ], + "on_violation": "block" + } + } +} diff --git a/testdata/rbac/config/settings.json b/testdata/rbac/config/settings.json new file mode 100644 index 0000000..3086182 --- /dev/null +++ b/testdata/rbac/config/settings.json @@ -0,0 +1,15 @@ +{ + "app": { + "name": "Test Application", + "version": "1.0.0", + "environment": "development" + }, + "api": { + "baseURL": "https://api.example.com", + "timeout": 30000 + }, + "features": { + "authentication": true, + "logging": true + } +} diff --git a/testdata/rbac/src/api/client.js b/testdata/rbac/src/api/client.js new file mode 100644 index 0000000..38cd34b --- /dev/null +++ b/testdata/rbac/src/api/client.js @@ -0,0 +1,21 @@ +// API client module - Developer CANNOT modify this file +class APIClient { + constructor(baseURL) { + this.baseURL = baseURL; + } + + async get(endpoint) { + const response = await fetch(`${this.baseURL}${endpoint}`); + return response.json(); + } + + async post(endpoint, data) { + const response = await fetch(`${this.baseURL}${endpoint}`, { + method: 'POST', + body: JSON.stringify(data), + }); + return response.json(); + } +} + +module.exports = APIClient; diff --git a/testdata/rbac/src/components/Button.js b/testdata/rbac/src/components/Button.js new file mode 100644 index 0000000..2c2ac2c --- /dev/null +++ b/testdata/rbac/src/components/Button.js @@ -0,0 +1,12 @@ +// Button component - Developer can modify this file +class Button { + constructor(label) { + this.label = label; + } + + render() { + return ``; + } +} + +module.exports = Button; diff --git a/testdata/rbac/src/core/engine.js b/testdata/rbac/src/core/engine.js new file mode 100644 index 0000000..c54e77a --- /dev/null +++ b/testdata/rbac/src/core/engine.js @@ -0,0 +1,19 @@ +// Core engine module - Developer CANNOT modify this file +class Engine { + constructor(config) { + this.config = config; + this.running = false; + } + + start() { + this.running = true; + console.log("Engine started"); + } + + stop() { + this.running = false; + console.log("Engine stopped"); + } +} + +module.exports = Engine; diff --git a/testdata/rbac/tests/test.js b/testdata/rbac/tests/test.js new file mode 100644 index 0000000..4953ace --- /dev/null +++ b/testdata/rbac/tests/test.js @@ -0,0 +1,10 @@ +// Test file - Developer can modify this file +const Button = require('../src/components/Button'); + +describe('Button', () => { + it('should render button with label', () => { + const button = new Button('Click me'); + const html = button.render(); + expect(html).toBe(''); + }); +}); diff --git a/tests/integration/helper.go b/tests/integration/helper.go index f5b02cd..ff26358 100644 --- a/tests/integration/helper.go +++ b/tests/integration/helper.go @@ -2,6 +2,7 @@ package integration import ( "os" + "os/exec" "path/filepath" "testing" @@ -54,7 +55,27 @@ func loadPolicyFromTestdata(t *testing.T, relativePath string) *schema.CodePolic // createTestValidator creates a validator with given policy and registers cleanup func createTestValidator(t *testing.T, pol *schema.CodePolicy) *validator.Validator { t.Helper() + + // Save current directory + originalDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current directory: %v", err) + } + + // Change to repo root so Validator.workDir is set correctly + repoRoot := getTestdataDir(t) + if err := os.Chdir(repoRoot); err != nil { + t.Fatalf("Failed to change to repo root: %v", err) + } + + // Create validator (it will use repo root as workDir) v := validator.NewValidator(pol, testing.Verbose()) + + // Change back to original directory + if err := os.Chdir(originalDir); err != nil { + t.Fatalf("Failed to change back to original directory: %v", err) + } + t.Cleanup(func() { if err := v.Close(); err != nil { t.Logf("Warning: failed to close validator: %v", err) @@ -63,15 +84,53 @@ func createTestValidator(t *testing.T, pol *schema.CodePolicy) *validator.Valida return v } +// createGitChangeFromFile creates a GitChange from a test file using git diff +func createGitChangeFromFile(t *testing.T, filePath string) validator.GitChange { + t.Helper() + + // Use git diff --no-index to generate a unified diff + // This treats the file as newly added (comparing /dev/null to the file) + cmd := exec.Command("git", "diff", "--no-index", "/dev/null", filePath) + output, err := cmd.CombinedOutput() + + // git diff --no-index returns exit code 1 when there are differences (which is expected) + // We only care about the output, not the exit code + if err != nil { + // Check if it's just the expected exit code 1 + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + // This is expected - there are differences between /dev/null and the file + } else { + // This is an actual error + t.Fatalf("Failed to generate diff for %s: %v", filePath, err) + } + } + + // Use absolute path for FilePath + // The adapters (checkstyle/pmd) will handle path resolution correctly + absPath := filePath + if !filepath.IsAbs(filePath) { + // If relative, make it absolute + cwd, _ := os.Getwd() + absPath, _ = filepath.Abs(filepath.Join(cwd, filePath)) + } + + return validator.GitChange{ + FilePath: absPath, + Status: "A", // Treat as Added file + Diff: string(output), + } +} + // assertViolationsDetected asserts that violations are found and logs them -func assertViolationsDetected(t *testing.T, result *validator.Result) { +func assertViolationsDetected(t *testing.T, result *validator.ValidationResult) { t.Helper() - assert.False(t, result.Passed, "Should detect violations") assert.Greater(t, len(result.Violations), 0, "Should have violations") + assert.Greater(t, result.Failed, 0, "Should have failed checks") - // Log violations for debugging + // Log violations and metrics for debugging if len(result.Violations) > 0 { - t.Logf("Found %d violation(s):", len(result.Violations)) + t.Logf("Found %d violation(s): Checked=%d, Passed=%d, Failed=%d", + len(result.Violations), result.Checked, result.Passed, result.Failed) for i, v := range result.Violations { t.Logf(" %d. [%s] %s at %s:%d:%d (severity: %s)", i+1, v.RuleID, v.Message, v.File, v.Line, v.Column, v.Severity) @@ -80,18 +139,83 @@ func assertViolationsDetected(t *testing.T, result *validator.Result) { } // assertNoPolicyViolations asserts that no violations are found -func assertNoPolicyViolations(t *testing.T, result *validator.Result) { +func assertNoPolicyViolations(t *testing.T, result *validator.ValidationResult) { t.Helper() - if !result.Passed || len(result.Violations) > 0 { + if len(result.Violations) > 0 { // Log violations if any for debugging - if len(result.Violations) > 0 { - t.Logf("Unexpected violations found:") - for i, v := range result.Violations { - t.Logf(" %d. [%s] %s at %s:%d:%d", - i+1, v.RuleID, v.Message, v.File, v.Line, v.Column) - } + t.Logf("Unexpected violations found: Checked=%d, Passed=%d, Failed=%d", + result.Checked, result.Passed, result.Failed) + for i, v := range result.Violations { + t.Logf(" %d. [%s] %s at %s:%d:%d", + i+1, v.RuleID, v.Message, v.File, v.Line, v.Column) } } - assert.True(t, result.Passed, "Should pass validation") assert.Equal(t, 0, len(result.Violations), "Should have no violations") + assert.Equal(t, 0, result.Failed, "Should have no failed checks") + if result.Checked > 0 { + assert.Equal(t, result.Checked, result.Passed, "All checks should pass") + } +} + +// setupRBACEnvironment sets up the environment for RBAC testing +// by setting the GIT_AUTHOR_NAME environment variable +func setupRBACEnvironment(t *testing.T, username string) { + t.Helper() + t.Setenv("GIT_AUTHOR_NAME", username) + t.Logf("Set GIT_AUTHOR_NAME=%s for RBAC testing", username) +} + +// createRBACTestValidator creates a validator with RBAC testdata policy +// and sets up the git user environment +func createRBACTestValidator(t *testing.T, username string) *validator.Validator { + t.Helper() + setupRBACEnvironment(t, username) + pol := loadPolicyFromTestdata(t, "testdata/rbac/code-policy.json") + return createTestValidator(t, pol) +} + +// assertRBACViolation asserts that an RBAC violation occurred +// for the expected user +func assertRBACViolation(t *testing.T, result *validator.ValidationResult, expectedUser string) { + t.Helper() + + // Find RBAC violations (RuleID is "rbac-permission-denied") + rbacViolations := []validator.Violation{} + for _, v := range result.Violations { + if v.RuleID == "rbac-permission-denied" { + rbacViolations = append(rbacViolations, v) + } + } + + assert.Greater(t, len(rbacViolations), 0, "Should have RBAC violations") + + // Log RBAC violations for debugging + if len(rbacViolations) > 0 { + t.Logf("Found %d RBAC violation(s) for user '%s':", len(rbacViolations), expectedUser) + for i, v := range rbacViolations { + t.Logf(" %d. %s (file: %s)", i+1, v.Message, v.File) + } + } +} + +// assertNoRBACViolation asserts that no RBAC violations occurred +func assertNoRBACViolation(t *testing.T, result *validator.ValidationResult) { + t.Helper() + + // Check for RBAC violations (RuleID is "rbac-permission-denied") + rbacViolations := []validator.Violation{} + for _, v := range result.Violations { + if v.RuleID == "rbac-permission-denied" { + rbacViolations = append(rbacViolations, v) + } + } + + if len(rbacViolations) > 0 { + t.Logf("Unexpected RBAC violations found:") + for i, v := range rbacViolations { + t.Logf(" %d. %s (file: %s)", i+1, v.Message, v.File) + } + } + + assert.Equal(t, 0, len(rbacViolations), "Should have no RBAC violations") } diff --git a/tests/integration/validator_policy_test.go b/tests/integration/validator_policy_test.go index a36a576..53e44f9 100644 --- a/tests/integration/validator_policy_test.go +++ b/tests/integration/validator_policy_test.go @@ -1,9 +1,11 @@ package integration import ( + "context" "path/filepath" "testing" + "github.com/DevSymphony/sym-cli/internal/validator" "github.com/stretchr/testify/require" ) @@ -26,7 +28,10 @@ func TestValidator_JavaScript_Pattern_Violations(t *testing.T) { // Test naming violations t.Run("NamingViolations", func(t *testing.T) { filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/pattern/naming-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) }) @@ -34,7 +39,10 @@ func TestValidator_JavaScript_Pattern_Violations(t *testing.T) { // Test security violations t.Run("SecurityViolations", func(t *testing.T) { filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/pattern/security-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) }) @@ -49,7 +57,10 @@ func TestValidator_JavaScript_Pattern_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/pattern/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -68,7 +79,10 @@ func TestValidator_JavaScript_Length_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/length/length-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -82,7 +96,10 @@ func TestValidator_JavaScript_Length_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/length/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -101,7 +118,10 @@ func TestValidator_JavaScript_Style_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/style/style-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -115,7 +135,10 @@ func TestValidator_JavaScript_Style_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/style/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -134,7 +157,10 @@ func TestValidator_JavaScript_AST_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/ast/naming-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -148,7 +174,10 @@ func TestValidator_JavaScript_AST_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/ast/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -167,7 +196,10 @@ func TestValidator_TypeScript_TypeChecker_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/typescript/typechecker/type-errors.ts") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -181,7 +213,10 @@ func TestValidator_TypeScript_TypeChecker_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/typescript/typechecker/valid.ts") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -200,7 +235,10 @@ func TestValidator_Java_Pattern_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/pattern/NamingViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -214,7 +252,10 @@ func TestValidator_Java_Pattern_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/pattern/ValidNaming.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -233,7 +274,10 @@ func TestValidator_Java_Length_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/length/LengthViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -247,7 +291,10 @@ func TestValidator_Java_Length_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/length/ValidLength.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -266,7 +313,10 @@ func TestValidator_Java_Style_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/style/StyleViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -280,7 +330,10 @@ func TestValidator_Java_Style_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/style/ValidStyle.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -299,7 +352,10 @@ func TestValidator_Java_AST_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/ast/AstViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -313,7 +369,10 @@ func TestValidator_Java_AST_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/ast/ValidAst.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } diff --git a/tests/integration/validator_rbac_test.go b/tests/integration/validator_rbac_test.go new file mode 100644 index 0000000..ad634fe --- /dev/null +++ b/tests/integration/validator_rbac_test.go @@ -0,0 +1,322 @@ +package integration + +import ( + "context" + "path/filepath" + "testing" + + "github.com/DevSymphony/sym-cli/internal/validator" +) + +// TestValidator_RBAC_AdminFullAccess verifies that admin role has full write access +func TestValidator_RBAC_AdminFullAccess(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "alice") // alice is admin + + // Admin should be able to modify any file + testFiles := []string{ + "testdata/rbac/src/components/Button.js", + "testdata/rbac/src/core/engine.js", + "testdata/rbac/src/api/client.js", + "testdata/rbac/tests/test.js", + "testdata/rbac/config/settings.json", + } + + for _, file := range testFiles { + filePath := filepath.Join(getTestdataDir(t), file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", file, err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ Admin successfully modified: %s", file) + } +} + +// TestValidator_RBAC_DeveloperAllowedFiles verifies that developer can modify allowed files +func TestValidator_RBAC_DeveloperAllowedFiles(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Developer should be able to modify src/components/* and tests/* + testFiles := []string{ + "testdata/rbac/src/components/Button.js", + "testdata/rbac/tests/test.js", + } + + for _, file := range testFiles { + filePath := filepath.Join(getTestdataDir(t), file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", file, err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ Developer successfully modified: %s", file) + } +} + +// TestValidator_RBAC_DeveloperDeniedCoreFiles verifies that developer cannot modify core files +func TestValidator_RBAC_DeveloperDeniedCoreFiles(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Developer should NOT be able to modify src/core/* + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Developer correctly blocked from modifying core file") +} + +// TestValidator_RBAC_DeveloperDeniedAPIFiles verifies that developer cannot modify API files +func TestValidator_RBAC_DeveloperDeniedAPIFiles(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Developer should NOT be able to modify src/api/* + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/api/client.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Developer correctly blocked from modifying API file") +} + +// TestValidator_RBAC_ViewerDenied verifies that viewer role has no write access +func TestValidator_RBAC_ViewerDenied(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "frank") // frank is viewer + + // Viewer should NOT be able to modify any file + testFiles := []string{ + "testdata/rbac/src/components/Button.js", + "testdata/rbac/tests/test.js", + "testdata/rbac/config/settings.json", + } + + for _, file := range testFiles { + filePath := filepath.Join(getTestdataDir(t), file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", file, err) + } + + assertRBACViolation(t, result, "frank") + t.Logf("āœ“ Viewer correctly blocked from modifying: %s", file) + } +} + +// TestValidator_RBAC_MixedPermissions verifies mixed file permissions in a single commit +func TestValidator_RBAC_MixedPermissions(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Mix of allowed and denied files + allowedFile := filepath.Join(getTestdataDir(t), "testdata/rbac/src/components/Button.js") + deniedFile := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + + changes := []validator.GitChange{ + createGitChangeFromFile(t, allowedFile), + createGitChangeFromFile(t, deniedFile), + } + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + // Should have RBAC violation for the denied file + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Mixed permissions correctly detected RBAC violation") +} + +// TestValidator_RBAC_DeletedFilesSkipped verifies that deleted files don't trigger RBAC +func TestValidator_RBAC_DeletedFilesSkipped(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Deleted files should not trigger RBAC violations + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + + changes := []validator.GitChange{ + { + FilePath: filePath, + Status: "D", // Deleted + Diff: "--- a/src/core/engine.js\n+++ /dev/null\n", + }, + } + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ Deleted files correctly skipped RBAC validation") +} + +// TestValidator_RBAC_Disabled verifies that RBAC can be disabled +func TestValidator_RBAC_Disabled(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + setupRBACEnvironment(t, "charlie") // charlie is developer + + // Load a policy with RBAC disabled + pol := loadPolicyFromTestdata(t, "testdata/rbac/code-policy.json") + + // Disable RBAC for this test + if pol.Enforce.RBACConfig != nil { + pol.Enforce.RBACConfig.Enabled = false + } + + v := createTestValidator(t, pol) + + // Developer should be able to modify core files when RBAC is disabled + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ RBAC disabled - no violations detected") +} + +// TestValidator_RBAC_UnknownUser verifies behavior with unknown users +func TestValidator_RBAC_UnknownUser(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "unknown_user") // User not in roles.json + + // Unknown user should have no permissions (default to viewer-like behavior) + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/components/Button.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + // Unknown user should be denied (no role = no permissions) + assertRBACViolation(t, result, "unknown_user") + t.Logf("āœ“ Unknown user correctly denied access") +} + +// TestValidator_RBAC_GlobPatternMatching verifies glob pattern matching works correctly +func TestValidator_RBAC_GlobPatternMatching(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Test various glob patterns + testCases := []struct { + file string + shouldAllow bool + description string + }{ + { + file: "testdata/rbac/src/components/Button.js", + shouldAllow: true, + description: "src/** allows src/components/*", + }, + { + file: "testdata/rbac/src/core/engine.js", + shouldAllow: false, + description: "src/core/** deny overrides src/** allow", + }, + { + file: "testdata/rbac/src/api/client.js", + shouldAllow: false, + description: "src/api/** deny overrides src/** allow", + }, + { + file: "testdata/rbac/tests/test.js", + shouldAllow: true, + description: "tests/** is allowed", + }, + } + + for _, tc := range testCases { + filePath := filepath.Join(getTestdataDir(t), tc.file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", tc.file, err) + } + + if tc.shouldAllow { + assertNoRBACViolation(t, result) + t.Logf("āœ“ Glob pattern test passed: %s (allowed)", tc.description) + } else { + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Glob pattern test passed: %s (denied)", tc.description) + } + } +}