Add toggle for externally managed users #1825
Conversation
📝 WalkthroughWalkthroughIntroduces a boolean User.is_managed_externally flag (model, migration, factory, transformer), surfaces it in admin/profile UIs, groups customization fields into a Collapsible Section, conditions client-side profile field visibility and API authorization on the flag, and updates tests and translations. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client / Filament UI
participant API as Backend API
participant DB as Database
UI->>API: Submit profile/email/password update
API->>DB: Load user (includes is_managed_externally)
DB-->>API: user { is_managed_externally }
alt is_managed_externally == true
note right of API: Password validation still runs\nbut authorize() returns false
API->>API: Validate password (may throw)
API-->>UI: 403 Forbidden / Authorization denied
else is_managed_externally == false
API->>API: Validate password & authorize
API->>DB: Persist changes
DB-->>API: Success
API-->>UI: 200 OK (updated)
end
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-10-07T14:44:18.583ZApplied to files:
📚 Learning: 2025-10-15T11:55:53.461ZApplied to files:
📚 Learning: 2025-10-15T11:55:53.461ZApplied to files:
🧬 Code graph analysis (1)app/Filament/Admin/Resources/Users/UserResource.php (2)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/Http/Requests/Api/Client/Account/UpdatePasswordRequest.php (1)
28-28: Authorization logic is correct, but consider a custom error message.The logic correctly blocks password updates for externally managed users. However, users will receive a generic 403 Forbidden response without context about why the update was blocked.
Consider throwing a custom exception with a more descriptive message:
- return !$this->user()->is_managed_externally; + if ($this->user()->is_managed_externally) { + throw new HttpException(403, 'Password cannot be changed for externally managed users.'); + } + + return true;This would improve the user experience by providing clear feedback about why their request was denied.
app/Http/Requests/Api/Client/Account/UpdateEmailRequest.php (1)
29-29: Authorization logic is correct, but consider a custom error message.The logic correctly blocks email updates for externally managed users. However, like in
UpdatePasswordRequest.php, users will receive a generic 403 Forbidden response without context about why the update was blocked.Consider throwing a custom exception with a more descriptive message:
- return !$this->user()->is_managed_externally; + if ($this->user()->is_managed_externally) { + throw new HttpException(403, 'Email cannot be changed for externally managed users.'); + } + + return true;This would improve the user experience by providing clear feedback about why their request was denied.
app/Filament/Admin/Resources/Users/UserResource.php (1)
237-285: Consider UX: Collapsible section hides important user attributes.The timezone, language, and avatar fields are now in a collapsible "Customization" section. Since these are frequently edited user attributes, hiding them by default may reduce discoverability. Consider whether these fields should be immediately visible or if the section should default to expanded.
If you prefer the section to be expanded by default:
Section::make(trans('profile.tabs.customization')) - ->collapsible() + ->collapsible() + ->collapsed(false) ->columnSpanFull()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/Filament/Admin/Resources/Users/UserResource.php(2 hunks)app/Filament/Pages/Auth/EditProfile.php(1 hunks)app/Http/Requests/Api/Application/Users/StoreUserRequest.php(2 hunks)app/Http/Requests/Api/Client/Account/UpdateEmailRequest.php(1 hunks)app/Http/Requests/Api/Client/Account/UpdatePasswordRequest.php(1 hunks)app/Models/User.php(4 hunks)app/Transformers/Api/Application/UserTransformer.php(1 hunks)database/Factories/UserFactory.php(1 hunks)database/migrations/2025_10_23_073209_add_is_managed_externally_to_users.php(1 hunks)lang/en/admin/user.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/Filament/Pages/Auth/EditProfile.php (1)
app/Models/User.php (2)
User(95-496)
app/Filament/Admin/Resources/Users/UserResource.php (3)
app/Services/Helpers/LanguageService.php (2)
LanguageService(8-30)getAvailableLanguages(22-29)app/Models/User.php (1)
User(95-496)app/Extensions/OAuth/OAuthService.php (1)
OAuthService(10-71)
🔇 Additional comments (14)
database/Factories/UserFactory.php (1)
29-29: LGTM! Factory default correctly set.The
is_managed_externallyfield is correctly defaulted tofalsein the factory, which aligns with the model's default value and the expected behavior (users are not externally managed by default).app/Transformers/Api/Application/UserTransformer.php (1)
34-34: LGTM! API output correctly includes the new field.The
is_managed_externallyfield is properly exposed in the API transformer, allowing API consumers to determine whether a user is externally managed.app/Filament/Pages/Auth/EditProfile.php (1)
96-130: LGTM! Thoughtful UX implementation for externally managed users.The implementation appropriately handles form fields for externally managed users:
- Username and email fields are disabled (visible but not editable) so users can still see their current values
- Password field is hidden since it's a "new password" input that wouldn't make sense to show
- Password confirmation field is already conditionally visible based on the password field, so it naturally becomes hidden as well
This provides clear feedback to users while maintaining a clean interface.
app/Http/Requests/Api/Application/Users/StoreUserRequest.php (2)
23-31: LGTM! Field correctly added to API rules.The
is_managed_externallyfield is properly included in the allowed rules for user creation via the API.
39-45: LGTM! Attribute mapping provides clear context.The human-readable attribute name "Is managed by Third Party?" is clear and appropriate for API validation messages. While it differs from the UI label "Is managed externally?", this variation is acceptable as they target different audiences.
database/migrations/2025_10_23_073209_add_is_managed_externally_to_users.php (2)
12-17: LGTM! Migration correctly adds the column.The migration properly adds the
is_managed_externallyboolean column with a sensible default offalseand places it logically after the relatedexternal_idfield.
22-27: LGTM! Proper rollback included.The down() method correctly drops the column, allowing for clean rollback if needed.
app/Models/User.php (4)
51-51: LGTM! PHPDoc annotation added correctly.The
@propertyannotation foris_managed_externallyis properly documented as a boolean type.
144-144: LGTM! Safe default value.Setting
is_managed_externallytofalseby default is appropriate—users are not externally managed unless explicitly configured.
159-159: LGTM! Validation rule is appropriate.The boolean validation rule correctly validates the
is_managed_externallyfield type.
121-121: Now I need to search the codebase directly for the StoreUserRequest implementation to check if it has an authorize() method:Based on my verification of the shell script output, I can now provide a rewritten review comment. The search results show:
- Client endpoints (UpdatePasswordRequest, UpdateEmailRequest) have proper authorization checks preventing externally-managed users from modifying settings
- StoreUserRequest includes
is_managed_externallyin its validated rules (line 25 of the original script output)- Application API requires administrative API keys per Pterodactyl documentation
However, from the shell script output, the
authorize()method implementations in the request classes couldn't be fully verified through ast-grep (likely due to pattern matching), but the authorization logic is visible in the rg results showing client-side request classes havereturn !$this->user()->is_managed_externally;in their authorize methods.The key finding is that:
- Client-facing endpoints (UpdatePasswordRequest, UpdateEmailRequest) have explicit authorization preventing externally-managed users from modifying their data
- The field is properly included in the model's
$fillablearray- The Application API (which handles user creation via StoreUserRequest) is administratively controlled
- The admin UI properly disables/hides fields based on the flag
Authorization for
is_managed_externallyis properly enforced.Client API endpoints (UpdatePasswordRequest, UpdateEmailRequest) include explicit authorization checks returning
!$this->user()->is_managed_externally, preventing externally-managed users from modifying their email, username, or password. The Application API endpoint (StoreUserRequest) is restricted to administrative API keys, ensuring only admins can create or modify users with this flag. Admin UI properly disables affected fields when the flag is enabled.app/Filament/Admin/Resources/Users/UserResource.php (3)
36-36: LGTM! Necessary import added.The
Togglecomponent import is required for the newis_managed_externallyfield.
294-294: Minor: Empty line added.This appears to be a formatting change. No functional impact.
228-236: Translation keys verified and defined.Both
admin/user.is_managed_externallyandadmin/user.is_managed_externally_helperare properly defined inlang/en/admin/user.php(lines 13-14). The code changes are correct.
# Conflicts: # app/Filament/Admin/Resources/Users/UserResource.php
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Filament/Pages/Auth/EditProfile.php (1)
96-96: ConsiderreadOnly()for better UX and add helper text.As noted in a previous review,
readOnly()allows users to select and copy text, which provides better UX thandisabled(). If you usereadOnly(), add->dehydrated(false)to prevent the value from being submitted.Additionally, consider adding
->helperText()to explain to users why these fields cannot be edited (e.g., "This field is managed externally and cannot be edited here").Apply this diff to improve UX:
TextInput::make('username') - ->disabled(fn (User $user) => $user->is_managed_externally) + ->readOnly(fn (User $user) => $user->is_managed_externally) + ->dehydrated(false) + ->helperText(fn (User $user) => $user->is_managed_externally ? trans('profile.managed_externally_help') : null) ->prefixIcon('tabler-user') ->label(trans('profile.username')) ->required() ->maxLength(255) ->unique(), TextInput::make('email') - ->disabled(fn (User $user) => $user->is_managed_externally) + ->readOnly(fn (User $user) => $user->is_managed_externally) + ->dehydrated(false) + ->helperText(fn (User $user) => $user->is_managed_externally ? trans('profile.managed_externally_help') : null) ->prefixIcon('tabler-mail')Note: You'll need to add the translation key
profile.managed_externally_helpto your language files.Also applies to: 103-103
tests/Integration/Api/Application/Users/ExternalUserControllerTest.php (1)
26-26: LGTM! Consider adding tests for externally managed user behavior.The assertions correctly include the new
is_managed_externallyfield in the expected API response structure and values.However, consider adding test cases that verify externally managed users cannot update their username, email, or password through the API. This would ensure the authorization logic is working correctly.
Would you like me to generate test cases that verify the blocking behavior for externally managed users?
Also applies to: 36-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Filament/Admin/Resources/Users/UserResource.php(2 hunks)app/Filament/Pages/Auth/EditProfile.php(1 hunks)tests/Integration/Api/Application/Users/ExternalUserControllerTest.php(2 hunks)tests/Integration/Api/Application/Users/UserControllerTest.php(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Admin/Resources/Users/UserResource.php
🧰 Additional context used
🧬 Code graph analysis (2)
app/Filament/Pages/Auth/EditProfile.php (1)
app/Models/User.php (2)
User(95-497)
tests/Integration/Api/Application/Users/UserControllerTest.php (1)
tests/Integration/Api/Application/ApplicationApiIntegrationTestCase.php (1)
getApiUser(49-52)
🔇 Additional comments (2)
tests/Integration/Api/Application/Users/UserControllerTest.php (1)
29-30: LGTM! Consistent test assertions for the new field.All test assertions have been correctly updated to include
is_managed_externallyin the expected API response structures and values. The changes are consistent across all endpoints (list, single, relationships, create, and update).Similar to
ExternalUserControllerTest.php, consider adding dedicated test cases that verify externally managed users are blocked from updating their profiles through the Application API.Also applies to: 54-54, 71-71, 97-97, 105-105, 132-132, 219-219, 250-250
app/Filament/Pages/Auth/EditProfile.php (1)
111-111: Backend authorization is properly enforced. Verification confirms bothUpdatePasswordRequestandUpdateEmailRequestreturn!$this->user()->is_managed_externallyin theirauthorize()methods, blocking profile updates for externally managed users at the API layer. Frontend hiding combined with backend authorization provides complete protection.
lancepioch
left a comment
lancepioch
left a comment
There was a problem hiding this comment.
Looks great, but can we switch is_managed_externally from a bool to a nullable timestamp (similar to deleted_at).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Pages/Auth/EditProfile.php(1 hunks)app/Http/Requests/Api/Client/Account/UpdateUsernameRequest.php(1 hunks)app/Models/User.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Models/User.php
🔇 Additional comments (1)
app/Http/Requests/Api/Client/Account/UpdateUsernameRequest.php (1)
29-29: Logic is correct; verify error messaging for user experience.The authorization logic correctly prevents externally managed users from updating their username. When
authorize()returnsfalse, Laravel responds with a 403 Forbidden status. The codebase appears to use standard Laravel error handling without custom context-specific messages for authorization failures.Please verify that the API returns a meaningful error message to clients when an externally managed user attempts to update their username, so users understand why the operation is forbidden. Check:
- Whether the API has a response wrapper or middleware that adds context to authorization failure messages
- Whether frontend or API documentation explains the 403 response for this endpoint
- Whether the actual error response to clients includes information about the external management restriction
|
I agree with Lance if we are ever gonna add timestamp, might aswell do it now. |
I still see no reason to have a timestamp... It's either on or off. |
Closes #1728