added user roles, migrations, app_user junction table #203
Conversation
- role can be owner or app_admin - only owner can create, modify delete users - check if the user with role app_admin has app access in floware_proxy_service - removed super_admin env variable and also from readme
📝 WalkthroughWalkthroughReplace email-based super-admin with role-based RBAC (OWNER, APP_ADMIN); add AppUser association, services, controllers, client APIs and UI for app-level access; update DB models, alembic migrations, DI wiring, and remove SUPER_ADMIN_EMAIL from docs/config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant UserRepo as User Repository
participant AppService as App Service
participant DB as Database
Note over Controller,UserRepo: App create/update/delete flows require OWNER
Client->>Controller: POST /v1/apps (create_app) with session.user_id
Controller->>UserRepo: get_user_by_id(session.user_id)
UserRepo->>DB: SELECT user WHERE id=...
DB-->>UserRepo: user row (role)
UserRepo-->>Controller: User(role)
alt role == OWNER
Controller->>AppService: create_app(payload)
AppService->>DB: INSERT app
DB-->>AppService: created app
AppService-->>Controller: app
Controller-->>Client: 201 Created
else
Controller-->>Client: 403 Forbidden
end
sequenceDiagram
participant Client
participant Proxy as FlowareProxyService
participant UserService
participant UserRepo as User Repository
participant AppUserRepo as AppUser Repository
participant DB as Database
Note over Proxy,UserService: Proxy enforces app access via UserService
Client->>Proxy: proxy_request(app_id) (session with user_id)
Proxy->>UserService: check_user_has_app_access(user_id, app_id)
UserService->>UserRepo: get_user_by_id(user_id)
UserRepo->>DB: SELECT user WHERE id=...
DB-->>UserRepo: user row
UserRepo-->>UserService: User(role)
alt role == OWNER
UserService-->>Proxy: true
else
UserService->>AppUserRepo: query app_user WHERE user_id & app_id
AppUserRepo->>DB: SELECT ...
DB-->>AppUserRepo: rows
AppUserRepo-->>UserService: association exists?
alt association exists
UserService-->>Proxy: true
else
UserService-->>Proxy: false
end
end
alt access granted
Proxy->>Client: forward/200
else
Proxy-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. ✨ Finishing touches
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: 6
🤖 Fix all issues with AI Agents
In
@wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py:
- Around line 79-126: The revoke_app_access handler lacks existence checks for
the app and target user; update revoke_app_access to mirror grant_app_access by
first verifying the target user and the app exist before calling
app_user_service.revoke_app_access: use user_repository.find_one(id=user_id) to
confirm the user, and likewise use the app_repository (or
app_service.find_one/find_by_id) to confirm the app exists, return a 404
JSONResponse via response_formatter.buildErrorResponse if either is missing,
then proceed to call app_user_service.revoke_app_access and keep the existing
logging and error handling.
- Around line 21-76: The grant_app_access handler currently skips validating
that app_id exists and returns raw exception text to clients; inject and use the
app repository or service (e.g., add app_repository: SQLAlchemyRepository[App]
or app_service: AppService as a dependency) in grant_app_access, call
app_repository.find_one(id=app_id, deleted=False) (or equivalent
app_service.get_app) and return a 404 with
response_formatter.buildErrorResponse('App not found') if absent before calling
app_user_service.grant_app_access; also stop exposing str(e) to clients — change
the exception handlers in grant_app_access (and apply the same pattern to the
other error handlers in this file) to logger.exception(...) or logger.error(...,
exc_info=True) to log full details, but return a generic
response_formatter.buildErrorResponse('Failed to grant app access') (no
exception text) with 500 to the client.
- Around line 157-184: The controller currently calls
user_repository.find(deleted=False) then filters in Python (variables
user_repository.find, user_ids, filtered_users, users_data); instead, call the
repository with an ID-IN filter so the DB does the work (e.g.
user_repository.find(deleted=False, id__in=user_ids) or the equivalent list/IN
filter supported by SQLAlchemyRepository.find) and then map the returned users
to users_data—replace the full-table fetch and post-filtering with a single find
call using the user_ids list.
In @wavefront/server/apps/floconsole/floconsole/db/alembic/env.py:
- Line 11: The Alembic env.py is missing the AppUser model import and
registration for autogeneration; add an import for AppUser (e.g., from
floconsole.db.models.app_user import AppUser or from floconsole.db.models.app
import AppUser if colocated) and ensure AppUser is included in the
models/target_metadata collection used by Alembic (the same list or MetaData
passed to autogenerate alongside App) so the app_user join table is tracked for
migrations.
In
@wavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1644-0644a81ee4e1_add_role_to_user.py:
- Around line 27-29: The migration currently limits the role update to
non-deleted users; change the SQL executed by conn (obtained via op.get_bind())
so it updates every row in the "user" table (remove the WHERE deleted = false
clause) — i.e. execute sa.text('UPDATE "user" SET role = \'owner\'') so deleted
users get the same role applied for historical consistency.
In
@wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py:
- Around line 70-82: Convert and validate session.user_id and app_id with
explicit UUID conversion inside minimal try/except blocks and raise a clear,
specific exception (e.g., BadRequest for invalid UUIDs) instead of letting
UUID(...) raise uncaught ValueError; remove the redundant try/except that simply
re-raises around user_service.check_user_has_app_access, call
check_user_has_app_access(user_id=UUID(...), app_id=UUID(...)) directly, and if
it returns False raise a well-constructed exception with a clear message (do not
catch and rethrow ValueError). Ensure the error messages are created when
raising the exceptions (not inline in a no-op except) and reference
session.user_id, user_id, app_id, and user_service.check_user_has_app_access to
locate the changes.
🧹 Nitpick comments (6)
wavefront/server/apps/floconsole/floconsole/db/models/__init__.py (1)
8-8: Consider sorting__all__alphabetically.Static analysis suggests sorting the
__all__list for consistency. While not required, this improves maintainability.🔎 Proposed fix for alphabetical sorting
-__all__ = ['User', 'Session', 'App', 'AppUser'] +__all__ = ['App', 'AppUser', 'Session', 'User']wavefront/server/apps/floconsole/floconsole/services/user_service.py (2)
38-43: Optimize count query to avoid loading all records.The current implementation fetches all owner users and counts them in memory. For better performance, use a database-level COUNT query.
🔎 Proposed optimization
async def count_owners(self) -> int: """Count users with owner role that are not deleted""" - users = await self.user_repository.find( - role=UserRole.OWNER.value, deleted=False - ) - return len(users) + # Use a COUNT query instead of loading all records + query = """ + SELECT COUNT(*) as count + FROM "user" + WHERE role = :role AND deleted = false + """ + result = await self.user_repository.execute_query( + query, params={'role': UserRole.OWNER.value} + ) + return result[0]['count'] if result else 0
50-75: Consider using ORM instead of raw SQL for consistency.The raw SQL query works correctly with parameterized values, but using the ORM would be more consistent with the rest of the codebase. Additionally, querying the app_user table through the user_repository feels conceptually misplaced.
🔎 Alternative ORM-based approach
If you have access to the AppUser model through relationships, you could refactor to:
async def check_user_has_app_access(self, user_id: UUID, app_id: UUID) -> bool: """ Check if user has access to app. Owners have access to all apps. App admins need explicit app_user entry. """ user = await self.user_repository.find_one(id=user_id, deleted=False) if not user: return False # Owners have access to all apps if user.role == UserRole.OWNER.value: return True # For app_admins, check app_user relationship via ORM # This assumes app_users relationship is properly loaded return any(au.app_id == app_id for au in user.app_users)Note: This assumes the relationship is eager-loaded or you fetch it explicitly. The raw SQL approach may actually be more efficient if you don't need the full objects.
wavefront/server/apps/floconsole/floconsole/services/app_user_service.py (1)
24-27: Consider returning deletion status.The method always returns
Trueregardless of whether any records were actually deleted. Consider returning the count of deleted records or a boolean indicating whether the access existed.🔎 Potential improvement
async def revoke_app_access(self, user_id: UUID, app_id: UUID) -> bool: """Revoke user access to app""" - await self.app_user_repository.delete_all(user_id=user_id, app_id=app_id) - return True + deleted_count = await self.app_user_repository.delete_all(user_id=user_id, app_id=app_id) + return deleted_count > 0wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (2)
187-233: Validate target user existence for consistency.The endpoint retrieves apps for
user_idwithout verifying the user exists. For consistency with the other endpoints and better error messages, consider adding user validation.🔎 Recommended validation check
if not current_user or current_user.role != UserRole.OWNER.value: return JSONResponse( status_code=status.HTTP_403_FORBIDDEN, content=response_formatter.buildErrorResponse( 'Only owners can view user apps' ), ) + # Verify target user exists + target_user = await user_repository.find_one(id=user_id, deleted=False) + if not target_user: + return JSONResponse( + status_code=status.HTTP_404_NOT_FOUND, + content=response_formatter.buildErrorResponse('User not found'), + ) + # Get user apps try:
69-76: Consider catching more specific exceptions.All endpoints catch broad
Exceptiontypes. While this prevents unhandled errors, catching more specific exceptions (e.g.,DatabaseError,IntegrityError) would enable better error handling and more informative responses to clients.Example pattern:
try: await app_user_service.grant_app_access(user_id, app_id) # ... except IntegrityError: return JSONResponse( status_code=status.HTTP_409_CONFLICT, content=response_formatter.buildErrorResponse('Access already granted') ) except DatabaseError as e: logger.error(f'Database error: {str(e)}') return JSONResponse( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, content=response_formatter.buildErrorResponse('Failed to grant app access') )Also applies to: 119-126, 177-184, 226-233
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
DOCKER_SETUP.mddocker-compose.sample.ymlwavefront/server/apps/floconsole/floconsole/config.iniwavefront/server/apps/floconsole/floconsole/constants/user.pywavefront/server/apps/floconsole/floconsole/controllers/app_controller.pywavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.pywavefront/server/apps/floconsole/floconsole/controllers/user_controller.pywavefront/server/apps/floconsole/floconsole/db/alembic/env.pywavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1644-0644a81ee4e1_add_role_to_user.pywavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1646-8d947e3f8ec6_create_app_user_table.pywavefront/server/apps/floconsole/floconsole/db/models/__init__.pywavefront/server/apps/floconsole/floconsole/db/models/app.pywavefront/server/apps/floconsole/floconsole/db/models/app_user.pywavefront/server/apps/floconsole/floconsole/db/models/user.pywavefront/server/apps/floconsole/floconsole/di/application_container.pywavefront/server/apps/floconsole/floconsole/server.pywavefront/server/apps/floconsole/floconsole/services/app_user_service.pywavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.pywavefront/server/apps/floconsole/floconsole/services/user_service.py
💤 Files with no reviewable changes (3)
- DOCKER_SETUP.md
- docker-compose.sample.yml
- wavefront/server/apps/floconsole/floconsole/config.ini
🧰 Additional context used
🧬 Code graph analysis (11)
wavefront/server/apps/floconsole/floconsole/services/user_service.py (1)
wavefront/server/apps/floconsole/floconsole/constants/user.py (1)
UserRole(4-6)
wavefront/server/apps/floconsole/floconsole/services/app_user_service.py (2)
wavefront/server/apps/floconsole/floconsole/db/models/app_user.py (1)
AppUser(11-23)wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (2)
grant_app_access(23-76)revoke_app_access(81-126)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (1)
wavefront/server/apps/floconsole/floconsole/services/user_service.py (2)
UserService(10-75)check_user_has_app_access(50-75)
wavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1644-0644a81ee4e1_add_role_to_user.py (1)
wavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1646-8d947e3f8ec6_create_app_user_table.py (2)
upgrade(20-32)downgrade(35-38)
wavefront/server/apps/floconsole/floconsole/db/models/__init__.py (1)
wavefront/server/apps/floconsole/floconsole/db/models/app_user.py (1)
AppUser(11-23)
wavefront/server/apps/floconsole/floconsole/db/models/user.py (2)
wavefront/server/apps/floconsole/floconsole/constants/user.py (1)
UserRole(4-6)wavefront/server/apps/floconsole/floconsole/db/models/app.py (1)
to_dict(37-49)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (4)
wavefront/server/apps/floconsole/floconsole/constants/user.py (1)
UserRole(4-6)wavefront/server/apps/floconsole/floconsole/db/models/user.py (2)
User(14-49)to_dict(42-49)wavefront/server/apps/floconsole/floconsole/di/application_container.py (1)
ApplicationContainer(21-100)wavefront/server/apps/floconsole/floconsole/services/app_user_service.py (5)
AppUserService(8-35)grant_app_access(14-22)revoke_app_access(24-27)get_app_users(33-35)get_user_apps(29-31)
wavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1646-8d947e3f8ec6_create_app_user_table.py (1)
wavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1644-0644a81ee4e1_add_role_to_user.py (2)
upgrade(20-35)downgrade(38-40)
wavefront/server/apps/floconsole/floconsole/di/application_container.py (3)
wavefront/server/apps/floconsole/floconsole/db/models/app_user.py (1)
AppUser(11-23)wavefront/server/apps/floconsole/floconsole/services/user_service.py (1)
UserService(10-75)wavefront/server/apps/floconsole/floconsole/services/app_user_service.py (1)
AppUserService(8-35)
wavefront/server/apps/floconsole/floconsole/controllers/user_controller.py (4)
wavefront/server/apps/floconsole/floconsole/constants/user.py (1)
UserRole(4-6)wavefront/server/apps/floconsole/floconsole/db/models/user.py (1)
User(14-49)wavefront/server/apps/floconsole/floconsole/di/application_container.py (1)
ApplicationContainer(21-100)wavefront/server/apps/floconsole/floconsole/services/user_service.py (2)
is_owner(45-48)count_owners(38-43)
wavefront/server/apps/floconsole/floconsole/db/alembic/env.py (1)
wavefront/server/apps/floconsole/floconsole/db/models/app.py (1)
App(13-49)
🪛 Ruff (0.14.10)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py
31-31: Possible hardcoded password assigned to function default: "token_prefix"
(S107)
80-80: Abstract raise to an inner function
(TRY301)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
81-82: Remove exception handler; error is immediately re-raised
(TRY203)
82-82: Use raise without specifying exception name
Remove exception name
(TRY201)
wavefront/server/apps/floconsole/floconsole/db/models/__init__.py
8-8: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
27-29: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
30-32: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-35: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
69-69: Do not catch blind exception: Exception
(BLE001)
70-70: Use explicit conversion flag
Replace with conversion flag
(RUF010)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
85-87: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
88-90: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
91-93: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
119-119: Do not catch blind exception: Exception
(BLE001)
120-120: Use explicit conversion flag
Replace with conversion flag
(RUF010)
124-124: Use explicit conversion flag
Replace with conversion flag
(RUF010)
134-136: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
137-139: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
140-142: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
177-177: Do not catch blind exception: Exception
(BLE001)
178-178: Use explicit conversion flag
Replace with conversion flag
(RUF010)
182-182: Use explicit conversion flag
Replace with conversion flag
(RUF010)
192-194: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
195-197: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
198-200: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
226-226: Do not catch blind exception: Exception
(BLE001)
227-227: Use explicit conversion flag
Replace with conversion flag
(RUF010)
231-231: Use explicit conversion flag
Replace with conversion flag
(RUF010)
wavefront/server/apps/floconsole/floconsole/controllers/app_controller.py
96-98: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
99-99: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
100-100: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
101-103: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
237-239: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
240-240: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
241-243: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
wavefront/server/apps/floconsole/floconsole/controllers/user_controller.py
47-49: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
50-52: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (20)
wavefront/server/apps/floconsole/floconsole/db/models/app.py (1)
8-8: LGTM! Proper relationship setup for association table.The
app_usersrelationship is correctly configured withcascade='all, delete-orphan', ensuring that when an App is deleted, all associated AppUser records are automatically removed. The bidirectional setup withback_populates='app'maintains referential integrity.Also applies to: 32-35
wavefront/server/apps/floconsole/floconsole/server.py (1)
20-20: LGTM! Router registration follows existing patterns.The
app_user_routeris correctly imported and registered with the same/floconsoleprefix as other routers, maintaining consistency across the API structure.Also applies to: 124-124
wavefront/server/apps/floconsole/floconsole/db/models/__init__.py (1)
6-6: LGTM! AppUser correctly exported from models package.The
AppUserimport and export are properly configured, making the model available through the package's public API.Also applies to: 8-8
wavefront/server/apps/floconsole/floconsole/constants/user.py (1)
1-6: LGTM! UserRole enum correctly defines access control roles.The
UserRoleenum properly defines the two roles (OWNER and APP_ADMIN) using the string enum pattern, which facilitates database storage and comparison operations. Role-based authorization is consistently enforced throughout the codebase:
- OWNER-only operations are properly protected across user management (create, modify, delete), app management, and app-user associations
- APP_ADMIN access is correctly validated in the floware_proxy_service via
check_user_has_app_access, which grants unrestricted app access to OWNER roles while requiring explicit app-user entries for APP_ADMIN userswavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1646-8d947e3f8ec6_create_app_user_table.py (1)
20-38: LGTM!The migration correctly creates the app_user junction table with:
- Composite primary key for uniqueness
- CASCADE deletes to maintain referential integrity
- Individual indexes for efficient queries by user or app
wavefront/server/apps/floconsole/floconsole/services/user_service.py (1)
45-48: LGTM!The method correctly checks user existence and role with proper null handling.
wavefront/server/apps/floconsole/floconsole/db/models/user.py (3)
9-9: LGTM!The role field is properly configured with the correct default value and nullable constraint, matching the migration strategy.
Also applies to: 25-25
37-40: LGTM!The app_users relationship is correctly configured with appropriate cascade behavior for the junction table pattern.
48-48: LGTM!The role field is correctly included in the serialization output.
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (1)
2-2: LGTM!The UserService dependency is properly integrated using dependency injection.
Also applies to: 13-13, 28-28, 36-36
wavefront/server/apps/floconsole/floconsole/di/application_container.py (1)
13-13: LGTM! Clean dependency injection wiring.The new AppUser components are properly integrated into the DI container following the established pattern. The repository and service are correctly typed and wired, and the user_service dependency is appropriately added to floware_proxy_service.
Also applies to: 18-18, 52-54, 64-67, 95-95
wavefront/server/apps/floconsole/floconsole/services/app_user_service.py (2)
29-35: LGTM! Simple and clear accessor methods.The
get_user_appsandget_app_usersmethods provide straightforward access to app-user relationships.
14-22: No changes needed.The
upsertcall is correct for theAppUsermodel. The model only contains two columns—user_idandapp_id, both of which are primary keys—making them sufficient to create a record. When the record already exists, the upsert performs an empty update (no additional fields to set), which is the intended behavior. The call successfully ensures the user-app association record exists without requiring additional update values.Likely an incorrect or invalid review comment.
wavefront/server/apps/floconsole/floconsole/db/models/app_user.py (1)
11-23: LGTM! Well-designed junction table model.The
AppUsermodel correctly implements a many-to-many association table with:
- Composite primary key on
(user_id, app_id)- Cascade delete for referential integrity
- Proper bidirectional relationships
wavefront/server/apps/floconsole/floconsole/controllers/app_controller.py (3)
95-118: LGTM! Proper owner authorization for app creation.The authorization check correctly restricts app creation to users with the OWNER role. The implementation properly retrieves the current user and validates their role before proceeding.
236-266: LGTM! Comprehensive authorization and validation for app updates.The update endpoint properly:
- Restricts updates to OWNER role users
- Filters out None values from the request
- Validates that at least one field is provided for update
This prevents both unauthorized access and empty update requests.
318-319: LGTM! Authorization updated for RBAC.The comment correctly reflects the transition from super-admin to OWNER-based authorization.
wavefront/server/apps/floconsole/floconsole/controllers/user_controller.py (3)
33-33: LGTM! Proper role-based user creation.The implementation correctly:
- Defaults new users to
APP_ADMINrole- Restricts user creation to OWNER role users
- Passes the role through to user creation
This ensures proper access control for user management.
Also applies to: 54-70, 90-90
169-172: LGTM! Flexible yet secure update authorization.The authorization logic correctly allows users to update their own profiles while giving owners the ability to update any user. The role-based check properly uses
UserRole.OWNER.value.
247-279: LGTM! Excellent multi-layered deletion protection.The delete logic implements critical security controls:
- Self-deletion prevention (Lines 247-255): Prevents users from accidentally locking themselves out
- Owner protection (Lines 257-267): Prevents non-owners from deleting owners, blocking privilege escalation
- Last owner constraint (Lines 270-279): Ensures the system always has at least one owner
These safeguards are essential for maintaining system integrity and access.
| @app_user_router.post('/apps/{app_id}/users/{user_id}') | ||
| @inject | ||
| async def grant_app_access( | ||
| app_id: UUID, | ||
| user_id: UUID, | ||
| request: Request, | ||
| response_formatter: ResponseFormatter = Depends( | ||
| Provide[CommonContainer.response_formatter] | ||
| ), | ||
| app_user_service: AppUserService = Depends( | ||
| Provide[ApplicationContainer.app_user_service] | ||
| ), | ||
| user_repository: SQLAlchemyRepository[User] = Depends( | ||
| Provide[ApplicationContainer.user_repository] | ||
| ), | ||
| ): | ||
| """Grant user access to app (owners only)""" | ||
| # Check authorization | ||
| _, current_user_id, _ = get_current_user(request) | ||
| current_user = await user_repository.find_one(id=current_user_id) | ||
|
|
||
| if not current_user or current_user.role != UserRole.OWNER.value: | ||
| return JSONResponse( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| content=response_formatter.buildErrorResponse( | ||
| 'Only owners can grant app access' | ||
| ), | ||
| ) | ||
|
|
||
| # Verify target user exists | ||
| target_user = await user_repository.find_one(id=user_id, deleted=False) | ||
| if not target_user: | ||
| return JSONResponse( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| content=response_formatter.buildErrorResponse('User not found'), | ||
| ) | ||
|
|
||
| # Grant access | ||
| try: | ||
| await app_user_service.grant_app_access(user_id, app_id) | ||
| logger.info(f'Granted user {user_id} access to app {app_id}') | ||
|
|
||
| return JSONResponse( | ||
| status_code=status.HTTP_201_CREATED, | ||
| content=response_formatter.buildSuccessResponse( | ||
| {'message': 'App access granted successfully'} | ||
| ), | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f'Failed to grant app access: {str(e)}') | ||
| return JSONResponse( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| content=response_formatter.buildErrorResponse( | ||
| f'Failed to grant app access: {str(e)}' | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Validate app existence before granting access.
The endpoint grants access to app_id without verifying the app exists. This could allow references to non-existent apps in the junction table, leading to orphaned records or downstream failures.
🔎 Recommended fix to add app validation
Inject app_service or app_repository and verify the app exists:
app_user_service: AppUserService = Depends(
Provide[ApplicationContainer.app_user_service]
),
+ app_repository: SQLAlchemyRepository[App] = Depends(
+ Provide[ApplicationContainer.app_repository]
+ ),
user_repository: SQLAlchemyRepository[User] = Depends(
Provide[ApplicationContainer.user_repository]
),
):
"""Grant user access to app (owners only)"""
# Check authorization
_, current_user_id, _ = get_current_user(request)
current_user = await user_repository.find_one(id=current_user_id)
if not current_user or current_user.role != UserRole.OWNER.value:
return JSONResponse(
status_code=status.HTTP_403_FORBIDDEN,
content=response_formatter.buildErrorResponse(
'Only owners can grant app access'
),
)
+ # Verify app exists
+ app = await app_repository.find_one(id=app_id, deleted=False)
+ if not app:
+ return JSONResponse(
+ status_code=status.HTTP_404_NOT_FOUND,
+ content=response_formatter.buildErrorResponse('App not found'),
+ )
+
# Verify target user exists
target_user = await user_repository.find_one(id=user_id, deleted=False)Avoid exposing internal error details to clients.
Lines 74 and other error responses include str(e), which may leak sensitive implementation details (stack traces, database errors, etc.) to API consumers.
🔎 Recommended fix to sanitize error responses
Log the full exception internally but return a generic message to the client:
except Exception as e:
logger.error(f'Failed to grant app access: {str(e)}')
return JSONResponse(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
content=response_formatter.buildErrorResponse(
- f'Failed to grant app access: {str(e)}'
+ 'Failed to grant app access'
),
)Apply the same fix to all error handlers in this file (lines 124, 182, 231).
🧰 Tools
🪛 Ruff (0.14.10)
27-29: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
30-32: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-35: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
69-69: Do not catch blind exception: Exception
(BLE001)
70-70: Use explicit conversion flag
Replace with conversion flag
(RUF010)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In
@wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
around lines 21 - 76, The grant_app_access handler currently skips validating
that app_id exists and returns raw exception text to clients; inject and use the
app repository or service (e.g., add app_repository: SQLAlchemyRepository[App]
or app_service: AppService as a dependency) in grant_app_access, call
app_repository.find_one(id=app_id, deleted=False) (or equivalent
app_service.get_app) and return a 404 with
response_formatter.buildErrorResponse('App not found') if absent before calling
app_user_service.grant_app_access; also stop exposing str(e) to clients — change
the exception handlers in grant_app_access (and apply the same pattern to the
other error handlers in this file) to logger.exception(...) or logger.error(...,
exc_info=True) to log full details, but return a generic
response_formatter.buildErrorResponse('Failed to grant app access') (no
exception text) with 500 to the client.
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Show resolved
Hide resolved
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Show resolved
Hide resolved
| # Set all existing users to 'owner' | ||
| conn = op.get_bind() | ||
| conn.execute(sa.text('UPDATE "user" SET role = \'owner\' WHERE deleted = false')) |
There was a problem hiding this comment.
Deleted users are excluded from the role update.
The UPDATE query only modifies non-deleted users. Deleted users will retain the default 'app_admin' role from the server_default. If historical data consistency is important, consider updating all users regardless of deletion status.
🔎 Proposed fix to update all users
- # Set all existing users to 'owner'
- conn = op.get_bind()
- conn.execute(sa.text('UPDATE "user" SET role = \'owner\' WHERE deleted = false'))
+ # Set all existing users to 'owner' (including deleted for data consistency)
+ conn = op.get_bind()
+ conn.execute(sa.text('UPDATE "user" SET role = \'owner\''))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Set all existing users to 'owner' | |
| conn = op.get_bind() | |
| conn.execute(sa.text('UPDATE "user" SET role = \'owner\' WHERE deleted = false')) | |
| # Set all existing users to 'owner' (including deleted for data consistency) | |
| conn = op.get_bind() | |
| conn.execute(sa.text('UPDATE "user" SET role = \'owner\'')) |
🤖 Prompt for AI Agents
In
@wavefront/server/apps/floconsole/floconsole/db/alembic/versions/2026_01_06_1644-0644a81ee4e1_add_role_to_user.py
around lines 27 - 29, The migration currently limits the role update to
non-deleted users; change the SQL executed by conn (obtained via op.get_bind())
so it updates every row in the "user" table (remove the WHERE deleted = false
clause) — i.e. execute sa.text('UPDATE "user" SET role = \'owner\'') so deleted
users get the same role applied for historical consistency.
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @wavefront/client/src/pages/apps/users/CreateUserDialog.tsx:
- Around line 148-158: The Select in CreateUserDialog is using
defaultValue={field.value} which makes it uncontrolled; change it to a
controlled component by replacing defaultValue with value={field.value} on the
Select and keep onValueChange={field.onChange} so react-hook-form updates and
resets propagate correctly; remove defaultValue and ensure field.value is passed
as value to Select (and verify field.onChange signature matches Select's
onValueChange).
🧹 Nitpick comments (4)
wavefront/client/src/types/user.ts (1)
6-6: Consider using a union type or enum for role validation.The
rolefield accepts any string, but the system only supports'owner'and'app_admin'. Adding client-side validation would provide better UX by catching invalid values before submission.🔎 Proposed refinement
+const validRoles = ['owner', 'app_admin'] as const; +type UserRole = typeof validRoles[number]; + export interface IUser { id: string; email: string; first_name: string; last_name: string; - role: string; + role: UserRole; }And for the schema:
export const createUserSchema = z.object({ // ... other fields - role: z.string().optional(), + role: z.enum(['owner', 'app_admin']).optional(), });Also applies to: 20-20
wavefront/client/src/pages/apps/users/ManageAppAccessDialog.tsx (2)
77-85: Sequential API calls may be slow and partial failures leave inconsistent state.The grant/revoke operations are executed sequentially. If multiple apps need updating, this can be slow. More critically, if an operation fails mid-way, some changes are applied while others are not, but the error handler doesn't indicate which operations succeeded.
🔎 Consider using Promise.allSettled for parallel execution with partial failure reporting
- // Grant access to new apps - const appsToGrant = Array.from(currentAppIds).filter((id) => !previousAppIds.has(id)); - for (const appId of appsToGrant) { - await floConsoleService.appUserService.grantAppAccess(appId, user.id); - } - - // Revoke access from removed apps - const appsToRevoke = Array.from(previousAppIds).filter((id) => !currentAppIds.has(id)); - for (const appId of appsToRevoke) { - await floConsoleService.appUserService.revokeAppAccess(appId, user.id); - } + const appsToGrant = Array.from(currentAppIds).filter((id) => !previousAppIds.has(id)); + const appsToRevoke = Array.from(previousAppIds).filter((id) => !currentAppIds.has(id)); + + const results = await Promise.allSettled([ + ...appsToGrant.map((appId) => floConsoleService.appUserService.grantAppAccess(appId, user.id)), + ...appsToRevoke.map((appId) => floConsoleService.appUserService.revokeAppAccess(appId, user.id)), + ]); + + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + notifyError(`${failures.length} operation(s) failed. Please verify app access.`); + return; + }
30-52: State not reset when dialog closes, potentially showing stale data on reopen.When the dialog closes (via cancel or clicking outside),
userAppIdsretains its last value. If the user reopens the dialog for the same user, the stale local state may briefly display before theuseEffectfetch completes.🔎 Reset state when dialog closes
useEffect(() => { const fetchUserApps = async () => { - if (!user || !isOpen) return; + if (!user || !isOpen) { + setUserAppIds([]); + return; + } setLoading(true); // ... }; fetchUserApps(); }, [user, isOpen, notifyError]);wavefront/client/src/pages/apps/users/index.tsx (1)
147-154: Empty state messaging could be improved for non-owners.When a non-owner sees no users (e.g., filtered results), the message says "No users match your search" even without an active search query. Also, passing an empty string and no-op function to
EmptyStateCardis awkward.🔎 Proposed improvement
) : ( <EmptyStateCard title="No users found" - description="No users match your search." - actionText="" - onActionClick={() => {}} + description={searchQuery ? 'No users match your search.' : 'No users available.'} /> )}This assumes
EmptyStateCardsupports optional action props. If not, consider updating the component to makeactionTextandonActionClickoptional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wavefront/client/src/api/app-user-service.tswavefront/client/src/api/index.tswavefront/client/src/api/user-service.tswavefront/client/src/pages/apps/users/CreateUserDialog.tsxwavefront/client/src/pages/apps/users/EditUserDialog.tsxwavefront/client/src/pages/apps/users/ManageAppAccessDialog.tsxwavefront/client/src/pages/apps/users/index.tsxwavefront/client/src/types/user.ts
💤 Files with no reviewable changes (1)
- wavefront/client/src/pages/apps/users/EditUserDialog.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
wavefront/client/src/pages/apps/users/CreateUserDialog.tsx (1)
wavefront/client/src/components/ui/select.tsx (5)
Select(133-133)SelectTrigger(136-136)SelectValue(135-135)SelectContent(137-137)SelectItem(139-139)
wavefront/client/src/api/index.ts (1)
wavefront/client/src/api/app-user-service.ts (1)
AppUserService(5-35)
wavefront/client/src/pages/apps/users/index.tsx (2)
wavefront/client/src/types/user.ts (1)
IUser(1-7)wavefront/client/src/hooks/data/fetch-hooks.ts (2)
useGetUsers(414-416)useGetCurrentUser(113-115)
wavefront/client/src/pages/apps/users/ManageAppAccessDialog.tsx (5)
wavefront/client/src/types/user.ts (1)
IUser(1-7)wavefront/client/src/store/notification-store.ts (1)
useNotifyStore(32-32)wavefront/client/src/hooks/data/fetch-hooks.ts (1)
useGetAllApps(109-111)wavefront/client/src/lib/utils.ts (1)
extractErrorMessage(21-66)wavefront/client/src/components/ui/dialog.tsx (6)
Dialog(111-111)DialogContent(113-113)DialogHeader(116-116)DialogTitle(119-119)DialogDescription(114-114)DialogFooter(115-115)
wavefront/client/src/api/app-user-service.ts (2)
wavefront/client/src/lib/axios.ts (1)
IApiResponse(16-16)wavefront/client/src/types/user.ts (1)
IUser(1-7)
🔇 Additional comments (5)
wavefront/client/src/api/index.ts (1)
6-6: LGTM!The
AppUserServiceimport and getter follow the established pattern used by other services inFloConsoleService.Also applies to: 43-45
wavefront/client/src/api/user-service.ts (1)
31-39: LGTM!The
createUserpayload correctly includes the optionalrolefield, aligning with the updated type definitions and supporting the new RBAC model.wavefront/client/src/pages/apps/users/index.tsx (1)
34-50: LGTM on the RBAC implementation.The owner-based permission checks are well-structured:
isOwnermemoized for efficiencycanEditUserallows self-editing while giving owners full accesscanDeleteUserprevents self-deletion- UI elements correctly gated behind
isOwnerchecks- Manage App Access appropriately restricted to app_admin users (owners have implicit full access)
Also applies to: 126-130, 185-194
wavefront/client/src/api/app-user-service.ts (1)
5-35: LGTM!The
AppUserServicefollows the established patterns in the codebase:
- Consistent constructor pattern with
AxiosInstanceinjection- RESTful endpoint design
- Proper TypeScript typing with
IApiResponsegenerics- Clear JSDoc comments documenting owner-only access requirements
wavefront/client/src/pages/apps/users/CreateUserDialog.tsx (1)
40-46: LGTM on form default values.The default role of
'app_admin'follows the principle of least privilege, ensuring new users don't inadvertently receive owner-level access.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py:
- Around line 81-88: The handler currently returns exception details to clients
by including str(e) in the JSONResponse; replace those occurrences so the client
gets a generic message (e.g., "Failed to grant app access") and log the full
traceback for debugging using logger.exception(...) instead of
logger.error(...). Specifically, in the except block around the grant logic
replace logger.error(f'Failed to grant app access: {str(e)}') with
logger.exception('Failed to grant app access') and remove str(e) from the
response_formatter.buildErrorResponse call so the JSONResponse contains only the
generic message; apply the same change pattern (use logger.exception and generic
messages) to the other except handlers in this file referenced in the review
(the blocks around lines 150-157, 206-213, 255-262).
- Around line 216-262: The list_user_apps handler lacks existence validation for
the target user_id; after the authorization check (in list_user_apps) call
user_repository.find_one(id=user_id) (similar to
grant_app_access/revoke_app_access) and if it returns None return a 404
JSONResponse using response_formatter.buildErrorResponse with a clear message
like "User not found" before calling app_user_service.get_user_apps; this
ensures consistent behavior and avoids returning an empty list for non-existent
users.
- Around line 160-213: The list_app_users endpoint (list_app_users) must
validate that the app exists before fetching users: add an injected
app_repository (matching ApplicationContainer.app_repository) to the function
signature, call its find_one/find method to check existence for app_id, and if
not found return a 404 JSONResponse using
response_formatter.buildErrorResponse('App not found') so the endpoint behavior
matches grant_app_access/revoke_app_access; keep the existing owner-only check
and only proceed to call app_user_service.get_app_users when the app exists.
In
@wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py:
- Around line 79-85: The UUID(app_id) conversion can raise an unhandled
ValueError; wrap the conversion in a try/except around UUID(app_id) in the
method containing self.user_service.check_user_has_app_access, catch ValueError
and raise a clear, consistent exception (or return the same error handling used
for session.user_id) before calling self.user_service.check_user_has_app_access
so that has_access is only evaluated with a valid UUID; reference the app_id
variable, the UUID(...) call, and the
self.user_service.check_user_has_app_access(...) invocation when making the
change.
🧹 Nitpick comments (2)
wavefront/server/apps/floconsole/floconsole/db/alembic/env.py (1)
32-38: Consider validating required environment variables.If any of the database environment variables (
CONSOLE_DB_USERNAME,CONSOLE_DB_PASSWORD, etc.) are missing, the URL construction will fail with a cryptic error. Adding validation with clear error messages would improve the developer experience during initial setup.🔎 Suggested validation approach
# Get database URL from environment variables db_user_name = os.getenv('CONSOLE_DB_USERNAME') db_password = os.getenv('CONSOLE_DB_PASSWORD') db_host = os.getenv('CONSOLE_DB_HOST') db_port = os.getenv('CONSOLE_DB_PORT') db_name = os.getenv('CONSOLE_DB_NAME') +# Validate required environment variables +required_vars = { + 'CONSOLE_DB_USERNAME': db_user_name, + 'CONSOLE_DB_PASSWORD': db_password, + 'CONSOLE_DB_HOST': db_host, + 'CONSOLE_DB_PORT': db_port, + 'CONSOLE_DB_NAME': db_name, +} +missing = [k for k, v in required_vars.items() if v is None] +if missing: + raise ValueError(f"Missing required environment variables: {', '.join(missing)}") + db_url = f'postgresql://{db_user_name}:{db_password}@{db_host}:{db_port}/{db_name}'wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (1)
55-68: Docstring step numbering is outdated.The docstring mentions step 4 as "Generate T2 service token with app-specific secret," but this doesn't appear to be implemented in the current code. Additionally, the comment on line 106 says "Step 3: Prepare request to floware" but should be Step 4 based on the docstring flow (Step 3 is "Fetch app details").
Consider updating the docstring to reflect the actual implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wavefront/client/src/pages/apps/users/CreateUserDialog.tsxwavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.pywavefront/server/apps/floconsole/floconsole/db/alembic/env.pywavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- wavefront/client/src/pages/apps/users/CreateUserDialog.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
wavefront/server/apps/floconsole/floconsole/db/alembic/env.py (2)
wavefront/server/apps/floconsole/floconsole/db/models/app.py (1)
App(13-49)wavefront/server/apps/floconsole/floconsole/db/models/app_user.py (1)
AppUser(11-23)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (1)
wavefront/server/apps/floconsole/floconsole/services/user_service.py (1)
check_user_has_app_access(50-75)
🪛 Ruff (0.14.10)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py
31-31: Possible hardcoded password assigned to function default: "token_prefix"
(S107)
75-77: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
28-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
31-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
34-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-39: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
81-81: Do not catch blind exception: Exception
(BLE001)
82-82: Use explicit conversion flag
Replace with conversion flag
(RUF010)
86-86: Use explicit conversion flag
Replace with conversion flag
(RUF010)
97-99: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
100-102: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
103-105: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
106-108: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
150-150: Do not catch blind exception: Exception
(BLE001)
151-151: Use explicit conversion flag
Replace with conversion flag
(RUF010)
155-155: Use explicit conversion flag
Replace with conversion flag
(RUF010)
165-167: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
168-170: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
171-173: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
206-206: Do not catch blind exception: Exception
(BLE001)
207-207: Use explicit conversion flag
Replace with conversion flag
(RUF010)
211-211: Use explicit conversion flag
Replace with conversion flag
(RUF010)
221-223: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
224-226: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
227-229: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
255-255: Do not catch blind exception: Exception
(BLE001)
256-256: Use explicit conversion flag
Replace with conversion flag
(RUF010)
260-260: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (4)
wavefront/server/apps/floconsole/floconsole/db/alembic/env.py (1)
11-12: Excellent - previous issue resolved!The
AppandAppUsermodels are now properly imported and registered in the Alembic configuration. This ensures that Alembic's autogenerate will track schema changes for both theappandapp_usertables, resolving the critical issue identified in the previous review.Also applies to: 25-26
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (3)
1-13: LGTM!The imports for
UUIDandUserServiceare appropriate additions to support the new RBAC functionality.
23-42: LGTM!The
UserServicedependency is correctly injected and stored, following the existing pattern for other service dependencies.
69-77: LGTM!The explicit error handling for
user_idUUID parsing addresses the concern from the previous review. The combined catch forValueErrorandAttributeErrorappropriately handles both invalid format and missing attribute cases.
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Show resolved
Hide resolved
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Show resolved
Hide resolved
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py:
- Around line 150-157: The exception details are being leaked to API consumers;
change the except block so it logs the full exception (use logger.exception(...)
or logger.error(..., exc_info=True)) but returns a generic error message to the
client (remove f'Failed to revoke app access: {str(e)}' from JSONResponse
content and replace with a non-sensitive message like 'Failed to revoke app
access'), keeping the HTTP_500 status; update the JSONResponse/content call that
uses response_formatter.buildErrorResponse to pass the generic message and keep
detailed error info only in the server log (references: logger.error /
logger.exception, JSONResponse, response_formatter.buildErrorResponse).
- Around line 81-88: The except block in app_user_controller.py currently logs
and returns the raw exception string (str(e)), which can leak sensitive details;
change the logging to use logger.exception(...) to capture full traceback in
logs and replace the client-facing message passed to
response_formatter.buildErrorResponse(...) with a generic message (e.g., "Failed
to grant app access") without including str(e); keep response status_code as
HTTP_500_INTERNAL_SERVER_ERROR and do not expose exception text in the JSON
response.
- Around line 273-280: The except block in app_user_controller.py currently
returns internal exception details to clients; change it to log the full
exception locally (e.g., use logger.exception(...) or logger.error(...,
exc_info=True)) and return a generic error message via
response_formatter.buildErrorResponse like "Failed to list user apps" without
including str(e); keep the status code/status.HTTP_500_INTERNAL_SERVER_ERROR and
remove the exception string from both the JSONResponse content and any
client-facing messages while preserving internal logging.
- Around line 217-224: The except block in app_user_controller.py currently logs
and returns the raw exception string (logger.error and
response_formatter.buildErrorResponse using str(e)), which leaks internal
details to clients; change the code so the logger records the full exception
(use logger.exception or include str(e) in the server log) but the JSONResponse
content returned by response_formatter.buildErrorResponse contains a generic
message like "Failed to list app users" without including exception details;
keep references to logger, JSONResponse, and
response_formatter.buildErrorResponse and update only the response payload to
remove str(e) while preserving detailed logging.
🧹 Nitpick comments (2)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (1)
92-100: Simplify redundant error handling.Since
app_idis already validated and converted toapp_uuidat lines 80-83, the "Invalid app_id format" error on line 100 is now dead code—it can never be reached. The try-except block only catches theValueErrorit raises itself (line 96), making the logic unnecessarily complex.Additionally, line 99 uses
raise einstead of bareraise(TRY201).🔎 Proposed simplification
# Step 3: Fetch app details from database - try: - app = await self.app_service.get_app_by_id(app_uuid) - if not app: - raise ValueError(f'App not found for ID: {app_id}') - except ValueError as e: - if 'App not found' in str(e): - raise e - raise ValueError(f'Invalid app_id format: {app_id}') from e + app = await self.app_service.get_app_by_id(app_uuid) + if not app: + raise ValueError(f'App not found for ID: {app_id}')wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (1)
263-271: Consider returning full app details for consistency.This endpoint returns only
app_idsas strings, while the correspondinglist_app_usersendpoint (line 207) returns full user objects viauser.to_dict(). This inconsistency in response formats may surprise API consumers.💡 Optional enhancement for API consistency
If full app details are desired, inject
app_repositoryand fetch app objects:app_user_service: AppUserService = Depends( Provide[ApplicationContainer.app_user_service] ), + app_repository: SQLAlchemyRepository[App] = Depends( + Provide[ApplicationContainer.app_repository] + ), user_repository: SQLAlchemyRepository[User] = Depends( Provide[ApplicationContainer.user_repository] ), ):Then fetch and return full app details:
# Get user apps try: user_apps = await app_user_service.get_user_apps(user_id) - app_ids = [str(user_app.app_id) for user_app in user_apps] + + # Fetch full app details + if user_apps: + app_ids = [user_app.app_id for user_app in user_apps] + apps = await app_repository.find(id=app_ids, deleted=False) + apps_data = [app.to_dict() for app in apps] + else: + apps_data = [] - logger.info(f'User {user_id} has access to {len(app_ids)} apps') + logger.info(f'User {user_id} has access to {len(apps_data)} apps') return JSONResponse( status_code=status.HTTP_200_OK, - content=response_formatter.buildSuccessResponse({'app_ids': app_ids}), + content=response_formatter.buildSuccessResponse({'apps': apps_data}), )Alternatively, if returning only IDs is intentional, consider updating
list_app_usersto match this pattern for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.pywavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py
🧰 Additional context used
🧬 Code graph analysis (2)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (5)
wavefront/server/apps/floconsole/floconsole/constants/user.py (1)
UserRole(4-6)wavefront/server/apps/floconsole/floconsole/db/models/app.py (2)
App(13-49)to_dict(37-49)wavefront/server/apps/floconsole/floconsole/db/models/user.py (2)
User(14-49)to_dict(42-49)wavefront/server/apps/floconsole/floconsole/di/application_container.py (1)
ApplicationContainer(21-100)wavefront/server/apps/floconsole/floconsole/services/app_user_service.py (5)
AppUserService(8-35)grant_app_access(14-22)revoke_app_access(24-27)get_app_users(33-35)get_user_apps(29-31)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (1)
wavefront/server/apps/floconsole/floconsole/services/user_service.py (2)
UserService(10-75)check_user_has_app_access(50-75)
🪛 Ruff (0.14.10)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
28-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
31-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
34-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-39: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
81-81: Do not catch blind exception: Exception
(BLE001)
82-82: Use explicit conversion flag
Replace with conversion flag
(RUF010)
86-86: Use explicit conversion flag
Replace with conversion flag
(RUF010)
97-99: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
100-102: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
103-105: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
106-108: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
150-150: Do not catch blind exception: Exception
(BLE001)
151-151: Use explicit conversion flag
Replace with conversion flag
(RUF010)
155-155: Use explicit conversion flag
Replace with conversion flag
(RUF010)
165-167: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
168-170: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
171-173: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
174-176: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
217-217: Do not catch blind exception: Exception
(BLE001)
218-218: Use explicit conversion flag
Replace with conversion flag
(RUF010)
222-222: Use explicit conversion flag
Replace with conversion flag
(RUF010)
232-234: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
235-237: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
238-240: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
273-273: Do not catch blind exception: Exception
(BLE001)
274-274: Use explicit conversion flag
Replace with conversion flag
(RUF010)
278-278: Use explicit conversion flag
Replace with conversion flag
(RUF010)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py
31-31: Possible hardcoded password assigned to function default: "token_prefix"
(S107)
75-77: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Avoid specifying long messages outside the exception class
(TRY003)
96-96: Abstract raise to an inner function
(TRY301)
96-96: Avoid specifying long messages outside the exception class
(TRY003)
99-99: Use raise without specifying exception name
Remove exception name
(TRY201)
100-100: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
wavefront/server/apps/floconsole/floconsole/services/floware_proxy_service.py (5)
24-42: LGTM!The constructor properly injects the
UserServicedependency needed for RBAC checks. The static analysis warning S107 ontoken_prefixis a false positive—it's a token prefix string, not a credential.
69-77: LGTM!The explicit error handling for
session.user_idUUID parsing properly addresses the previously flagged concern, catching bothValueError(invalid UUID format) andAttributeError(missing attribute).
79-90: LGTM!The explicit UUID validation for
app_idand the RBAC access check are correctly implemented, addressing the previously flagged concern.
136-178: LGTM!The streaming implementation correctly uses manual context management to keep the connection alive during iteration. The
finallyblock instream_generator()properly cleans up both the stream context and client when streaming completes or on error.
180-203: LGTM!The non-streaming path correctly uses the async context manager for automatic cleanup. The 600-second timeout is notably higher than the streaming path (120s), which may be intentional for heavier operations.
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Outdated
Show resolved
Hide resolved
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Outdated
Show resolved
Hide resolved
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Outdated
Show resolved
Hide resolved
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (1)
223-274: Consider validating returned apps for consistency.The endpoint correctly enforces OWNER role and validates the target user exists. However, unlike
list_app_users(which validates the app and fetches full user details), this endpoint returnsapp_idsfrom the junction table without verifying those apps still exist or aren't deleted (line 261).This could return IDs of soft-deleted apps, which may confuse API consumers. If the intent is to show only currently active apps, consider filtering:
🔎 Suggested enhancement
+ app_repository: SQLAlchemyRepository[App] = Depends( + Provide[ApplicationContainer.app_repository] + ), ): """List apps accessible to user (owners only)""" # ... existing authorization and user validation ... # Get user apps try: user_apps = await app_user_service.get_user_apps(user_id) - app_ids = [str(user_app.app_id) for user_app in user_apps] + + # Filter out deleted apps + if user_apps: + app_ids_list = [user_app.app_id for user_app in user_apps] + active_apps = await app_repository.find(id=app_ids_list, deleted=False) + app_ids = [str(app.id) for app in active_apps] + else: + app_ids = []This would mirror the pattern used in
list_app_usersand ensure consistency.Alternatively, if returning historical associations (including deleted apps) is intentional for audit purposes, the current implementation is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
🧰 Additional context used
🧬 Code graph analysis (1)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (4)
wavefront/server/apps/floconsole/floconsole/constants/user.py (1)
UserRole(4-6)wavefront/server/apps/floconsole/floconsole/db/models/app.py (2)
App(13-49)to_dict(37-49)wavefront/server/apps/floconsole/floconsole/db/models/user.py (2)
User(14-49)to_dict(42-49)wavefront/server/apps/floconsole/floconsole/services/app_user_service.py (5)
AppUserService(8-35)grant_app_access(14-22)revoke_app_access(24-27)get_app_users(33-35)get_user_apps(29-31)
🪛 Ruff (0.14.10)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py
28-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
31-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
34-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-39: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
81-81: Do not catch blind exception: Exception
(BLE001)
95-97: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
98-100: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
101-103: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
104-106: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
148-148: Do not catch blind exception: Exception
(BLE001)
163-165: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
166-168: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
169-171: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
172-174: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
215-215: Do not catch blind exception: Exception
(BLE001)
228-230: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
231-233: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
234-236: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
269-269: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
wavefront/server/apps/floconsole/floconsole/controllers/app_user_controller.py (3)
22-86: LGTM! Solid implementation with proper validations.The
grant_app_accessendpoint correctly:
- Enforces OWNER-only access
- Validates both app and user existence before granting access
- Uses
logger.exception()for detailed server-side logging while returning generic error messages to clients- Returns appropriate HTTP status codes (201, 403, 404, 500)
The exception handling has been properly addressed from previous reviews.
89-155: LGTM! Consistent implementation with proper validations.The
revoke_app_accessendpoint mirrorsgrant_app_accesswith:
- OWNER role enforcement
- App and user existence validation
- Proper exception handling and logging
- Appropriate HTTP status codes (200, 403, 404, 500)
The implementation is consistent and secure.
158-220: LGTM! Well-optimized query with proper validations.The
list_app_usersendpoint correctly:
- Enforces OWNER role
- Validates app existence
- Uses
user_repository.find(id=user_ids, deleted=False)at line 204, which should generate an efficient SQLWHERE id IN (...)query instead of fetching all users- Returns full user details via
to_dict()- Handles errors securely
The query optimization from previous reviews has been properly implemented.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.