Added link to user-creatable-servers, for automatic creation of User Resource Limits.#131
Added link to user-creatable-servers, for automatic creation of User Resource Limits.#131TheOriginalCER06 wants to merge 2 commits intopelican-dev:mainfrom
Conversation
Not tested without both plugins.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request adds a configurable maximum user registration cap and automatic initialization of default per-user resource limits on signup. It includes a new Filament settings UI that allows administrators to configure registration limits and default CPU, memory, disk, and server limits, with settings persisted to a PHP config file. ChangesRegistration Limit Cap and Auto Resource Limits
Sequence Diagram(s)sequenceDiagram
participant Client
participant RegisterPage
participant UserDB
participant UserResourceLimitsDB
Client->>RegisterPage: GET mount()
RegisterPage->>UserDB: count users (check max_users)
alt limit reached
RegisterPage->>Client: 403 (registration_closed)
else
Client->>RegisterPage: POST registration data
RegisterPage->>UserDB: verify limit (lock optional)
alt limit reached
RegisterPage->>Client: ValidationException (registration_closed)
else
RegisterPage->>UserDB: create user
RegisterPage->>UserResourceLimitsDB: updateOrCreate defaults
RegisterPage->>Client: return created user
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@register/src/Filament/Pages/Auth/Register.php`:
- Around line 68-96: The current registration cap check in
throwIfRegistrationLimitReached/abortIfRegistrationLimitReached is non-atomic
and can be bypassed under concurrency because you read count() before creating a
user; change handleRegistration so the check and the actual user creation occur
inside a serialized critical section (e.g., a DB transaction with a table/row
lock or a DB advisory lock) to guarantee atomicity: acquire the lock, re-check
the count via getUserModel()::query()->lockForUpdate() or an advisory lock,
abort/throw if limit reached, then perform the parent::handleRegistration($data)
create and call createDefaultUserResourceLimits($user) before releasing the
lock; keep mount()/abortIfRegistrationLimitReached as a UX-only precheck.
In `@register/src/RegisterPlugin.php`:
- Around line 101-107: The config assignment in RegisterPlugin.php does not
guard against negative integers because UI checks can be bypassed; update the
$config population (the array keys 'max_users', 'default_cpu', 'default_memory',
'default_disk', 'default_server_limit' in the RegisterPlugin class) to clamp
each value to a non-negative integer before persisting (e.g., cast to int then
apply max(0, value) or equivalent) so all stored values are guaranteed >= 0.
- Around line 97-119: persistSettingsToConfig writes to the config file
($configPath) but doesn't clear Laravel's cached config; after the successful
file_put_contents write (i.e., after confirming it didn't return false) call the
config cache clear operation (e.g., invoke cache('config')->clear() or an
equivalent Artisan config clear) so subsequent calls to config('register.*')
reflect the new values; place this call immediately after the write succeeds and
before the method finishes, and keep it guarded so failures still throw the
existing RuntimeException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b266dd4-c868-4c25-91bb-5d0737b27c4c
📒 Files selected for processing (6)
register/README.mdregister/config/register.phpregister/lang/en/messages.phpregister/plugin.jsonregister/src/Filament/Pages/Auth/Register.phpregister/src/RegisterPlugin.php
📜 Review details
🧰 Additional context used
🪛 LanguageTool
register/README.md
[grammar] ~12-~12: Use a hyphen to join words.
Context: ...` is installed ## Integration with User Creatable Servers This plugin can autom...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (4)
register/config/register.php (1)
3-9: Config keys and defaults look consistent.The new defaults align with the registration/resource-limit settings introduced in this PR.
register/lang/en/messages.php (1)
3-5: Translation string is clear and correctly scoped.Message text is suitable for the new max-user registration guard.
register/plugin.json (1)
4-5: Manifest metadata update looks good.Author/version updates are coherent with the feature additions in this PR.
register/README.md (1)
9-23: README changes are aligned with behavior.The new docs accurately describe
max_usersand default resource-limit provisioning for new users.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
I don't see a reason for a user limit on the registration. Defaults for newly created users are fine, but this should go in the user-creatable-servers plugin and apply for all new users. |
|
Limit is because it might be accidentally exposed to the whole internet, and when allowing new user registration you might not want them to use all your available resources. (A safety guard for the average user). Agree, it was just my first thought. It makes a lot more sense for the default to be there. Might move in the future, but currently I don't have the need or time to change this. You can close this, if you want. |
2 participants
Not tested without both plugins.
Made for own use, works for me, thought it might be good to contribute back to the community.
Summary by CodeRabbit
New Features
Documentation
Localization
Chores