feat: implement SecretController with shared secrets filter (#187) #191

kevalyq · 2025-11-17T05:39:03Z

🎯 Summary

This PR implements the SecretController CRUD API with shared secrets filtering functionality as specified in issue #187.

📋 Changes

SecretController Enhancements

✅ Implemented filter parameter in index(): owned, shared, all
✅ Shared secrets filter uses whereHas('shares') with user_id/role_id checks
✅ Expires_at validation for active shares (non-expired only)
✅ All 5 CRUD endpoints: index, store, show, update, destroy

SecretPolicy Extensions

✅ Extended from 7 to 9 methods (added share(), viewShares())
✅ Both new methods use userHasPermission($user, 'admin') check
✅ Consistent permission hierarchy: admin > write > read

Documentation

✅ CHANGELOG.md updated with detailed implementation notes
✅ Created GitHub Issue Bug: Tenant ID hardcoded in SecretController::store() - needs proper resolution #190 for tenant resolution technical debt

✅ Quality Gates (Local)

✅ PHPStan Level Max: PASSED (no errors)
✅ Laravel Pint: PASSED (1 style issue auto-fixed)
✅ REUSE 3.3: PASSED (SPDX headers correct)
✅ Tests: 22 PASSED (101 assertions in 5.54s)

🔗 Related Issues

Fixes #187
Part of: #182

📝 Notes

Tenant resolution uses temporary TenantKey::first() pattern (documented in Bug: Tenant ID hardcoded in SecretController::store() - needs proper resolution #190)
All 10 acceptance criteria from Sub-Issue 182.1: SecretController + SecretPolicy (CRUD API) #187 met
PR remains DRAFT until all CI checks pass ✅

…187) - Extend SecretPolicy with share() and viewShares() methods (9 total) - Implement filter parameter in index() (owned/shared/all) - Share-based access respects expires_at validation - 22 comprehensive feature tests (require DDEV for execution) - All quality gates passed: PHPStan level max, Pint, REUSE - Create GitHub Issue #190 for tenant resolution tech debt - Update CHANGELOG.md with detailed implementation notes Part of: #182

codecov · 2025-11-17T05:40:58Z

Codecov Report

❌ Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
app/Models/Secret.php	91.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copilot

Pull Request Overview

This PR implements the SecretController's index() method with a new filtering mechanism that allows users to view owned, shared, or all secrets. It also extends the SecretPolicy with two new authorization methods for sharing functionality.

Adds filter parameter support (owned, shared, all) to the secrets listing endpoint
Extends SecretPolicy with share() and viewShares() authorization methods
Updates CHANGELOG to document the new filtering functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
app/Http/Controllers/Api/V1/SecretController.php	Added filter parameter logic with three modes: owned (user's secrets), shared (secrets shared with user via SecretShare), and all (combination of both)
app/Policies/SecretPolicy.php	Added two new policy methods (`share()`, `viewShares()`) for share-related authorization using admin permission checks
CHANGELOG.md	Updated documentation to reflect filter parameter options and clarified that default is now 'all' instead of 'owned'

app/Http/Controllers/Api/V1/SecretController.php

CHANGELOG.md

- Add 3 filter parameter tests (owned, shared, all) - covers lines 119-129 - Add 12 SecretPolicy tests (restore, forceDelete, share, viewShares) - SecretController: 97.4% coverage (only TenantKey error case missing) - SecretPolicy: 100% coverage - Total: 37 tests (121 assertions) Addresses TDD compliance - all new code paths now tested.

- Add IndexSecretRequest for filter parameter validation - Validates filter as one of: all, owned, shared - Returns clear error message for invalid values - Extract share filter logic to Secret::scopeSharedWith() - Eliminates DRY violation between 'shared' and 'default' cases - Improves maintainability and testability - Cache user role IDs to avoid N+1 query issues - Pluck role IDs once before match expression - Reuse across both 'shared' and 'default' filter cases - Fix CHANGELOG inconsistency: default filter is 'all', not 'owned' - Add PHPStan type hints for array<int> role IDs - Add test for invalid filter parameter validation All review comments from Copilot PR Reviewer addressed. Resolves: #191 review comments

kevalyq · 2025-11-18T18:14:43Z

✅ All Review Comments Addressed

I've systematically addressed all 5 Copilot review comments:

1. ✅ Filter Validation (Comment #2532795625)

Fixed: Created IndexSecretRequest with validation rule Rule::in(['all', 'owned', 'shared']) + custom error message. Added test case for invalid filter values (422 response).

2. ✅ N+1 Query - Role IDs (Comment #2532795638)

Fixed: Cached $roleIds = $user->roles->pluck('id')->toArray() before the match expression to prevent multiple DB queries.

3. ✅ Duplicate Role Query (Comment #2532795651)

Fixed: Same solution as #2 - single $roleIds variable reused in both 'shared' and 'default' cases.

4. ✅ Code Duplication - Share Filter Logic (Comment #2532795664)

Fixed: Extracted share filtering logic into Secret::scopeSharedWith() query scope. Both 'shared' and 'default' cases now use ->sharedWith($user, $roleIds) - DRY principle applied.

5. ✅ CHANGELOG Inconsistency (Comment #2532795685)

Fixed: Corrected CHANGELOG to reflect all as default filter (not owned).

Code Quality:

✅ PHPStan Level Max: PASSED
✅ Laravel Pint: PASSED
✅ REUSE 3.3: PASSED
✅ All Tests: 419 tests (1343 assertions) PASSED
✅ Codecov: SUCCESS

Changes Summary:

New: IndexSecretRequest (filter validation)
New: Secret::scopeSharedWith() (DRY query scope)
Refactored: SecretController::index() (validated request, cached role IDs, uses scope)
Fixed: CHANGELOG default filter documentation
Added: Test case for invalid filter validation

All CI checks green ✅

Copilot

Pull Request Overview

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

app/Models/Secret.php

app/Http/Controllers/Api/V1/SecretController.php

…dling - Use explicit match expression return value capture (clearer intent) - Skip orWhereIn for empty roleIds array (query optimization) - Update CHANGELOG with implementation details (filter validation, N+1 fix, DRY scope) Addresses Copilot review comments #2539257460 and #2539257492

kevalyq added 2 commits November 17, 2025 06:28

style(changelog): apply prettier formatting

511192d

style: apply prettier formatting (remove blank lines)

e781408

kevalyq marked this pull request as ready for review November 17, 2025 05:54

Copilot AI review requested due to automatic review settings November 17, 2025 05:54

Copilot started reviewing on behalf of kevalyq November 17, 2025 05:54 View session

Copilot finished reviewing on behalf of kevalyq November 17, 2025 05:56

Copilot AI reviewed Nov 17, 2025

View reviewed changes

kevalyq added 2 commits November 17, 2025 07:58

kevalyq requested a review from Copilot November 18, 2025 18:16

Copilot started reviewing on behalf of kevalyq November 18, 2025 18:17 View session

Copilot finished reviewing on behalf of kevalyq November 18, 2025 18:26

Copilot AI reviewed Nov 18, 2025

View reviewed changes

app/Models/Secret.php Outdated Show resolved Hide resolved

app/Http/Controllers/Api/V1/SecretController.php Outdated Show resolved Hide resolved

kevalyq merged commit c522006 into main Nov 18, 2025
17 checks passed

kevalyq deleted the feat/secret-controller-shared-filter-187 branch November 18, 2025 19:07

This was referenced Nov 19, 2025

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

Closed

[EPIC] Secret Management System (Password Vault) #173

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: implement SecretController with shared secrets filter (#187) #191

feat: implement SecretController with shared secrets filter (#187) #191

Uh oh!

kevalyq commented Nov 17, 2025

Uh oh!

codecov bot commented Nov 17, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevalyq commented Nov 18, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat: implement SecretController with shared secrets filter (#187) #191

feat: implement SecretController with shared secrets filter (#187) #191

Uh oh!

Conversation

kevalyq commented Nov 17, 2025

🎯 Summary

📋 Changes

SecretController Enhancements

SecretPolicy Extensions

Documentation

✅ Quality Gates (Local)

🔗 Related Issues

📝 Notes

Uh oh!

codecov bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevalyq commented Nov 18, 2025

✅ All Review Comments Addressed

1. ✅ Filter Validation (Comment #2532795625)

2. ✅ N+1 Query - Role IDs (Comment #2532795638)

3. ✅ Duplicate Role Query (Comment #2532795651)

4. ✅ Code Duplication - Share Filter Logic (Comment #2532795664)

5. ✅ CHANGELOG Inconsistency (Comment #2532795685)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Nov 17, 2025 •

edited

Loading