feat: add RBAC (Role-Based Access Control) support by sehwan505 · Pull Request #9 · DevSymphony/sym-cli

sehwan505 · 2025-11-12T05:21:50Z

Add RBAC role definitions and validation logic
Implement export command for conventions and roles
Add test-rbac utility command
Include integration tests for RBAC functionality

- Add RBAC role definitions and validation logic - Implement export command for conventions and roles - Add test-rbac utility command - Include integration tests for RBAC functionality 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

…rintln - Update TestComplexRBACPatterns to match actual role structure - Replace frontend-dev/senior-dev with admin/developer/viewer - Add test case for viewer (read-only) role - Fix redundant newline in fmt.Println (go vet error)

Copilot

Pull Request Overview

This PR adds Role-Based Access Control (RBAC) support to the sym-cli tool, enabling fine-grained file access control based on user roles. The implementation introduces pattern-based permission validation for file write operations and provides utilities for testing RBAC functionality.

Key Changes:

Implemented RBAC validation logic with glob pattern matching for file permissions
Added export command infrastructure (not yet fully implemented)
Created test-rbac utility for manual RBAC testing

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File	Description
internal/roles/rbac.go	Core RBAC implementation with pattern matching and file permission validation logic
internal/cmd/export.go	Export command scaffold for extracting conventions (implementation incomplete)
cmd/test-rbac/main.go	Manual testing utility for RBAC validation with hardcoded test scenarios
tests/integration/rbac_test.go	Integration tests for RBAC (currently all tests are skipped and provide no coverage)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-12T07:15:24Z

internal/roles/rbac.go

+	return path == pattern || strings.HasPrefix(path, pattern+"/")
+}
+
+// checkFilePermission checks if a single file is allowed for the given role


The function comment for checkFilePermission is missing. As this is an internal helper function that implements critical RBAC logic (deny-first, then allow-if-specified), it should have a clear comment explaining:

The precedence order (deny takes precedence over allow)

The default behavior when no patterns are specified

The return value semantics (true = allowed, false = denied)

Suggested change

// checkFilePermission checks if a single file is allowed for the given role

// checkFilePermission determines if a single file operation is permitted for the given role.

//

// Precedence: Deny patterns (DenyWrite) are checked first and take precedence over allow patterns.

// If any deny pattern matches, the file is denied regardless of allow patterns.

//

// Default behavior: If no allow patterns (AllowWrite) are specified, the file is allowed by default

// (unless denied by a deny pattern).

//

// Return value: Returns true if the file is allowed, false if denied.

Copilot · 2025-11-12T07:15:25Z

internal/roles/rbac.go

+// matchPattern checks if a file path matches a glob pattern
+// Supports ** (match any directory level) and * (match within directory)


The function matchPattern lacks documentation. This is a critical pattern-matching function that supports multiple glob patterns (**, *, exact matches, directory prefixes). It should have comprehensive documentation explaining:

The supported pattern types with examples

How ** differs from *

Special handling for trailing slashes

Expected behavior for edge cases

This is especially important since the function is not exported and its behavior is tested indirectly.

Suggested change

// matchPattern checks if a file path matches a glob pattern

// Supports ** (match any directory level) and * (match within directory)

// matchPattern checks if a file path matches a glob pattern.

//

// Supported pattern types:

// - Exact match: The pattern matches the path exactly.

// Example: pattern = "foo/bar.txt", path = "foo/bar.txt" => true

// - Directory prefix match (trailing slash): The pattern ends with a slash and matches any path with that prefix.

// Example: pattern = "foo/", path = "foo/bar.txt" => true

// - Single asterisk '*' (within directory): Matches any sequence of non-separator characters within a single directory level.

// Example: pattern = "foo/*.txt", path = "foo/bar.txt" => true

// pattern = "foo/*.txt", path = "foo/baz/bar.txt" => false

// - Double asterisk '**' (any directory level): Matches any sequence of characters, including path separators, across directory levels.

// Example: pattern = "foo/**/bar.txt", path = "foo/a/b/bar.txt" => true

// pattern = "foo/**", path = "foo/a/b/c.txt" => true

// pattern = "**/bar.txt", path = "a/b/bar.txt" => true

//

// Difference between '*' and '**':

// - '*' matches within a single directory level (does not cross '/').

// - '**' matches across any number of directory levels (can cross '/').

//

// Special handling for trailing slashes:

// - If the pattern ends with '/', it matches any path with that prefix.

// Example: pattern = "foo/", path = "foo/bar.txt" => true

// Example: pattern = "foo/", path = "foo/bar/baz.txt" => true

//

// Edge cases:

// - If the pattern is empty, only an empty path matches.

// - If the pattern is '*', it matches any file in the current directory (no subdirectories).

// - If the pattern is '**', it matches any path.

// - Patterns with both '*' and '**' are supported as described above.

//

// Returns true if the path matches the pattern, false otherwise.

Copilot · 2025-11-12T07:15:25Z

cmd/test-rbac/main.go

+	// Change to test directory
+	if err := os.Chdir("/tmp/rbac-test"); err != nil {
+		fmt.Printf("❌ Failed to change directory: %v\n", err)
+		return
+	}


The hardcoded path /tmp/rbac-test may not exist and will cause the utility to fail immediately. Consider using a configurable path via command-line flag or environment variable, or creating the directory if it doesn't exist. Additionally, provide a more helpful error message explaining what the user needs to set up before running this utility.

Copilot · 2025-11-12T07:15:25Z

internal/cmd/export.go

+func init() {
+	rootCmd.AddCommand(exportCmd)
+
+	exportCmd.Flags().StringP("context", "c", "", "work context description")
+	exportCmd.Flags().StringP("format", "f", "text", "output format (text|json|markdown)")
+	exportCmd.Flags().StringSlice("files", []string{}, "files being modified")
+	exportCmd.Flags().StringSlice("languages", []string{}, "programming languages involved")
+}


The export command is registered with rootCmd in its init() function, but according to root.go lines 53-54, this command is marked as TODO and commented out in the main command registration. This creates an inconsistency - the command registers itself but is documented as not being included. Either uncomment the registration in root.go or remove the init() function from this file to avoid confusion.

Suggested change

func init() {

rootCmd.AddCommand(exportCmd)

exportCmd.Flags().StringP("context", "c", "", "work context description")

exportCmd.Flags().StringP("format", "f", "text", "output format (text|json|markdown)")

exportCmd.Flags().StringSlice("files", []string{}, "files being modified")

exportCmd.Flags().StringSlice("languages", []string{}, "programming languages involved")

}

Copilot · 2025-11-12T07:15:26Z

internal/cmd/export.go

+	exportCmd.Flags().StringSlice("files", []string{}, "files being modified")
+	exportCmd.Flags().StringSlice("languages", []string{}, "programming languages involved")


The flags --files and --languages are defined but never used in the command implementation. They are retrieved from the command flags but ignored. Either implement the logic to use these flags or remove them if they're not needed yet.

Copilot · 2025-11-12T07:15:26Z

internal/roles/rbac.go

+				// Exact match or contains the path
+				return strings.Contains(remaining, suffix) || strings.HasSuffix(remaining, suffix)


The matchPattern function has an edge case bug with patterns containing **. When a pattern like src/**/test is matched against paths, the logic on line 80 uses both strings.Contains(remaining, dir+"/") and strings.HasPrefix(remaining, dir+"/") which are redundant (HasPrefix is a special case of Contains). More importantly, this logic may incorrectly match paths. For example, pattern src/**/test would incorrectly match src/testing because strings.Contains("testing", "test") returns true. The suffix check should use exact path segment matching or strings.HasSuffix() for the exact suffix.

Suggested change

// Exact match or contains the path

return strings.Contains(remaining, suffix) || strings.HasSuffix(remaining, suffix)

// Exact match on path segment or as suffix

segments := strings.Split(remaining, "/")

for _, seg := range segments {

if seg == suffix {

return true

}

}

// Also check if the path ends with the suffix as a complete segment

return strings.HasSuffix(remaining, "/"+suffix) || remaining == suffix

Copilot · 2025-11-12T07:15:26Z

internal/roles/rbac.go

+	if strings.Contains(pattern, "**") {
+		parts := strings.Split(pattern, "**")
+		if len(parts) == 2 {
+			prefix := strings.TrimSuffix(parts[0], "/")
+			suffix := strings.TrimPrefix(parts[1], "/")
+
+			// Check prefix
+			if prefix != "" && !strings.HasPrefix(path, prefix) {
+				return false
+			}
+
+			// Check suffix
+			if suffix != "" {
+				// Remove prefix from path
+				remaining := path
+				if prefix != "" {
+					remaining = strings.TrimPrefix(path, prefix+"/")
+				}
+
+				// Check if suffix matches
+				if suffix == "*" {
+					return true
+				}
+				if strings.HasSuffix(suffix, "/*") {
+					// Match directory and any file in it
+					dir := strings.TrimSuffix(suffix, "/*")
+					return strings.Contains(remaining, dir+"/") || strings.HasPrefix(remaining, dir+"/")
+				}
+				// Exact match or contains the path
+				return strings.Contains(remaining, suffix) || strings.HasSuffix(remaining, suffix)
+			}
+			return true
+		}
+	}


The pattern matching logic doesn't handle patterns with multiple ** segments (e.g., src/**/test/**/file.js). The code on line 56 splits by ** and only handles the case where len(parts) == 2. Patterns with more than one ** will fall through to the simple * pattern matching on line 90-92, which will fail for these complex patterns. Consider adding validation to reject unsupported patterns or implement full support for multiple ** segments.

Copilot · 2025-11-12T07:15:26Z

internal/roles/rbac.go

+	deniedFiles := []string{}
+	for _, file := range files {
+		if !checkFilePermission(file, &roleConfig) {
+			deniedFiles = append(deniedFiles, file)
+		}
+	}
+
+	// Return result
+	if len(deniedFiles) == 0 {
+		return &ValidationResult{
+			Allowed:     true,
+			DeniedFiles: []string{},


[nitpick] Inconsistent empty slice initialization. Throughout this file, empty slices are initialized as []string{} (lines 153, 156, 171, 182), but Go's idiomatic approach is to use nil for empty slices when the slice will be checked for length or appended to. This is a minor inconsistency but using nil would be more idiomatic and save a small allocation.

BlueLemon18 and others added 3 commits November 12, 2025 05:15

Merge origin/main into feature/rbac

3a5a9f3

BlueLemon18 force-pushed the feature/rbac branch from 1cf0927 to dff3749 Compare November 12, 2025 06:19

sehwan505 requested a review from Copilot November 12, 2025 07:10

sehwan505 merged commit 61612b5 into main Nov 12, 2025
6 checks passed

Copilot started reviewing on behalf of sehwan505 November 12, 2025 07:11 View session

Copilot finished reviewing on behalf of sehwan505 November 12, 2025 07:13

Copilot AI reviewed Nov 12, 2025

View reviewed changes

sehwan505 deleted the feature/rbac branch December 10, 2025 04:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add RBAC (Role-Based Access Control) support#9

feat: add RBAC (Role-Based Access Control) support#9
sehwan505 merged 3 commits intomainfrom
feature/rbac

sehwan505 commented Nov 12, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-// checkFilePermission checks if a single file is allowed for the given role
+// checkFilePermission determines if a single file operation is permitted for the given role.
+//
+// Precedence: Deny patterns (DenyWrite) are checked first and take precedence over allow patterns.
+// If any deny pattern matches, the file is denied regardless of allow patterns.
+//
+// Default behavior: If no allow patterns (AllowWrite) are specified, the file is allowed by default
+// (unless denied by a deny pattern).
+//
+// Return value: Returns true if the file is allowed, false if denied.

		// matchPattern checks if a file path matches a glob pattern
		// Supports ** (match any directory level) and * (match within directory)

-// matchPattern checks if a file path matches a glob pattern
-// Supports ** (match any directory level) and * (match within directory)
+// matchPattern checks if a file path matches a glob pattern.
+//
+// Supported pattern types:
+//   - Exact match: The pattern matches the path exactly.
+//       Example: pattern = "foo/bar.txt", path = "foo/bar.txt" => true
+//   - Directory prefix match (trailing slash): The pattern ends with a slash and matches any path with that prefix.
+//       Example: pattern = "foo/", path = "foo/bar.txt" => true
+//   - Single asterisk '*' (within directory): Matches any sequence of non-separator characters within a single directory level.
+//       Example: pattern = "foo/*.txt", path = "foo/bar.txt" => true
+//                pattern = "foo/*.txt", path = "foo/baz/bar.txt" => false
+//   - Double asterisk '**' (any directory level): Matches any sequence of characters, including path separators, across directory levels.
+//       Example: pattern = "foo/**/bar.txt", path = "foo/a/b/bar.txt" => true
+//                pattern = "foo/**", path = "foo/a/b/c.txt" => true
+//                pattern = "**/bar.txt", path = "a/b/bar.txt" => true
+//
+// Difference between '*' and '**':
+//   - '*' matches within a single directory level (does not cross '/').
+//   - '**' matches across any number of directory levels (can cross '/').
+//
+// Special handling for trailing slashes:
+//   - If the pattern ends with '/', it matches any path with that prefix.
+//     Example: pattern = "foo/", path = "foo/bar.txt" => true
+//     Example: pattern = "foo/", path = "foo/bar/baz.txt" => true
+//
+// Edge cases:
+//   - If the pattern is empty, only an empty path matches.
+//   - If the pattern is '*', it matches any file in the current directory (no subdirectories).
+//   - If the pattern is '**', it matches any path.
+//   - Patterns with both '*' and '**' are supported as described above.
+//
+// Returns true if the path matches the pattern, false otherwise.

		exportCmd.Flags().StringSlice("files", []string{}, "files being modified")
		exportCmd.Flags().StringSlice("languages", []string{}, "programming languages involved")

		// Exact match or contains the path
		return strings.Contains(remaining, suffix) \|\| strings.HasSuffix(remaining, suffix)

-				// Exact match or contains the path
-				return strings.Contains(remaining, suffix) || strings.HasSuffix(remaining, suffix)
+				// Exact match on path segment or as suffix
+				segments := strings.Split(remaining, "/")
+				for _, seg := range segments {
+					if seg == suffix {
+						return true
+					}
+				}
+				// Also check if the path ends with the suffix as a complete segment
+				return strings.HasSuffix(remaining, "/"+suffix) || remaining == suffix

Conversation

sehwan505 commented Nov 12, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants