Scope resolvers #37
Scope resolvers #37
Conversation
…lter by organizations and improve error handling
…rganizations field from User model
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| location_service = PatientMutation._get_location_service(db) | ||
| accessible_location_ids = await auth_service.get_user_accessible_location_ids( | ||
| info.context.user | ||
| ) |
There was a problem hiding this comment.
Missing context parameter in authorization call breaks caching
The call to get_user_accessible_location_ids is missing the second context parameter. All other calls in the codebase pass info.context.user, info.context, but this one only passes info.context.user. This bypasses the caching mechanism in AuthorizationService, causing redundant database queries during update_patient operations and potential inconsistency within a single request.
| result = await info.context.db.execute( | ||
| select(models.Task) | ||
| .where(models.Task.id == id) | ||
| .options(selectinload(models.Task.patient).selectinload(models.Patient.assigned_locations)) |
There was a problem hiding this comment.
Task queries missing teams relationship for authorization check
The can_access_patient method checks both patient.assigned_locations and patient.teams for authorization. However, all task queries only eager-load assigned_locations with selectinload(models.Task.patient).selectinload(models.Patient.assigned_locations) but not teams. In async SQLAlchemy, accessing an unloaded relationship raises a MissingGreenlet error. This will cause authorization checks to fail for any user whose access is granted through teams. Similarly, delete_patient uses repo.get_by_id() which loads no relationships before calling can_access_patient.
Additional Locations (2)
| f"User {self.id} has {len(user_root_location_ids)} root location IDs but query returned empty. " | ||
| f"Checking if locations exist: {[loc.id for loc in existing_locations]} " | ||
| f"with parent_ids: {[loc.parent_id for loc in existing_locations]}" | ||
| ) |
There was a problem hiding this comment.
Debugging queries and logging left in production code
The root_locations field resolver contains debugging code that appears unintended for production. It performs a redundant query to user_root_locations before the actual query, logs user IDs and location IDs at INFO level, and conditionally performs another diagnostic query when results are empty. This causes unnecessary database load and verbose logging. The actual work only requires lines 114-123; the rest appears to be diagnostic code that was left in.
| @strawberry.field | ||
| def organizations(self, info) -> str | None: | ||
| """Get organizations from the context""" | ||
| return info.context.organizations |
There was a problem hiding this comment.
Organizations field returns requester's data for any user
The organizations field on UserType returns info.context.organizations, which is always the requesting user's organizations regardless of which user is being queried. When using user(id: "other-user") or users queries, the organizations field will incorrectly return the requester's organizations instead of the target user's. Since the organizations column was removed from the User model, this field now has misleading semantics - it appears to be user-specific data but actually reflects the auth context.
| raise GraphQLError( | ||
| "Insufficient permission. Please contact an administrator if you believe this is an error.", | ||
| extensions={"code": "FORBIDDEN"}, | ||
| ) |
There was a problem hiding this comment.
Users can create inaccessible root locations
The authorization check in create_location_node only validates parent_id when it's truthy. When parent_id is None (creating a root location), the check is bypassed. This allows any authenticated user with at least one accessible location to create orphaned root locations that they cannot subsequently access, since the new location is not added to their user_root_locations. The mutation succeeds but returns a location the user can never query again.
Note
Implements location-scoped authorization and wiring end-to-end, plus Location CRUD.
organizationclaim; on login compute/updateuser_root_locations(creating org rootLocationNodes as needed); cache accessible location IDs; thread-safeLockedAsyncSession.AuthorizationServiceto compute/ cache accessible locations, check patient/task access, and filter queries.patient,patients,recent_patients,task,tasks,recent_tasks; addLocationqueries/create|update|deletemutations andlocation_nodesubscriptions.CreateLocationNodeInput/UpdateLocationNodeInput,LocationNodeType.organization_ids; extend queries withrootLocationIdsfilters; update generated client.user_root_locationsandlocation_organizationstables; changeLocationNode.kindto enum; expose new association tables via models; merge migration head.rootLocationsand selectedrootLocationIds(persisted); newLocationSelectionDialog(multi-select root); filter Patients/Tasks/GlobalData byrootLocationIds; UI tweaks (header picker, chips coloring, settings show organizations).organization_ids; Keycloak export emitsorganizationattribute and scope.Written by Cursor Bugbot for commit 0596f3a. This will update automatically on new commits. Configure here.