Add function to authorize user to register patient #9000
Conversation
WalkthroughThe changes introduce a new function, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for care-ohc failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/FacilityHome.tsx (1)
488-500: Button implementation needs minor improvementsThe conditional rendering successfully prevents unauthorized access, addressing the PR objective. However, there are a few improvements to consider:
- Update the function name once fixed (from previous comment)
- Add an id attribute for testing/accessibility
Apply these improvements:
-{isAuthorizedToRegisterPatientsterAuth() && ( +{isAuthorizedToRegisterPatientsAuth() && ( <ButtonV2 + id="add-patient-details-button" variant="primary" ghost border className="mt-2 flex w-full flex-row justify-center md:w-auto" onClick={() => navigate(`/facility/${facilityId}/patient`)} authorizeFor={NonReadOnlyUsers} > <CareIcon icon="l-plus" className="text-lg" /> <span className="text-sm">{t("add_details_of_patient")}</span> </ButtonV2> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityHome.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Facility/FacilityHome.tsx (1)
160-182: Verify the authorization implementation fixes the infinite loop issue
The implementation appears to properly address the infinite loop issue by:
- Preventing unauthorized access through proper authorization checks
- Hiding the "Add Details of Patient" button for unauthorized users
- Properly handling different user types and their permissions
Let's verify there are no other instances of patient registration routes that could bypass this check.
Also applies to: 488-500
✅ Verification successful
Authorization checks are properly implemented across patient registration flow
The verification confirms that the infinite loop issue is properly addressed through consistent authorization checks:
FacilityHome.tsxhas proper authorization checks before showing the registration buttonPatientRegister.tsximplements identical authorization logic preventing unauthorized accessManagePatients.tsxenforces the same authorization rules with proper redirects- Router configuration in
PatientRoutes.tsxshows all patient-related routes are properly structured
The implementation successfully prevents unauthorized access and infinite loops by consistently checking user permissions across all components in the patient registration flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patient registration routes and verify they're protected
echo "Searching for patient registration routes..."
rg -l "facility/\$\{.*\}/patient" --type ts --type tsx
echo "Searching for navigation to patient registration..."
rg "navigate\(.*facility.*patient" --type ts --type tsx
Length of output: 381
Script:
#!/bin/bash
# Search for patient registration routes and verify they're protected
echo "Searching for patient registration routes..."
rg -l "facility.*patient"
echo "Searching for navigation to patient registration..."
rg "navigate.*facility.*patient" -A 2
Length of output: 5813
Script:
#!/bin/bash
# Check the PatientRegister component for authorization checks
echo "Checking PatientRegister component for auth checks..."
rg "isAuthorized|user_type|authUser" src/components/Patient/PatientRegister.tsx -B 2 -A 2
# Check ManagePatients component for auth checks
echo "Checking ManagePatients component for auth checks..."
rg "isAuthorized|user_type|authUser" src/components/Patient/ManagePatients.tsx -B 2 -A 2
# Check router configuration for patient routes
echo "Checking router protection..."
rg "PatientRegister|patient" src/Routers/routes/PatientRoutes.tsx -B 2 -A 2
Length of output: 4653
| const isAuthorizedToRegisterPatientsterAuth = () => { | ||
| const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"]; | ||
| if ( | ||
| !showAllFacilityUsers.includes(authUser.user_type) && | ||
| authUser.home_facility_object?.id === facilityId | ||
| ) { | ||
| return true; | ||
| } | ||
| if ( | ||
| authUser.user_type === "DistrictAdmin" && | ||
| authUser.district === facilityData?.district | ||
| ) { | ||
| return true; | ||
| } | ||
| if ( | ||
| authUser.user_type === "StateAdmin" && | ||
| authUser.state === facilityData?.state | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function implementation needs improvements
The authorization function has several issues that should be addressed:
- The function name contains a typo: "PatientsterAuth" should be "PatientsAuth"
- The array name
showAllFacilityUsersis misleading as it's used for authorization checks - The first condition's logic can be simplified
- Type safety improvements needed for user types
Consider applying these improvements:
-const isAuthorizedToRegisterPatientsterAuth = () => {
- const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"];
- if (
- !showAllFacilityUsers.includes(authUser.user_type) &&
- authUser.home_facility_object?.id === facilityId
- ) {
- return true;
- }
+const isAuthorizedToRegisterPatientsAuth = () => {
+ type AdminType = "DistrictAdmin" | "StateAdmin";
+ const adminUserTypes: AdminType[] = ["DistrictAdmin", "StateAdmin"];
+
+ // Regular users can only register in their home facility
+ if (authUser.home_facility_object?.id === facilityId) {
+ return true;
+ }
+
+ // Admins have extended privileges
+ if (!adminUserTypes.includes(authUser.user_type as AdminType)) {
+ return false;
+ }
if (
authUser.user_type === "DistrictAdmin" &&
- authUser.district === facilityData?.district
+ authUser.district === facilityData?.district_object?.id
) {
return true;
}
if (
authUser.user_type === "StateAdmin" &&
- authUser.state === facilityData?.state
+ authUser.state === facilityData?.state_object?.id
) {
return true;
}
return false;
};Committable suggestion skipped: line range outside the PR's diff.
|
@JavidSumra iam seeing changes suggested by ai, look into that as well |
|
we already have this auth defined in the code base, so use that rather than duplicating it :) |
…f github.com:JavidSumra/care_fe into issues/8988/fix-infinite-rendering-non-home-facility
…ring-non-home-facility
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/PatientRegister.tsx(4 hunks)
🔇 Additional comments (1)
src/components/Patient/PatientRegister.tsx (1)
1877-1903: Verify authorization implementation consistency.
Based on the PR comments mentioning existing authorization checks, let's verify:
- If this implementation duplicates existing authorization logic
- If this function is used consistently across all patient registration points
✅ Verification successful
Authorization implementation is consistent and well-integrated
The verification reveals that:
- The
PatientRegisterAuthfunction follows existing authorization patterns in the codebase, similar to those inpermissions.tsandAuthorizedChild.tsx - It's properly integrated in both
PatientRegister.tsxandFacilityHome.tsxcomponents - The implementation aligns with the existing role-based access control system using
user_typechecks - No duplicate authorization logic was found for patient registration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing authorization checks and patient registration points
# Test 1: Look for existing authorization checks
echo "Searching for existing authorization checks..."
rg -l "isAuthorized|canAccess|hasPermission|checkPermission" --type ts --type tsx
# Test 2: Look for patient registration components/routes
echo "Searching for patient registration components..."
rg -l "PatientRegister|addPatient|registerPatient" --type ts --type tsx
# Test 3: Look for facility access checks
echo "Searching for facility access checks..."
rg "facility.*access|access.*facility" --type ts --type tsx
Length of output: 602
Script:
#!/bin/bash
# Description: Search for existing authorization checks and patient registration points
# Test 1: Look for existing authorization checks
echo "Searching for existing authorization checks..."
rg -l "isAuthorized|canAccess|hasPermission|checkPermission"
# Test 2: Look for patient registration components/routes
echo "Searching for patient registration components..."
rg -l "PatientRegister|addPatient|registerPatient"
# Test 3: Look for facility access checks
echo "Searching for facility access checks..."
rg "facility.*access|access.*facility"
# Test 4: Look for similar authorization patterns
echo "Searching for similar authorization patterns..."
rg "user_type.*=.*(\"|').*Admin(\"|')"
# Test 5: Look for usage of PatientRegisterAuth
echo "Searching for PatientRegisterAuth usage..."
rg "PatientRegisterAuth"
Length of output: 4319
Script:
#!/bin/bash
# Let's examine the existing authorization implementations
# Check AuthorizedChild implementation
echo "Checking AuthorizedChild implementation..."
cat src/CAREUI/misc/AuthorizedChild.tsx
# Check permissions.ts implementation
echo "Checking permissions.ts implementation..."
cat src/Utils/permissions.ts
# Check FacilityHome usage context
echo "Checking FacilityHome usage context..."
rg -A 5 -B 5 "PatientRegisterAuth" src/components/Facility/FacilityHome.tsx
Length of output: 2955
| if ( | ||
| !isLoading && | ||
| facilityId && | ||
| facilityObject && | ||
| !PatientRegisterAuth(authUser, facilityObject, facilityId) | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more appropriate error component for unauthorized access.
Instead of using Error404 which indicates a missing resource, consider using a dedicated unauthorized access component or error message that clearly communicates the authorization issue to the user.
Example implementation:
- if (
- !isLoading &&
- facilityId &&
- facilityObject &&
- !PatientRegisterAuth(authUser, facilityObject, facilityId)
- ) {
- return <Error404 />;
- }
+ if (
+ !isLoading &&
+ facilityId &&
+ facilityObject &&
+ !PatientRegisterAuth(authUser, facilityObject, facilityId)
+ ) {
+ return (
+ <div className="text-center">
+ <h1 className="text-2xl font-bold text-red-600">Unauthorized Access</h1>
+ <p>You are not authorized to register patients in this facility.</p>
+ </div>
+ );
+ }📝 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.
| if ( | |
| !isLoading && | |
| facilityId && | |
| facilityObject && | |
| !PatientRegisterAuth(authUser, facilityObject, facilityId) | |
| ) { | |
| if ( | |
| !isLoading && | |
| facilityId && | |
| facilityObject && | |
| !PatientRegisterAuth(authUser, facilityObject, facilityId) | |
| ) { | |
| return ( | |
| <div className="text-center"> | |
| <h1 className="text-2xl font-bold text-red-600">Unauthorized Access</h1> | |
| <p>You are not authorized to register patients in this facility.</p> | |
| </div> | |
| ); | |
| } |
| export function PatientRegisterAuth( | ||
| authUser: UserModel, | ||
| facilityObject: FacilityModel | undefined, | ||
| facilityId: string, | ||
| ) { | ||
| const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"]; | ||
| if ( | ||
| !showAllFacilityUsers.includes(authUser.user_type) && | ||
| authUser.home_facility_object?.id === facilityId | ||
| ) { | ||
| return true; | ||
| } | ||
| if ( | ||
| authUser.user_type === "DistrictAdmin" && | ||
| authUser.district === facilityObject?.district | ||
| ) { | ||
| return true; | ||
| } | ||
| if ( | ||
| authUser.user_type === "StateAdmin" && | ||
| authUser.state === facilityObject?.state | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the PatientRegisterAuth function implementation.
The function needs several improvements for better maintainability and type safety:
- Add return type annotation and JSDoc documentation
- Move constants outside function
- Simplify logic with early returns
+const FACILITY_ADMIN_TYPES = ["DistrictAdmin", "StateAdmin"] as const;
+
+/**
+ * Checks if a user is authorized to register patients in a specific facility
+ * @param authUser - The authenticated user
+ * @param facilityObject - The facility to check authorization for
+ * @param facilityId - The ID of the facility
+ * @returns boolean indicating if the user is authorized
+ */
-export function PatientRegisterAuth(
+export function PatientRegisterAuth(
authUser: UserModel,
facilityObject: FacilityModel | undefined,
facilityId: string,
-) {
- const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"];
- if (
- !showAllFacilityUsers.includes(authUser.user_type) &&
- authUser.home_facility_object?.id === facilityId
- ) {
- return true;
- }
- if (
- authUser.user_type === "DistrictAdmin" &&
- authUser.district === facilityObject?.district
- ) {
- return true;
- }
- if (
- authUser.user_type === "StateAdmin" &&
- authUser.state === facilityObject?.state
- ) {
- return true;
- }
-
- return false;
+): boolean {
+ // Regular users can only access their home facility
+ if (!FACILITY_ADMIN_TYPES.includes(authUser.user_type)) {
+ return authUser.home_facility_object?.id === facilityId;
+ }
+
+ // District admins can access facilities in their district
+ if (authUser.user_type === "DistrictAdmin") {
+ return authUser.district === facilityObject?.district;
+ }
+
+ // State admins can access facilities in their state
+ if (authUser.user_type === "StateAdmin") {
+ return authUser.state === facilityObject?.state;
+ }
+
+ return false;
}📝 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.
| export function PatientRegisterAuth( | |
| authUser: UserModel, | |
| facilityObject: FacilityModel | undefined, | |
| facilityId: string, | |
| ) { | |
| const showAllFacilityUsers = ["DistrictAdmin", "StateAdmin"]; | |
| if ( | |
| !showAllFacilityUsers.includes(authUser.user_type) && | |
| authUser.home_facility_object?.id === facilityId | |
| ) { | |
| return true; | |
| } | |
| if ( | |
| authUser.user_type === "DistrictAdmin" && | |
| authUser.district === facilityObject?.district | |
| ) { | |
| return true; | |
| } | |
| if ( | |
| authUser.user_type === "StateAdmin" && | |
| authUser.state === facilityObject?.state | |
| ) { | |
| return true; | |
| } | |
| return false; | |
| } | |
| const FACILITY_ADMIN_TYPES = ["DistrictAdmin", "StateAdmin"] as const; | |
| /** | |
| * Checks if a user is authorized to register patients in a specific facility | |
| * @param authUser - The authenticated user | |
| * @param facilityObject - The facility to check authorization for | |
| * @param facilityId - The ID of the facility | |
| * @returns boolean indicating if the user is authorized | |
| */ | |
| export function PatientRegisterAuth( | |
| authUser: UserModel, | |
| facilityObject: FacilityModel | undefined, | |
| facilityId: string, | |
| ): boolean { | |
| // Regular users can only access their home facility | |
| if (!FACILITY_ADMIN_TYPES.includes(authUser.user_type)) { | |
| return authUser.home_facility_object?.id === facilityId; | |
| } | |
| // District admins can access facilities in their district | |
| if (authUser.user_type === "DistrictAdmin") { | |
| return authUser.district === facilityObject?.district; | |
| } | |
| // State admins can access facilities in their state | |
| if (authUser.user_type === "StateAdmin") { | |
| return authUser.state === facilityObject?.state; | |
| } | |
| return false; | |
| } |
Proposed Changes
isAuthorizedToRegisterPatientfunction to handle authorization checks when adding a new patient from the "View Facility" page.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes