bug: Restrict user data access in User query #2623
Conversation
WalkthroughThe changes made in the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
![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 (3)
src/resolvers/Query/user.ts (3)
8-10: Enhance documentation with specific authorization details.While the documentation clearly states the access control purpose, consider adding details about how authorization is performed (using
context.userId).- * 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 by comparing the requested user ID + * with the authenticated user's ID (context.userId), protecting sensitive data.
Line range hint
17-29: Consider combining existence check with data retrieval.The current implementation has a potential race condition between checking existence and fetching data. Consider combining these operations to ensure atomicity.
- // Check if the current user exists in the system - const currentUserExists = !!(await User.exists({ - _id: context.userId, - })); - - if (currentUserExists === false) { + // Fetch the current user to verify existence + const currentUser = await User.findById(context.userId); + if (!currentUser) { throw new errors.NotFoundError( USER_NOT_FOUND_ERROR.DESC, USER_NOT_FOUND_ERROR.CODE, USER_NOT_FOUND_ERROR.PARAM, ); }
30-35: Authorization check implementation looks good!The access control logic correctly prevents unauthorized access to other users' data. Consider making the error message more specific about the attempted action.
- "Access denied. You can only view your own profile.", + `Access denied. Cannot view profile for user ${args.id}. You can only view your own profile.`,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/resolvers/Query/user.ts (2 hunks)
🔇 Additional comments (1)
src/resolvers/Query/user.ts (1)
Line range hint
17-41: Verify GraphQL query implementations in the frontend.The access control implementation looks solid. Let's verify that frontend implementations handle the new authorization errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
tests/resolvers/Query/userAccess.spec.ts (3)
9-11: Consider adding strict typing for test variables.The test variables could benefit from more explicit typing to prevent potential undefined values and improve type safety.
-let testUser: TestUserType; -let anotherTestUser: TestUserType; +let testUser: TestUserType | undefined; +let anotherTestUser: TestUserType | undefined;
13-21: Add error handling for test user creation.While the setup is generally good, it lacks error handling for test user creation which could make test failures harder to debug.
beforeAll(async () => { MONGOOSE_INSTANCE = await connect(); - testUser = await createTestUser(); - anotherTestUser = await createTestUser(); + try { + testUser = await createTestUser(); + anotherTestUser = await createTestUser(); + } catch (error) { + console.error('Failed to create test users:', error); + throw error; + } });
23-74: Add tests for edge cases in user access control.The test suite should include additional cases to ensure robust access control:
- Test behavior when context.userId is undefined (unauthenticated user)
- Test behavior when provided user ID is null
Example additional test cases:
it("throws error when user is not authenticated", async () => { expect.assertions(1); const args = { id: testUser?.id }; const context = { userId: undefined }; try { await user?.({}, args, context); } catch (error: unknown) { expect((error as Error).message).toEqual("User must be authenticated"); } }); it("throws error when user ID is null", async () => { expect.assertions(1); const args = { id: null }; const context = { userId: testUser?.id }; try { await user?.({}, args, context); } catch (error: unknown) { expect((error as Error).message).toEqual("Invalid user ID"); } });src/resolvers/Query/user.ts (1)
Line range hint
17-29: Consider removing redundant existence check.Since
context.userIdis already verified during authentication, this existence check may be redundant. If you want to keep it for extra safety, consider moving it to a middleware or auth guard for better separation of concerns.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/resolvers/Query/user.ts(2 hunks)tests/resolvers/Query/userAccess.spec.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 (5)
tests/resolvers/Query/userAccess.spec.ts (1)
1-7: LGTM! Well-structured imports with proper type declarations.
The imports are well-organized, using proper type imports where needed and including all necessary testing utilities.
src/resolvers/Query/user.ts (4)
8-10: LGTM! Clear documentation update.
The JSDoc comment accurately describes the new access control behavior.
30-35: LGTM! Proper authorization check implementation.
The authorization check effectively ensures users can only access their own data, with a clear error message.
37-48: Remove redundant null check after findById.
Since context.userId and args.id are always the same, and we've already verified the user exists, this null check is redundant. The user will always be found at this point.
Apply this diff to remove the redundant check:
// Fetch the user data from the database based on the provided ID (args.id)
const user: InterfaceUser = (await User.findById(
args.id,
).lean()) as InterfaceUser;
- if (!user) {
- throw new errors.NotFoundError(
- USER_NOT_FOUND_ERROR.DESC,
- USER_NOT_FOUND_ERROR.CODE,
- USER_NOT_FOUND_ERROR.PARAM,
- );
- }Line range hint 17-48: Add tests for the new access control logic.
While the implementation looks good, the PR mentions no tests were added. Consider adding test cases to verify:
- Unauthorized access attempts are properly blocked
- Users can successfully access their own data
- Error scenarios (user not found, etc.)
Would you like me to help generate test cases for these scenarios?
palisadoes
requested review from
pranshugupta54,
gautam-divyanshu and
palisadoes
|
@pranshugupta54 Only admins of the specific organization in which the user is a member can access that user's data. Am I right ? |
|
Yes, and also SuperAdmin |
|
@pranshugupta54 SuperAdmin means the creator of the org ? In the organization schema , creatorId is SuperAdmin ? |
|
Superadmin is like admin of full community of all organisations |
|
nitintwt
dismissed stale reviews from palisadoes and coderabbitai[bot]
via
a9d4636
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
src/resolvers/Query/user.ts (1)
17-28: Optimize the current user existence check.The check is correct but can be simplified by removing the double negation.
- const currentUserExists = !!(await User.exists({ + const currentUserExists = await User.exists({ _id: context.userId, }));tests/resolvers/Query/userAccess.spec.ts (1)
123-142: Add edge case tests for user profile access.Consider adding the following test cases:
- Access with invalid/expired session
- Access with missing context
- Access with malformed user ID
Example test case:
it("throws error when accessing with invalid session", async () => { expect.assertions(1); const args = { id: testUser?.id }; const context = { userId: null }; try { await userResolver?.({}, args, context); } catch (error) { expect(error instanceof Error && error.message).toEqual( INVALID_SESSION_ERROR.DESC ); } });
📜 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 (3)
src/resolvers/Query/user.ts (3)
4-4: LGTM! Documentation and imports are well-structured.
The updated documentation clearly explains the privacy constraints, and the new Organization import supports the added access control functionality.
Also applies to: 8-10
48-51: LGTM! Secure data fetching and proper sanitization.
The implementation:
- Uses proper null checks
- Securely fetches user data
- Correctly populates related data
- Appropriately filters sensitive information (organizationsBlockedBy)
Also applies to: 53-83
Line range hint 1-83: Add test coverage for the new access control logic.
While the implementation successfully addresses the security concerns, test coverage is crucial for such sensitive functionality. Please add tests to verify:
- Access denied for non-admin users
- Access granted for organization admins
- Access granted for super admins
- Access granted for self
- Error cases for non-existent users
Would you like me to help create the test cases?
| expect((error as Error).message).toEqual( | ||
| "Access denied. You can only view your own profile.", | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move error message to constants.
The error message for unauthorized access should be defined in the constants file for consistency and maintainability.
- "Access denied. You can only view your own profile.",
+ UNAUTHORIZED_ACCESS_ERROR.DESC,Add to constants file:
export const UNAUTHORIZED_ACCESS_ERROR = {
CODE: 'UNAUTHORIZED_ACCESS',
DESC: 'Access denied. You can only view your own profile.'
};There was a problem hiding this comment.
@nitintwt, please fix this. We don't want hardcoded text directly inside the code. Check if any similar message is already there.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| it("allows an admin to access another user's data within the same organization", async () => { | ||
| const args = { | ||
| id: anotherTestUser?.id, | ||
| }; | ||
|
|
||
| const context = { | ||
| userId: adminUser?.id, | ||
| apiRootURL: BASE_URL, | ||
| }; | ||
|
|
||
| const result = await userResolver?.({}, args, context); | ||
|
|
||
| const user = await User.findById(anotherTestUser?._id).lean(); | ||
|
|
||
| expect(result?.user).toEqual({ | ||
| ...user, | ||
| organizationsBlockedBy: [], | ||
| image: user?.image ? `${BASE_URL}${user.image}` : null, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance admin access test validation.
Consider the following improvements:
- Verify that the user being accessed is actually in the admin's organization
- Create a custom matcher for user object validation
// Create a custom matcher in test helpers
expect.extend({
toBeValidUserResponse(received, expected, baseUrl) {
return {
pass: this.equals(received, {
...expected,
organizationsBlockedBy: [],
image: expected.image ? `${baseUrl}${expected.image}` : null,
}),
message: () => 'User response does not match expected format'
};
},
});
// Use in test
const org = await Organization.findOne({ admins: adminUser?.id });
expect(org?.members).toContain(anotherTestUser?.id);
expect(result?.user).toBeValidUserResponse(user, BASE_URL);| it("allows a super admin to access any user's data", async () => { | ||
| const args = { | ||
| id: anotherTestUser?.id, | ||
| }; | ||
|
|
||
| const context = { | ||
| userId: superAdminUser?.id, | ||
| apiRootURL: BASE_URL, | ||
| }; | ||
|
|
||
| const result = await userResolver?.({}, args, context); | ||
|
|
||
| const user = await User.findById(anotherTestUser?._id).lean(); | ||
|
|
||
| expect(result?.user).toEqual({ | ||
| ...user, | ||
| organizationsBlockedBy: [], | ||
| image: user?.image ? `${BASE_URL}${user.image}` : null, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in admin test cases.
The super admin test case shares significant code with the admin test case. Consider extracting common validation logic into a shared utility function.
// Create a test utility
const testUserAccess = async (
accessingUser: TestUserType,
targetUser: TestUserType,
context: Record<string, unknown>
) => {
const args = { id: targetUser?.id };
const result = await userResolver?.({}, args, context);
const user = await User.findById(targetUser?._id).lean();
expect(result?.user).toBeValidUserResponse(user, context.apiRootURL as string);
return result;
};
// Use in tests
it("allows a super admin to access any user's data", async () => {
await testUserAccess(superAdminUser, anotherTestUser, {
userId: superAdminUser?.id,
apiRootURL: BASE_URL,
});
});
|
Please fix failing checks |
|
The error says the source and target branches should be different. According to the docs, I need to use the develop branch locally for contributions. So, to resolve the failed check, I have to create a branch from the develop branch and then push it. However, I've already created the PR and can’t change the branch of my local repo. I’ll need to create a new PR to resolve this. @pranshugupta54 Am I correct? |
|
Yes, please open a new PR. Ensure the develop branch stays synced with the original repository, and create separate branches for any fixes. Also, please reference this PR in your new one with a brief explanation for clarity. |
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?
No
Snapshots/Videos:
If relevant, did you update the documentation?
No
Summary
This PR introduces user access control to ensure that users can only query their own profile data, addressing privacy concerns and data access violations. Previously, users could query information about other users, which goes against common privacy principles. This change mitigates the risk of unauthorized data access by checking the user ID before returning user information.
Does this PR introduce a breaking change?
No, this PR does not introduce any breaking changes.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Tests