feat(admin-rbac): implement role-based admin permissions, limits, and owner setup flow#507
Open
ImMohammad20000 wants to merge 76 commits into
Open
feat(admin-rbac): implement role-based admin permissions, limits, and owner setup flow#507ImMohammad20000 wants to merge 76 commits into
ImMohammad20000 wants to merge 76 commits into
Conversation
…ns and permissions
…ns and permissions
- Add setup router with owner creation, password reset, and deletion endpoints - Implement temp key CRUD operations with 5-minute TTL for secure one-time actions - Add OwnerCreateRequest, OwnerResetRequest, and OwnerDeleteRequest models - Extract get_client_ip utility function for reuse across routers - Add get_owner helper to retrieve the owner admin (role_id=1) - Remove TUI (Terminal UI) components and related dependencies - Remove system.py CLI module and consolidate functionality - Update Makefile to remove TUI-related targets - Add setup API tests for owner initialization workflows - Migrate admin router to use shared get_client_ip utility
- Add seeding of three default admin roles (owner, administrator, operator) to session factory fixture - Ensure FK constraints on admins.role_id are satisfied during test execution - Prevent foreign key constraint violations when tests create admin records - Roles are seeded with locked/unlocked status matching production defaults
- Add new permissions module with PermissionDenied and LimitExceeded exceptions - Implement enforce_permission() to check admin access to resources and actions - Implement enforce_scope() to enforce "own" vs "all" scope restrictions - Add permission-based decorators for route protection (@require_permission, @require_scope) - Register exception handlers in app factory for 403 and 400 responses - Update Admin model to include role and permission_overrides fields - Optimize admin queries with selectinload for role relationship - Add update_owner_password() CRUD function for owner password resets - Add comprehensive permission enforcement tests - Update test fixtures to seed default admin roles - Enable role-based access control across admin and user management endpoints
- Rename is_locked column to is_owner across database models and migrations - Update AdminRole model, CRUD operations, and Pydantic schemas to use is_owner - Create AdminRoleData model for runtime role data in permission checks - Add is_owner property to AdminDetails for convenient access - Update permission enforcement logic to reference role.is_owner - Clarify intent: owner role has unconditional access, not just "locked" status - Update test fixtures and API permission tests to reflect new naming
…tions - Remove operator type conditional logic from admin creation, modification, and removal - Eliminate CLI-specific sudo admin creation restrictions - Remove API/WEB-specific sudo promotion and modification restrictions - Simplify get_admins method by removing compact mode logic based on operator type - Remove _is_non_blocking_sync_operator static method no longer needed - Consolidate logging to always execute regardless of operator type - Update deprecation warnings to remove version references - Reorganize imports in alphabetical order for consistency - Rename local variables for clarity (new_admin to new_admin_details, modified_admin to modified_admin_details) - Simplify conditional checks (use current_admin existence instead of operator type checks) - Streamline method signatures and remove unnecessary docstring details
- Add admin_id filtering to get_user and get_user_by_id CRUD operations - Integrate scope-based access control in BaseOperation.get_validated_user methods - Replace manual admin ownership checks with get_scope_admin_id permission lookups - Add _get_group_with_access helper to enforce group access boundaries - Apply group access filtering in GroupOperation methods using apply_group_access - Update all router endpoints to respect admin scope isolation for users, groups, and related resources - Remove OperatorType import from admin.py as permission system replaces operator checks - Ensure admins can only access resources belonging to their scope, preventing cross-admin data leakage
- Add admin_role notification enable configuration to NotificationEnable model - Add admin_role notification channel to NotificationChannels settings - Implement admin_role notification handlers for Discord with create, modify, remove operations - Implement admin_role notification handlers for Telegram with create, modify, remove operations - Add Discord message templates for admin role create, modify, and remove events - Add Telegram message templates for admin role create, modify, and remove events - Create admin_role operation module for business logic - Add admin_role router with endpoints for CRUD operations - Add admin_role dependency injection for permission validation - Add comprehensive test suite for admin role API endpoints - Integrate admin_role notifications into main notification dispatcher
- Implemented per-user limits enforcement in bulk user creation and modification processes. - Added new permission checks for user-related operations, ensuring proper access control. - Introduced scope-based permission handling, allowing for more granular access management. - Refactored user creation and modification methods to utilize new template access validation. - Updated tests to cover new permission and scope functionalities, ensuring robust access control.
… injection
- Remove unused get_current import from authentication module
- Replace get_current dependency with require_permission("admins", "read") in get_admin_usage endpoint
- Replace get_current dependency with require_permission("admins", "read") in get_admin_usage_by_username endpoint
- Replace get_current dependency with require_permission("admins", "read") in get_admin_usage_by_id endpoint
- Enforce explicit permission checks for all admin usage retrieval operations
- Introduced HasPermission and IsScopeAll filters for RBAC in Telegram handlers. - Updated user and admin handlers to utilize new permission filters. - Enhanced AdminPanel and UserPanel to conditionally display buttons based on permissions. - Refactored main menu rendering to be permission-aware. - Improved code readability and maintainability by consolidating permission checks.
… role-based permissions
…uage - Remove is_sudo field checks from JWT token validation in authentication - Update env admin fallback logic to rely on sudoers list without is_sudo flag - Replace "sudo admin" with "authorized admin" in API documentation and docstrings - Update authorization comments from "non-sudo" to "non-owner" for clarity - Simplify admin permission checks to use role-based access control - Update test helpers and fixtures to reflect new authorization model - Remove redundant safety limit comment in admin query - Align terminology across routers (group, node, user) and CRUD operations - Consolidate authorization language to use consistent role-based terminology throughout codebase
- Add admin roles management feature with create, edit, delete, and bulk operations - Create new admin-roles feature module with components, dialogs, and forms - Implement RBAC utility functions for permission and scope validation - Add admin roles page with list view, search, and filtering capabilities - Update admin management to integrate with role-based permissions system - Add role assignment and permission scoping (none, own, all) for admin actions - Implement built-in role detection and protection against modification - Add comprehensive i18n translations for admin roles across 4 languages - Update navigation, routing, and access control to support role-based features - Add role-based limits and feature flags configuration in role forms - Integrate template and group restrictions with role permissions - Update API service layer to support admin roles endpoints - Enhance route guards and permission checks throughout dashboard
- Replace sequential get_validated_user_by_id calls with single batched query for improved performance - Add early return for empty user_ids list to avoid unnecessary processing - Add validation to ensure all requested user IDs are found, maintaining consistency with per-user retrieval behavior
…n-readable format
…ort owner-level access
- Replace serialize_users_for_node call with inline list comprehension for direct user serialization - Update docstring to clarify batch removal behavior and node-side inbound dropping - Simplify remove_users function by removing intermediate function call overhead
- Add on_hold_expire_duration parameter to user creation flow - Add on_hold_expire_duration parameter to modify_user method signature - Implement validation for on_hold_expire_duration against role limits - Validate minimum expiration duration constraint with readable error message - Validate maximum expiration duration constraint with readable error message - Pass on_hold_expire_duration through user creation, modification, and sync operations - Ensures on-hold duration respects configured role-level expiration limits
…notification system
…ction to filter by usage percentage
…ns data limit query
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/operation/client_template.py (1)
223-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssign the replacement default after the delete, not before it.
remove_client_template()flips the default only after the old default is gone. Doing it in the opposite order here can temporarily create two defaults for the same type or fail insideset_default_template(), so the bulk path no longer matches the single-delete path.Suggested fix
- # Handle default template replacements + # Resolve default replacements up front, but apply them after deletion + replacements_by_type = {} for template_type, templates_of_type in templates_by_type.items(): defaults_to_replace = [t for t in templates_of_type if t.is_default] if defaults_to_replace: exclude_ids = {t.id for t in templates_of_type} replacement = await get_first_template_by_type(db, template_type, exclude_ids=exclude_ids) if replacement: - await set_default_template(db, replacement) + replacements_by_type[template_type] = replacement @@ cleared_hosts = await clear_host_subscription_template_overrides(db, template_ids) await remove_client_templates(db, template_ids) + + for replacement in replacements_by_type.values(): + await set_default_template(db, replacement)🤖 Prompt for 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. In `@app/operation/client_template.py` around lines 223 - 229, The current bulk-delete loop preemptively calls get_first_template_by_type and set_default_template before removing the old default, which can create two defaults or cause set_default_template to fail; instead, for each template_type in templates_by_type, defer selecting and calling set_default_template until after remove_client_template has deleted the old default(s): i.e., call remove_client_template (or the bulk delete) first for the templates in templates_of_type, then if defaults were present call get_first_template_by_type(db, template_type, exclude_ids=...) and finally call set_default_template(db, replacement) so the replacement is assigned only after the original default(s) are gone.app/telegram/handlers/admin/bulk_actions.py (1)
69-79:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass
admininto the RBAC-aware template lookup.
UserTemplateOperation.get_user_templates()now requiresadmin, so this callback will raise before rendering the selector and won't apply the new template-access filtering.Suggested fix
async def bulk_create_from_template(event: CallbackQuery, db: AsyncSession, state: FSMContext): - templates = await user_templates.get_user_templates(db, UserTemplateListQuery()) +async def bulk_create_from_template( + event: CallbackQuery, + db: AsyncSession, + state: FSMContext, + admin: AdminDetails, +): + templates = await user_templates.get_user_templates(db, UserTemplateListQuery(), admin)🤖 Prompt for 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. In `@app/telegram/handlers/admin/bulk_actions.py` around lines 69 - 79, The callback bulk_create_from_template calls user_templates.get_user_templates without the required admin argument; update the call to pass the RBAC admin (e.g., the current admin user/context) into get_user_templates so the method can apply template-access filtering—locate the bulk_create_from_template function and change the invocation user_templates.get_user_templates(db, UserTemplateListQuery()) to include the admin parameter expected by UserTemplateOperation.get_user_templates (for example user_templates.get_user_templates(db, UserTemplateListQuery(), admin=admin) or equivalent based on how the admin is obtained in this handler).app/routers/admin_role.py (1)
91-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn no body for 204 responses.
Line 91 declares
status_code=status.HTTP_204_NO_CONTENT, but line 101 returns{}. According to HTTP specifications, a 204 response must not include a response body. Returning an empty object will serialize to a JSON body, which violates the 204 contract.Suggested fix
-from fastapi import APIRouter, Depends, status +from fastapi import APIRouter, Depends, Response, status @@ async def delete_role( @@ await role_operator.delete_role(db, role_id, admin) - return {} + return Response(status_code=status.HTTP_204_NO_CONTENT)🤖 Prompt for 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. In `@app/routers/admin_role.py` around lines 91 - 102, The delete_role endpoint currently declares status_code=HTTP_204_NO_CONTENT but returns an empty dict ({}), which produces a JSON body; change delete_role so it returns no body — e.g., return a Response/JSONResponse with only the 204 status (or simply return None) after calling role_operator.delete_role(db, role_id, admin); ensure you use the FastAPI/Starlette Response type (import if missing) so the response contains only the 204 status and no serialized body.app/db/crud/admin.py (1)
237-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
get_adminstype/doc contract with the actual 5-value return.Line 237 and the docstring still describe a 4-item count tuple, but Line 353 returns 5 items (
limitedadded). This can cause unpacking and typing drift for callers.Also applies to: 353-353
🤖 Prompt for 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. In `@app/db/crud/admin.py` around lines 237 - 248, The get_admins signature and docstring incorrectly state a 4-item tuple for return_with_count but the function actually returns 5 values (admins, total, active, disabled, limited); update the type annotation and docstring in get_admins to reflect the five-value tuple when return_with_count is True, mention the added 'limited' count in the Returns section, and ensure any references to the return shape (including the return_with_count parameter description) are consistent with the variable name limited returned at the end of the function.
🟡 Minor comments (6)
dashboard/src/features/admin-roles/components/admin-role-card.tsx-36-36 (1)
36-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix inflated feature-flag count.
Line 36 adds a hardcoded
+ 2, so the UI reports extra feature flags even when they don't exist.Suggested fix
- const featureCount = Object.keys(role.features || {}).length + 2 + const featureCount = Object.keys(role.features || {}).lengthAlso applies to: 74-74
🤖 Prompt for 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. In `@dashboard/src/features/admin-roles/components/admin-role-card.tsx` at line 36, The feature count is being inflated by a hardcoded "+ 2" in the calculation for featureCount; update the computation in the AdminRoleCard (where featureCount is defined and also at the duplicate location around line 74) to compute only Object.keys(role.features || {}).length (or include only real defaults programmatically) so it reflects the actual number of feature flags instead of adding two extra. Locate the featureCount variable and its duplicate and remove the "+ 2" so the UI displays the correct count.dashboard/src/hooks/use-dynamic-errors.ts-29-33 (1)
29-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMap nested validation paths instead of only
loc[1].Line 30 collapses nested FastAPI/Pydantic paths, so errors like
permission_overrides.max_userswon’t bind to the actual field key.Suggested fix
- detail.forEach((err: any) => { - const field = err?.loc?.[1] - if (field && fields.includes(field)) { - form.setError(field, { + detail.forEach((err: any) => { + const fieldPath = Array.isArray(err?.loc) + ? err.loc.filter((part: unknown) => part !== 'body').join('.') + : undefined + if (fieldPath && fields.includes(fieldPath)) { + form.setError(fieldPath, { type: 'manual', message: err.msg, }) } })🤖 Prompt for 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. In `@dashboard/src/hooks/use-dynamic-errors.ts` around lines 29 - 33, The current code only reads err?.loc?.[1] which collapses nested FastAPI/Pydantic paths; update the mapping in use-dynamic-errors.ts (inside the detail.forEach loop where err, fields and form.setError are used) to construct the full path by joining err.loc segments (e.g., err?.loc?.slice(1).join('.') or similar) and use that joined path to find a matching key in fields and call form.setError; keep a fallback to the existing single-segment behavior if needed (e.g., check both the joined path and the original err.loc[1]) so nested errors like "permission_overrides.max_users" correctly bind to the form field.dashboard/public/statics/locales/en.json-690-691 (1)
690-691:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
permissionOverrideshint text.This hint talks about inheriting limits and using
0, but the field is labeled as permission overrides. That will mislead admins configuring RBAC.Suggested fix
"permissionOverrides": "Permission overrides", - "permissionOverridesHint": "Leave empty to inherit limits from the selected role. Set to 0 to disable.", + "permissionOverridesHint": "Leave empty to inherit permissions from the selected role.",🤖 Prompt for 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. In `@dashboard/public/statics/locales/en.json` around lines 690 - 691, The hint for permissionOverrides is misleading; update permissionOverridesHint so it explicitly refers to permission overrides (not limits) and tells admins how to use the field: e.g., "Leave empty to inherit permissions from the selected role. Configure entries here to add or remove permissions; to disable overrides remove all entries." Change the string for permissionOverridesHint accordingly (see keys permissionOverrides and permissionOverridesHint).dashboard/src/features/admin-roles/components/admin-roles-list.tsx-168-175 (1)
168-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the clear-search button.
This is an icon-only
<button>without anaria-label, so assistive tech gets an unlabeled control.Suggested fix
{searchQuery && ( <button type="button" onClick={() => setSearchQuery('')} + aria-label={t('clearAllFilters', { defaultValue: 'Clear search' })} className={cn('absolute', dir === 'rtl' ? 'left-2' : 'right-2', 'top-1/2 -translate-y-1/2 text-muted-foreground hover:text-foreground')} > <X className="h-4 w-4" /> </button> )}🤖 Prompt for 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. In `@dashboard/src/features/admin-roles/components/admin-roles-list.tsx` around lines 168 - 175, The clear-search icon button rendered when searchQuery is truthy is missing an accessible name; update the button in admin-roles-list (the element that calls setSearchQuery('') and renders the X icon) to include an accessible label—e.g., add aria-label="Clear search" or use the app's i18n helper (like t('clearSearch'))—or include visually hidden text inside the button so assistive technologies can identify the control while keeping the X icon visible.dashboard/src/components/layout/nav-user.tsx-172-175 (1)
172-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRole badge is permanently hidden in the expanded trigger.
hidden+lg:hiddenmeans the badge never appears on any breakpoint.Suggested fix
-<Badge variant={isOwner(admin) ? 'secondary' : 'outline'} className="hidden h-4 px-1 py-0 text-[10px] lg:hidden"> +<Badge variant={isOwner(admin) ? 'secondary' : 'outline'} className="hidden h-4 px-1 py-0 text-[10px] lg:flex">🤖 Prompt for 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. In `@dashboard/src/components/layout/nav-user.tsx` around lines 172 - 175, The Badge element is using both "hidden" and "lg:hidden" which keeps it hidden at all breakpoints; update the className on the Badge (the Badge component inside nav-user.tsx near RoleIcon/roleLabel/isOwner) to actually show at the intended breakpoint—e.g., remove the global "hidden" and replace with breakpoint-aware visibility like "hidden lg:inline-flex" or "inline-flex lg:hidden" depending on whether it should appear on large screens or small screens so the roleLabel and RoleIcon become visible at the correct breakpoint.app/db/models.py-105-106 (1)
105-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote
AdminRolein theAdmin.roleannotation.
AdminRoleis defined 800+ lines later in this module (line 879), and this file doesn't enable postponed annotation evaluation. The unquoted forward reference violates Python's annotation rules and is inconsistent with similar relationship patterns in the same file (e.g., lines 8–9, 12) which correctly use quoted forward refs.Suggested fix
- role: Mapped[Optional[AdminRole]] = relationship(back_populates="admins", init=False, lazy="selectin") + role: Mapped[Optional["AdminRole"]] = relationship(back_populates="admins", init=False, lazy="selectin")Additionally, the
users_sync_blockedproperty (line 146) dereferencesself.role.disable_users_when_limitedwithout checking for None, butroleis typedOptional[AdminRole]. This should be protected with a null check or the type should be narrowed.🤖 Prompt for 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. In `@app/db/models.py` around lines 105 - 106, Update the Admin.role annotation to use a forward-reference string ("AdminRole") to match other relationships and avoid runtime annotation errors (change the type on the role field declared near role_id and role in Admin to "AdminRole"). Also fix users_sync_blocked (the property that reads self.role.disable_users_when_limited) to guard against a None role by checking self.role is not None before dereferencing (or narrow the type so role cannot be Optional) so you no longer access disable_users_when_limited on a possible None value.
🧹 Nitpick comments (5)
tests/api/helpers.py (1)
37-37: ⚡ Quick winUse
OPERATOR_ROLE_IDas the default instead of hardcoding3.
create_adminstill hardcodes3, which can diverge from the constant introduced for seeded role IDs.♻️ Small consistency fix
def create_admin( - access_token: str, *, username: str | None = None, password: str | None = None, role_id: int = 3 + access_token: str, *, username: str | None = None, password: str | None = None, role_id: int = OPERATOR_ROLE_ID ) -> dict:🤖 Prompt for 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. In `@tests/api/helpers.py` at line 37, Replace the hardcoded role id 3 with the seeded constant OPERATOR_ROLE_ID: update the function signature that currently uses "role_id: int = 3" to "role_id: int = OPERATOR_ROLE_ID" and also change create_admin (the create_admin call/site that currently passes or defaults to 3) to use OPERATOR_ROLE_ID instead. Ensure OPERATOR_ROLE_ID is imported/available in tests/api/helpers.py before updating the default and any create_admin usages.tests/test_record_usages.py (1)
108-108: ⚡ Quick winResolve role IDs dynamically in tests instead of hardcoding
role_id=3.Line 108 and Line 199 couple test correctness to implicit insertion order. Querying the operator role ID by name makes these tests resilient to seed/order changes.
Refactor sketch
-from app.db.models import Admin, Node, NodeUsage, NodeUserUsage, System, User +from app.db.models import Admin, AdminRole, Node, NodeUsage, NodeUserUsage, System, User ... - admin = Admin(username="admin", hashed_password="secret", role_id=3) + operator_role_id = ( + await session.execute(select(AdminRole.id).where(AdminRole.name == "operator")) + ).scalar_one() + admin = Admin(username="admin", hashed_password="secret", role_id=operator_role_id) ... - admin = Admin(username="admin", hashed_password="secret", role_id=3) + operator_role_id = ( + await session.execute(select(AdminRole.id).where(AdminRole.name == "operator")) + ).scalar_one() + admin = Admin(username="admin", hashed_password="secret", role_id=operator_role_id)Also applies to: 199-199
🤖 Prompt for 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. In `@tests/test_record_usages.py` at line 108, Replace the hardcoded role_id used when constructing Admin instances with a lookup against the Role model: query Role for the desired role name (e.g., "admin" or "operator") using your ORM (Role.get/filter_by/one or session.query(Role).filter_by(name=...).one().id) and pass that dynamic id into Admin(username=..., hashed_password=..., role_id=role.id); update both occurrences where Admin(... role_id=3) is used so tests no longer depend on insertion order.app/operation/host.py (1)
142-149: ⚡ Quick winShort-circuit empty bulk selections before querying.
If
bulk_hosts.idsis empty, this now buildsHostListQuery(ids=[], limit=0)and hands it to a generic list query. If the downstream CRUD helper treats falsy filters as “no filter”, a no-op bulk action becomes “all hosts”.🛡️ Suggested guard
ids_list = list(bulk_hosts.ids) + if not ids_list: + return RemoveHostsResponse(hosts=[], count=0) db_hosts = await get_hosts(db, HostListQuery(ids=ids_list, limit=len(ids_list))) @@ ids_list = list(bulk_hosts.ids) + if not ids_list: + return self._build_bulk_action_response([]) db_hosts = await get_hosts(db, HostListQuery(ids=ids_list, limit=len(ids_list)))Also applies to: 177-184
🤖 Prompt for 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. In `@app/operation/host.py` around lines 142 - 149, The code builds a HostListQuery and calls get_hosts even when bulk_hosts.ids is empty, which can be treated as “no filter” and affect all hosts; add an early guard that checks if not bulk_hosts.ids (or if ids_list is empty) and immediately raise the 404 or return the appropriate no-results behavior instead of calling HostListQuery/get_hosts. Update the same pattern around the other occurrence (the block using ids_list at the later location) so both places short-circuit before constructing HostListQuery(ids=..., limit=...).app/operation/__init__.py (1)
161-206: ⚡ Quick winMirror the scope override on the username lookup helper.
Line 179 still hard-codes
users:read, while the ID-based helper now acceptsscope_resource/scope_action. That makes the by-username path easy to reuse in write/delete flows with read-scope enforcement by mistake.♻️ Proposed change
async def get_validated_user( self, db: AsyncSession, username: str, admin: AdminDetails, *, load_admin: bool = True, load_next_plan: bool = True, load_usage_logs: bool = True, load_groups: bool = True, + scope_resource: str = "users", + scope_action: str = "read", ) -> User: db_user = await get_user( db, username, load_admin=load_admin, load_next_plan=load_next_plan, load_usage_logs=load_usage_logs, load_groups=load_groups, - admin_id=get_scope_admin_id(admin, "users", "read"), + admin_id=get_scope_admin_id(admin, scope_resource, scope_action), )🤖 Prompt for 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. In `@app/operation/__init__.py` around lines 161 - 206, The username-based helper get_validated_user currently hard-codes get_scope_admin_id(admin, "users", "read") which mismatches get_validated_user_by_id's flexible scope_resource/scope_action; change get_validated_user signature to accept scope_resource: str = "users" and scope_action: str = "read" (matching get_validated_user_by_id) and pass get_scope_admin_id(admin, scope_resource, scope_action) into the get_user call so callers can override scope when needed; update any call sites to pass the new args where appropriate.tests/api/test_user.py (1)
2643-2745: ⚡ Quick winAvoid hard-coding builtin role IDs in these RBAC tests.
Using
role_id=2/role_id=3ties the tests to migration seed order instead of the role behavior they care about. Looking the role up by stable name/slug, or creating a role with the required permission set in the fixture, would make these tests much less brittle.🤖 Prompt for 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. In `@tests/api/test_user.py` around lines 2643 - 2745, Tests use hard-coded role_id values in create_admin (e.g., role_id=2 and role_id=3) which makes them brittle; update calls to create_admin to resolve roles by stable identifier instead: fetch or create the role by slug/name (e.g., lookup_role_by_slug("administrator") or get_role_by_name(...)) or use a fixture that returns a role object with the needed permissions, then pass role["id"] (or role_id) to create_admin in tests like test_get_users_sub_update_chart_administrator_can_view_other_admin, test_get_users_sub_update_chart_other_admin_requires_admins_read, test_get_users_sub_update_chart_operator_can_view_own, and any other usages; ensure helper functions (create_admin) accept the resolved role id and update fixtures/helpers if necessary so tests no longer rely on migration seed order.
🤖 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 `@app/db/crud/temp_key.py`:
- Around line 25-34: The consume_temp_key function is vulnerable to race
conditions and doesn't enforce TTL; instead of separate lookup via get_temp_key
then updating, perform the validation and update atomically inside
consume_temp_key—either issue a conditional UPDATE that sets
action/used_at/used_by_ip where key == TempKey.key AND used_at IS NULL AND
(expires_at IS NULL OR expires_at > now()) and verify rows_affected == 1, or
SELECT the row FOR UPDATE inside the same transaction, check expires_at and
used_at, then set fields and commit; update callers to rely on consume_temp_key
to both validate and mark single-use (refer to functions get_temp_key and
consume_temp_key to locate the code).
In `@app/jobs/review_admins.py`:
- Around line 93-103: The current module registers limit_admins_job at import
time via scheduler.add_job when runtime_settings.role.runs_scheduler, which
causes DB-dependent jobs to run before migrations (admins table missing); change
this by moving the scheduler.add_job call into a new function
register_review_admins_jobs() that performs the same scheduler.add_job(...)
(preserving options: "interval",
seconds=job_settings.review_admin_limits_interval, coalesce=True,
max_instances=1, start_date=dt.now(tz.utc), id="limit_admins",
replace_existing=True) and do NOT call it on import—instead call
register_review_admins_jobs() from the application startup path that runs after
DB bootstrap/migrations.
In `@app/operation/admin.py`:
- Around line 355-359: Bulk admin mutations call remove_admins after validating
existence via _get_validated_bulk_admins, bypassing the single-item safety
checks in _remove_admin/_modify_admin; update the bulk handlers (the code using
_get_validated_bulk_admins and remove_admins) to enforce the same protections by
either iterating over db_admins and invoking the single-item helpers
(_remove_admin or _modify_admin) for each id or by extracting and calling the
shared validation/authorization logic used by _remove_admin/_modify_admin before
calling remove_admins/modify_admins so that owner protection and peer-admin
protections are applied for every item.
In `@app/operation/node.py`:
- Around line 1008-1020: The current _get_validated_nodes uses get_nodes(...)
which bypasses per-node RBAC checks implemented by get_validated_node, causing a
security regression; update _get_validated_nodes to either (A) iterate over
ids_list and call await self.get_validated_node(db, node_id) for each id to
preserve per-node scope/permission checks, collecting results, or (B) if you
keep the batched get_nodes call for performance, pass the same
authorization/scope predicates used by get_validated_node into get_nodes so the
returned db_nodes are already filtered by the same RBAC logic (ensure missing
IDs are computed after applying those predicates and raise the same error via
self.raise_error when any node is not authorized or not found).
In `@app/operation/permissions.py`:
- Around line 64-70: The code currently treats only None and
PermissionScope.NONE as denials, but _get_resource_action can return the boolean
False which should also be denied; update the check after calling
_get_resource_action(action_perm) to explicitly treat False as a denial (e.g.,
if action_perm is None or action_perm is False: raise PermissionDenied(...)),
keeping the existing scope resolution and the PermissionScope.NONE check using
_resolve_scope(action_perm).
In `@app/routers/setup.py`:
- Around line 17-26: The current _validate_key flow fetches the temp key via
get_temp_key and only marks it used after the privileged action, allowing race
conditions; replace this two-step pattern with a single atomic consume operation
that sets used_at (e.g., UPDATE ... SET used_at = now() WHERE key = :key AND
used_at IS NULL AND expires_at > now() RETURNING *), and treat a non-returned
row as failure (raise the appropriate HTTPException) before proceeding with
create/reset/delete owner actions; apply the same change for the other
occurrences referenced (lines with create/reset/delete flows) so consumption and
validation happen in one DB statement rather than separate fetch + update.
In `@app/telegram/handlers/admin/user.py`:
- Around line 633-641: The current check only blocks when user.admin exists and
differs from admin.username, letting unowned users bypass scoped-own checks;
change the flow to first call enforce_permission(admin, "users", "read") (catch
PermissionDenied and reply Texts.user_not_found), then fetch scope_id =
get_scope_admin_id(admin, "users", "read") and if scope_id is not None and
user.admin_id != scope_id return await event.reply(Texts.user_not_found); keep
references to user.admin/user.admin_id, enforce_permission, get_scope_admin_id,
PermissionDenied and Texts.user_not_found to locate and update the logic.
In `@dashboard/src/features/admin-roles/components/admin-role-actions-menu.tsx`:
- Line 2: In AdminRoleActionsMenu (component in admin-role-actions-menu.tsx)
replace the unsafe rendering that uses dangerouslySetInnerHTML for role.name
(the delete confirmation/label rendering) with react-i18next's Trans component:
import Trans from react-i18next or use the existing useTranslation, construct a
translation string that includes a variable placeholder for the role name and
render the role name as a normal React element/escaped text inside <Trans> so it
is not injected as HTML; update the delete confirmation/modal (where role.name
is interpolated) to pass role.name as a value or child element to Trans instead
of using dangerouslySetInnerHTML, ensuring the role string is rendered safely
and any formatting uses React elements rather than raw HTML.
---
Outside diff comments:
In `@app/db/crud/admin.py`:
- Around line 237-248: The get_admins signature and docstring incorrectly state
a 4-item tuple for return_with_count but the function actually returns 5 values
(admins, total, active, disabled, limited); update the type annotation and
docstring in get_admins to reflect the five-value tuple when return_with_count
is True, mention the added 'limited' count in the Returns section, and ensure
any references to the return shape (including the return_with_count parameter
description) are consistent with the variable name limited returned at the end
of the function.
In `@app/operation/client_template.py`:
- Around line 223-229: The current bulk-delete loop preemptively calls
get_first_template_by_type and set_default_template before removing the old
default, which can create two defaults or cause set_default_template to fail;
instead, for each template_type in templates_by_type, defer selecting and
calling set_default_template until after remove_client_template has deleted the
old default(s): i.e., call remove_client_template (or the bulk delete) first for
the templates in templates_of_type, then if defaults were present call
get_first_template_by_type(db, template_type, exclude_ids=...) and finally call
set_default_template(db, replacement) so the replacement is assigned only after
the original default(s) are gone.
In `@app/routers/admin_role.py`:
- Around line 91-102: The delete_role endpoint currently declares
status_code=HTTP_204_NO_CONTENT but returns an empty dict ({}), which produces a
JSON body; change delete_role so it returns no body — e.g., return a
Response/JSONResponse with only the 204 status (or simply return None) after
calling role_operator.delete_role(db, role_id, admin); ensure you use the
FastAPI/Starlette Response type (import if missing) so the response contains
only the 204 status and no serialized body.
In `@app/telegram/handlers/admin/bulk_actions.py`:
- Around line 69-79: The callback bulk_create_from_template calls
user_templates.get_user_templates without the required admin argument; update
the call to pass the RBAC admin (e.g., the current admin user/context) into
get_user_templates so the method can apply template-access filtering—locate the
bulk_create_from_template function and change the invocation
user_templates.get_user_templates(db, UserTemplateListQuery()) to include the
admin parameter expected by UserTemplateOperation.get_user_templates (for
example user_templates.get_user_templates(db, UserTemplateListQuery(),
admin=admin) or equivalent based on how the admin is obtained in this handler).
---
Major comments:
In `@app/db/crud/admin_role.py`:
- Around line 44-46: get_roles_simple is selecting scalar columns so it returns
SQLAlchemy Row tuples, not list[AdminRole]; change the query in get_roles_simple
to select the ORM entity (use select(AdminRole) and .scalars().all() so the
function truly returns list[AdminRole]) and then update the call site that
consumes get_roles_simple (the code that indexes rows) to access attributes (use
role.id / role.name / role.is_owner) on the returned AdminRole instances instead
of tuple indexing.
In `@app/db/crud/admin.py`:
- Around line 483-491: The get_active_to_limited_admins function currently runs
a query that crashes when the admins table/schema isn't yet present; wrap the
query in a defensive guard: check schema availability or surround the
db.execute(stmt) with a try/except that catches SQLAlchemy schema/operational
errors (e.g., OperationalError / ProgrammingError / SQLAlchemyError) and return
an empty list if the table is missing, optionally logging the condition; ensure
you reference the existing function name get_active_to_limited_admins, the Admin
and AdminStatus models, and the db.execute(stmt) call so the fix is applied
around that query.
In `@app/db/migrations/versions/66c38b8a687a_admin_rbac_roles.py`:
- Around line 107-115: The backfill currently writes hardcoded role IDs (2/3) to
admins.role_id which is brittle; instead query the roles table by name and use
those resolved IDs in the UPDATEs. In your migration (where you call
op.get_bind() / conn.execute), SELECT id FROM roles WHERE name = 'administrator'
and WHERE name = 'operator' via conn.execute(...) to fetch administrator_id and
operator_id, then run parameterized UPDATEs against admins (using
admins.role_id, is_sudo) substituting those IDs (and handling NULL role_id as in
your original logic); this avoids engine/PK assumptions and makes the migration
safe to rerun.
In `@app/db/migrations/versions/a1d3f5b7c9e2_admin_status_and_data_limit.py`:
- Around line 83-90: The downgrade mapping currently sets is_disabled only when
status == 'disabled', which re-enables users with status 'limited'; update the
SQL in the migration (in the block that executes conn.execute(...) in
a1d3f5b7c9e2_admin_status_and_data_limit.py) so that both 'disabled' and
'limited' statuses set is_disabled to true/1: for the PostgreSQL branch include
both statuses in the boolean expression (e.g., status = 'disabled'::adminstatus
OR status = 'limited'::adminstatus), and for the non-Postgres branch use a
CASE/IN check so CASE WHEN status IN ('disabled','limited') THEN 1 ELSE 0 END.
In `@app/db/migrations/versions/b1e4f9a2c3d5_drop_is_sudo.py`:
- Around line 26-34: The rollback backfill hardcodes administrator as role_id =
2; change the logic in this migration to first query the roles table for the
administrator role id (e.g. SELECT id FROM roles WHERE name = 'administrator')
via conn.execute/sa.text, store that id (handle the case where it’s missing),
and then use that dynamic id in the UPDATE statements against the admins table
instead of the literal 2; keep the existing dialect-aware boolean/int branches
(use the retrieved admin_role_id in the WHERE clauses and, if the role lookup
returns no row, default to setting is_sudo=false for all or fail loudly as
appropriate).
In `@app/db/migrations/versions/bb4a32b7f5ce_add_admin_notification_reminders.py`:
- Around line 25-37: The migration uses postgresql.ENUM with create_type=False
for the remindertype enum which may not exist at runtime; update the upgrade()
migration to ensure the enum type is created before calling op.create_table by
either invoking postgresql.ENUM(...).create(op.get_bind(), checkfirst=True) (or
equivalent op.execute to create the type) or switch to postgresql.ENUM(...,
create_type=True) when constructing reminder_type, and reference the
reminder_type variable and the op.create_table call so the enum creation happens
prior to creating the admin_notification_reminders table in upgrade().
In `@app/db/models.py`:
- Around line 143-146: The users_sync_blocked property can raise AttributeError
because self.role may be None; update the users_sync_blocked property to guard
access to role (e.g., check if self.role is truthy or use getattr with a
default) and return False when role is missing, so that the expression involving
AdminStatus.limited and role.disable_users_when_limited safely defaults to
False; update the property on the model (users_sync_blocked) to explicitly
handle a missing or None self.role before reading disable_users_when_limited.
In `@app/models/admin.py`:
- Around line 120-123: The is_owner property on the Admin model is a plain
`@property` and thus won't be included in serialized responses; add the
computed-field decorator by replacing/removing the `@property` and annotating the
method with `@computed_field` (matching how is_disabled and is_limited are
defined) so that is_owner (the method named is_owner) is exposed by Pydantic
serialization and retains the same Boolean return logic (return
self.role.is_owner if self.role is not None else False).
In `@app/models/setup.py`:
- Around line 4-16: Update the Pydantic models OwnerCreateRequest,
OwnerResetRequest, and OwnerDeleteRequest to enforce non-empty, non-whitespace
strings for the key, username, and password fields at the schema layer; replace
plain str annotations for key/username/password with constrained types (e.g.,
pydantic.constr(...)/Field(..., min_length=1, strip_whitespace=True) or similar)
so empty or whitespace-only values fail validation before reaching handlers —
change the annotations on OwnerCreateRequest.key, OwnerCreateRequest.username,
OwnerCreateRequest.password, OwnerResetRequest.key, OwnerResetRequest.password,
and OwnerDeleteRequest.key accordingly.
In `@app/notification/telegram/admin.py`:
- Around line 16-20: The role string is inserted raw into Telegram
HTML-formatted templates (e.g., when building messages.CREATE_ADMIN and the
similar UPDATE/other message at the 32-36 block) which allows injected markup;
before formatting, HTML-escape the dynamic role value (the local variable role)
using a proper HTML-escaping helper (e.g., html.escape or the Telegram library’s
escape helper) and pass the escaped value into messages.CREATE_ADMIN / other
message.format calls so no untrusted markup can be rendered.
In `@app/operation/admin.py`:
- Around line 417-423: The bulk-reset loop currently calls reset_admin_usage(db,
db_admin=...) which skips the re-sync logic in _reset_admin_usage(), so change
the loop to call and await _reset_admin_usage(db, db_admin=...) (or extract and
reuse the re-sync portion from _reset_admin_usage into a helper and call that)
for each admin returned by _get_validated_bulk_admins; preserve the
notification.admin_usage_reset(reseted_admin, admin.username) and logger.info
lines, ensure you convert the returned admin to AdminDetails.model_validate as
before, and return the same _build_bulk_action_response(db_admins) so the bulk
path performs the same state transition and resync as the single-admin flow.
- Around line 314-324: When handling non-self queries for admins, also check
whether the caller has the nodes.stats permission and strip node-scoped output
if they do not: after the existing enforce_permission(admin, "admins", "read")
check (and its except block), call enforce_permission(admin, "nodes", "stats")
in a try/except; on PermissionDenied set node_id = None and group_by_node =
False so node-level breakdowns are removed. Keep the existing self-access branch
(which already clears node_id/group_by_node) but ensure the new nodes.stats
gating is applied for non-self access paths that passed admins.read.
In `@app/operation/group.py`:
- Around line 145-147: The bulk-action code is converting the "unrestricted"
sentinel from apply_group_access(admin, requested_ids) into an empty list,
breaking admin full-access paths used by _get_group_with_access; change the
calls that pass allowed_ids into get_groups_by_ids (and similar bulk blocks
around the other occurrence) to preserve None as the sentinel (i.e., pass
allowed_ids through unchanged or pass None when allowed_ids is None) so
get_groups_by_ids receives None for full access instead of [] and bulk
delete/enable/disable will not 404 for admins.
In `@app/operation/user_template.py`:
- Around line 107-110: The code incorrectly converts a None "unrestricted"
result from apply_template_access into an empty list by using "allowed_ids or
[]", causing admins to see no templates; change the calls that build
UserTemplateListQuery to pass allowed_ids directly (e.g.,
UserTemplateListQuery(ids=allowed_ids)) so that None remains None and
_get_template_with_access / get_user_templates can treat None as unrestricted
access; update the other occurrence around the block that references lines
142-143 similarly.
In `@app/operation/user.py`:
- Around line 1001-1004: The RBAC action used when deriving list scope is
swapped: change the get_scope_admin_id calls so get_users() uses the
"users.read" action and get_users_simple() uses "users.read_simple" (i.e., use
the action that matches the endpoint's contract). Update the call sites that
currently pass the wrong action (including the second occurrence around the
get_scope_admin_id call at the later block) to use the matching action strings,
ensuring get_scope_admin_id(admin, "users", "<correct_action>") is consistent
with the function (get_users vs get_users_simple).
- Around line 668-676: The code incorrectly uses get_scope_admin_id(admin,
"users", "read") for bulk mutation flows; change the lookup to use the actual
required write permission for the operation instead of the fixed "read" scope
(e.g., pass "write", "delete", "update" or a caller-provided permission name).
Update the call site that builds UserListQuery (the block using
get_scope_admin_id, UserListQuery, and get_users) to accept or compute the
correct permission string for the mutation and call get_scope_admin_id(admin,
"users", "<required_action>") so admin_ids reflect write/delete/update scope;
ensure callers of this helper supply the intended action scope for each bulk
endpoint.
In `@app/routers/host.py`:
- Around line 73-76: The delete endpoints are currently checking
require_permission("hosts", "update") which lets update-only roles delete hosts;
update remove_host's dependency to require_permission("hosts", "delete") and do
the same for the other host-delete handler(s) in this module so destructive
operations require the "hosts:delete" permission (look for require_permission
calls in the delete route functions and replace "update" with "delete").
In `@app/routers/user.py`:
- Around line 261-266: The endpoint reset_users_data_usage currently uses
require_scope_all("users", "reset_usage") but also calls
node_operator.restart_all_node, allowing users-only roles to trigger cluster
restarts; update the authorization so that restart_all_node requires node-level
permission (e.g. require_scope_all("nodes","restart")) by either adding a second
dependency to reset_users_data_usage that enforces node restart scope or move
the restart call into a separate node-authorized endpoint; locate
reset_users_data_usage and the calls to user_operator.reset_users_data_usage and
node_operator.restart_all_node and enforce the proper RBAC check via
require_scope_all before invoking restart_all_node.
- Around line 803-808: The endpoint bulk_reallocate_wireguard_peer_ips is using
require_scope_all("users", "update") which blocks admins with owner-scoped
update rights; change the dependency to use the same regular update permission
used by other bulk user actions (i.e., the require_scope variant that allows
owner-scoped admins) so the route-level check permits owner-scoped admins and
let user_operator.bulk_reallocate_wireguard_peer_ips(...) enforce per-admin
scope; update the dependency on the handler signature (replace require_scope_all
usage) accordingly.
In `@dashboard/src/features/admin-roles/components/admin-roles-list.tsx`:
- Line 60: The component currently calls useGetRoles({ limit: 100, offset: 0 })
and thus silently truncates results; change the fetching to support pagination
or unlimited fetches: replace the hard-coded call by driving limit/offset (or
page) from component state (e.g. rolesPage, rolesLimit) and pass those into
useGetRoles, add UI controls (load more / next/previous / page size) that update
the state and call refetch, and ensure search/edit/bulk-delete logic (which
reads rolesData) operates on the full current page or triggers server-side
filtered queries so no roles beyond the current page are invisible. Reference
useGetRoles, rolesData, refetch, isLoading/isFetching and update the component
to manage and pass dynamic pagination parameters instead of the fixed { limit:
100, offset: 0 }.
In `@dashboard/src/features/admin-roles/dialogs/admin-role-modal.tsx`:
- Around line 83-88: The feature/access cards' onClick handlers in AccessSection
are still mutating the shared form state (toggling field.value) when readOnly is
true; update AccessSection to accept a disabled/readOnly prop and early-return
in the card click handlers (e.g., at the top of the onClick that flips
field.value) when disabled/readOnly is truthy, and when rendering AccessSection
from AdminRoleModal pass the modal's readOnly flag (or formDisabled) down; also
apply the same guard to any other card components that toggle field.value so
clicks no longer mutate the form in read-only mode.
In `@dashboard/src/features/admin-roles/forms/admin-role-form.ts`:
- Around line 20-107: PERMISSION_GROUPS is missing several RBAC actions that
VALID_PERMISSION_ACTIONS accepts, so the UI can't grant or display them; update
the PERMISSION_GROUPS array to include the missing action entries under the
appropriate groups: add { resource: 'users', action: 'reset_usage' }, {
resource: 'users', action: 'revoke_sub' }, { resource: 'users', action:
'set_owner' }, { resource: 'users', action: 'activate_next_plan' } into the
group with labelKey 'users'; add { resource: 'admins', action: 'reset_usage' }
into the group with labelKey 'admins'; and add { resource: 'nodes', action:
'reconnect' } and { resource: 'nodes', action: 'update_core' } into the group
with labelKey 'nodes' so the role editor renders and can manage these
permissions (ensure strings match VALID_PERMISSION_ACTIONS exactly).
In `@dashboard/src/features/admins/dialogs/admin-modal.tsx`:
- Around line 123-129: The UI shows a visual default role '3' but the form
payload can still send undefined; fix by keeping the form state and submit
payload in sync: set the form's initial value for role_id to '3' when creating a
new admin (i.e. when editingAdmin is false) or, alternatively, coerce
values.role_id to '3' when building the submit payload (use values.role_id ??
'3') so every payload (the places that build the submit object around role_id
and the other payload builders noted) always sends the same default role as
displayed.
- Around line 339-343: The Telegram ID input's onChange currently converts an
empty string to 0 and sets field.onChange(0), preventing removal; update the
onChange handler in the Telegram ID input so that when e.target.value is empty
it calls field.onChange(null) (or undefined) instead of 0, and keep the value
prop as field.value ?? '' so the input shows empty when unbound; ensure the
form/type handling for the related field (the controller/form schema that reads
this Telegram ID) accepts null/undefined as "unbound" instead of treating 0 as a
valid id.
In `@dashboard/src/pages/_dashboard.admins.tsx`:
- Around line 138-152: getRoleIdForAdmin currently returns a default id (3) when
it can’t find a role, which causes silent reassignment; change getRoleIdForAdmin
(and any callers like handleEdit) to return undefined/null when rolesQuery has
no match or is still loading instead of falling back to 3, and update the
handleEdit flow that calls form.reset({ role_id: ... }) so it passes that
undefined/null (or omits role_id) when unresolved; also consider
short-circuiting when rolesQuery.isLoading to avoid preselecting a role while
roles are loading.
In `@dashboard/src/pages/_dashboard.settings.tsx`:
- Around line 56-58: The read permission checks are incorrectly coupled to
update permission: remove the dependency on canUpdateSettings so reads use
hasPermission directly; specifically change canReadSettings and canReadGeneral
(and any other similar checks that use "&& canUpdateSettings") to call
hasPermission(admin, 'settings', 'read') and hasPermission(admin, 'settings',
'read_general') respectively without ANDing with canUpdateSettings, and update
any downstream gating logic that currently expects those booleans so that
read-only users can fetch and view settings while update remains controlled by
canUpdateSettings.
In `@dashboard/src/service/api/index.ts`:
- Around line 3228-3229: The generated type AdminModifyPermissionOverrides
incorrectly aliases permission_overrides to RoleLimits; update the OpenAPI/spec
source so the permission_overrides property references the correct
permission-override schema (the admin permission-override shape) instead of the
limits schema, then re-run the client generator to regenerate types; apply the
same fix for the other generated aliases where permission_overrides was wired to
RoleLimits (the other occurrences around the AdminCreate/related permission
override types) so all permission_overrides types use the permission-override
shape.
In `@dashboard/src/utils/rbac.ts`:
- Line 84: The route-guard currently returns true for unknown paths (the `return
true` at line 84), which makes RBAC permissive; change this to deny-by-default
by returning false for unrecognized routes (replace the `return true` fallback
with `return false`), or better: explicitly check the known route-to-role
mapping in the route guard (the function containing that `return true`) and only
return true when a match exists and the user's roles authorize it; add a short
debug log when denying unknown routes to aid troubleshooting.
In `@tests/api/test_bulk_delete_entities.py`:
- Around line 323-339: The test test_bulk_delete_admins_rejects_owner_account is
asserting successful deletion of a normal admin but should verify
owner-protection; change it to attempt a bulk-delete of the owner (role_id=1)
and assert the operation is rejected (HTTP 403) and the owner is still present.
Locate the owner record using an available helper (e.g., get_owner_admin(),
admin_service.get_owner(), or a fixture that returns the owner) and use that
owner's id in the client.post to "/api/admins/bulk/delete" (same auth_headers
usage), then assert response.status_code is 403 and that the owner's username/id
remains in the system rather than asserting HTTP_200 or deletion as currently
done in test_bulk_delete_admins_rejects_owner_account and its use of
create_admin().
In `@tests/api/test_usage_functions_timezone.py`:
- Line 47: The test currently hard-codes role_id=3 when creating an Admin
instance; replace that with a dynamic lookup or fixture: query the Role model
for the intended role (e.g., by name or unique slug) and use its id (or attach
the Role instance) when constructing Admin instead of role_id=3, e.g., resolve
role = Role.get(...)/session.query(Role).filter_by(name="...").first() and then
use role.id or role when creating Admin; update any related fixtures (Admin
creation code) to derive role id dynamically so tests don't depend on seed PK
ordering.
---
Minor comments:
In `@app/db/models.py`:
- Around line 105-106: Update the Admin.role annotation to use a
forward-reference string ("AdminRole") to match other relationships and avoid
runtime annotation errors (change the type on the role field declared near
role_id and role in Admin to "AdminRole"). Also fix users_sync_blocked (the
property that reads self.role.disable_users_when_limited) to guard against a
None role by checking self.role is not None before dereferencing (or narrow the
type so role cannot be Optional) so you no longer access
disable_users_when_limited on a possible None value.
In `@dashboard/public/statics/locales/en.json`:
- Around line 690-691: The hint for permissionOverrides is misleading; update
permissionOverridesHint so it explicitly refers to permission overrides (not
limits) and tells admins how to use the field: e.g., "Leave empty to inherit
permissions from the selected role. Configure entries here to add or remove
permissions; to disable overrides remove all entries." Change the string for
permissionOverridesHint accordingly (see keys permissionOverrides and
permissionOverridesHint).
In `@dashboard/src/components/layout/nav-user.tsx`:
- Around line 172-175: The Badge element is using both "hidden" and "lg:hidden"
which keeps it hidden at all breakpoints; update the className on the Badge (the
Badge component inside nav-user.tsx near RoleIcon/roleLabel/isOwner) to actually
show at the intended breakpoint—e.g., remove the global "hidden" and replace
with breakpoint-aware visibility like "hidden lg:inline-flex" or "inline-flex
lg:hidden" depending on whether it should appear on large screens or small
screens so the roleLabel and RoleIcon become visible at the correct breakpoint.
In `@dashboard/src/features/admin-roles/components/admin-role-card.tsx`:
- Line 36: The feature count is being inflated by a hardcoded "+ 2" in the
calculation for featureCount; update the computation in the AdminRoleCard (where
featureCount is defined and also at the duplicate location around line 74) to
compute only Object.keys(role.features || {}).length (or include only real
defaults programmatically) so it reflects the actual number of feature flags
instead of adding two extra. Locate the featureCount variable and its duplicate
and remove the "+ 2" so the UI displays the correct count.
In `@dashboard/src/features/admin-roles/components/admin-roles-list.tsx`:
- Around line 168-175: The clear-search icon button rendered when searchQuery is
truthy is missing an accessible name; update the button in admin-roles-list (the
element that calls setSearchQuery('') and renders the X icon) to include an
accessible label—e.g., add aria-label="Clear search" or use the app's i18n
helper (like t('clearSearch'))—or include visually hidden text inside the button
so assistive technologies can identify the control while keeping the X icon
visible.
In `@dashboard/src/hooks/use-dynamic-errors.ts`:
- Around line 29-33: The current code only reads err?.loc?.[1] which collapses
nested FastAPI/Pydantic paths; update the mapping in use-dynamic-errors.ts
(inside the detail.forEach loop where err, fields and form.setError are used) to
construct the full path by joining err.loc segments (e.g.,
err?.loc?.slice(1).join('.') or similar) and use that joined path to find a
matching key in fields and call form.setError; keep a fallback to the existing
single-segment behavior if needed (e.g., check both the joined path and the
original err.loc[1]) so nested errors like "permission_overrides.max_users"
correctly bind to the form field.
---
Nitpick comments:
In `@app/operation/__init__.py`:
- Around line 161-206: The username-based helper get_validated_user currently
hard-codes get_scope_admin_id(admin, "users", "read") which mismatches
get_validated_user_by_id's flexible scope_resource/scope_action; change
get_validated_user signature to accept scope_resource: str = "users" and
scope_action: str = "read" (matching get_validated_user_by_id) and pass
get_scope_admin_id(admin, scope_resource, scope_action) into the get_user call
so callers can override scope when needed; update any call sites to pass the new
args where appropriate.
In `@app/operation/host.py`:
- Around line 142-149: The code builds a HostListQuery and calls get_hosts even
when bulk_hosts.ids is empty, which can be treated as “no filter” and affect all
hosts; add an early guard that checks if not bulk_hosts.ids (or if ids_list is
empty) and immediately raise the 404 or return the appropriate no-results
behavior instead of calling HostListQuery/get_hosts. Update the same pattern
around the other occurrence (the block using ids_list at the later location) so
both places short-circuit before constructing HostListQuery(ids=..., limit=...).
In `@tests/api/helpers.py`:
- Line 37: Replace the hardcoded role id 3 with the seeded constant
OPERATOR_ROLE_ID: update the function signature that currently uses "role_id:
int = 3" to "role_id: int = OPERATOR_ROLE_ID" and also change create_admin (the
create_admin call/site that currently passes or defaults to 3) to use
OPERATOR_ROLE_ID instead. Ensure OPERATOR_ROLE_ID is imported/available in
tests/api/helpers.py before updating the default and any create_admin usages.
In `@tests/api/test_user.py`:
- Around line 2643-2745: Tests use hard-coded role_id values in create_admin
(e.g., role_id=2 and role_id=3) which makes them brittle; update calls to
create_admin to resolve roles by stable identifier instead: fetch or create the
role by slug/name (e.g., lookup_role_by_slug("administrator") or
get_role_by_name(...)) or use a fixture that returns a role object with the
needed permissions, then pass role["id"] (or role_id) to create_admin in tests
like test_get_users_sub_update_chart_administrator_can_view_other_admin,
test_get_users_sub_update_chart_other_admin_requires_admins_read,
test_get_users_sub_update_chart_operator_can_view_own, and any other usages;
ensure helper functions (create_admin) accept the resolved role id and update
fixtures/helpers if necessary so tests no longer rely on migration seed order.
In `@tests/test_record_usages.py`:
- Line 108: Replace the hardcoded role_id used when constructing Admin instances
with a lookup against the Role model: query Role for the desired role name
(e.g., "admin" or "operator") using your ORM (Role.get/filter_by/one or
session.query(Role).filter_by(name=...).one().id) and pass that dynamic id into
Admin(username=..., hashed_password=..., role_id=role.id); update both
occurrences where Admin(... role_id=3) is used so tests no longer depend on
insertion order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+93
to
+103
| if runtime_settings.role.runs_scheduler: | ||
| scheduler.add_job( | ||
| limit_admins_job, | ||
| "interval", | ||
| seconds=job_settings.review_admin_limits_interval, | ||
| coalesce=True, | ||
| max_instances=1, | ||
| start_date=dt.now(tz.utc), | ||
| id="limit_admins", | ||
| replace_existing=True, | ||
| ) |
There was a problem hiding this comment.
Defer limit_admins_job scheduling until schema readiness is guaranteed.
The job is registered at import time and starts immediately, which matches the migration CI failures (admins table not found). This can crash startup/test flows before migrations finish.
💡 Suggested direction
-if runtime_settings.role.runs_scheduler:
- scheduler.add_job(
- limit_admins_job,
- "interval",
- seconds=job_settings.review_admin_limits_interval,
- coalesce=True,
- max_instances=1,
- start_date=dt.now(tz.utc),
- id="limit_admins",
- replace_existing=True,
- )
+def register_review_admins_jobs() -> None:
+ if not runtime_settings.role.runs_scheduler:
+ return
+ scheduler.add_job(
+ limit_admins_job,
+ "interval",
+ seconds=job_settings.review_admin_limits_interval,
+ coalesce=True,
+ max_instances=1,
+ start_date=dt.now(tz.utc),
+ id="limit_admins",
+ replace_existing=True,
+ )Then call register_review_admins_jobs() from the app startup path that runs after DB bootstrap/migrations.
🤖 Prompt for 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.
In `@app/jobs/review_admins.py` around lines 93 - 103, The current module
registers limit_admins_job at import time via scheduler.add_job when
runtime_settings.role.runs_scheduler, which causes DB-dependent jobs to run
before migrations (admins table missing); change this by moving the
scheduler.add_job call into a new function register_review_admins_jobs() that
performs the same scheduler.add_job(...) (preserving options: "interval",
seconds=job_settings.review_admin_limits_interval, coalesce=True,
max_instances=1, start_date=dt.now(tz.utc), id="limit_admins",
replace_existing=True) and do NOT call it on import—instead call
register_review_admins_jobs() from the application startup path that runs after
DB bootstrap/migrations.
| @@ -0,0 +1,105 @@ | |||
| import { useState } from 'react' | |||
| import { useTranslation } from 'react-i18next' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "admin-role-actions-menu.tsx" | head -5Repository: PasarGuard/panel
Length of output: 136
🏁 Script executed:
cat -n ./dashboard/src/features/admin-roles/components/admin-role-actions-menu.tsx | head -100Repository: PasarGuard/panel
Length of output: 5355
🏁 Script executed:
cat -n ./dashboard/src/features/admin-roles/components/admin-role-actions-menu.tsx | tail -10Repository: PasarGuard/panel
Length of output: 436
🏁 Script executed:
rg "import.*Trans.*from.*react-i18next" --type tsx --type ts | head -10Repository: PasarGuard/panel
Length of output: 88
🏁 Script executed:
rg "import.*Trans.*from.*react-i18next" | head -10Repository: PasarGuard/panel
Length of output: 1011
🏁 Script executed:
rg "Trans" --type-list | head -20Repository: PasarGuard/panel
Length of output: 450
🏁 Script executed:
rg "Trans" --no-filename | grep -E "from.*react-i18next|<Trans" | head -15Repository: PasarGuard/panel
Length of output: 765
🏁 Script executed:
rg "<Trans" | head -10Repository: PasarGuard/panel
Length of output: 655
🏁 Script executed:
grep -A 5 "import.*Trans" dashboard/src/components/common/groups-selector.tsx | head -20Repository: PasarGuard/panel
Length of output: 306
🏁 Script executed:
grep -A 10 "<Trans" dashboard/src/components/common/groups-selector.tsx | head -20Repository: PasarGuard/panel
Length of output: 521
🏁 Script executed:
rg "AdminRoleResponse" dashboard/src/service/api | head -5Repository: PasarGuard/panel
Length of output: 734
🏁 Script executed:
grep -A 10 "export interface AdminRoleResponse" dashboard/src/service/api/index.tsRepository: PasarGuard/panel
Length of output: 343
Replace dangerouslySetInnerHTML with the Trans component to safely handle role name interpolation.
Line 92 renders role.name into raw HTML through dangerouslySetInnerHTML, creating a stored XSS vulnerability. A role name containing HTML/script (e.g., <img src=x onerror="alert('xss')">) would execute when an admin attempts to delete it. Use the Trans component with React elements for formatting instead.
Proposed fix
-import { useTranslation } from 'react-i18next'
+import { Trans, useTranslation } from 'react-i18next'
...
<AlertDialogDescription>
- <span dir={dir} dangerouslySetInnerHTML={{ __html: t('adminRoles.deleteConfirm', { name: role.name, defaultValue: 'Are you sure you want to delete role <b>{{name}}</b>?' }) }} />
+ <span dir={dir}>
+ <Trans
+ i18nKey="adminRoles.deleteConfirm"
+ values={{ name: role.name }}
+ defaults="Are you sure you want to delete role <b>{{name}}</b>?"
+ components={{ b: <b /> }}
+ />
+ </span>
</AlertDialogDescription>🤖 Prompt for 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.
In `@dashboard/src/features/admin-roles/components/admin-role-actions-menu.tsx` at
line 2, In AdminRoleActionsMenu (component in admin-role-actions-menu.tsx)
replace the unsafe rendering that uses dangerouslySetInnerHTML for role.name
(the delete confirmation/label rendering) with react-i18next's Trans component:
import Trans from react-i18next or use the existing useTranslation, construct a
translation string that includes a variable placeholder for the role name and
render the role name as a normal React element/escaped text inside <Trans> so it
is not injected as HTML; update the delete confirmation/modal (where role.name
is interpolated) to pass role.name as a value or child element to Trans instead
of using dangerouslySetInnerHTML, ensuring the role string is rendered safely
and any formatting uses React elements rather than raw HTML.
…mponent reorganization - Add "Toggle all notifications" label across all locale files (en, fa, ru, zh) - Add "Profile & contact" section label for admin forms in all locales - Add "Admin data limit" label in admin management UI across all locales - Move usage-slider-compact component from features/users to common components - Refactor AdminStatusBadge to simplify status info logic and remove unused icon imports - Update admin role modal badge max-width from rem to Tailwind class for consistency - Simplify admin and user data table column definitions for improved maintainability
…anagement - Add reset_usage, revoke_sub, set_owner, and activate_next_plan permissions for users resource - Add reset_usage permission for admins resource - Add reconnect and update_core permissions for nodes resource - Add corresponding i18n translations across en, fa, ru, and zh locales - Refactor VALID_PERMISSION_ACTIONS to derive from PERMISSION_GROUPS for single source of truth - Improve maintainability by eliminating duplicate permission definitions
- Update migration to set disable_users_when_limited server default to 1 - Remove explicit UPDATE query for built-in roles from migration - Change AdminRole model default from False to True - Update AdminRoleData model default to True - Update AdminRoleBase model default to True - Update admin role form default values to true - Update form response mapping to default to true when undefined - Ensures all admin roles disable users when data-limited by default
ImMohammad20000
force-pushed
the
admin-perm
branch
from
c222cd2 to
f92372d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaking Changes
Production Upgrade Checklist
1. Pre-Deployment
2. Access and Data Preparation
3. Deployment Order
4. Immediate Smoke Tests
5. Post-Deployment Validation
6. Rollback Plan
Release Sign-Off Focus
Summary by CodeRabbit
New Features
Bug Fixes
Chores