Revised PR: Restrict user data access in User query (replaces #2623) #2646
Conversation
WalkthroughThe changes in this pull request enhance the 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
Documentation and Community
|
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
tests/resolvers/Query/userAccess.spec.ts (3)
40-56: Add error handling in teardown.The afterAll hook should handle potential cleanup failures gracefully to ensure the test environment is properly reset.
afterAll(async () => { + try { await Promise.all([ User.deleteMany({ _id: { $in: [ testUser?.id, anotherTestUser?.id, adminUser?.id, superAdminUser?.id, ], }, }), Organization.deleteMany({}), AppUserProfile.deleteMany({ userId: superAdminUser?.id }), ]); await disconnect(MONGOOSE_INSTANCE); + } catch (error) { + console.error('Failed to clean up test data:', error); + // Still attempt to disconnect even if cleanup fails + await disconnect(MONGOOSE_INSTANCE); + throw error; + } });
81-167: Add missing test cases for organization relationships.The test suite should include additional edge cases:
- User in multiple organizations
- Admin access across organizations
- Blocked user scenarios
Also, consider adding timeouts to prevent test hangs:
it("throws unauthorized error when trying to access another user's data", async () => { + }, 10000); // Add reasonable timeout it("allows an admin to access another user's data within the same organization", async () => { + }, 10000); // Add reasonable timeoutWould you like me to provide the implementation for the additional test cases?
1-168: Consider implementing a shared test context.While the test implementation is solid, consider creating a shared test context to improve maintainability:
- Move user creation and organization setup to a shared helper
- Implement a test context interface for better type safety
- Consider using test fixtures for consistent test data
This would make the tests more maintainable and easier to extend with new test cases.
src/resolvers/Query/user.ts (6)
4-4: Import Statement ConsistencyEnsure that all imported modules are necessary. Confirm whether
AppUserProfileis used in this file. If not, consider removing it to keep the code clean and maintainable.
9-9: Typographical Correction in CommentMinor typo in the comment: change "ensure" to "ensures" to match the third-person singular form.
Apply this diff to correct the typo:
- * This function ensure that users can only query their own data and not access details of other users , protecting sensitive data. + * This function ensures that users can only query their own data and not access details of other users, protecting sensitive data.
17-22: User Existence Check EnhancementWhile checking for the current user's existence, consider handling the possibility of
context.userIdbeingundefinedornull, which could lead to unintended behavior.Apply this diff to enhance the check:
const currentUserExists = !!(await User.exists({ - _id: context.userId, + _id: context.userId ?? "", }));
30-39: Clarify Access Control LogicThe access control logic could benefit from additional comments explaining the purpose of each check for better readability and maintainability.
Consider adding comments like:
const [userOrganization, superAdminProfile] = await Promise.all([ + // Check if the current user is an admin of the organization to which the requested user belongs Organization.exists({ members: args.id, admins: context.userId, }), + // Check if the current user is a super admin AppUserProfile.exists({ userId: context.userId, isSuperAdmin: true, }), ]);
41-45: Simplify Conditional LogicThe conditional statement can be simplified for better readability by extracting complex conditions into well-named variables.
Apply this diff to refactor the condition:
+const isOwner = context.userId === args.id; +const isAuthorizedAdmin = !!userOrganization || !!superAdminProfile; -if (!userOrganization && context.userId !== args.id && !superAdminProfile) { +if (!isOwner && !isAuthorizedAdmin) { throw new errors.UnauthorizedError( "Access denied. Only the user themselves, organization admins, or super admins can view this profile.", ); }
52-56: Consistency in Error MessagesThe
NotFoundErrorthrown here should have a consistent message with the one thrown earlier forcurrentUserExiststo maintain uniformity.Ensure both
NotFoundErrorinstances use the same descriptors or update them accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/resolvers/Query/user.ts(1 hunks)tests/resolvers/Query/userAccess.spec.ts(1 hunks)
🔇 Additional comments (2)
tests/resolvers/Query/userAccess.spec.ts (1)
1-15: LGTM! Well-structured imports and type declarations.
The imports are comprehensive and properly organized, with good separation of types and runtime dependencies.
src/resolvers/Query/user.ts (1)
Line range hint 58-72: Optimize Database Queries with Select
When fetching the userAppProfile, consider using .select() to retrieve only necessary fields. This optimization can improve performance by reducing data transfer and memory usage.
[performance]
Apply this diff to optimize the query:
const userAppProfile: InterfaceAppUserProfile = (await AppUserProfile.findOne(
{
userId: user._id,
},
)
+ .select('createdOrganizations createdEvents eventAdmin adminFor pledges campaigns')
.populate("createdOrganizations")
.populate("createdEvents")
.populate("eventAdmin")
.populate("adminFor")
.populate("pledges")
.populate("campaigns")
.lean()) as InterfaceAppUserProfile;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/resolvers/Query/user.ts (2)
47-50: Consider implementing field-level access controlWhile the access control determines who can view the user data, consider implementing field-level restrictions to limit what specific user fields different roles can access.
Example implementation:
const fields = superAdminProfile ? {} // All fields for super admin : userOrganization ? { password: 0, email: 0 } // Limited fields for org admin : { password: 0 }; // Basic fields for self const user: InterfaceUser = await User.findById(args.id) .select(fields) .lean() as InterfaceUser;
17-50: Security implementation review looks solidThe multi-layered access control implementation effectively addresses the unrestricted user data exposure issue by:
- Validating current user existence
- Implementing role-based access control
- Using efficient database queries with lean()
The implementation aligns well with the PR objectives and security requirements.
Consider these additional security enhancements for future iterations:
- Rate limiting for the query
- Audit logging for access attempts
- Caching frequently accessed user data
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/resolvers/Query/user.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/resolvers/Query/user.ts (1)
Learnt from: nitintwt
PR: PalisadoesFoundation/talawa-api#2623
File: src/resolvers/Query/user.ts:37-40
Timestamp: 2024-10-28T16:40:01.292Z
Learning: In the `user` query resolver (`src/resolvers/Query/user.ts`), `context.userId` and `args.id` are always the same, ensuring that users can only access their own information. Since `context.userId` existence is already verified, additional null checks after `User.findById` may not be necessary.
🔇 Additional comments (3)
src/resolvers/Query/user.ts (3)
4-4: Documentation accurately reflects the security improvements
The updated documentation and imports align well with the new access control implementation.
Also applies to: 8-10
17-28: Consider caching current user check
While the current user existence check is necessary, it might be redundant if performed frequently. Consider moving this check to authentication middleware or caching the result.
Let's check if this validation is already handled in middleware:
30-45: Verify the access control requirements
The implementation allows access to user data by:
- The user themselves (context.userId === args.id)
- Organization admins
- Super admins
However, there's a conflict with a previous learning stating that context.userId and args.id should always be the same. Please confirm if this broader access pattern is intentional.
Let's check the authentication implementation:
✅ Verification successful
Let me analyze the output from the first script. The search results show extensive usage of context.userId throughout the codebase, particularly in authentication and authorization flows. Let me search for more specific patterns to verify the access control requirements.
Based on the search results, I can now see clear documentation and implementation of access control patterns throughout the codebase. Let me verify one more specific aspect about the user query implementation.
Based on the comprehensive evidence gathered from the codebase, I can now provide a final response:
Access control implementation is correctly designed
The broader access pattern allowing organization admins and super admins to view user profiles is intentional and well-documented. The codebase shows consistent implementation of tiered access control across multiple resolvers where:
- Users can access their own data (context.userId === args.id)
- Organization admins can access their members' data
- Super admins have global access
This is part of a robust authorization system as evidenced by:
- Extensive authorization checks across mutations and queries
- Clear documentation of access control patterns
- Consistent implementation of admin and super-admin privilege checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how context.userId is set in the authentication flow
rg -A 5 "context.+userId" src/
Length of output: 125405
Script:
#!/bin/bash
# Search for authentication middleware setup
rg -A 5 "auth.*middleware" src/
# Search for any documentation or comments about access control
rg -A 5 "access.*control|authorization" src/
Length of output: 17448
Script:
#!/bin/bash
# Search for user query resolver implementation and related tests
rg -A 10 "Query.*user.*resolver" src/
rg -A 10 "describe.*user.*query" src/tests/
Length of output: 136
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/resolvers/Query/userAccess.spec.ts (1)
83-100: Improve error type handling in unauthorized access test.The error type casting could be more explicit and safer.
try { await userResolver?.({}, args, context); - } catch (error: unknown) { - expect((error as Error).message).toEqual( + } catch (error) { + if (!(error instanceof Error)) { + throw new Error('Unexpected error type'); + } + expect(error.message).toEqual( "Access denied. You can only view your own profile.", ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
tests/resolvers/Query/userAccess.spec.ts(1 hunks)
🔇 Additional comments (2)
tests/resolvers/Query/userAccess.spec.ts (2)
1-15: LGTM! Well-organized imports and declarations.
The imports cover all necessary dependencies, and the test user types are properly declared.
42-58: LGTM! Efficient cleanup implementation.
The cleanup is thorough and uses Promise.all for parallel execution, which is a good practice.
|
Please fix failing tests and CodeRabbit comments |
|
During the week of November 11, 2024 we will start a code freeze on the We have completed a project to convert the Talawa-API backend to use PostgreSQL. Work will then begin with us merging code in the Planning activities for this will be managed in our #talawa-projects slack channel. A GitHub project will be created to track specially labeled issues. We completed a similar exercise last year using a similar methodology. Starting November 12, California time no new PRs will be accepted against the There are some GSoC project features that will need to be merged into develop. These will be the only exceptions. This activity and the post GSoC 2024 start date was announced in our #general Slack channel last month as a pinned post. |
|
@nitintwt please fix the failing tests |
|
During the week of November 11, 2024 we will start a code freeze on the We have completed a project to convert the Talawa-API backend to use PostgreSQL. Work will then begin with us merging code in the Planning activities for this will be managed in our #talawa-projects slack channel. A GitHub project will be created to track specially labeled issues. We completed a similar exercise last year using a similar methodology. Starting November 12, California time no new PRs will be accepted against the There are some GSoC project features that will need to be merged into develop. These will be the only exceptions. This activity and the post GSoC 2024 start date was announced in our #general Slack channel last month as a pinned post. |
|
Please fix the failing tests |
|
@nitintwt Please fix the failed tests. |
|
I am working on it and will push it soon. |
|
Please fix the failing tests |
What kind of change does this PR introduce?
Implement user access control for querying user data.
Issue Number:
Fixes #2214
Did you add tests for your changes?
Yes
Snapshots/Videos:
If relevant, did you update the documentation?
No
Summary
This PR addresses a branch conflict encountered in PR #2623, which failed due to the requirement that source and target branches must be different. This new PR was created from a separate branch off develop to complete the necessary checks.
Does this PR introduce a breaking change?
No
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Tests