Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 19, 2025

📌 Summary

Resolves #190 by replacing the hardcoded TenantKey::first() workaround in SecretController::store() with a proper middleware-based tenant resolution system.


🔧 What Changed

Added

  • InjectTenantId Middleware (app/Http/Middleware/InjectTenantId.php)
    • Automatically injects tenant_id into request for Secret routes
    • Single-tenant development mode: Uses first available TenantKey
    • Production-ready pattern: Can be extended for user-based tenant resolution
    • Returns 503 when no TenantKey exists
    • Respects pre-existing tenant_id (SetTenant compatibility)

Modified

  • SecretController::store(): Removed hardcoded tenant logic and TODO comment
  • bootstrap/app.php: Registered middleware as tenant.inject alias
  • routes/api.php: Applied middleware to all /v1/secrets and /v1/attachments routes
  • CHANGELOG.md: Documented fix in Unreleased section

Tests

  • InjectTenantIdMiddlewareTest.php: 5 comprehensive tests
    • Injects tenant_id from first available TenantKey
    • Returns 503 when no TenantKey exists
    • Does not overwrite existing tenant_id
    • Works with authenticated users
    • Handles multiple TenantKeys correctly

✅ Quality Gates

  • All 444 tests pass (100%)
  • PHPStan level max: ✓
  • Laravel Pint: ✓
  • REUSE 3.3 compliant: ✓
  • CHANGELOG.md updated
  • No breaking changes

🎯 Implementation Details

Current (Single-Tenant Mode):

// Middleware uses first available TenantKey
$tenantId = TenantKey::oldest('id')->value('id');

Future (Multi-Tenant Production):
The middleware is structured to easily support:

  • User-based tenant resolution ($user->tenant_id)
  • Subdomain-based resolution
  • JWT claim-based resolution

🔗 Related


📝 Migration Notes

No migration required - This is a transparent refactor. All existing Secret routes continue to work as before, but now use proper middleware instead of inline workaround.

Breaking Changes: None


Type: Bug Fix (Technical Debt)
Priority: Medium
LOC: +208, -20
Estimated Review Time: 10-15 minutes

…ID (#190)

Replaces TenantKey::first() workaround in SecretController with proper
middleware-based tenant resolution.

**What Changed:**
- Added InjectTenantId middleware for automatic tenant_id injection
- Registered as 'tenant.inject' alias in bootstrap/app.php
- Applied to all /v1/secrets and /v1/attachments routes
- Removed TODO comment and hardcoded tenant logic from SecretController
- Middleware respects pre-existing tenant_id (SetTenant compatibility)

**Implementation:**
- Single-tenant development mode: Uses first available TenantKey
- Production-ready pattern: Can be extended for user-based resolution
- Returns 503 when no TenantKey exists
- 5 comprehensive middleware tests added

**Tests:**
- All 444 Secret-related tests pass
- PHPStan level max: ✓
- Laravel Pint: ✓
- REUSE 3.3 compliant: ✓

Fixes #190
Copilot AI review requested due to automatic review settings November 19, 2025 09:46
@github-actions
Copy link

💡 Tip: Consider Using Draft PRs

Benefits of opening PRs as drafts initially:

  • 💰 Saves CI runtime and Copilot review credits
  • 🎯 Automatically sets linked issues to "🚧 In Progress" status
  • 🚀 Mark "Ready for review" when done to trigger full CI pipeline

How to convert:

  1. Click "Still in progress? Convert to draft" in the sidebar, OR
  2. Use gh pr ready when ready for review

This is just a friendly reminder - feel free to continue as is! 😊

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
app/Http/Controllers/Api/V1/SecretController.php 60.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copilot finished reviewing on behalf of kevalyq November 19, 2025 09:49
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 replaces the hardcoded TenantKey::first() workaround in SecretController::store() with a proper middleware-based tenant resolution system. The new InjectTenantId middleware automatically injects tenant_id into requests for Secret-related routes, enabling cleaner controller code while maintaining the single-tenant development workflow.

Key Changes

  • Introduced InjectTenantId middleware for automatic tenant resolution in single-tenant mode
  • Removed inline tenant resolution logic from SecretController::store()
  • Applied middleware to all /v1/secrets, /v1/attachments, and /v1/shares routes for consistent tenant handling

Reviewed Changes

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

Show a summary per file
File Description
app/Http/Middleware/InjectTenantId.php New middleware that injects tenant_id from first available TenantKey; returns 503 when none exists
app/Http/Controllers/Api/V1/SecretController.php Removed hardcoded tenant resolution and TODO; now reads tenant_id from request
bootstrap/app.php Registered InjectTenantId middleware with 'tenant.inject' alias
routes/api.php Applied tenant.inject middleware to Secret, SecretAttachment, and SecretShare route groups
tests/Feature/Middleware/InjectTenantIdMiddlewareTest.php Comprehensive test suite with 5 tests covering injection, 503 response, non-overwrite, authentication, and multiple tenants
CHANGELOG.md Documented the fix in Unreleased section with implementation details

- Improved error message clarity in InjectTenantId middleware
- Added tenant_id validation in SecretController::store()
- Updated middleware test to match new error message

Changes based on Copilot PR review comments.
@kevalyq kevalyq merged commit 1d45a63 into main Nov 19, 2025
25 checks passed
@kevalyq kevalyq deleted the fix/tenant-id-hardcoded-190 branch November 19, 2025 09:57
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.

Bug: Tenant ID hardcoded in SecretController::store() - needs proper resolution

2 participants