Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions cmd/test-rbac/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package main

import (
"fmt"
"os"

"github.com/DevSymphony/sym-cli/internal/roles"
)

func main() {
// Change to test directory
if err := os.Chdir("/tmp/rbac-test"); err != nil {
fmt.Printf("❌ Failed to change directory: %v\n", err)
return
}
Comment on lines +11 to +15
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

fmt.Println("🧪 RBAC 검증 테스트 시작")
fmt.Println("================================================================")

// Test scenarios
testCases := []struct {
name string
username string
files []string
}{
{
name: "Frontend Dev - 허용된 파일만",
username: "alice",
files: []string{
"src/components/Button.js",
"src/components/ui/Modal.js",
"src/hooks/useAuth.js",
},
},
{
name: "Frontend Dev - 거부된 파일 포함",
username: "alice",
files: []string{
"src/components/Button.js",
"src/core/engine.js",
"src/api/client.js",
},
},
{
name: "Senior Dev - 모든 파일",
username: "charlie",
files: []string{
"src/components/Button.js",
"src/core/engine.js",
"src/api/client.js",
"src/utils/helper.js",
},
},
{
name: "Viewer - 읽기 전용",
username: "david",
files: []string{
"src/components/Button.js",
},
},
{
name: "Frontend Dev - 혼합 케이스",
username: "bob",
files: []string{
"src/hooks/useData.js",
"src/core/config.js",
"src/utils/format.js",
"src/components/Header.js",
},
},
}

for i, tc := range testCases {
fmt.Printf("\n📋 테스트 %d: %s\n", i+1, tc.name)
fmt.Printf(" 사용자: %s\n", tc.username)
fmt.Printf(" 파일 수: %d개\n", len(tc.files))

result, err := roles.ValidateFilePermissions(tc.username, tc.files)
if err != nil {
fmt.Printf(" ❌ 오류: %v\n", err)
continue
}

if result.Allowed {
fmt.Printf(" ✅ 결과: 모든 파일 수정 가능\n")
} else {
fmt.Printf(" ❌ 결과: %d개 파일 수정 불가\n", len(result.DeniedFiles))
fmt.Printf(" 거부된 파일:\n")
for _, file := range result.DeniedFiles {
fmt.Printf(" - %s\n", file)
}
}
}

fmt.Println("\n================================================================")
fmt.Println("✅ 테스트 완료!")
}
40 changes: 40 additions & 0 deletions internal/cmd/export.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package cmd

import (
"fmt"

"github.com/spf13/cobra"
)

var exportCmd = &cobra.Command{
Use: "export [path]",
Short: "현재 작업에 필요한 컨벤션을 추출하여 반환합니다",
Long: `현재 작업 컨텍스트에 맞는 관련 컨벤션만 추출하여 반환합니다.
LLM이 작업 시 컨텍스트에 포함할 수 있도록 최적화된 형태로 제공됩니다.`,
Args: cobra.MaximumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
path := "."
if len(args) > 0 {
path = args[0]
}

context, _ := cmd.Flags().GetString("context")
format, _ := cmd.Flags().GetString("format")

fmt.Printf("Exporting conventions for: %s\n", path)
fmt.Printf("Context: %s\n", context)
fmt.Printf("Format: %s\n", format)

// TODO: 실제 내보내기 로직 구현
return nil
},
}

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")
Comment on lines +38 to +39
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
Comment on lines +33 to +40
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
190 changes: 190 additions & 0 deletions internal/roles/rbac.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package roles

import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/DevSymphony/sym-cli/internal/git"
"github.com/DevSymphony/sym-cli/internal/policy"
"github.com/DevSymphony/sym-cli/pkg/schema"
)

// ValidationResult represents the result of RBAC validation
type ValidationResult struct {
Allowed bool // true if all files are allowed, false if any are denied
DeniedFiles []string // list of files that are denied (empty if Allowed is true)
}

// GetUserPolicyPath returns the path to user-policy.json in the current repo
func GetUserPolicyPath() (string, error) {
repoRoot, err := git.GetRepoRoot()
if err != nil {
return "", err
}
return filepath.Join(repoRoot, ".sym", "user-policy.json"), nil
}

// LoadUserPolicyFromRepo loads user-policy.json from the current repository
func LoadUserPolicyFromRepo() (*schema.UserPolicy, error) {
policyPath, err := GetUserPolicyPath()
if err != nil {
return nil, err
}

// Check if file exists
if _, err := os.Stat(policyPath); os.IsNotExist(err) {
return nil, fmt.Errorf("user-policy.json not found at %s. Run 'sym init' to create it", policyPath)
}

// Use existing loader
loader := policy.NewLoader(false)
return loader.LoadUserPolicy(policyPath)
}

// matchPattern checks if a file path matches a glob pattern
// Supports ** (match any directory level) and * (match within directory)
Comment on lines +46 to +47
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The supported pattern types with examples
  2. How ** differs from *
  3. Special handling for trailing slashes
  4. 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 uses AI. Check for mistakes.
func matchPattern(pattern, path string) bool {
// Normalize paths
pattern = filepath.ToSlash(pattern)
path = filepath.ToSlash(path)

// Handle ** pattern (match any directory level)
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)
Comment on lines +82 to +83
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
return true
}
}
Comment on lines +54 to +87
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

// Handle simple * pattern
if strings.Contains(pattern, "*") {
matched, _ := filepath.Match(pattern, path)
return matched
}

// Exact match or prefix match
if strings.HasSuffix(pattern, "/") {
return strings.HasPrefix(path, pattern)
}

return path == pattern || strings.HasPrefix(path, pattern+"/")
}

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

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The precedence order (deny takes precedence over allow)
  2. The default behavior when no patterns are specified
  3. 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 uses AI. Check for mistakes.
func checkFilePermission(filePath string, role *schema.UserRole) bool {
// Check denyWrite first (deny takes precedence)
for _, denyPattern := range role.DenyWrite {
if matchPattern(denyPattern, filePath) {
return false
}
}

// If no allowWrite patterns, allow by default
if len(role.AllowWrite) == 0 {
return true
}

// Check allowWrite patterns
for _, allowPattern := range role.AllowWrite {
if matchPattern(allowPattern, filePath) {
return true
}
}

// Not explicitly allowed
return false
}

// ValidateFilePermissions validates if a user can modify the given files
// Returns ValidationResult with Allowed=true if all files are permitted,
// or Allowed=false with a list of denied files
func ValidateFilePermissions(username string, files []string) (*ValidationResult, error) {
// Get user's role (this internally loads roles.json)
userRole, err := GetUserRole(username)
if err != nil {
return nil, fmt.Errorf("failed to get user role: %w", err)
}

if userRole == "none" {
return &ValidationResult{
Allowed: false,
DeniedFiles: files, // All files denied if user has no role
}, nil
}

// Load user-policy.json
userPolicy, err := LoadUserPolicyFromRepo()
if err != nil {
return nil, fmt.Errorf("failed to load user policy: %w", err)
}

// Check if RBAC is defined in policy
if userPolicy.RBAC == nil || userPolicy.RBAC.Roles == nil {
// No RBAC defined, allow all files
return &ValidationResult{
Allowed: true,
DeniedFiles: []string{},
}, nil
}

// Get role configuration from policy
roleConfig, exists := userPolicy.RBAC.Roles[userRole]
if !exists {
// Role not defined in policy, deny all
return &ValidationResult{
Allowed: false,
DeniedFiles: files,
}, nil
}

// Check each file
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{},
Comment on lines +171 to +182
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copilot uses AI. Check for mistakes.
}, nil
}

return &ValidationResult{
Allowed: false,
DeniedFiles: deniedFiles,
}, nil
}
Loading
Loading