Skip to content

feat: acl endpoint#45

Merged
Pertempto merged 51 commits intomainfrom
007-acl-endpoint
Oct 18, 2025
Merged

feat: acl endpoint#45
Pertempto merged 51 commits intomainfrom
007-acl-endpoint

Conversation

@Pertempto
Copy link
Contributor

@Pertempto Pertempto commented Oct 4, 2025

Resolves #41

Also did a TON of other clean up and fixes

- Verified go.mod contains required dependencies (Gin, testify, crypto, uuid)
- Confirmed project structure with src/handlers, src/models, etc.
- Ready for linting configuration
- Added Makefile with lint target using gofmt and go vet
- Includes test, build, clean, and fmt targets
- Enables consistent code formatting and static analysis
- Added failing contract test for ACL endpoint
- Tests POST /api/v1/acl with valid ACL event data
- Expects 200 OK response (will fail until endpoint implemented)
- Follows existing test patterns with auth middleware and ACL setup
- Added failing integration test for ACL submission flow
- Tests POST /api/v1/acl followed by GET /api/v1/events to verify storage
- Sets up users, API keys, and ACL permissions
- Expects ACL event to be stored and retrievable (will fail until implemented)
- Added failing test for ACL event rejection through /events endpoint
- Tests that submitting ACL events via /events returns 400 Bad Request
- Sets up user permissions and attempts forbidden ACL submission
- Expects rejection (will fail until rejection logic implemented)
- Added failing test for ACL validation on dedicated endpoint
- Tests POST /api/v1/acl with invalid data (empty user field)
- Expects 400 Bad Request for validation errors
- Sets up user permissions and API authentication
- Will fail until validation logic implemented
- Added ACLEvent struct with User, Item, Action fields
- Implemented Validate() method with required field checks
- Added control character validation for security
- Follows existing model patterns in the codebase
…l.go

- Implemented PostACL handler with JSON binding and validation
- Converts ACL events to regular events with current timestamp
- Stores events using existing storage interface
- Returns appropriate HTTP status codes for success/error cases
- Follows existing handler patterns in the codebase
…L events

- Added check in PostEvents to reject ACL events (item == '.acl' and action starts with '.acl.')
- Returns 400 Bad Request with descriptive error message
- Prevents submission of ACL events with arbitrary timestamps
- Maintains existing validation and permission checks
- ACL events are stored as regular events via existing SaveEvents/LoadEvents interface
- No changes required to storage layer - reuses existing event storage mechanism
- ACL events stored with item='.acl' and action='.acl.allow' for identification
… in main.go

- Added POST /api/v1/acl route with existing auth middleware
- Ensures ACL endpoint requires API key authentication
- Follows same pattern as other protected endpoints
- Route registered in auth group with AuthMiddleware
…_validation_test.go

- Added comprehensive unit tests for ACLEvent.Validate() method
- Tests valid events, empty fields, and control character validation
- Covers all validation rules: required fields and security checks
- Uses table-driven tests for maintainability
…cs/acl.mdx

- Updated ACL submission instructions to reference dedicated POST /api/v1/acl endpoint
- Added note that ACL events via /events are rejected
- Updated ACL Management section with new endpoint details and JSON format
- Clarified authentication and timestamp handling requirements
…c/content/docs/internal-events.mdx

- Updated ACL section to reflect new API-triggered creation via dedicated endpoint
- Clarified that ACL events are created automatically with current timestamp
- Noted that action is always .acl.allow (deny not supported)
- Changed trigger from User to API to reflect server-generated events
…cs/api/v1.md

- Added complete documentation for new POST /api/v1/acl endpoint
- Updated POST /api/v1/events to note ACL event rejection
- Included request/response examples and error codes
- Documented authentication and ACL validation requirements
- Verified quickstart.md test scenarios are implemented
- Dedicated ACL endpoint accepts valid events with current timestamp
- ACL events are stored and retrievable via /events
- ACL events via /events are rejected with 400 error
- Invalid data returns appropriate validation errors
- Authentication is properly enforced
- All quickstart validation steps completed successfully
- Added 'type' field to ACLEvent struct (allow/deny)
- Updated validation to require valid type
- Modified handler to create .acl.allow or .acl.deny events based on type
- Updated all tests, contracts, docs to include type field
- Removed incorrect note about deny rules not being supported
- All ACL rules now support both allow and deny types
- Removed src/models/acl.go and custom ACLEvent struct
- Updated PostACL handler to use models.AclRule directly
- Added validation functions directly in handler
- Updated all tests to use AclRule structure
- Removed unit test file (validation now in handler)
- Updated contracts, data-model, and tasks to reflect changes
- Maintains all functionality while using existing codebase patterns
- Changed test to use .root user instead of user-123
- Use default admin API key 'sk_ATlUSWpdQVKROfmh47z7q60KjlkQcCaC9ps181Jov8E'
- Simplified test setup by removing unnecessary user creation
- Maintains same test logic for ACL event rejection
- Reverted to user-123 as the default user
- Use default API key 'sk_ATlUSWpdQVKROfmh47z7q60KjlkQcCaC9ps181Jov8E' for user-123
- Updated ACL event to use user-123
- Maintains simplified test setup without dynamic key generation
- Removed manual creation of user-123 since NewTestHandlersWithStorage already handles it
- Simplified test setup further
- Test now relies on default user setup provided by test handlers
- Removed boilerplate files: plan.md, research.md, data-model.md, quickstart.md, tasks.md, contracts/
- Created compact spec.md summarizing the JWT authentication feature
- Preserved key technical decisions, implementation details, and validation checklist
- Maintained essential information for future reference while removing redundant content
@Pertempto
Copy link
Contributor Author

/opencode please fix the changelog

@github-actions
Copy link
Contributor

The CHANGELOG references PR #44 but this is PR #45 - please update to reference the correct PR number for accurate tracking.

@github-actions
Copy link
Contributor

The type assertion userId.(string) on line 58 could panic if the context value isn't a string. Consider using the safe assertion from line 23-27 for consistency.

@github-actions
Copy link
Contributor

Add validation to prevent submission of empty ACL rule arrays, similar to the suggestion in previous comments but not yet implemented.

@Pertempto
Copy link
Contributor Author

/opencode please fix the changelog

@opencode-agent
Copy link

opencode-agent bot commented Oct 16, 2025

Failed to parse the text response

opencode session  |  github run

@github-actions
Copy link
Contributor

Add validation to reject empty ACL rule arrays to prevent unnecessary processing. Add this after binding the JSON but before the validation loop:

if len(aclRules) == 0 {
    c.JSON(http.StatusBadRequest, gin.H{"error": "At least one ACL rule required"})
    return
}

@github-actions
Copy link
Contributor

In src/handlers/acl.go, the validateAclRule function should take the rule by value instead of pointer since it doesn't modify the rule, and clean up the redundant type assertion.

@github-actions
Copy link
Contributor

In doc/src/content/docs/acl.mdx, the example requests still show the old format with .acl.allow action and payload containing user/item/action. They should be updated to show .acl.addRule action with the new payload format including the type field.

- Add Action Values section explaining custom action support
- Clarify that action names can be any strings beyond examples
@github-actions
Copy link
Contributor

Nit: In validateAclRule, pass the rule by value instead of pointer since it doesn't modify the rule.

@Pertempto Pertempto self-assigned this Oct 18, 2025
@github-actions
Copy link
Contributor

In src/handlers/user.go, lines 58 and 85: The type assertions callerUserId.(string) could panic if the context value isn't a string. Consider using safe assertions like in PostAcl to prevent runtime errors.

if callerUserIdStr, ok := callerUserId.(string); !ok {
    log.Printf("... unexpected user_id type")
    c.JSON(http.StatusInternalServerError, gin.H{"error": "Internal server error"})
    return
}
// use callerUserIdStr

@github-actions
Copy link
Contributor

@Pertempo, in src/handlers/user.go, lines 40, 58, and 85: The type assertions callerUserId.(string) lack safe type assertions, which could panic if the context value isn't a string. Please use safe assertions with error handling.

if callerUserIdStr, ok := callerUserId.(string); !ok {
    log.Printf("unexpected user_id type")
    c.JSON(http.StatusInternalServerError, gin.H{"error": "Internal server error"})
    return
}
// use callerUserIdStr

@github-actions
Copy link
Contributor

In src/handlers/acl.go, the validateAclRule function checks patterns but doesn't explicitly validate that required fields (user, item, action) are non-empty. The error messages are generic and could benefit from specific validation of empty required fields.

func validateAclRule(rule models.AclRule) error {
    if strings.TrimSpace(rule.User) == "" {
        return errors.New("user field is required")
    }
    if strings.TrimSpace(rule.Item) == "" {
        return errors.New("item field is required")
    }
    if strings.TrimSpace(rule.Action) == "" {
        return errors.New("action field is required")
    }
    if rule.Type != "allow" && rule.Type != "deny" {
        return errors.New("type must be either 'allow' or 'deny'")
    }
    // existing pattern validations...
    return nil
}

@github-actions
Copy link
Contributor

@Pertempto, the code review looks good! I've reviewed the ACL endpoint implementation and have some minor feedback. Can you take a look at these points?

@Pertempto
Copy link
Contributor Author

This is far from perfect but this PR is big enough and accomplishes most of my main objectives. Merging for now and cleaning up minor things later.

@Pertempto Pertempto merged commit 8608401 into main Oct 18, 2025
5 checks passed
@Pertempto Pertempto deleted the 007-acl-endpoint branch October 18, 2025 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add ACL rule endpoint

1 participant