Skip to content

Phase 3: Secret Sharing & Access Control (RBAC Integration) #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 16, 2025
Merged

Phase 3: Secret Sharing & Access Control (RBAC Integration) #185

merged 3 commits into from
Nov 16, 2025

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 16, 2025

📌 Related Issues

Fixes #182
Part of: #173

📝 Summary

Implements full REST API for Secret CRUD and Sharing functionality with RBAC integration, completing Phase 3 of the Secret Management System Epic.

🎯 What This PR Does

Secret CRUD API (5 endpoints)

  • GET /v1/secrets - List user's secrets with pagination
  • POST /v1/secrets - Create secret with encrypted fields
  • GET /v1/secrets/{id} - View secret details
  • PATCH /v1/secrets/{id} - Update secret with version increment
  • DELETE /v1/secrets/{id} - Soft delete secret

Secret Sharing API (3 endpoints)

  • POST /v1/secrets/{id}/shares - Grant read/write/admin access
  • GET /v1/secrets/{id}/shares - List all shares
  • DELETE /v1/secrets/{id}/shares/{shareId} - Revoke access

Key Features

  • XOR Constraint: Share with user OR role (not both)
  • Permission Hierarchy: admin > write > read
  • Optional Expiration: Time-limited access support
  • Owner Authorization: Only secret owners can grant/revoke shares
  • Automatic Version Incrementing: Updates increment version field

🔧 Implementation Details

Files Created

  • app/Http/Controllers/Api/V1/SecretController.php - CRUD controller (5 methods)
  • app/Http/Controllers/Api/V1/SecretShareController.php - Sharing controller (3 methods)
  • app/Http/Requests/StoreSecretRequest.php - Create validation
  • app/Http/Requests/UpdateSecretRequest.php - Update validation
  • app/Http/Requests/GrantShareRequest.php - Share validation with XOR enforcement
  • app/Policies/SecretPolicy.php - CRUD authorization
  • app/Policies/SecretSharePolicy.php - Sharing authorization

Files Modified

  • routes/api.php - Added 8 new routes under /v1/ prefix
  • app/Providers/AppServiceProvider.php - Policy registration
  • app/Http/Controllers/Controller.php - Added AuthorizesRequests trait
  • tests/Pest.php - Enhanced createTestSecret() helper
  • CHANGELOG.md - Comprehensive feature documentation

Test Coverage

  • 17 SecretController tests - CRUD operations with authorization
  • 18 SecretShareController tests - Grant/list/revoke with XOR validation
  • Total: 107 Secret-related tests (35 Controller + 13 Model + rest)
  • Full test suite: 390 tests passing

✅ Quality Gates

  • ✅ All Tests Passing: 390 tests (1288 assertions)
  • ✅ Pint: Code style compliant (0 issues)
  • ✅ PHPStan: Level max passing (0 errors) - Fixed 14 type warnings with @var DocBlocks
  • ✅ REUSE: 3.3 compliant
  • ✅ CHANGELOG: Updated with detailed documentation

⚠️ Known Limitations

  • Tenant Resolution: Uses temporary TenantKey::first() pattern
    TODO: Implement TenantMiddleware to inject tenant_id into requests

  • Admin Permission: SecretSharePolicy currently owner-only
    TODO: Add support for users with 'admin' permission to grant/revoke shares

🚫 Breaking Changes

None - all new functionality

🧪 Testing

# Run Secret-related tests
ddev exec "php artisan test --filter='Secret' --no-coverage"

# Full test suite
ddev exec "php artisan test --parallel --no-coverage"

# Quality gates
./vendor/bin/pint --test
./vendor/bin/phpstan analyse

📚 Documentation

See CHANGELOG.md for comprehensive API documentation including:

  • All endpoint descriptions
  • Validation rules
  • XOR constraint behavior
  • Permission hierarchy
  • Test coverage breakdown

🔐 Security Considerations

  • Owner-based authorization via Policies
  • XOR constraint enforced at validation level (prevents DB errors)
  • All encrypted fields remain encrypted at rest
  • No plaintext logging

📝 Checklist

  • Tests added/updated and passing (35 Controller tests)
  • PHPStan level max passing (14 warnings fixed)
  • Pint code style compliant
  • REUSE 3.3 compliant
  • CHANGELOG.md updated
  • OpenAPI spec updated (separate PR in contracts repo)
  • Self-review completed (4-Pass Review minimum 2x iterations)

Estimated LOC: ~1520 lines (14 files changed)
Test Coverage: 35 Controller tests, 13 Model tests
Related: Issue #184 (PHPStan type warnings - now resolved)

@kevalyq
feat: Secret Sharing & Access Control (Phase 3) - Issue #182
58c9dd6 
Implements full REST API for password manager Secret CRUD and Sharing functionality.

## Secret CRUD API (5 endpoints)
- GET /v1/secrets - List user's secrets with pagination
- POST /v1/secrets - Create secret with encrypted fields
- GET /v1/secrets/{id} - View secret details
- PATCH /v1/secrets/{id} - Update secret with version increment
- DELETE /v1/secrets/{id} - Soft delete secret

## Secret Sharing API (3 endpoints)
- POST /v1/secrets/{id}/shares - Grant read/write/admin access
- GET /v1/secrets/{id}/shares - List all shares
- DELETE /v1/secrets/{id}/shares/{shareId} - Revoke access

## Implementation Details
- XOR constraint: share with user OR role (not both)
- Permission hierarchy: admin > write > read
- Optional expiration dates for time-limited access
- Owner-based authorization via SecretPolicy and SecretSharePolicy
- Validation via StoreSecretRequest, UpdateSecretRequest, GrantShareRequest
- Automatic version incrementing on updates

## Test Coverage
- 17 SecretController tests (all passing)
- 18 SecretShareController tests (all passing)
- Total: 390 tests passing (107 Secret-related)

## Known Issues
- PHPStan: 14 type warnings in SecretController (tracked in #184)
- Tenant resolution uses temporary TenantKey::first() pattern (TODO: TenantMiddleware)

## Breaking Changes
None - all new functionality

Closes #182
Related: #184 (non-blocking PHPStan warnings)
@kevalyq kevalyq added the large-pr-approved Legitimate large PR (e.g., boilerplate templates, auto-generated code) label Nov 16, 2025
@codecov
Copy link

codecov bot commented Nov 16, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 96.65272% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
app/Policies/SecretPolicy.php 71.42% 4 Missing ⚠️
app/Http/Controllers/Api/V1/SecretController.php 96.87% 3 Missing ⚠️
app/Policies/SecretSharePolicy.php 87.50% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@kevalyq kevalyq marked this pull request as ready for review November 16, 2025 18:32
Copilot AI review requested due to automatic review settings November 16, 2025 18:32
Copilot finished reviewing on behalf of kevalyq November 16, 2025 18:36
Copilot AI reviewed Nov 16, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the Secret CRUD and Sharing REST API endpoints (Phase 3), adding full password manager functionality with role-based access control. The implementation includes 8 new API endpoints for creating, reading, updating, deleting, and sharing secrets, with comprehensive validation and authorization.

Key changes:

  • Secret CRUD API with 5 endpoints supporting encrypted storage, pagination, and soft deletes
  • Secret Sharing API with 3 endpoints for granting/revoking access with XOR constraint enforcement
  • Authorization policies for owner-based and share-based access control

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
app/Http/Controllers/Api/V1/SecretController.php Implements 5 CRUD endpoints for secrets with owner authorization
app/Http/Controllers/Api/V1/SecretShareController.php Implements 3 sharing endpoints with XOR validation and permission hierarchy
app/Http/Requests/StoreSecretRequest.php Validation rules for secret creation
app/Http/Requests/UpdateSecretRequest.php Validation rules for secret updates
app/Http/Requests/GrantShareRequest.php XOR constraint validation for sharing
app/Policies/SecretPolicy.php Authorization policy for secret access (owner-only currently)
app/Policies/SecretSharePolicy.php Authorization policy for share management
app/Providers/AppServiceProvider.php Registers new policies
app/Http/Controllers/Controller.php Adds AuthorizesRequests trait
routes/api.php Registers 8 new v1 API routes
tests/Pest.php Enhanced helper with additional secret field support
tests/Feature/Controllers/Api/V1/SecretControllerTest.php 17 comprehensive CRUD tests
tests/Feature/Controllers/Api/V1/SecretShareControllerTest.php 18 comprehensive sharing tests
CHANGELOG.md Documents new API features

@kevalyq
fix: Resolve all Copilot review feedback
cdb9360 
- SecretPolicy: Implement userHasPermission() calls (CRITICAL)
- SecretController: Add viewAny authorization, fix error handling
- Tests: Add 6 share-based access tests (read/write/admin permissions)
- Tests: Remove duplicate code
- SecretController: Fix DocBlock accuracy, improve error messages

Fixes all 11 Copilot review comments (8 functional, 3 nitpicks addressed).
All 23 SecretController tests + 18 SecretShare tests passing.
@kevalyq kevalyq requested a review from Copilot November 16, 2025 18:52
Copilot finished reviewing on behalf of kevalyq November 16, 2025 18:56
Copilot AI reviewed Nov 16, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

@kevalyq
refactor: Address all Copilot review nitpicks
4667552 
- Add missing create authorization in store()
- Use active() scope in SecretShareController
- Remove useless filter test (filter=owned not implemented)
- Fix route ordering (nested routes before parameterized)
- Fix version inflation (only increment if modified)
- Refactor field assignment to private helper method
- Reduce code duplication in store() and update()

All 53 Secret/SecretShare tests passing (211 assertions).
@kevalyq kevalyq merged commit 90de9d8 into main Nov 16, 2025
17 checks passed
@kevalyq kevalyq deleted the feat/issue-182-secret-sharing branch November 16, 2025 19:08
This was referenced Nov 16, 2025
Closed
Open
Merged
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

large-pr-approved Legitimate large PR (e.g., boilerplate templates, auto-generated code)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Phase 3: Secret Sharing & Access Control (RBAC Integration)

2 participants

@kevalyq