feat: Implement organization hierarchy and multi-tenancy system #209
Conversation
- Organization model with hierarchical structure support - OrganizationUser model for role-based access control - CloudProviderCredential model for cloud provider integration - TerraformDeployment model for infrastructure management - OrganizationService for business logic - ResourceProvisioningService for cloud resource management - OrganizationContext helper for context-aware operations - OrganizationController for REST API management - EnsureOrganizationContext middleware for enforcement - Livewire Components: - OrganizationManager: Full organization CRUD UI - OrganizationHierarchy: Visual hierarchy display - OrganizationSwitcher: User-facing organization switching - UserManagement: Role-based user management within organizations - Database migrations for all models and relationships - Test factories for comprehensive testing
WalkthroughThis pull request introduces a comprehensive organization management system with hierarchical support, role-based access control, and cloud infrastructure integration. It adds service contracts, helper utilities, API controllers, middleware, Livewire components, models for cloud credentials and Terraform deployments, and supporting database migrations for the organization and user relationship structures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller as OrganizationController
participant Service as OrganizationService
participant Cache as Cache
participant DB as Database
User->>Controller: POST /organizations
Controller->>Controller: Validate input
Controller->>Service: createOrganization(data, parent)
Service->>Service: Validate hierarchy
Service->>DB: Create organization
Service->>Cache: Invalidate org cache
Service->>DB: Attach user as owner
DB-->>Service: Organization created
Service-->>Controller: Organization instance
Controller-->>User: 201 Organization data
sequenceDiagram
participant User
participant Middleware as EnsureOrganizationContext
participant Service as OrganizationService
participant Auth as Auth Facade
User->>Middleware: Request
Middleware->>Auth: Check authentication
alt Authenticated
Middleware->>Auth: Get current_organization_id
alt Missing context
Middleware->>Service: getUserOrganizations(user)
Service-->>Middleware: Collection of orgs
Middleware->>Service: switchUserOrganization(user, first org)
Service->>Auth: Update user current_organization_id
else Has context
Middleware->>Service: canUserPerformAction('view_organization')
alt Access valid
Middleware->>User: Continue
else Access revoked
Middleware->>Service: Switch to first available org or clear
end
end
else Unauthenticated
Middleware->>User: Continue (passthrough)
end
sequenceDiagram
participant UI as Livewire Component
participant Service as OrganizationService
participant Model as Organization Model
participant License as Licensing Check
UI->>Service: canUserPerformAction(user, org, 'manage')
Service->>Model: Fetch user role & permissions
Service->>License: Check license feature availability
alt License valid & has feature
Service->>Service: Verify user permission level
alt User has permission
Service-->>UI: true
else No permission
Service-->>UI: false
end
else License invalid or feature unavailable
Service-->>UI: false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // Check if change would break relationships with children | ||
| foreach ($organization->children as $child) { | ||
| $this->validateHierarchyCreation($organization, $child->hierarchy_type); | ||
| } |
There was a problem hiding this comment.
Bug: Hierarchy validation uses old type instead of new
In validateHierarchyTypeChange, when checking if the new hierarchy type is valid with existing children, the code passes $organization to validateHierarchyCreation. This method reads $parent->hierarchy_type from the passed object, which still contains the OLD type before the update. The validation should check whether the NEW type ($newType) can have children of the existing child types, but instead it validates the old configuration which would always pass.
| { | ||
| $this->validate([ | ||
| 'newRole' => 'required|in:owner,admin,member,viewer', | ||
| ]); |
There was a problem hiding this comment.
Bug: Livewire validate checks non-existent property not method parameter
The updateUserRole method uses $this->validate(['newRole' => ...]) which in Livewire validates component properties, not method parameters. Since there is no $newRole property on this component (only $userRole), this validation either fails because the property doesn't exist, or validates null/missing data. The method parameter $newRole is never actually validated, allowing potentially invalid role values to pass through.
|
|
||
| if (! $user) { | ||
| return collect(); | ||
| } |
There was a problem hiding this comment.
Bug: Method returns wrong Collection type when no user
The getUserOrganizations method declares a return type of \Illuminate\Database\Eloquent\Collection but returns collect() when there's no authenticated user. The collect() helper returns \Illuminate\Support\Collection, not an Eloquent Collection. This type mismatch can cause issues when callers attempt to use Eloquent-specific methods on the returned collection.
| return response()->json([ | ||
| 'message' => 'Failed to create organization: '.$e->getMessage(), | ||
| ], 400); | ||
| } |
There was a problem hiding this comment.
Bug: Missing authorization check for parent when creating organizations
The store endpoint allows any authenticated user to create an organization under any existing parent organization by specifying a parent_organization_id. The validation only checks that the parent exists in the database (exists:organizations,id), but doesn't verify that the current user has permission to create child organizations under that parent. In contrast, update, hierarchy, and users endpoints all include authorization checks. This allows unauthorized users to establish hierarchy relationships with organizations they don't have access to.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
app/Livewire/Organization/OrganizationSwitcher.php-12-95 (1)
12-95: Verify collection serialization behavior in Livewire 3 with modern Laravel.Based on my verification of the codebase, I can confirm that the review comment identifies valid concerns:
Type Drift (Collection vs Array):
OrganizationContext::getUserOrganizations()has an explicit return type of\Illuminate\Database\Eloquent\Collection(line 95 of OrganizationContext.php). However,$userOrganizationsis initialized as[](array) on line 14. With Livewire 3.6.4 + Laravel 12, collections are serialized to JSON arrays during hydration cycles. Calling->contains()or->count()on a deserialized array would fail.Auth Guard:
auth()->user()is called at line 64 without a null check. TheswitchUserOrganization()method expects aUserparameter (non-nullable, per the interface at line 39 of OrganizationServiceInterface.php). If the session expires or auth state changes during a Livewire cycle, this would throw aTypeError.The defensive suggestions in the review—wrapping with
collect()and adding an auth guard—are appropriate safeguards for these real but edge-case scenarios.app/Models/CloudProviderCredential.php-252-302 (1)
252-302: Missing regions for Linode and Vultr providers.The
getAvailableRegions()method returns an empty array for Linode and Vultr, but these providers have well-defined regions. This may cause issues when trying to set regions for these providers.Consider adding region lists for Linode and Vultr, or update
setRegionto skip validation whengetAvailableRegions()returns empty.app/Models/Organization.php-95-106 (1)
95-106: Model method missingis_activecheck present in service layer.The
OrganizationService::canUserPerformActionchecks$userOrg->pivot->is_activebefore allowing actions, but this model method does not. This inconsistency could lead to inactive users bypassing restrictions when the model method is called directly.Consider adding the same check for consistency:
public function canUserPerformAction(User $user, string $action, $resource = null): bool { $userOrg = $this->users()->where('user_id', $user->id)->first(); - if (! $userOrg) { + if (! $userOrg || ! $userOrg->pivot->is_active) { return false; } $role = $userOrg->pivot->role;app/Models/TerraformDeployment.php-386-398 (1)
386-398:toArray()override may expose sensitive data.The
toArray()override callsparent::toArray()which includesterraform_statecontaining SSH keys. Consider filtering sensitive data before returning.public function toArray() { $array = parent::toArray(); + + // Remove sensitive state data + unset($array['terraform_state']); // Add computed properties $array['provider_name'] = $this->getProviderName();app/Models/Organization.php-113-133 (1)
113-133: Exception handling masks real errors and may return inconsistent data.Catching all exceptions and returning fallback values can hide database issues or configuration problems. The fallback also calls
$this->users()->count()which could throw the same exception, leading to a second failure.Consider logging the exception or narrowing the catch to specific expected exceptions:
} catch (\Exception $e) { - // Handle missing columns gracefully for development + \Log::warning('Failed to get full usage metrics', ['error' => $e->getMessage()]); return [ - 'users' => $this->users()->count(), + 'users' => 0, 'servers' => 0,app/Models/TerraformDeployment.php-156-161 (1)
156-161:setConfigValueandsetStateValuedo not persist changes.These methods modify the attribute in memory but don't call
save(). Callers must remember to save manually, which could lead to lost changes. Consider either documenting this behavior or providing a persist option.Either document the behavior:
+/** + * Set a configuration value. Call save() to persist changes. + */ public function setConfigValue(string $key, $value): voidOr add an optional persist parameter:
-public function setConfigValue(string $key, $value): void +public function setConfigValue(string $key, $value, bool $save = false): void { $config = $this->deployment_config ?? []; data_set($config, $key, $value); $this->deployment_config = $config; + if ($save) { + $this->save(); + } }app/Models/TerraformDeployment.php-265-288 (1)
265-288: Based on my verification, I can now provide the rewritten review comment. The review comment's concern is valid and well-founded.Provider check methods can throw if
providerCredentialis null.All
is*Deployment()methods callgetProviderName()which accesses$this->providerCredential->provider_namewithout null protection. Whileprovider_credential_idis required in the database schema, the relationship can still be null (as evidenced by the nullsafe operator used at line 170 for the same relationship). IfproviderCredentialis null, these methods will throw a "Call to a member function on null" error instead of returning false. Either add null checks togetProviderName()using a nullsafe operator (?->) or update the is*Deployment() methods to handle null gracefully.app/Http/Controllers/Api/OrganizationController.php-148-168 (1)
148-168: Email addresses in organization users endpoint are accessible only to organization owners and admins.The
usersendpoint returns email addresses, but access is already restricted: theOrganizationContext::can('view_organization', $organization)permission check enforces role-based access where only Owner and Admin roles can perform theview_organizationaction (percheckPermissionlogic in Organization model). Members and viewers cannot access this endpoint.While this design is intentional and security is enforced, there is no explicit documentation clarifying that email visibility is restricted to privileged roles. Consider adding clarity to endpoint documentation or inline comments if email visibility policies should be transparent to API consumers. Additionally, verify with your compliance team that this role-based exposure aligns with your GDPR/CCPA requirements.
🧹 Nitpick comments (24)
app/Services/ResourceProvisioningService.php (2)
23-210: Consider DRY-ing up repeated license/limit checks acrosscan*methods
canProvisionServer,canDeployApplication,canManageDomains, andcanProvisionInfrastructureall share the same control flow (fetch active license → feature flag check →checkUsageLimits→ resource-specific violation handling → remaining count). Consider extracting a private helper likecanProvisionResource(Organization $org, string $feature, string $violationType, string $remainingKey)to centralize this pattern and reduce future drift when license behavior changes.
215-268: Hard‑coded deployment options map might belong in configThe tier → options map is currently inlined in the service. If these options are likely to change, be localized, or vary per environment, consider moving them to a config file (e.g.,
config/licensing.php) or onto the license model to avoid code changes for content tweaks.database/migrations/2025_08_26_224900_create_organizations_table.php (1)
14-28: Consider adding a self‑referential FK (or updating the comment) forparent_organization_id.The comment says a foreign key will be added, but there is no FK defined;
parent_organization_idis only indexed. This allows orphaned parent references and can be confusing for future maintainers. Either add the FK here or clarify that it is intentionally unconstrained.Example inside the
Schema::createcallback:- $table->uuid('parent_organization_id')->nullable(); + $table->uuid('parent_organization_id')->nullable(); @@ - // Foreign key constraint will be added after table creation + $table->foreign('parent_organization_id') + ->references('id') + ->on('organizations') + ->nullOnDelete();Adjust the
onDeletebehavior (nullOnDeletevscascade) to match your intended hierarchy semantics.database/migrations/2025_08_26_225903_add_organization_id_to_servers_table.php (1)
14-18: Possible redundant index onorganization_id.The foreign key on
organization_idwill typically ensure the column is indexed by the database/driver. Adding an explicit$table->index('organization_id');may create a duplicate index depending on your database, which is wasted space.If your DB already indexes FK columns, you can safely drop the explicit index (and corresponding
dropIndexindown) to keep the schema lean.database/migrations/2025_08_26_230017_create_terraform_deployments_table.php (1)
14-29: Consider constraining or centralizing allowedstatusvalues.
statusis a free‑form string here (default'pending'). Over time it’s easy for typos or new, undocumented values to creep in, which complicates querying and state handling.Consider:
- Using an
enumcolumn (if your DB strategy allows), or- Keeping it as
stringbut centralizing allowed values in a PHP enum / constants and, optionally, a DB check constraint in a follow‑up migration.This keeps deployment state transitions predictable and queryable.
app/Http/Middleware/EnsureOrganizationContext.php (1)
39-54: Refresh the user after switching org in the access‑loss branch for consistency.In the “no current org” path you call
$user->refresh()afterswitchUserOrganization, but in the access‑loss path you don’t. IfOrganizationServiceInterface::switchUserOrganizationupdates the user via a fresh query, the in‑memoryAuth::user()may still hold a stalecurrent_organization_id/currentOrganizationfor the rest of the request.You can mirror the earlier behavior:
if (! $currentOrg || ! $this->organizationService->canUserPerformAction($user, $currentOrg, 'view_organization')) { @@ $organizations = $this->organizationService->getUserOrganizations($user); if ($organizations->isNotEmpty()) { $firstOrg = $organizations->first(); - $this->organizationService->switchUserOrganization($user, $firstOrg); + $this->organizationService->switchUserOrganization($user, $firstOrg); + $user->refresh(); } else { $user->update(['current_organization_id' => null]); }This keeps user context consistent regardless of which branch triggered the switch.
app/Livewire/Organization/OrganizationHierarchy.php (5)
12-16: Add type hints to public properties for Livewire hydration safety.Livewire 3 benefits from explicit type declarations on public properties for proper serialization/hydration. Without type hints, complex objects like
Organizationmay not rehydrate correctly.- public $rootOrganization = null; + public ?Organization $rootOrganization = null; - public $hierarchyData = []; + public array $hierarchyData = []; - public $expandedNodes = []; + public array $expandedNodes = [];
53-58: Consider limiting stack trace exposure in logs.Logging the full stack trace can be helpful for debugging but may expose sensitive information in production environments. Consider using a configurable approach or summarizing the trace.
\Log::error('Failed to load organization hierarchy', [ 'organization_id' => $this->rootOrganization->id, 'error' => $e->getMessage(), - 'trace' => $e->getTraceAsString(), + 'trace' => config('app.debug') ? $e->getTraceAsString() : null, ]);
75-96: Simplify toggleNode validation logic.The validation for
$organizationIdallows both numeric and string, but the subsequent array operations work regardless. The validation adds limited value since invalid IDs simply won't expand anything meaningful. Also, the try-catch is overly broad for a simple toggle operation.public function toggleNode($organizationId) { - try { - // Validate organization ID - if (! is_numeric($organizationId) && ! is_string($organizationId)) { - throw new \InvalidArgumentException('Invalid organization ID format'); - } - - if (isset($this->expandedNodes[$organizationId])) { - unset($this->expandedNodes[$organizationId]); - } else { - $this->expandedNodes[$organizationId] = true; - } - } catch (\Exception $e) { - \Log::error('Failed to toggle organization node', [ - 'organization_id' => $organizationId, - 'error' => $e->getMessage(), - ]); - - session()->flash('error', 'Failed to toggle organization view.'); + if (isset($this->expandedNodes[$organizationId])) { + unset($this->expandedNodes[$organizationId]); + } else { + $this->expandedNodes[$organizationId] = true; } }
98-135: Consider adding rate limiting for organization switching.The
switchToOrganizationmethod performs database lookups and permission checks. While the permission check provides some protection, consider whether rapid switching attempts should be throttled to prevent abuse.
137-147: Silent failure in getOrganizationUsage hides errors.Returning an empty array on exception silently masks potential issues. Consider logging the error for debugging while returning the empty fallback.
public function getOrganizationUsage($organizationId) { try { $organization = Organization::findOrFail($organizationId); $organizationService = app(OrganizationServiceInterface::class); return $organizationService->getOrganizationUsage($organization); } catch (\Exception $e) { + \Log::warning('Failed to load organization usage', [ + 'organization_id' => $organizationId, + 'error' => $e->getMessage(), + ]); return []; } }app/Models/CloudProviderCredential.php (2)
76-81:setCredentialsvalidates but doesn't persist - potential confusion.The method validates and sets the property but doesn't call
save(). This is inconsistent withmarkAsValidated()andmarkAsInvalid()which do save. Consider either documenting this behavior or making it consistent.public function setCredentials(array $credentials): void { // Validate credentials based on provider $this->validateCredentialsForProvider($credentials); $this->credentials = $credentials; + // Note: Call save() to persist changes }Alternatively, rename to
fillCredentials()to follow Laravel conventions for non-persisting setters.
241-249: 24-hour revalidation window is hardcoded.Consider making the revalidation interval configurable via config or a class constant for easier adjustment.
+ public const REVALIDATION_INTERVAL_HOURS = 24; + public function needsValidation(): bool { if (! $this->last_validated_at) { return true; } - // Re-validate every 24 hours - return $this->last_validated_at->isBefore(now()->subDay()); + return $this->last_validated_at->isBefore(now()->subHours(self::REVALIDATION_INTERVAL_HOURS)); }app/Livewire/Organization/UserManagement.php (3)
17-52: Add type hints to public properties.For better Livewire hydration and IDE support, add explicit type declarations.
- public $organization; + public Organization $organization; - public $showAddUserForm = false; + public bool $showAddUserForm = false; - public $showEditUserForm = false; + public bool $showEditUserForm = false; - public $selectedUser = null; + public ?User $selectedUser = null;
172-187: Redundant ownership check in removeUser.The check at line 183 is redundant because
isLastOwner($user)was already checked at line 176. If the user is the last owner, the method returns early before reaching line 183.public function removeUser(User $user) { try { // Prevent removing the last owner if ($this->isLastOwner($user)) { session()->flash('error', 'Cannot remove the last owner from the organization.'); return; } - // Prevent users from removing themselves unless they're not the last owner - if ($user->id === Auth::id() && $this->isLastOwner($user)) { - session()->flash('error', 'You cannot remove yourself as the last owner.'); - - return; - } - $organizationService = app(OrganizationServiceInterface::class);If you intended to allow non-owners to remove themselves, add a separate self-removal check without the
isLastOwnercondition.
220-232: Repeated queries in getUserRole and getUserPermissions.Both methods perform nearly identical queries. Consider a single helper method or caching the pivot data.
+ protected function getUserOrganizationPivot(User $user) + { + return $this->organization->users()->where('user_id', $user->id)->first()?->pivot; + } + public function getUserRole(User $user) { - $userOrg = $this->organization->users()->where('user_id', $user->id)->first(); - - return $userOrg?->pivot->role ?? 'unknown'; + return $this->getUserOrganizationPivot($user)?->role ?? 'unknown'; } public function getUserPermissions(User $user) { - $userOrg = $this->organization->users()->where('user_id', $user->id)->first(); - - return $userOrg?->pivot->permissions ?? []; + return $this->getUserOrganizationPivot($user)?->permissions ?? []; }app/Services/OrganizationService.php (2)
344-362: Unused$roleparameter in validateUserCanBeAttached.The
$roleparameter is passed but never used in the validation logic. Consider removing it or implementing role-specific validation if needed.- protected function validateUserCanBeAttached(Organization $organization, User $user, string $role): void + protected function validateUserCanBeAttached(Organization $organization, User $user): void { // Check if user is already in organization if ($organization->users()->where('user_id', $user->id)->exists()) {And update the call site at line 80:
- $this->validateUserCanBeAttached($organization, $user, $role); + $this->validateUserCanBeAttached($organization, $user);
451-464: Potential N+1 query issue in buildHierarchyTree.The recursive
buildHierarchyTreecallsusers()for each organization in the tree, which can lead to N+1 queries for deep hierarchies.Consider eager loading users with children or using a different approach:
protected function buildHierarchyTree(Organization $organization): array { - $children = $organization->children()->with('users')->get(); + $children = $organization->children; return [ 'id' => $organization->id, 'name' => $organization->name, 'hierarchy_type' => $organization->hierarchy_type, 'hierarchy_level' => $organization->hierarchy_level, - 'user_count' => $organization->users()->wherePivot('is_active', true)->count(), + 'user_count' => $organization->users->where('pivot.is_active', true)->count(), 'is_active' => $organization->is_active, 'children' => $children->map(fn ($child) => $this->buildHierarchyTree($child))->toArray(), ]; }And ensure the initial call eager loads the full tree with users.
app/Models/Organization.php (1)
164-187: Remove unused$resourceparameter or implement resource-level checks.The static analysis correctly identifies that
$resourceis never used incheckPermission. Either implement resource-level permission checks or remove the parameter to avoid confusion.If resource-level checks are not needed:
-protected function checkPermission(string $role, array $permissions, string $action, $resource = null): bool +protected function checkPermission(string $role, array $permissions, string $action): boolAnd update the caller in
canUserPerformActionaccordingly.app/Livewire/Organization/OrganizationManager.php (1)
330-342: Potential N+1 query ingetAvailableParents.Each organization in the collection makes a separate query to fetch users. Consider eager loading the user relationship when fetching organizations from the service.
return $organizationService->getUserOrganizations($user) + ->load(['users' => fn($q) => $q->where('user_id', Auth::id())]) ->filter(function ($org) { - $userOrg = $org->users()->where('user_id', Auth::id())->first(); + $userOrg = $org->users->first(); return $userOrg && in_array($userOrg->pivot->role, ['owner', 'admin']); });app/Http/Controllers/Api/OrganizationController.php (3)
22-47: Consider adding authorization check before returning organization data.The
indexendpoint returns organizations without explicitly checking if the authenticated user should see this data. WhilegetAccessibleOrganizationsfilters by user, this endpoint may benefit from an explicit authorization check for consistency.
276-340: Code duplication with OrganizationManager Livewire component.The helper methods
getAccessibleOrganizations,getHierarchyTypes, andgetAvailableParentsare duplicated between this controller and theOrganizationManagerLivewire component. Consider extracting these to a shared trait or theOrganizationContexthelper.Example trait extraction:
// app/Concerns/HasOrganizationAccess.php trait HasOrganizationAccess { protected function getAccessibleOrganizations() { ... } protected function getHierarchyTypes() { ... } protected function getAvailableParents() { ... } }
293-311: Environment-based hierarchy type exposure could be a security risk.Allowing all hierarchy types in
local,testing, anddevelopmentenvironments is reasonable for development, but misconfigured production environments or staging environments that use production data could expose this. Consider using a configuration flag instead.-if (app()->environment(['local', 'testing', 'development'])) { +if (config('organization.allow_all_hierarchy_types', false)) {app/Models/TerraformDeployment.php (1)
114-148: Consider adding state transition validation.The
markAs*methods allow any status transition without validation. This could lead to invalid state transitions (e.g., going fromdestroyedback topending). Consider validating allowed transitions.Example validation approach:
protected function assertValidTransition(string $newStatus): void { $allowedTransitions = [ self::STATUS_PENDING => [self::STATUS_PLANNING, self::STATUS_FAILED], self::STATUS_PLANNING => [self::STATUS_PROVISIONING, self::STATUS_FAILED], // ... define allowed transitions ]; if (!in_array($newStatus, $allowedTransitions[$this->status] ?? [])) { throw new \LogicException("Invalid transition from {$this->status} to {$newStatus}"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/Contracts/OrganizationServiceInterface.php(1 hunks)app/Helpers/OrganizationContext.php(1 hunks)app/Http/Controllers/Api/OrganizationController.php(1 hunks)app/Http/Middleware/EnsureOrganizationContext.php(1 hunks)app/Livewire/Organization/OrganizationHierarchy.php(1 hunks)app/Livewire/Organization/OrganizationManager.php(1 hunks)app/Livewire/Organization/OrganizationSwitcher.php(1 hunks)app/Livewire/Organization/UserManagement.php(1 hunks)app/Models/CloudProviderCredential.php(1 hunks)app/Models/Organization.php(1 hunks)app/Models/OrganizationUser.php(1 hunks)app/Models/TerraformDeployment.php(1 hunks)app/Services/OrganizationService.php(1 hunks)app/Services/ResourceProvisioningService.php(1 hunks)database/migrations/2025_08_26_224900_create_organizations_table.php(1 hunks)database/migrations/2025_08_26_225351_create_organization_users_table.php(1 hunks)database/migrations/2025_08_26_225903_add_organization_id_to_servers_table.php(1 hunks)database/migrations/2025_08_26_230017_create_terraform_deployments_table.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
database/migrations/2025_08_26_230017_create_terraform_deployments_table.php (3)
database/migrations/2025_08_26_224900_create_organizations_table.php (2)
up(12-30)down(35-38)database/migrations/2025_08_26_225351_create_organization_users_table.php (2)
up(12-28)down(33-36)database/migrations/2025_08_26_225903_add_organization_id_to_servers_table.php (2)
up(12-19)down(24-31)
app/Livewire/Organization/UserManagement.php (5)
app/Models/Organization.php (2)
Organization(12-229)users(46-52)app/Helpers/OrganizationContext.php (4)
OrganizationContext(9-210)can(32-43)getUserRole(180-192)getUserPermissions(197-209)app/Http/Controllers/Api/OrganizationController.php (4)
users(148-168)addUser(170-207)updateUser(209-236)removeUser(238-255)app/Contracts/OrganizationServiceInterface.php (3)
attachUserToOrganization(24-24)updateUserRole(29-29)detachUserFromOrganization(34-34)app/Models/User.php (1)
role(331-339)
app/Helpers/OrganizationContext.php (4)
app/Models/Organization.php (5)
Organization(12-229)canUserPerformAction(95-106)hasFeature(108-111)isWithinLimits(135-153)users(46-52)app/Models/EnvironmentVariable.php (1)
resource(106-109)app/Contracts/OrganizationServiceInterface.php (5)
canUserPerformAction(49-49)getOrganizationUsage(69-69)getUserOrganizations(44-44)switchUserOrganization(39-39)getOrganizationHierarchy(54-54)app/Livewire/Organization/OrganizationHierarchy.php (1)
getOrganizationUsage(137-147)
database/migrations/2025_08_26_224900_create_organizations_table.php (3)
database/migrations/2025_08_26_225351_create_organization_users_table.php (2)
up(12-28)down(33-36)database/migrations/2025_08_26_225903_add_organization_id_to_servers_table.php (2)
up(12-19)down(24-31)database/migrations/2025_08_26_230017_create_terraform_deployments_table.php (2)
up(12-30)down(35-38)
app/Services/OrganizationService.php (2)
app/Models/Organization.php (11)
Organization(12-229)parent(36-39)users(46-52)canUserPerformAction(95-106)children(41-44)servers(64-67)applications(84-87)activeLicense(54-57)hasFeature(108-111)getAllDescendants(210-215)terraformDeployments(79-82)app/Models/User.php (1)
role(331-339)
app/Http/Controllers/Api/OrganizationController.php (5)
app/Helpers/OrganizationContext.php (5)
OrganizationContext(9-210)current(14-19)can(32-43)getUserOrganizations(95-105)isAdmin(163-175)app/Models/Organization.php (5)
Organization(12-229)parent(36-39)users(46-52)children(41-44)getAllDescendants(210-215)app/Http/Middleware/EnsureOrganizationContext.php (1)
__construct(13-15)app/Contracts/OrganizationServiceInterface.php (8)
createOrganization(14-14)updateOrganization(19-19)switchUserOrganization(39-39)getOrganizationHierarchy(54-54)attachUserToOrganization(24-24)updateUserRole(29-29)detachUserFromOrganization(34-34)getUserOrganizations(44-44)app/Models/User.php (1)
role(331-339)
app/Models/Organization.php (6)
app/Models/OrganizationUser.php (1)
OrganizationUser(8-30)app/Contracts/OrganizationServiceInterface.php (1)
canUserPerformAction(49-49)app/Services/OrganizationService.php (1)
canUserPerformAction(150-179)app/Models/EnvironmentVariable.php (1)
resource(106-109)app/Models/User.php (2)
role(331-339)teams(218-221)app/Helpers/OrganizationContext.php (3)
hasFeature(48-51)isWithinLimits(71-74)current(14-19)
app/Livewire/Organization/OrganizationManager.php (4)
app/Models/Organization.php (5)
Organization(12-229)users(46-52)parent(36-39)children(41-44)getAllDescendants(210-215)app/Helpers/OrganizationContext.php (5)
OrganizationContext(9-210)can(32-43)current(14-19)getUserOrganizations(95-105)isAdmin(163-175)app/Contracts/OrganizationServiceInterface.php (10)
createOrganization(14-14)updateOrganization(19-19)switchUserOrganization(39-39)attachUserToOrganization(24-24)detachUserFromOrganization(34-34)getOrganizationHierarchy(54-54)getOrganizationUsage(69-69)deleteOrganization(64-64)updateUserRole(29-29)getUserOrganizations(44-44)app/Models/User.php (1)
role(331-339)
app/Models/CloudProviderCredential.php (2)
app/Models/TerraformDeployment.php (3)
organization(44-47)TerraformDeployment(9-399)scopeForProvider(347-352)app/Models/Organization.php (1)
Organization(12-229)
app/Models/TerraformDeployment.php (2)
app/Models/CloudProviderCredential.php (1)
CloudProviderCredential(9-338)app/Models/Organization.php (1)
Organization(12-229)
🪛 PHPMD (2.15.0)
app/Services/OrganizationService.php
291-291: Avoid unused local variables such as '$rules'. (undefined)
(UnusedLocalVariable)
347-347: Avoid unused parameters such as '$role'. (undefined)
(UnusedFormalParameter)
416-416: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
526-526: Avoid unused parameters such as '$organization'. (undefined)
(UnusedFormalParameter)
536-536: Avoid unused parameters such as '$organization'. (undefined)
(UnusedFormalParameter)
580-580: Avoid unused local variables such as '$pattern'. (undefined)
(UnusedLocalVariable)
app/Models/Organization.php
164-164: Avoid unused parameters such as '$resource'. (undefined)
(UnusedFormalParameter)
app/Livewire/Organization/OrganizationManager.php
89-89: Avoid unused local variables such as '$organization'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (32)
app/Services/ResourceProvisioningService.php (2)
15-18: Constructor DI withLicensingServiceInterfacelooks solidUsing interface-based constructor injection keeps this service testable and aligns with Laravel’s container patterns; no changes needed here.
324-333: Logging helper is clear and provides good contextStructured logging of provisioning attempts (including org id, resource type, outcome, reason, and license tier) is useful for audit and troubleshooting; the implementation is straightforward and ready to use.
app/Models/OrganizationUser.php (1)
8-29: Pivot model setup looks consistent with the schema.
HasUuids,incrementing = false,keyType = 'string', fillable fields, and casts line up with theorganization_userstable. This is a solid, conventional Eloquent pivot configuration.database/migrations/2025_08_26_225351_create_organization_users_table.php (1)
14-27: organization_users schema aligns well with the pivot use case.The UUID primary key, org/user foreign keys with cascades, composite unique index, and
(organization_id, role)index are all appropriate for an organization membership pivot and match theOrganizationUsermodel.app/Livewire/Organization/OrganizationHierarchy.php (1)
165-185: LGTM!The
getHierarchyTypeIconandgetHierarchyTypeColormethods are well-implemented using PHP 8's match expression with sensible defaults.app/Contracts/OrganizationServiceInterface.php (1)
1-70: Well-structured service interface.The interface provides a clean contract with clear method signatures, proper type hints, and good documentation. The methods cover all essential organization management operations.
app/Models/CloudProviderCredential.php (1)
22-26: Good use of encrypted cast for sensitive credentials.The
encrypted:arraycast ensures credentials are encrypted at rest, and the$hiddenproperty prevents accidental exposure in JSON responses.app/Livewire/Organization/UserManagement.php (2)
260-268: LGTM on isLastOwner implementation.The method correctly checks for active owners and compares both count and ID to determine if the user is the sole owner.
89-120: Good implementation of addUser with duplicate check.The method properly validates, checks for existing membership, and uses the service layer for the actual attachment.
app/Services/OrganizationService.php (3)
526-541: Placeholder implementations are acceptable for initial PR.The
calculateStorageUsageandcalculateMonthlyCostsmethods return placeholder values, which is fine for the initial implementation. The static analysis hints about unused$organizationparameters are expected for placeholders.Consider adding TODO comments to track these implementations:
protected function calculateStorageUsage(Organization $organization): int { - // This would integrate with actual storage monitoring - // For now, return a placeholder + // TODO: Integrate with actual storage monitoring service return 0; }
19-50: Good transactional handling in createOrganization.The method properly uses
DB::transactionto ensure atomicity when creating the organization and attaching the owner.
196-232: Good circular dependency check in moveOrganization.The
wouldCreateCircularDependencymethod correctly walks up the parent chain to prevent creating cycles in the organization hierarchy.app/Models/Organization.php (6)
14-14: LGTM!Correct use of
HasFactoryandHasUuidstraits for UUID primary keys and factory support.
16-33: LGTM!The fillable attributes and casts are well-defined for the organization model's properties, with appropriate casting for JSON columns and booleans.
35-52: LGTM!The hierarchical relationships (
parent,children) and theusersmany-to-many relationship with theOrganizationUserpivot are correctly defined with appropriate pivot columns and timestamps.
54-92: LGTM!The relationships to
EnterpriseLicense,Server,WhiteLabelConfig,CloudProviderCredential,TerraformDeployment,Application, andDomainare correctly defined and align with the multi-tenancy architecture.
217-228: LGTM!The
getAncestors()method correctly traverses the parent chain iteratively, which is appropriate for upward traversal.
210-215: I'll verify the review comment by examining the codebase to assess whether the performance concerns are valid and if better approaches exist.
<function_calls>
#!/bin/bashSearch for hierarchy traversal patterns and getAllDescendants usage
rg -n --type=php 'getAllDescendants|getAncestors|getParent|with.*children' -C2 | head -100
</function_calls><function_calls>
#!/bin/bashCheck the Organization model file structure
fd -t f 'Organization.php' | head -5
</function_calls><function_calls>
#!/bin/bashRead the Organization model to see the full context
if [ -f "app/Models/Organization.php" ]; then
wc -l app/Models/Organization.php
fi
</function_calls>app/Livewire/Organization/OrganizationManager.php (5)
1-11: LGTM!Appropriate imports and namespace for the Livewire component with pagination support.
43-48: LGTM!Validation rules are well-defined with proper type constraints and enum validation for hierarchy types.
50-56: LGTM!Permission check in
mount()appropriately restricts access to the component.
78-104: LGTM on the create flow.The
createOrganizationmethod correctly validates, resolves the parent, delegates to the service, and handles errors gracefully. The static analysis hint about$organizationbeing unused is a false positive—it's returned from the service and could be used for further operations if needed.
284-302: LGTM!The
getAccessibleOrganizationsmethod correctly merges user organizations with descendants when the user is an admin, with proper deduplication.app/Http/Controllers/Api/OrganizationController.php (3)
13-20: LGTM!Proper constructor injection of the organization service interface.
257-274: LGTM!The
rolesAndPermissionsendpoint provides a clean reference for available roles and permissions.
109-127: Access verification IS properly implemented in the service layer.The
switchUserOrganization()method inapp/Services/OrganizationService.phpcallsuserHasAccessToOrganization()before processing the switch, which validates that the user is an active member of the organization through the pivot table relationship. If the user lacks access, anInvalidArgumentExceptionis thrown and caught by the controller's try-catch block, returning a 400 response. The security concern raised in this review comment is unfounded.app/Models/TerraformDeployment.php (6)
9-26: LGTM!Model setup with appropriate fillable fields and casts for JSON columns is correct.
28-42: LGTM!Status constants are well-defined and cover the complete deployment lifecycle.
43-57: LGTM!Relationships are correctly defined with appropriate foreign key specifications.
59-112: LGTM!Status check methods provide a clean API for querying deployment state.
326-357: LGTM!Query scopes are well-implemented and provide useful filtering capabilities for deployments.
359-384: LGTM!Duration calculation and formatting helpers are correctly implemented with appropriate null handling for in-progress deployments.
| public static function isOwner(): bool | ||
| { | ||
| $user = Auth::user(); | ||
| $organization = static::current(); | ||
|
|
||
| if (! $user || ! $organization) { | ||
| return false; | ||
| } | ||
|
|
||
| $userOrg = $organization->users()->where('user_id', $user->id)->first(); | ||
|
|
||
| return $userOrg && $userOrg->pivot->role === 'owner'; | ||
| } | ||
|
|
||
| /** | ||
| * Check if the current user is an admin of the current organization | ||
| */ | ||
| public static function isAdmin(): bool | ||
| { | ||
| $user = Auth::user(); | ||
| $organization = static::current(); | ||
|
|
||
| if (! $user || ! $organization) { | ||
| return false; | ||
| } | ||
|
|
||
| $userOrg = $organization->users()->where('user_id', $user->id)->first(); | ||
|
|
||
| return $userOrg && in_array($userOrg->pivot->role, ['owner', 'admin']); | ||
| } | ||
|
|
||
| /** | ||
| * Get the current user's role in the current organization | ||
| */ | ||
| public static function getUserRole(): ?string | ||
| { | ||
| $user = Auth::user(); | ||
| $organization = static::current(); | ||
|
|
||
| if (! $user || ! $organization) { | ||
| return null; | ||
| } | ||
|
|
||
| $userOrg = $organization->users()->where('user_id', $user->id)->first(); | ||
|
|
||
| return $userOrg?->pivot->role; | ||
| } | ||
|
|
||
| /** | ||
| * Get the current user's permissions in the current organization | ||
| */ | ||
| public static function getUserPermissions(): array | ||
| { | ||
| $user = Auth::user(); | ||
| $organization = static::current(); | ||
|
|
||
| if (! $user || ! $organization) { | ||
| return []; | ||
| } | ||
|
|
||
| $userOrg = $organization->users()->where('user_id', $user->id)->first(); | ||
|
|
||
| return $userOrg?->pivot->permissions ?? []; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find migration files and model definitions related to organization_users
find . -type f -name "*migration*" -o -name "*Organization*" -o -name "*User*" | grep -E "\.(php|js)$" | head -20Repository: johnproblems/topgun
Length of output: 947
🏁 Script executed:
#!/bin/bash
# Search for organization_users table definition or migrations
rg "organization_users" -A 5 -B 5 --type php | head -100Repository: johnproblems/topgun
Length of output: 3218
🏁 Script executed:
#!/bin/bash
# Check the Organization model for relationship definition
fd -e php | xargs grep -l "class Organization" | head -5Repository: johnproblems/topgun
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Read the full migration file
cat database/migrations/2025_08_26_225351_create_organization_users_table.phpRepository: johnproblems/topgun
Length of output: 1213
🏁 Script executed:
#!/bin/bash
# Search for is_active usage in the codebase
rg "is_active" -A 3 -B 3 --type php | head -150Repository: johnproblems/topgun
Length of output: 9913
🏁 Script executed:
#!/bin/bash
# Check the current OrganizationContext helpers specifically
cat -n app/Helpers/OrganizationContext.php | sed -n '146,208p'Repository: johnproblems/topgun
Length of output: 2162
🏁 Script executed:
#!/bin/bash
# Search for usage of OrganizationContext helpers
rg "OrganizationContext::(isOwner|isAdmin|getUserRole|getUserPermissions)" -A 2 -B 2 --type phpRepository: johnproblems/topgun
Length of output: 1403
🏁 Script executed:
#!/bin/bash
# Search for tests related to OrganizationContext or membership
fd -e php | xargs grep -l "test.*OrganizationContext\|test.*isOwner\|test.*isAdmin" 2>/dev/null | head -10Repository: johnproblems/topgun
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look for canUserPerformAction in OrganizationService to see how it differs
rg "canUserPerformAction" -A 10 -B 2 --type phpRepository: johnproblems/topgun
Length of output: 5354
🏁 Script executed:
#!/bin/bash
# Search for where is_active is set to false
rg "is_active.*false|is_active\s*=\s*false" -B 5 -A 2 --type phpRepository: johnproblems/topgun
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Search for any removal/disable logic on organization membership
rg "removeUser|disableUser|deactivate.*user|soft.*delete" -i -A 5 --type php | head -100Repository: johnproblems/topgun
Length of output: 8721
🏁 Script executed:
#!/bin/bash
# Check Organization model canUserPerformAction more closely
rg "canUserPerformAction" -B 5 -A 20 app/Models/Organization.phpRepository: johnproblems/topgun
Length of output: 773
🏁 Script executed:
#!/bin/bash
# Check for any comments or TODOs about is_active usage
rg "is_active" -B 3 -A 3 --type php | rg -i "todo|fixme|disable|deactivate|revoke|suspend" -B 5 -A 5Repository: johnproblems/topgun
Length of output: 45
Inconsistent handling of is_active pivot flag across permission checks.
The organization_users pivot includes an is_active flag (defined in the migration with default(true)), which is already checked in OrganizationService::canUserPerformAction(). However, the membership helpers in OrganizationContext—isOwner(), isAdmin(), getUserRole(), and getUserPermissions()—do not check this flag. This creates an inconsistency: OrganizationService respects is_active while OrganizationContext does not.
Although is_active is not currently set to false anywhere, this inconsistency is a maintenance risk. If is_active is used to soft-disable memberships (e.g., for revocation or suspension), these helpers would still expose the role and permissions of disabled users, undermining the disable mechanism.
Add .wherePivot('is_active', true) to the membership lookup in each helper:
- $userOrg = $organization->users()->where('user_id', $user->id)->first();
+ $userOrg = $organization->users()
+ ->where('user_id', $user->id)
+ ->wherePivot('is_active', true)
+ ->first();Optionally, extract this lookup into a small private/static helper to avoid repetition across all four methods.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/Helpers/OrganizationContext.php around lines 146 to 208, the membership
lookups in isOwner(), isAdmin(), getUserRole(), and getUserPermissions() do not
respect the pivot is_active flag; update each lookup to include
->wherePivot('is_active', true) so only active memberships are considered, and
to avoid repetition extract the membership query into a single private/static
helper (e.g., getUserOrganizationPivot() that returns the userOrg pivot or null)
and have all four methods call that helper.
| public function store(Request $request) | ||
| { | ||
| $request->validate([ | ||
| 'name' => 'required|string|max:255', | ||
| 'hierarchy_type' => 'required|in:top_branch,master_branch,sub_user,end_user', | ||
| 'parent_organization_id' => 'nullable|exists:organizations,id', | ||
| 'is_active' => 'boolean', | ||
| ]); | ||
|
|
||
| try { | ||
| $parent = $request->parent_organization_id | ||
| ? Organization::find($request->parent_organization_id) | ||
| : null; | ||
|
|
||
| $organization = $this->organizationService->createOrganization([ | ||
| 'name' => $request->name, | ||
| 'hierarchy_type' => $request->hierarchy_type, | ||
| 'is_active' => $request->is_active ?? true, | ||
| 'owner_id' => Auth::id(), | ||
| ], $parent); | ||
|
|
||
| return response()->json([ | ||
| 'message' => 'Organization created successfully', | ||
| 'organization' => $organization, | ||
| ]); | ||
| } catch (\Exception $e) { | ||
| return response()->json([ | ||
| 'message' => 'Failed to create organization: '.$e->getMessage(), | ||
| ], 400); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing authorization check for organization creation.
The store endpoint validates input and creates an organization but doesn't verify the user has permission to create organizations. Consider adding OrganizationContext::can('create_organization') check.
public function store(Request $request)
{
+ if (! OrganizationContext::can('create_organization')) {
+ return response()->json(['message' => 'Unauthorized'], 403);
+ }
+
$request->validate([Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/Http/Controllers/Api/OrganizationController.php around lines 49 to 79,
the store method lacks an authorization check before creating an organization;
call OrganizationContext::can('create_organization') (or the appropriate auth
helper) immediately after validation and before resolving $parent and calling
$this->organizationService->createOrganization, and if it returns false respond
with a 403 JSON error (or abort with 403) so unauthorized users cannot create
organizations.
| public function removeUserFromOrganization(User $user) | ||
| { | ||
| try { | ||
| $organizationService = app(OrganizationServiceInterface::class); | ||
|
|
||
| $organizationService->detachUserFromOrganization($this->selectedOrganization, $user); | ||
|
|
||
| session()->flash('success', 'User removed from organization successfully.'); | ||
|
|
||
| } catch (\Exception $e) { | ||
| session()->flash('error', 'Failed to remove user: '.$e->getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing permission check before removing user.
Unlike manageUsers() and addUserToOrganization() which check OrganizationContext::can('manage_users', ...), this method directly calls the service without verifying the current user has permission to remove users.
Add a permission check:
public function removeUserFromOrganization(User $user)
{
+ if (! OrganizationContext::can('manage_users', $this->selectedOrganization)) {
+ session()->flash('error', 'You do not have permission to remove users from this organization.');
+ return;
+ }
+
try {
$organizationService = app(OrganizationServiceInterface::class);🤖 Prompt for AI Agents
In app/Livewire/Organization/OrganizationManager.php around lines 203 to 215,
add the same permission guard used in manageUsers() and addUserToOrganization():
check OrganizationContext::can('manage_users', $this->selectedOrganization)
before calling detachUserFromOrganization; if the check fails, do not call the
service and instead flash an authorization error (or abort/return early) so
unauthorized users cannot remove members; keep the try/catch for service errors
after the permission check.
| public function updateUserRole(User $user, string $newRole) | ||
| { | ||
| $this->validate([ | ||
| 'newRole' => 'required|in:owner,admin,member,viewer', | ||
| ]); | ||
|
|
||
| try { | ||
| $organizationService = app(OrganizationServiceInterface::class); | ||
|
|
||
| $organizationService->updateUserRole( | ||
| $this->selectedOrganization, | ||
| $user, | ||
| $newRole | ||
| ); | ||
|
|
||
| session()->flash('success', 'User role updated successfully.'); | ||
|
|
||
| } catch (\Exception $e) { | ||
| session()->flash('error', 'Failed to update user role: '.$e->getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validation rule references wrong variable name.
The validation uses the string 'newRole' as the field key, but the parameter is $newRole. Livewire validation expects the field name to match a public property, which newRole is not. This validation will likely not work as intended.
Consider validating the parameter directly or restructuring:
public function updateUserRole(User $user, string $newRole)
{
- $this->validate([
- 'newRole' => 'required|in:owner,admin,member,viewer',
- ]);
+ if (! in_array($newRole, ['owner', 'admin', 'member', 'viewer'])) {
+ session()->flash('error', 'Invalid role specified.');
+ return;
+ }
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function updateUserRole(User $user, string $newRole) | |
| { | |
| $this->validate([ | |
| 'newRole' => 'required|in:owner,admin,member,viewer', | |
| ]); | |
| try { | |
| $organizationService = app(OrganizationServiceInterface::class); | |
| $organizationService->updateUserRole( | |
| $this->selectedOrganization, | |
| $user, | |
| $newRole | |
| ); | |
| session()->flash('success', 'User role updated successfully.'); | |
| } catch (\Exception $e) { | |
| session()->flash('error', 'Failed to update user role: '.$e->getMessage()); | |
| } | |
| } | |
| public function updateUserRole(User $user, string $newRole) | |
| { | |
| if (! in_array($newRole, ['owner', 'admin', 'member', 'viewer'])) { | |
| session()->flash('error', 'Invalid role specified.'); | |
| return; | |
| } | |
| try { | |
| $organizationService = app(OrganizationServiceInterface::class); | |
| $organizationService->updateUserRole( | |
| $this->selectedOrganization, | |
| $user, | |
| $newRole | |
| ); | |
| session()->flash('success', 'User role updated successfully.'); | |
| } catch (\Exception $e) { | |
| session()->flash('error', 'Failed to update user role: '.$e->getMessage()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/Livewire/Organization/OrganizationManager.php around lines 262-282, the
Livewire validate call uses the field name 'newRole' which must match a Livewire
public property, but here $newRole is a method parameter; fix by validating the
parameter directly (e.g., use Validator::make(['newRole' => $newRole],
['newRole' => 'required|in:owner,admin,member,viewer'])->validate()) or
alternatively convert newRole into a public property on the component and use
the existing $this->validate rules; then proceed with calling
organizationService and session flashes as before.
| private function validateAwsCredentials(array $credentials): void | ||
| { | ||
| if (strlen($credentials['access_key_id']) !== 20) { | ||
| throw new \InvalidArgumentException('Invalid AWS Access Key ID format'); | ||
| } | ||
|
|
||
| if (strlen($credentials['secret_access_key']) !== 40) { | ||
| throw new \InvalidArgumentException('Invalid AWS Secret Access Key format'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AWS access key ID format length specification documentation
💡 Result:
AWS Access Key ID constraints: string matching [\w]+ (letters, digits, underscore), minimum length 16, maximum length 128. Examples: long‑term keys start with "AKIA", temporary STS keys start with "ASIA". (docs.aws.amazon.com)
Citations:
🌐 Web query:
AWS secret access key format length specification
💡 Result:
Short answer: AWS does not publish a formal length constraint for SecretAccessKey in the IAM API schema (it's just "String"), but in practice AWS secret access keys are 40 characters long (the canonical examples in AWS docs and SDK/CLI output use 40-character secret keys). [1][2][3]
Sources:
- IAM AccessKey (SecretAccessKey is typed String; no length constraint shown). [1]
- STS / GetAccessKeyInfo and other AWS docs show the canonical secret-access-key example "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" (40 chars). [2]
- AWS SDK / CLI examples and documentation that show the same 40-character secret key example. [3]
References:
[1] AWS IAM API — AccessKey.
[2] AWS STS — GetAccessKeyInfo (example showing access key + 40-char secret).
[3] AWS SDK/CLI docs and examples (credentials examples with 40-char secret).
🏁 Script executed:
# Check if there are any tests or other references to AWS credential validation
fd CloudProviderCredential.php --type fRepository: johnproblems/topgun
Length of output: 101
🏁 Script executed:
# View the full CloudProviderCredential.php file to understand context
wc -l app/Models/CloudProviderCredential.phpRepository: johnproblems/topgun
Length of output: 105
🏁 Script executed:
# Look for test files related to CloudProviderCredential
fd -e test.php -e spec.php | xargs rg -l CloudProviderCredentialRepository: johnproblems/topgun
Length of output: 164
🏁 Script executed:
# Check if there are validation tests for AWS credentials
rg -n "validateAwsCredentials|access_key_id|secret_access_key" app/Models/CloudProviderCredential.php -A 2 -B 2Repository: johnproblems/topgun
Length of output: 1115
AWS credential validation uses overly restrictive fixed-length checks that will reject valid credentials.
Based on AWS documentation, Access Key IDs support lengths from 16 to 128 characters (not fixed at 20), and can vary by key type (long-term "AKIA" keys, temporary "ASIA" keys, etc.). The current validation at line 147 will reject valid AWS credentials. Secret Access Keys are typically 40 characters in practice, though AWS doesn't formally specify an exact length.
Replace the strict length checks with format validation (character set, prefix validation if needed) or use AWS SDK validation:
private function validateAwsCredentials(array $credentials): void
{
// Access keys should match [\w]+ and be 16-128 characters
if (!preg_match('/^[\w]{16,128}$/', $credentials['access_key_id'])) {
throw new \InvalidArgumentException('Invalid AWS Access Key ID format');
}
// Secret keys are typically 40 chars but use character set validation instead of fixed length
if (!preg_match('/^[A-Za-z0-9\/+]{20,}$/', $credentials['secret_access_key'])) {
throw new \InvalidArgumentException('Invalid AWS Secret Access Key format');
}
}🤖 Prompt for AI Agents
In app/Models/CloudProviderCredential.php around lines 145 to 154, the AWS
credential validation uses fixed-length checks (exact 20 and 40 chars) which
will reject valid keys; replace those strict length checks with robust format
validation: validate access_key_id with a regex allowing word characters and a
length range (e.g. 16–128) and validate secret_access_key by checking allowed
character set and a sensible minimum length instead of exact 40, or defer to the
AWS SDK validator if available; throw the same InvalidArgumentException on
mismatch.
| public function canUserPerformAction(User $user, Organization $organization, string $action, $resource = null): bool | ||
| { | ||
| $cacheKey = "user_permissions_{$user->id}_{$organization->id}_{$action}"; | ||
|
|
||
| return Cache::remember($cacheKey, now()->addMinutes(15), function () use ($user, $organization, $action, $resource) { | ||
| // Check if user is in organization | ||
| $userOrg = $organization->users()->where('user_id', $user->id)->first(); | ||
| if (! $userOrg || ! $userOrg->pivot->is_active) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check license restrictions | ||
| if (! $this->isActionAllowedByLicense($organization, $action)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check role-based permissions | ||
| $permissions = $userOrg->pivot->permissions ?? []; | ||
| if (is_string($permissions)) { | ||
| $permissions = json_decode($permissions, true) ?? []; | ||
| } | ||
|
|
||
| return $this->checkRolePermission( | ||
| $userOrg->pivot->role, | ||
| $permissions, | ||
| $action, | ||
| $resource | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Cache key includes $resource but doesn't include its value.
The permission check caches results keyed by user_id, organization_id, and action, but the $resource parameter affects the result. If different resources are passed with the same action, stale cache entries may be returned.
public function canUserPerformAction(User $user, Organization $organization, string $action, $resource = null): bool
{
- $cacheKey = "user_permissions_{$user->id}_{$organization->id}_{$action}";
+ $resourceKey = $resource ? '_' . (is_object($resource) ? get_class($resource) . '_' . $resource->id : $resource) : '';
+ $cacheKey = "user_permissions_{$user->id}_{$organization->id}_{$action}{$resourceKey}";
return Cache::remember($cacheKey, now()->addMinutes(15), function () use ($user, $organization, $action, $resource) {🤖 Prompt for AI Agents
In app/Services/OrganizationService.php around lines 150 to 179, the cache key
omits the $resource value so permission results may be reused across different
resources; update the cache key to incorporate a stable representation of
$resource (e.g. if $resource is an object use its id or unique identifier, if
array/string use json_encode and for large/complex values use a short hash like
md5(json_encode($resource))), handle null resources explicitly, and ensure the
final cache key remains within safe length limits (or call a helper like
getResourceCacheKey($resource) to normalize and shorten it) before calling
Cache::remember.
| protected function validateOrganizationData(array $data, ?Organization $existing = null): void | ||
| { | ||
| $rules = [ | ||
| 'name' => 'required|string|max:255', | ||
| 'hierarchy_type' => 'required|in:top_branch,master_branch,sub_user,end_user', | ||
| ]; | ||
|
|
||
| // Check slug uniqueness | ||
| if (isset($data['slug'])) { | ||
| $slugQuery = Organization::where('slug', $data['slug']); | ||
| if ($existing) { | ||
| $slugQuery->where('id', '!=', $existing->id); | ||
| } | ||
| if ($slugQuery->exists()) { | ||
| throw new InvalidArgumentException('Organization slug must be unique'); | ||
| } | ||
| } | ||
|
|
||
| // Validate hierarchy type | ||
| $validTypes = ['top_branch', 'master_branch', 'sub_user', 'end_user']; | ||
| if (isset($data['hierarchy_type']) && ! in_array($data['hierarchy_type'], $validTypes)) { | ||
| throw new InvalidArgumentException('Invalid hierarchy type'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused $rules variable - validation not applied.
The $rules array is defined but never used. The validation logic only checks slug uniqueness and hierarchy type manually. Either use Laravel's Validator or remove the unused variable.
protected function validateOrganizationData(array $data, ?Organization $existing = null): void
{
- $rules = [
- 'name' => 'required|string|max:255',
- 'hierarchy_type' => 'required|in:top_branch,master_branch,sub_user,end_user',
- ];
+ // Validate required fields
+ if (empty($data['name']) || !is_string($data['name']) || strlen($data['name']) > 255) {
+ throw new InvalidArgumentException('Name is required and must be a string with max 255 characters');
+ }
// Check slug uniquenessOr use Laravel's Validator facade:
$validator = \Validator::make($data, $rules);
if ($validator->fails()) {
throw new InvalidArgumentException($validator->errors()->first());
}🧰 Tools
🪛 PHPMD (2.15.0)
291-291: Avoid unused local variables such as '$rules'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In app/Services/OrganizationService.php around lines 289 to 312, the $rules
array is defined but never applied; replace the unused $rules by running Laravel
validation and throwing an error when it fails, e.g. call Validator::make($data,
$rules) and if it fails throw an InvalidArgumentException with the first
validation message; keep the existing manual slug-uniqueness check (validator
won't cover that) and remove the separate manual hierarchy-type check (since the
rule 'in:top_branch,master_branch,sub_user,end_user' covers it); ensure the
Validator facade is imported or referenced via \Validator.
| protected function validateHierarchyTypeChange(Organization $organization, string $newType): void | ||
| { | ||
| // Check if change would break parent-child relationships | ||
| if ($organization->parent) { | ||
| $this->validateHierarchyCreation($organization->parent, $newType); | ||
| } | ||
|
|
||
| // Check if change would break relationships with children | ||
| foreach ($organization->children as $child) { | ||
| $this->validateHierarchyCreation($organization, $child->hierarchy_type); | ||
| } | ||
| } |
There was a problem hiding this comment.
Logic error in validateHierarchyTypeChange - wrong validation order.
At line 555, when checking children, validateHierarchyCreation is called with $organization (current org) as parent, but it should use the hypothetical new type. The current code validates that the current org can have the child, not whether the org with new type can have the child.
protected function validateHierarchyTypeChange(Organization $organization, string $newType): void
{
// Check if change would break parent-child relationships
if ($organization->parent) {
$this->validateHierarchyCreation($organization->parent, $newType);
}
// Check if change would break relationships with children
+ $allowedChildren = [
+ 'top_branch' => ['master_branch'],
+ 'master_branch' => ['sub_user'],
+ 'sub_user' => ['end_user'],
+ 'end_user' => [],
+ ];
+
foreach ($organization->children as $child) {
- $this->validateHierarchyCreation($organization, $child->hierarchy_type);
+ if (!isset($allowedChildren[$newType]) || !in_array($child->hierarchy_type, $allowedChildren[$newType])) {
+ throw new InvalidArgumentException("A {$newType} cannot have a {$child->hierarchy_type} as a child");
+ }
}
}🤖 Prompt for AI Agents
In app/Services/OrganizationService.php around lines 546 to 557, the child-check
uses the current org type instead of the proposed new type; change the child
validation so it verifies whether the organization with hierarchy_type =
$newType can accept each child (i.e., call validateHierarchyCreation using a
parent object whose hierarchy_type is the new type or refactor
validateHierarchyCreation to accept an explicit parent type), and ensure you
restore any temporary state if you mutate the organization object.
| protected function clearUserCache(User $user): void | ||
| { | ||
| Cache::forget("user_organizations_{$user->id}"); | ||
|
|
||
| // Clear permission caches for all organizations this user belongs to | ||
| $user->organizations->each(function ($org) use ($user) { | ||
| $pattern = "user_permissions_{$user->id}_{$org->id}_*"; | ||
| // In a real implementation, you'd want a more sophisticated cache clearing mechanism | ||
| // For now, we'll clear specific known permission keys | ||
| $actions = ['view_servers', 'manage_servers', 'deploy_applications', 'manage_billing']; | ||
| foreach ($actions as $action) { | ||
| Cache::forget("user_permissions_{$user->id}_{$org->id}_{$action}"); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Incomplete cache clearing - $pattern unused and hardcoded actions.
The $pattern variable is defined but unused. The current implementation only clears a hardcoded list of actions, leaving other cached permissions stale.
Consider using cache tags (if your cache driver supports them) or a more comprehensive approach:
protected function clearUserCache(User $user): void
{
Cache::forget("user_organizations_{$user->id}");
- // Clear permission caches for all organizations this user belongs to
- $user->organizations->each(function ($org) use ($user) {
- $pattern = "user_permissions_{$user->id}_{$org->id}_*";
- // In a real implementation, you'd want a more sophisticated cache clearing mechanism
- // For now, we'll clear specific known permission keys
- $actions = ['view_servers', 'manage_servers', 'deploy_applications', 'manage_billing'];
- foreach ($actions as $action) {
- Cache::forget("user_permissions_{$user->id}_{$org->id}_{$action}");
- }
- });
+ // Clear all known permission actions
+ $allActions = [
+ 'view_servers', 'manage_servers', 'view_applications', 'manage_applications',
+ 'deploy_applications', 'view_billing', 'manage_billing', 'manage_users',
+ 'manage_organization', 'delete_organization', 'manage_licenses',
+ 'provision_infrastructure', 'manage_domains', 'process_payments',
+ 'manage_white_label', 'view_organization', 'switch_organization',
+ ];
+
+ $user->organizations->each(function ($org) use ($user, $allActions) {
+ foreach ($allActions as $action) {
+ Cache::forget("user_permissions_{$user->id}_{$org->id}_{$action}");
+ }
+ });
}Alternatively, consider using cache tags for easier invalidation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function clearUserCache(User $user): void | |
| { | |
| Cache::forget("user_organizations_{$user->id}"); | |
| // Clear permission caches for all organizations this user belongs to | |
| $user->organizations->each(function ($org) use ($user) { | |
| $pattern = "user_permissions_{$user->id}_{$org->id}_*"; | |
| // In a real implementation, you'd want a more sophisticated cache clearing mechanism | |
| // For now, we'll clear specific known permission keys | |
| $actions = ['view_servers', 'manage_servers', 'deploy_applications', 'manage_billing']; | |
| foreach ($actions as $action) { | |
| Cache::forget("user_permissions_{$user->id}_{$org->id}_{$action}"); | |
| } | |
| }); | |
| } | |
| protected function clearUserCache(User $user): void | |
| { | |
| Cache::forget("user_organizations_{$user->id}"); | |
| // Clear all known permission actions | |
| $allActions = [ | |
| 'view_servers', 'manage_servers', 'view_applications', 'manage_applications', | |
| 'deploy_applications', 'view_billing', 'manage_billing', 'manage_users', | |
| 'manage_organization', 'delete_organization', 'manage_licenses', | |
| 'provision_infrastructure', 'manage_domains', 'process_payments', | |
| 'manage_white_label', 'view_organization', 'switch_organization', | |
| ]; | |
| $user->organizations->each(function ($org) use ($user, $allActions) { | |
| foreach ($allActions as $action) { | |
| Cache::forget("user_permissions_{$user->id}_{$org->id}_{$action}"); | |
| } | |
| }); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
580-580: Avoid unused local variables such as '$pattern'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In app/Services/OrganizationService.php around lines 574 to 588, the method
defines an unused $pattern and clears only a hardcoded list of permission keys
which leaves other permission caches stale; remove the unused $pattern and
replace the hardcoded loop with a proper invalidation strategy: if your cache
driver supports tags, use Cache::tags([...])->flush() for the user+organization
permission tag(s); otherwise enumerate actual permission keys from the
permission store/DB for that user and organization (or use the cache store's key
pattern deletion method for Redis) and call Cache::forget on each generated key;
ensure the code checks the cache driver capability and falls back to the safe
per-key removal.
| public function getResourceLimits(Organization $organization): array | ||
| { | ||
| $license = $organization->activeLicense; | ||
| if (! $license) { | ||
| return [ | ||
| 'has_license' => false, | ||
| 'limits' => [], | ||
| 'usage' => [], | ||
| ]; | ||
| } | ||
|
|
||
| $usage = $organization->getUsageMetrics(); | ||
| $limits = $license->limits ?? []; | ||
|
|
||
| $resourceLimits = []; | ||
| foreach (['servers', 'applications', 'domains', 'cloud_providers'] as $resource) { | ||
| $limit = $limits[$resource] ?? null; | ||
| $current = $usage[$resource] ?? 0; | ||
|
|
||
| $resourceLimits[$resource] = [ | ||
| 'current' => $current, | ||
| 'limit' => $limit, | ||
| 'unlimited' => $limit === null, | ||
| 'remaining' => $limit ? max(0, $limit - $current) : null, | ||
| 'percentage_used' => $limit ? round(($current / $limit) * 100, 2) : 0, | ||
| 'near_limit' => $limit ? ($current / $limit) >= 0.8 : false, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Fix limit math: 0 limits are treated as “no limit” and hide remaining/usage
In getResourceLimits, the checks use $limit ? ... which makes a 0 limit behave like “unlimited”: remaining becomes null, percentage_used is 0, and near_limit is false. That’s misleading for any resource explicitly capped at 0 and can hide misconfiguration.
You can keep the “null means unlimited” semantics while handling 0 safely like this:
- $resourceLimits = [];
- foreach (['servers', 'applications', 'domains', 'cloud_providers'] as $resource) {
- $limit = $limits[$resource] ?? null;
- $current = $usage[$resource] ?? 0;
-
- $resourceLimits[$resource] = [
- 'current' => $current,
- 'limit' => $limit,
- 'unlimited' => $limit === null,
- 'remaining' => $limit ? max(0, $limit - $current) : null,
- 'percentage_used' => $limit ? round(($current / $limit) * 100, 2) : 0,
- 'near_limit' => $limit ? ($current / $limit) >= 0.8 : false,
- ];
- }
+ $resourceLimits = [];
+ foreach (['servers', 'applications', 'domains', 'cloud_providers'] as $resource) {
+ $limit = $limits[$resource] ?? null;
+ $current = $usage[$resource] ?? 0;
+
+ $isUnlimited = $limit === null;
+
+ if ($isUnlimited) {
+ $remaining = null;
+ $percentageUsed = 0;
+ $nearLimit = false;
+ } elseif ($limit === 0) {
+ // Explicit zero-capacity: nothing remaining, avoid division by zero
+ $remaining = 0;
+ $percentageUsed = 0;
+ $nearLimit = false;
+ } else {
+ $remaining = max(0, $limit - $current);
+ $percentageUsed = round(($current / $limit) * 100, 2);
+ $nearLimit = ($current / $limit) >= 0.8;
+ }
+
+ $resourceLimits[$resource] = [
+ 'current' => $current,
+ 'limit' => $limit,
+ 'unlimited' => $isUnlimited,
+ 'remaining' => $remaining,
+ 'percentage_used' => $percentageUsed,
+ 'near_limit' => $nearLimit,
+ ];
+ }This preserves the null = unlimited contract, handles 0 without division by zero, and surfaces correct remaining/usage information.
🤖 Prompt for AI Agents
In app/Services/ResourceProvisioningService.php around lines 283 to 310, the
code treats a limit of 0 as “unlimited” because it uses truthy checks (e.g.
$limit ? ...), which hides remaining/usage for explicit 0 caps; update the logic
to treat only null as unlimited by using strict null checks ($limit !== null)
and compute remaining as max(0, $limit - $current) when $limit !== null, compute
percentage_used as $limit !== null ? round(($current / max(1, $limit)) * 100, 2)
: 0 (or avoid division when $limit === 0 by setting percentage to 100 if
appropriate), and set near_limit only when $limit !== null (e.g. ($limit > 0) ?
(($current / $limit) >= 0.8) : ($current >= $limit)), ensuring no division by
zero and that 0 limits surface correct remaining, percentage_used, and
near_limit values.
Summary
Complete multi-tenancy system with organizational hierarchy, role-based access control, cloud provider integration, and infrastructure management.
Features
API Endpoints
POST /api/v1/organizations- Create organizationGET /api/v1/organizations- List organizationsGET /api/v1/organizations/{id}- Get organizationPUT /api/v1/organizations/{id}- Update organizationDELETE /api/v1/organizations/{id}- Delete organization🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Note
Introduce multi-tenant organization hierarchy with role-based access, API/UI management, cloud provider credentials, Terraform deployment tracking, and supporting middleware/migrations.
OrganizationServiceInterface; implementOrganizationService(hierarchy, RBAC, switching, usage, caching) andResourceProvisioningService(license/limits checks).Organization,OrganizationUser(pivot with roles/permissions),CloudProviderCredential(encrypted creds, provider validation/regions),TerraformDeployment(status, config/state helpers).EnsureOrganizationContextto enforce/set current org.app/Http/Controllers/Api/OrganizationController.phpexposing org CRUD, user mgmt, hierarchy, and org switching endpoints.OrganizationManager,OrganizationHierarchy,OrganizationSwitcher,UserManagementcomponents for managing orgs, hierarchy, switching, and users.OrganizationContextfor current org, permissions, features, usage, hierarchy.create_organizations_table,create_organization_users_table,add_organization_id_to_servers_table,create_terraform_deployments_table.Written by Cursor Bugbot for commit 0fdfda7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.