Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions apps/backend/lambdas/users/handler.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda';
import db from './db'
import { authenticateRequest, checkAuthorization, AuthContext } from './auth';
import { UserValidationUtils } from './validation-utils';

function requireAuth(authContext: AuthContext, level: Parameters<typeof checkAuthorization>[1], resourceUserId?: number | string): APIGatewayProxyResult | undefined {
const authCheck = checkAuthorization(authContext, level, resourceUserId);
Expand Down Expand Up @@ -124,15 +125,31 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
let user = await db.selectFrom("branch.users").where("user_id", "=", Number(userId)).selectAll().executeTakeFirst();
if (!user) return json(404, { message: 'User not found' });

// vars to update
let email = body.email as string;
let name = body.name as string;
let isAdmin = body.isAdmin as boolean;
let profileImage = body.profileImage as string | undefined;
const updates: { email?: string; name?: string; is_admin?: boolean; profile_image?: string } = {};

const emailResult = UserValidationUtils.validateEmail(body.email);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much a nit but I wonder if we should just throw the whole body into the validation utils and let it handle all these conditionals 👀 I don't feel super strongly about this just an idea

if (!emailResult.isValid) return json(400, { message: emailResult.error });
if (emailResult.value != null) updates.email = emailResult.value;

const nameResult = UserValidationUtils.validateName(body.name);
if (!nameResult.isValid) return json(400, { message: nameResult.error });
if (nameResult.value != null) updates.name = nameResult.value;

const isAdminResult = UserValidationUtils.validateIsAdmin(body.isAdmin);
if (!isAdminResult.isValid) return json(400, { message: isAdminResult.error });
if (isAdminResult.value != null) updates.is_admin = isAdminResult.value;

const profileImageResult = UserValidationUtils.validateProfileImage(body.profileImage);
if (!profileImageResult.isValid) return json(400, { message: profileImageResult.error });
if (profileImageResult.value != null) updates.profile_image = profileImageResult.value;

if (Object.keys(updates).length === 0) {
return json(400, { message: 'No valid fields provided to update' });
}

// update
await db.updateTable('branch.users')
.set({ email, name, is_admin: isAdmin, profile_image: profileImage })
.set(updates)
.where('user_id', '=', Number(userId))
.execute();

Expand Down Expand Up @@ -168,14 +185,28 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
? (JSON.parse(event.body) as Record<string, unknown>)
: {};

// extract fields to create user
let email = body.email as string;
let name = body.name as string;
let isAdmin = body.isAdmin as boolean;
if (!email || !name || typeof isAdmin !== 'boolean') {
// email, name, and isAdmin are required on create
if (!body.email || !body.name || body.isAdmin === undefined || body.isAdmin === null) {
return json(400, { message: 'email, name, and isAdmin are required' });
}
let profile_image = body.profileImage as string;

// validate the type/format of each field
const emailResult = UserValidationUtils.validateEmail(body.email);
if (!emailResult.isValid) return json(400, { message: emailResult.error });

const nameResult = UserValidationUtils.validateName(body.name);
if (!nameResult.isValid) return json(400, { message: nameResult.error });

const isAdminResult = UserValidationUtils.validateIsAdmin(body.isAdmin);
if (!isAdminResult.isValid) return json(400, { message: isAdminResult.error });

const profileImageResult = UserValidationUtils.validateProfileImage(body.profileImage);
if (!profileImageResult.isValid) return json(400, { message: profileImageResult.error });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something obvious but do isAdmin and profile_image also need to be checked against the validation utils here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added checking isAdmin against validation utils!

const email = emailResult.value as string;
const name = nameResult.value as string;
const isAdmin = isAdminResult.value as boolean;
const profile_image = profileImageResult.value ?? undefined;

// Check if user with this email already exists
const existingUser = await db
Expand Down
202 changes: 200 additions & 2 deletions apps/backend/lambdas/users/test/user.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,45 @@ function postEvent(body: Record<string, unknown>) {
};
}

// Helper to create a PATCH event for /{userId}
function patchEvent(userId: string | number, body: unknown) {
return {
rawPath: `/${userId}`,
requestContext: {
http: {
method: 'PATCH',
},
},
body: typeof body === 'string' ? body : JSON.stringify(body),
};
}

function mockExistingUserForPatch(updated?: Record<string, unknown>) {
const existing = {
user_id: 1,
name: 'Existing User',
email: 'existing@example.com',
is_admin: false,
profile_image: null,
};

mockDb.selectFrom.mockReturnValue({
where: jest.fn().mockReturnValue({
selectAll: jest.fn().mockReturnValue({
executeTakeFirst: (jest.fn() as any).mockResolvedValue(updated ?? existing),
}),
}),
});

mockDb.updateTable.mockReturnValue({
set: jest.fn().mockReturnValue({
where: jest.fn().mockReturnValue({
execute: (jest.fn() as any).mockResolvedValue(undefined),
}),
}),
});
}

function mockAdminAuth() {
mockAuthenticateRequest.mockResolvedValue({
isAuthenticated: true,
Expand Down Expand Up @@ -232,7 +271,7 @@ describe('POST /users unit tests', () => {
expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('required');
expect(json.message).toContain('isAdmin');
});

test('400: empty email field', async () => {
Expand Down Expand Up @@ -262,6 +301,21 @@ describe('POST /users unit tests', () => {
const json = JSON.parse(res.body);
expect(json.message).toContain('required');
});

test('400: name is not a string', async () => {
const res = await handler(
postEvent({
name: 42,
email: 'john@example.com',
isAdmin: false,
})
);

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('name');
});
});

describe('Response Format', () => {
Expand Down Expand Up @@ -307,7 +361,7 @@ describe('POST /users unit tests', () => {
expect(json).toHaveProperty('body');
});

test('409: returns 409 when user already exists', async () => {
test('409: POST returns 409 when user already exists', async () => {
// Mock: user already exists
const whereChain = {
selectAll: jest.fn().mockReturnValue({
Expand Down Expand Up @@ -338,3 +392,147 @@ describe('POST /users unit tests', () => {
});
});
});

describe('PATCH /users/{userId} unit tests', () => {
beforeEach(() => {
jest.clearAllMocks();
mockAdminAuth();
});

describe('Input Validation', () => {
test('400: invalid email format', async () => {
mockExistingUserForPatch();

const res = await handler(patchEvent(1, { email: 'not-an-email' }));

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('email');
});

test('400: email is not a string', async () => {
mockExistingUserForPatch();

const res = await handler(patchEvent(1, { email: 123 }));

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('email');
});

test('400: name is not a string', async () => {
mockExistingUserForPatch();

const res = await handler(patchEvent(1, { name: 42 }));

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('name');
});

test('400: empty name field', async () => {
mockExistingUserForPatch();

const res = await handler(patchEvent(1, { name: ' ' }));

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('name');
});

test('400: isAdmin is not a boolean', async () => {
mockExistingUserForPatch();

const res = await handler(patchEvent(1, { isAdmin: 'yes' }));

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('isAdmin');
});

test('400: profileImage is not a string', async () => {
mockExistingUserForPatch();

const res = await handler(patchEvent(1, { profileImage: 5 }));

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('profileImage');
});

test('400: no valid fields provided', async () => {
mockExistingUserForPatch();

const res = await handler(patchEvent(1, {}));

expect(res.statusCode).toBe(400);
const json = JSON.parse(res.body);
expect(json.message).toBeDefined();
expect(json.message).toContain('No valid fields');
});

});

describe('Success Cases', () => {
test('404: returns 404 when user does not exist', async () => {
mockDb.selectFrom.mockReturnValue({
where: jest.fn().mockReturnValue({
selectAll: jest.fn().mockReturnValue({
executeTakeFirst: (jest.fn() as any).mockResolvedValue(null),
}),
}),
});

const res = await handler(patchEvent(999, { name: 'Whoever' }));

expect(res.statusCode).toBe(404);
const json = JSON.parse(res.body);
expect(json).toHaveProperty('message');
});

test('200: valid partial update returns updated fields', async () => {
mockExistingUserForPatch({
user_id: 1,
name: 'Existing User',
email: 'new@example.com',
is_admin: true,
profile_image: null,
});

const res = await handler(patchEvent(1, { email: 'new@example.com', isAdmin: true }));

expect(res.statusCode).toBe(200);
const json = JSON.parse(res.body);
expect(json).toHaveProperty('ok');
expect(json).toHaveProperty('route');
expect(json).toHaveProperty('body');
expect(json.body.email).toBe('new@example.com');
expect(json.body.isAdmin).toBe(true);
});

test('200: only the provided field is written to the database', async () => {
mockExistingUserForPatch({
user_id: 1,
name: 'New Name',
email: 'existing@example.com',
is_admin: false,
profile_image: null,
});

const res = await handler(patchEvent(1, { name: 'New Name' }));

expect(res.statusCode).toBe(200);
// Only the provided field should be passed to .set(). toStrictEqual (unlike
// toEqual) does NOT ignore undefined keys, so this fails if the handler ever
// regresses to setting every column and leaving omitted ones undefined.
const setCall = (mockDb.updateTable.mock.results[0].value.set as jest.Mock).mock.calls[0][0];
expect(setCall).toStrictEqual({ name: 'New Name' });
});
});
});
58 changes: 58 additions & 0 deletions apps/backend/lambdas/users/validation-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Result type for input validation operations
export type ValidationResult<T> = {
isValid: boolean;
value?: T;
error?: string;
};

// Utility class for validating user-related input fields.
export class UserValidationUtils {
static readonly EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;

// Validates email - if provided, must be a correctly formatted string
static validateEmail(input: unknown): ValidationResult<string | null> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think email, name, isAdmin should be required. profile image optional makes sense

if (input === undefined || input === null || input === '') {
return { isValid: true, value: null };
}
if (typeof input !== 'string') {
return { isValid: false, error: 'email must be a string' };
}
if (!this.EMAIL_REGEX.test(input)) {
return { isValid: false, error: 'Invalid email format' };
}
return { isValid: true, value: input };
}

// Validates name - if provided, must be a non-empty string
static validateName(input: unknown): ValidationResult<string | null> {
if (input === undefined || input === null) {
return { isValid: true, value: null };
}
if (typeof input !== 'string' || input.trim().length === 0) {
return { isValid: false, error: 'name must be a non-empty string' };
}
return { isValid: true, value: input };
}

// Validates isAdmin - if provided, must be a boolean
static validateIsAdmin(input: unknown): ValidationResult<boolean | null> {
if (input === undefined || input === null) {
return { isValid: true, value: null };
}
if (typeof input !== 'boolean') {
return { isValid: false, error: 'isAdmin must be a boolean' };
}
return { isValid: true, value: input };
}

// Validates profileImage - if provided, must be a string
static validateProfileImage(input: unknown): ValidationResult<string | null> {
if (input === undefined || input === null) {
return { isValid: true, value: null };
}
if (typeof input !== 'string') {
return { isValid: false, error: 'profileImage must be a string' };
}
return { isValid: true, value: input };
}
}
Loading