Restrict user data access in User query #2797
Conversation
Signed-off-by: NishantSinghhhhh <[email protected]>
WalkthroughThis pull request introduces enhanced security and access control mechanisms for user data retrieval. The changes focus on refining the user query resolver to implement stricter permission checks, ensuring that users can only access their own profile or have appropriate organizational permissions. Modifications include updating the user resolver logic, adding comprehensive test cases to validate access scenarios, and extending the Changes
Suggested labels
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. 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.
Signed-off-by: NishantSinghhhhh <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/resolvers/Query/user.ts (2)
8-9: Minor grammar improvement in docstring
"ensure" should be "ensures" to align with third-person usage:"This function ensures that users can only query their own data..."
52-52: Redundant user existence checks
The secondNotFoundErrormirrors the earlier existence check. While this is protective, confirm that it isn’t duplicated logic. If the user was found prior, consider handling it in a single code path.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.node-version(0 hunks)src/resolvers/Query/user.ts(1 hunks)tests/resolvers/User/userAccess.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .node-version
🔇 Additional comments (11)
src/resolvers/Query/user.ts (6)
4-4: Validate imported model usage
You have added the Organization model, which is crucial for checking user membership. Ensure that each usage of this model is relevant to the business requirement—verifying membership or admin privileges for the same organization.
17-21: Efficient approach for checking current user existence
Using User.exists({ _id: context.userId }) is efficient for a quick existence check. Confirm that no additional user fields are needed. If more fields are eventually required, consider retrieving the user directly to minimize the number of queries.
22-28: Clarify user existence assumption
Throwing a NotFoundError here ensures no further processing occurs if the current user is missing. This is correct for secure GraphQL resolvers.
30-39: Parallel checks for org membership and super admin
Using Promise.all to execute membership and super admin checks in parallel is a solid performance optimization. Verify that membership logic accurately enforces intended organizational boundaries.
41-45: Robust permission check condition
The condition (!userOrganization && context.userId !== args.id && !superAdminProfile) correctly enforces restricted access to the profile. This helps prevent unauthorized data exposure. Keep these conditions updated if future roles are introduced.
47-51: Lean query usage
Calling User.findById(...).lean() is optimal for performance. If the user record is mutable later in this flow, consider omitting .lean() to allow direct document manipulation.
tests/resolvers/User/userAccess.spec.ts (5)
19-26: Potential ordering concern in test setup
deleteUserFromCache(testUser?.id) is called before testUser is created (line 27). This might be intentional to remove stale user data, but confirm that testUser won’t remain undefined. Also, consider removing or redirecting the console.log(pledges); (line 25) if it’s purely for debugging.
97-114: Meaningful unauthorized access test
The assertion ensures that a non-admin, non-superadmin user cannot retrieve another user’s info. This closely aligns with the updated access logic in user.ts.
117-141: Admin access test matches business rules
Confirming that an admin can view a member’s data within the same organization aligns well with membership checks in the resolver.
143-164: Thorough super admin coverage
This test confirms a super admin may access any user profile. The result is correctly validated against the stored user document.
166-187: Accurate test for self-access case
Ensuring a user can retrieve their own data completes the core use cases for the new resolver logic. This test helps maintain coverage for personal data access.
Signed-off-by: NishantSinghhhhh <[email protected]>
…pi into bug-user-query
Signed-off-by: NishantSinghhhhh <[email protected]>
Signed-off-by: NishantSinghhhhh <[email protected]>
Signed-off-by: NishantSinghhhhh <[email protected]>
…pi into bug-user-query
|
@palisadoes , can you please review this PR |
|
Please fix the failing tests. We need to get the code coverage for your patch to 100% without the use of ignore statements |
Signed-off-by: NishantSinghhhhh <[email protected]>
Signed-off-by: NishantSinghhhhh <[email protected]>
Signed-off-by: NishantSinghhhhh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/resolvers/Query/user.spec.ts (2)
68-89: Effective test for non-existent user scenario
These lines provide clear and robust coverage for the scenario where a user ID does not exist. The usage ofexpect.assertions(2)ensures both expectations are verified. One minor suggestion is to consider removing theelseblock ifuserResolveris not defined, since the test would fail anyway, but this is more of a stylistic choice than a requirement.
91-117: Well-structured test for null user scenario
MockingUser.findByIdto simulate a null return is a solid approach. This test thoroughly checks the resolver’s behavior and properly restores mocks afterward. Consider verifying thatUser.findByIdis also invoked with the correct argument(s) to strengthen the test’s thoroughness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/resolvers/Query/user.spec.ts(2 hunks)tests/resolvers/User/userAccess.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/resolvers/User/userAccess.spec.ts
🔇 Additional comments (1)
tests/resolvers/Query/user.spec.ts (1)
9-9: Use of Vitest imports is correct
Good to see that you're using the vi import for mocking in Vitest. This aligns well with best practices for testing in this framework.
|
Please fix the failing tests. Our test code coverage system will fail if any of these conditions occur:
Here are some steps to take:
We do this to ensure the overall reliability of the code base is maintained. This is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. Additional notes:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2797 +/- ##
========================================
Coverage 97.72% 97.72%
========================================
Files 364 364
Lines 18545 18569 +24
Branches 2676 2681 +5
========================================
+ Hits 18123 18147 +24
Misses 417 417
Partials 5 5 ☔ View full report in Codecov by Sentry. |
Signed-off-by: NishantSinghhhhh <[email protected]>
…pi into bug-user-query
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/resolvers/User/userAccess.spec.ts (1)
27-35:⚠️ Potential issueFix TypeScript 'any' type usage.
Replace the
anytype with a more specific type to improve type safety.- [key: string]: any; + [key: string]: string | mongoose.Types.ObjectId | string[] | null | undefined;🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[warning] 34-34:
Unexpected any. Specify a different type
🧹 Nitpick comments (4)
src/resolvers/Query/user.ts (2)
31-46: Consider caching permission check results.The permission checks are correctly implemented but consider caching the results of these checks, especially for super admin status, to improve performance in high-traffic scenarios.
+ // Add caching for superAdminProfile check + const SUPER_ADMIN_CACHE_TTL = 300; // 5 minutes + const superAdminCacheKey = `superAdmin:${context.userId}`; + + let superAdminProfile = await cache.get(superAdminCacheKey); + if (superAdminProfile === null) { superAdminProfile = await AppUserProfile.exists({ userId: context.userId, isSuperAdmin: true, }); + await cache.set(superAdminCacheKey, superAdminProfile, SUPER_ADMIN_CACHE_TTL); + }
48-53: Consider using projection for better performance.While the implementation is correct, consider using projection to fetch only the required fields, especially when dealing with large user documents.
- const user: InterfaceUser = (await User.findById( - args.id, - ).lean()) as InterfaceUser; + const user: InterfaceUser = (await User.findById( + args.id, + { + email: 1, + firstName: 1, + lastName: 1, + image: 1, + organizationsBlockedBy: 1 + } + ).lean()) as InterfaceUser;tests/resolvers/User/userAccess.spec.ts (2)
222-231: Consolidate duplicate test cases.This test case duplicates the functionality already tested in the test at lines 134-141. Consider removing this duplicate test or consolidating them using a test helper function.
- it("should throw UnauthorizedError when trying to access another user's data", async () => { - const args = { id: users.anotherTestUser._id.toString() }; - const context = { userId: users.testUser._id.toString() }; - - await expect(userResolver?.({}, args, context)).rejects.toThrowError( - new errors.UnauthorizedError( - "Access denied. Only the user themselves, organization admins, or super admins can view this profile.", - ), - ); - });
124-132: Add test cases for edge cases.Consider adding test cases for:
- Malformed user IDs
- Empty string IDs
- Non-string ID values
Would you like me to generate the additional test cases for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resolvers/Query/user.ts(1 hunks)tests/resolvers/User/userAccess.spec.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Check for linting, formatting, and type errors
tests/resolvers/User/userAccess.spec.ts
[warning] 34-34:
Unexpected any. Specify a different type
🔇 Additional comments (5)
src/resolvers/Query/user.ts (3)
4-4: LGTM! Clear documentation of access control policy.
The updated imports and documentation clearly communicate the data access restrictions, which is crucial for security-sensitive changes.
Also applies to: 8-10
18-29: LGTM! Robust user existence validation.
The implementation correctly validates the current user's existence before proceeding, with proper error handling using the NotFoundError class.
42-46: Verify complete sanitization of sensitive data.
While the code clears organizationsBlockedBy, let's verify that all sensitive fields are properly handled before being returned to the client.
Also applies to: 54-83
✅ Verification successful
Let me gather more information about the user fields and their handling.
Let me gather more information about the user query implementation.
Let me try one more search to understand the complete user query implementation.
Sensitive data handling is properly implemented
Based on the code analysis:
- Password field is not exposed as it's not included in the lean query
organizationsBlockedByis explicitly cleared by returning an empty array- Access control is properly implemented with checks for user themselves, organization admins, or super admins
- Email and other sensitive fields are protected by the access control checks
- The user model schema shows proper field definitions with appropriate access controls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sensitive field definitions in user model and schema
echo "Searching for sensitive fields in User model..."
ast-grep --pattern 'const userSchema = new Schema({
$$$
password: $_,
$$$
email: $_,
$$$
private: $_,
$$$
})'
echo "Searching for fields marked as sensitive in comments..."
rg -i "sensitive|private|secret|internal" --type ts
Length of output: 15926
Script:
#!/bin/bash
# Search for user type definition and field selection in GraphQL
echo "Searching for User type definition..."
ast-grep --pattern 'type User {
$$$
}'
echo "Searching for field selection in user query..."
ast-grep --pattern 'const user = await User.findById($_).select($_)'
# Search for any data sanitization or field exclusion
echo "Searching for data sanitization patterns..."
rg "select\(-|\.select\(" --type ts -A 3
Length of output: 42605
Script:
#!/bin/bash
# Search for user query implementation
echo "Searching for user query implementation..."
ast-grep --pattern 'export const user = async ($_) => {
$$$
}'
# Search for user model fields
echo "Searching for user model fields..."
rg "new Schema\({" -A 20 --type ts
# Search for any field sanitization in the user query
echo "Searching for field sanitization in user query..."
rg "organizationsBlockedBy|password|email|private" -B 2 -A 2 src/resolvers/Query/user.ts
Length of output: 671
Script:
#!/bin/bash
# Search for user model schema
echo "Searching for User model schema..."
rg "const userSchema = new Schema" -A 30 --type ts
# Search for complete user query implementation
echo "Searching for complete user query implementation..."
cat src/resolvers/Query/user.ts
# Search for any field selection in user query
echo "Searching for field selection in user query..."
rg "\.select\(" src/resolvers/Query/user.ts -A 2
Length of output: 4142
tests/resolvers/User/userAccess.spec.ts (2)
52-107: LGTM! Comprehensive test setup with proper error handling.
The test setup is thorough and includes proper error handling and verification of test data creation.
109-122: LGTM! Thorough test cleanup.
The cleanup implementation properly removes all test data in parallel, preventing test pollution.
|
@palisadoes , Done If there are any more components that do not have 100% coverage, I am ready to work on those too |
|
Please fix the linting error
|
Signed-off-by: NishantSinghhhhh <[email protected]>
|
@palisadoes , Done with the changes |
|
@palisadoes , my current code is not having the type any in userAcees.spec.ts Screencast.from.2024-12-31.03-23-31.webmand the the changes that were told by codeRabbit was on a chunk of tests that I removed as they were not increasing the %coverage |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/resolvers/User/userAccess.spec.ts (1)
27-46: Consider consolidating type definitions.You've added several new types (
AdditionalUserFields,UserType,ResolverReturnType) to model the user data in tests. If these types overlap with existing application interfaces or are needed elsewhere, consider placing them in a shared file to ensure a single source of truth and maintain consistent type definitions.src/resolvers/Query/user.ts (3)
9-10: Enhanced function documentation is a good step.Specifying that users can only retrieve their own data (or those who are admins/super admins) provides clarity for future maintainers. Consider adding an explicit mention of the error(s) thrown for unauthorized scenarios.
42-46: UnauthorizedError message is clear, might consider i18n.Your error message is explicit about the conditions for access, which is ideal for maintainers. If your application is localized, ensure the text is captured for translation or externalized to a constants file.
54-58: Customized NotFoundError usage.Throwing
NotFoundErrorwith descriptive properties (DESC,CODE,PARAM) is well-structured. Ensure these properties match the new fields you introduced inNotFoundErrorso the end-to-end error context is consistent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/libraries/errors/notFoundError.ts(2 hunks)src/resolvers/Query/user.ts(1 hunks)tests/resolvers/User/userAccess.spec.ts(1 hunks)
🔇 Additional comments (8)
tests/resolvers/User/userAccess.spec.ts (4)
13-25: Mocking approach looks good, but verify coverage.
Using vi.mock on FundraisingCampaignPledge helps prevent unwanted side effects in tests. However, ensure that all methods you rely on in your code are properly mocked. If any unmocked methods are used, it could reduce test coverage or cause unpredictable behavior. Consider verifying whether additional mocked methods or partial mocking is needed to increase coverage.
72-129: Database setup in beforeAll is robust.
Your beforeAll hook efficiently handles the database connection, user creation, and organization setup. The sequential creation of mocks and resources ensures a stable test environment. If coverage drops, confirm each code path (e.g., error scenarios during organization creation) is tested.
129-142: Confirm teardown logic for consistent test isolation.
You’re deleting the users, organizations, and profiles, then disconnecting. This helps keep tests independent. If you notice any leftover documents or relational data that persists between tests (like FundraisingCampaignPledge references), consider explicitly clearing them as well.
Would you like me to open a new GitHub issue proposing a DB cleanup helper that runs after all test suites for an extra safety net?
154-161: Duplicate Unauthorized Error checks.
You have two test blocks verifying the same unauthorized scenario:
- Line 154: "throws unauthorized error when trying to access another user's data"
- Line 242: "should throw UnauthorizedError when trying to access another user's data"
While they might cover slightly different code paths, consider merging them or ensuring each test covers a unique aspect to avoid redundancy.
Also applies to: 242-251
src/libraries/errors/notFoundError.ts (1)
8-10: New properties enable richer error context.
Defining code and param as string | null is helpful for providing more detailed error information. Just ensure they are consistently documented in your error-handling guidelines and used throughout the codebase where relevant.
src/resolvers/Query/user.ts (3)
4-4: Good addition of Organization import.
Properly including Organization is crucial for verifying membership and admin status. Confirm that all references to membership roles in other parts of the code are consistent with how they’re defined in your data models.
18-22: Efficient existence check.
Using User.exists to confirm the current user’s presence is concise. Just be mindful that User.exists() returns a truthy/falsy result for existence rather than the user object. This helps performance but be sure it’s correct if you need any user fields later.
49-53: Lean usage for performance is recommended.
Invoking User.findById(...).lean() is a good practice, returning a plain JS object instead of a Mongoose document. Keep it consistent in the rest of your queries for performance gains.
What kind of change does this PR introduce?
Implement user access control for querying user data.
Issue Number: #2214
Did you add tests for your changes?
Yes
Snapshots/Videos:
If relevant, did you update the documentation?
No
Summary
Does this PR introduce a breaking change?
No
Summary by CodeRabbit
Security Enhancements
Error Handling
NotFoundErrorwith additional properties for detailed error informationTesting