From 3a1cd1e892a3683a174b90384f19ff4ed9c23a93 Mon Sep 17 00:00:00 2001 From: NISHANT SINGH <151461374+NishantSinghhhhh@users.noreply.github.com> Date: Tue, 31 Dec 2024 10:17:45 +0530 Subject: [PATCH] Restrict user data access in User query (#2797) * Restrict user data access in User query Signed-off-by: NishantSinghhhhh * fixes Signed-off-by: NishantSinghhhhh * fixes Signed-off-by: NishantSinghhhhh * fixes for tests Signed-off-by: NishantSinghhhhh * Rename node-version to .node-version * Update users.ts * fixes for tests Signed-off-by: NishantSinghhhhh * fixes for failing tests Signed-off-by: NishantSinghhhhh * full test coverage Signed-off-by: NishantSinghhhhh * full test coverage Signed-off-by: NishantSinghhhhh * fixes Signed-off-by: NishantSinghhhhh * fixes Signed-off-by: NishantSinghhhhh * removing unnessary changes Signed-off-by: NishantSinghhhhh * adding tests Signed-off-by: NishantSinghhhhh * code-Rabbit's changes Signed-off-by: NishantSinghhhhh * rabbit's changes Signed-off-by: NishantSinghhhhh * adding tests Signed-off-by: NishantSinghhhhh * fixes Signed-off-by: NishantSinghhhhh * removed tests and added tests with full coverage Signed-off-by: NishantSinghhhhh * linting users Signed-off-by: NishantSinghhhhh --------- Signed-off-by: NishantSinghhhhh --- src/libraries/errors/notFoundError.ts | 6 + src/resolvers/Query/user.ts | 43 +++- tests/resolvers/User/userAccess.spec.ts | 252 ++++++++++++++++++++++++ 3 files changed, 296 insertions(+), 5 deletions(-) create mode 100644 tests/resolvers/User/userAccess.spec.ts diff --git a/src/libraries/errors/notFoundError.ts b/src/libraries/errors/notFoundError.ts index 68fbb0b4b06..885a1482bd9 100644 --- a/src/libraries/errors/notFoundError.ts +++ b/src/libraries/errors/notFoundError.ts @@ -5,6 +5,9 @@ import { ApplicationError } from "./applicationError"; * and is used to handle situations where a requested resource is not found. */ export class NotFoundError extends ApplicationError { + code: string | null; + param: string | null; + /** * Creates an instance of NotFoundError. * @param message - The error message. Defaults to "Not Found". @@ -28,5 +31,8 @@ export class NotFoundError extends ApplicationError { }, ]; super(errorJson, 404, message); + + this.code = code; // Ensure the code is directly accessible + this.param = param; // Ensure the param is directly accessible } } diff --git a/src/resolvers/Query/user.ts b/src/resolvers/Query/user.ts index dc6233a0116..8cc35a6dda6 100644 --- a/src/resolvers/Query/user.ts +++ b/src/resolvers/Query/user.ts @@ -1,21 +1,57 @@ import { USER_NOT_FOUND_ERROR } from "../../constants"; import { errors } from "../../libraries"; import type { InterfaceAppUserProfile, InterfaceUser } from "../../models"; -import { AppUserProfile, User } from "../../models"; +import { AppUserProfile, User, Organization } from "../../models"; import type { QueryResolvers } from "../../types/generatedGraphQLTypes"; /** * This query fetch the user from the database. + * + * This function ensure that users can only query their own data and not access details of other users , protecting sensitive data. + * * @param _parent- * @param args - An object that contains `id` for the user. * @param context- * @returns An object that contains user data. If the user is not found then it throws a `NotFoundError` error. */ + export const user: QueryResolvers["user"] = async (_parent, args, context) => { + // Check if the current user exists in the system const currentUserExists = !!(await User.exists({ _id: context.userId, })); - if (currentUserExists === false) { + if (!currentUserExists) { + throw new errors.NotFoundError( + USER_NOT_FOUND_ERROR.DESC, + USER_NOT_FOUND_ERROR.CODE, + USER_NOT_FOUND_ERROR.PARAM, + ); + } + + const [userOrganization, superAdminProfile] = await Promise.all([ + Organization.exists({ + members: args.id, + admins: context.userId, + }), + AppUserProfile.exists({ + userId: context.userId, + isSuperAdmin: true, + }), + ]); + + if (!userOrganization && context.userId !== args.id && !superAdminProfile) { + throw new errors.UnauthorizedError( + "Access denied. Only the user themselves, organization admins, or super admins can view this profile.", + ); + } + + // Fetch the user data from the database based on the provided ID (args.id) + // 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, @@ -23,9 +59,6 @@ export const user: QueryResolvers["user"] = async (_parent, args, context) => { ); } - const user: InterfaceUser = (await User.findOne({ - _id: args.id, - }).lean()) as InterfaceUser; const userAppProfile: InterfaceAppUserProfile = (await AppUserProfile.findOne( { userId: user._id, diff --git a/tests/resolvers/User/userAccess.spec.ts b/tests/resolvers/User/userAccess.spec.ts new file mode 100644 index 00000000000..20b07086dbd --- /dev/null +++ b/tests/resolvers/User/userAccess.spec.ts @@ -0,0 +1,252 @@ +import { USER_NOT_FOUND_ERROR, BASE_URL } from "../../../src/constants"; +import { user as userResolver } from "../../../src/resolvers/Query/user"; +import { User, Organization, AppUserProfile } from "../../../src/models"; +import { connect, disconnect } from "../../helpers/db"; +import type { TestUserType } from "../../helpers/userAndOrg"; +import { createTestUser } from "../../helpers/userAndOrg"; +import { beforeAll, afterAll, describe, it, expect, vi } from "vitest"; +import type mongoose from "mongoose"; +import { Types } from "mongoose"; +import { FundraisingCampaignPledge } from "../../../src/models/FundraisingCampaignPledge"; +import { errors } from "../../../src/libraries"; + +// Mock FundraisingCampaignPledge populate +vi.mock("../../../src/models/FundraisingCampaignPledge", () => ({ + FundraisingCampaignPledge: { + schema: { + obj: {}, + paths: {}, + tree: {}, + virtuals: {}, + methods: {}, + statics: {}, + }, + }, +})); + +type AdditionalUserFields = { + createdAt?: Date; + updatedAt?: Date; + isAdmin?: boolean; + isSuperAdmin?: boolean; + blocked?: boolean; + role?: string; + userType?: string; + appLanguageCode?: string; + pluginCreationAllowed?: boolean; + adminApproved?: boolean; + adminFor?: mongoose.Types.ObjectId[]; + memberOf?: mongoose.Types.ObjectId[]; + createdOrganizations?: mongoose.Types.ObjectId[]; + joinedOrganizations?: mongoose.Types.ObjectId[]; + registeredEvents?: mongoose.Types.ObjectId[]; + eventAdmin?: mongoose.Types.ObjectId[]; + createdEvents?: mongoose.Types.ObjectId[]; + tokenVersion?: number; +}; + +type UserType = { + _id: mongoose.Types.ObjectId; + email: string; + firstName?: string; + lastName?: string; + image?: string | null; + organizationsBlockedBy: string[]; +} & AdditionalUserFields; + +type ResolverReturnType = { + user: UserType; +}; + +// Rename `ITestUsers` to `TestInterfaceUsers` +interface TestInterfaceUsers { + testUser: NonNullable; + anotherTestUser: NonNullable; + adminUser: NonNullable; + superAdminUser: NonNullable; +} + +let users: TestInterfaceUsers; +let MONGOOSE_INSTANCE: typeof mongoose; + +beforeAll(async () => { + try { + MONGOOSE_INSTANCE = await connect(); + + // Register FundraisingCampaignPledge schema mock + if (!MONGOOSE_INSTANCE.models.FundraisingCampaignPledge) { + MONGOOSE_INSTANCE.model( + "FundraisingCampaignPledge", + FundraisingCampaignPledge.schema, + ); + } + + // Create users sequentially + const [testUser, anotherTestUser, adminUser, superAdminUser] = + await Promise.all([ + createTestUser(), + createTestUser(), + createTestUser(), + createTestUser(), + ]); + + // Verify all users were created + if (!testUser || !anotherTestUser || !adminUser || !superAdminUser) { + throw new Error("Failed to create test users"); + } + + users = { testUser, anotherTestUser, adminUser, superAdminUser }; + + // Create organization + const org = await Organization.create({ + creatorId: users.adminUser._id, + members: [users.anotherTestUser._id], + admins: [users.adminUser._id], + name: "Test Organization", + description: "Test organization", + }); + + if (!org) { + throw new Error("Failed to create organization"); + } + + // Update super admin profile + const superAdminUpdate = await AppUserProfile.findOneAndUpdate( + { userId: users.superAdminUser._id }, + { isSuperAdmin: true }, + { new: true }, + ); + + if (!superAdminUpdate) { + throw new Error("Failed to update super admin profile"); + } + } catch (error) { + console.error("Setup failed:", error); + throw error; + } +}, 30000); + +afterAll(async () => { + if (users) { + await Promise.all([ + User.deleteMany({ + _id: { $in: Object.values(users).map((user) => user._id) }, + }), + Organization.deleteMany({}), + AppUserProfile.deleteMany({ + userId: { $in: Object.values(users).map((user) => user._id) }, + }), + ]); + } + await disconnect(MONGOOSE_INSTANCE); +}, 30000); + +describe("user Query", () => { + it("throws error if user doesn't exist", async () => { + const args = { id: new Types.ObjectId().toString() }; + const context = { userId: new Types.ObjectId().toString() }; + + await expect(userResolver?.({}, args, context)).rejects.toThrow( + USER_NOT_FOUND_ERROR.DESC, + ); + }); + + it("throws unauthorized error 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.toThrow( + "Access denied. Only the user themselves, organization admins, or super admins can view this profile.", + ); + }); + + it("allows an admin to access another user's data within the same organization", async () => { + const args = { id: users.anotherTestUser._id.toString() }; + const context = { + userId: users.adminUser._id.toString(), + apiRootURL: BASE_URL, + }; + + const result = (await userResolver?.( + {}, + args, + context, + )) as ResolverReturnType; + const user = await User.findById(users.anotherTestUser._id).lean(); + + expect(result.user._id.toString()).toBe( + users.anotherTestUser._id.toString(), + ); + expect(result.user).toEqual({ + ...user, + organizationsBlockedBy: [], + image: user?.image ? `${BASE_URL}${user.image}` : null, + }); + }); + + it("allows a super admin to access any user's data", async () => { + const args = { id: users.anotherTestUser._id.toString() }; + const context = { + userId: users.superAdminUser._id.toString(), + apiRootURL: BASE_URL, + }; + + const result = (await userResolver?.( + {}, + args, + context, + )) as ResolverReturnType; + const user = await User.findById(users.anotherTestUser._id).lean(); + + expect(result.user).toEqual({ + ...user, + organizationsBlockedBy: [], + image: user?.image ? `${BASE_URL}${user.image}` : null, + }); + }); + + it("successfully returns user data when accessing own profile", async () => { + const args = { id: users.testUser._id.toString() }; + const context = { + userId: users.testUser._id.toString(), + apiRootURL: BASE_URL, + }; + + const result = (await userResolver?.( + {}, + args, + context, + )) as ResolverReturnType; + const user = await User.findById(users.testUser._id).lean(); + + expect(result.user).toEqual({ + ...user, + organizationsBlockedBy: [], + image: user?.image ? `${BASE_URL}${user.image}` : null, + }); + }); + + it("should throw NotFoundError with correct error details when requested user does not exist but current user exists", async () => { + const args = { id: new Types.ObjectId().toString() }; + const context = { userId: users.superAdminUser._id.toString() }; + + await expect(userResolver?.({}, args, context)).rejects.toThrowError( + new errors.NotFoundError( + USER_NOT_FOUND_ERROR.DESC, + USER_NOT_FOUND_ERROR.CODE, + USER_NOT_FOUND_ERROR.PARAM, + ), + ); + }); + + 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.", + ), + ); + }); +});