Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PS-3323: Adding New Fields from the backend #131

Merged
merged 6 commits into from
Jan 16, 2025
Merged

PS-3323: Adding New Fields from the backend #131

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e512b83
PS-3323: Adding New Fields from the backend
Sourav-Tekdi Jan 14, 2025
2bb4709
remove comented code
Sourav-Tekdi Jan 14, 2025
d8a04e7
removename field from dto
Sourav-Tekdi Jan 14, 2025
38bf4f9
coderabbitai comment resolved
Sourav-Tekdi Jan 15, 2025
ea303be
Add email on keycloak
Sourav-Tekdi Jan 15, 2025
3177f36
add
Sourav-Tekdi Jan 16, 2025
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
63 changes: 39 additions & 24 deletions src/adapters/postgres/user-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ export class PostgresUserService implements IServicelocator {
whereCondition += ` AND `;
}
if (userKeys.includes(key)) {
if (key === "name") {
if (key === "firstName") {
whereCondition += ` U."${key}" ILIKE '%${value}%'`;
} else {
Sourav-Tekdi marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +405 to 407
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

SQL injection vulnerabilities confirmed and expanded

String interpolation in SQL queries is present across multiple files, creating security vulnerabilities:

  • src/adapters/postgres/user-adapter.ts: Multiple instances in WHERE conditions using ILIKE and direct value injection
  • src/adapters/postgres/fields-adapter.ts: Direct value injection in field queries
  • src/adapters/postgres/cohortMembers-adapter.ts: ILIKE condition with string interpolation

Consider using parameterized queries consistently across all database adapters to prevent SQL injection attacks.

🔗 Analysis chain

Reconsider SQL injection prevention

While marked as "not required" in past reviews, using string interpolation for SQL queries remains a security risk. Consider using parameterized queries to prevent potential SQL injection attacks.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other instances of string interpolation in SQL queries
rg -A 2 "ILIKE '%\$\{.*\}%'" --type ts

Length of output: 45

Script:

#!/bin/bash
# Search for string concatenation in SQL queries using different patterns
echo "=== Searching for string concatenation in WHERE clauses ==="
rg "WHERE.*\+.*\$\{" --type ts

echo -e "\n=== Searching for template literals in SQL queries ==="
rg "SELECT.*\$\{.*\}" --type ts

echo -e "\n=== Searching for LIKE/ILIKE operations ==="
rg "(LIKE|ILIKE).*(\$\{|\+)" --type ts

echo -e "\n=== Searching for direct value interpolation in queries ==="
ast-grep --pattern 'query = $_ + "${$_}"'

Length of output: 640

Script:

#!/bin/bash
# Search for whereCondition concatenation
echo "=== Searching for whereCondition concatenation ==="
rg "whereCondition.*\+=" --type ts -A 2

echo -e "\n=== Searching for ILIKE with double quotes ==="
rg "ILIKE.*%.*%" --type ts

echo -e "\n=== Searching for string concatenation with SQL keywords ==="
rg "(\+=|\+).*\"(SELECT|WHERE|FROM|LIKE|ILIKE)" --type ts

Length of output: 2584

if (key === "status" || key === "email") {
Expand Down Expand Up @@ -498,7 +498,7 @@ export class PostgresUserService implements IServicelocator {
}

//Get user core fields data
const query = `SELECT U."userId", U."username",U."email", U."name", R."name" AS role, U."mobile", U."createdBy",U."updatedBy", U."createdAt", U."updatedAt", U.status, COUNT(*) OVER() AS total_count
const query = `SELECT U."userId", U."username",U."email", U."firstName", U."middleName", U."lastName", U."gender", U."dob", R."name" AS role, U."mobile", U."createdBy",U."updatedBy", U."createdAt", U."updatedAt", U.status, COUNT(*) OVER() AS total_count
FROM public."Users" U
LEFT JOIN public."CohortMembers" CM
ON CM."userId" = U."userId"
Expand Down Expand Up @@ -688,7 +688,9 @@ export class PostgresUserService implements IServicelocator {
select: [
"userId",
"username",
"name",
"firstName",
"middleName",
"lastName",
"mobile",
"email",
"temporaryPassword",
Expand Down Expand Up @@ -900,16 +902,30 @@ export class PostgresUserService implements IServicelocator {
return deviceIds;
}

async updateBasicUserDetails(userId, userData: Partial<User>): Promise<User> {
const user = await this.usersRepository.findOne({
where: { userId: userId },
});
if (!user) {
return null;
async updateBasicUserDetails(userId: string, userData: Partial<User>): Promise<User | null> {
try {
// Fetch the user by ID
const user = await this.usersRepository.findOne({ where: { userId } });

if (!user) {
// If the user is not found, return null
return null;
}

// Update the user's details
await this.usersRepository.update(userId, userData);

// Fetch and return the updated user
const updatedUser = await this.usersRepository.findOne({ where: { userId } });

return updatedUser;
} catch (error) {
// Re-throw or handle the error as needed
throw new Error('An error occurred while updating user details');
}
Object.assign(user, userData);
return this.usersRepository.save(user);
}



async createUser(
request: any,
Expand Down Expand Up @@ -987,7 +1003,7 @@ export class PostgresUserService implements IServicelocator {
HttpStatus.BAD_REQUEST
);
}

resKeycloak = await createUserInKeyCloak(userSchema, token).catch(
(error) => {
LoggerUtil.error(
Expand All @@ -1008,6 +1024,7 @@ export class PostgresUserService implements IServicelocator {
);

LoggerUtil.log(API_RESPONSES.USER_CREATE_KEYCLOAK, apiId);


userCreateDto.userId = resKeycloak;

Expand Down Expand Up @@ -1083,7 +1100,7 @@ export class PostgresUserService implements IServicelocator {
HttpStatus.CREATED,
API_RESPONSES.USER_CREATE_SUCCESSFULLY
);
} catch (e) {
} catch (e) {
LoggerUtil.error(
`${API_RESPONSES.SERVER_ERROR}: ${request.url}`,
`Error: ${e.message}`,
Expand Down Expand Up @@ -1294,17 +1311,15 @@ export class PostgresUserService implements IServicelocator {
response: Response
) {
const user = new User();
(user.username = userCreateDto?.username),
(user.name = userCreateDto?.name),
(user.email = userCreateDto?.email),
(user.mobile = Number(userCreateDto?.mobile) || null),
(user.createdBy = userCreateDto?.createdBy || userCreateDto?.userId),
(user.updatedBy = userCreateDto?.updatedBy || userCreateDto?.userId),
(user.userId = userCreateDto?.userId),
(user.state = userCreateDto?.state),
(user.district = userCreateDto?.district),
(user.address = userCreateDto?.address),
(user.pincode = userCreateDto?.pincode);
user.userId = userCreateDto?.userId,
user.username = userCreateDto?.username,
user.firstName = userCreateDto?.firstName,
user.middleName = userCreateDto?.middleName,
user.lastName = userCreateDto?.lastName,
user.gender = userCreateDto?.gender,
user.email = userCreateDto?.email,
user.mobile = Number(userCreateDto?.mobile) || null,
user.createdBy = userCreateDto?.createdBy || userCreateDto?.createdBy;

if (userCreateDto?.dob) {
user.dob = new Date(userCreateDto.dob);
Expand Down
17 changes: 5 additions & 12 deletions src/common/utils/keycloak.adapter.util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,22 +55,15 @@ async function getKeycloakAdminToken() {

async function createUserInKeyCloak(query, token) {
const axios = require("axios");
const name = query.name;
const nameParts = name.split(" ");
let lname = "";

if (nameParts[2]) {
lname = nameParts[2];
} else if (nameParts[1]) {
lname = nameParts[1];
}

if (!query.password) {
return "User cannot be created, Password missing";
}

const data = JSON.stringify({
firstName: nameParts[0],
lastName: lname,
firstName: query.firstName,
lastName: query.lastName,
email: query.email ? query.email : null,
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Email should not be set to null in Keycloak user creation

The codebase shows that email is a critical field used for user identification, password reset, and notifications. Setting it to null could break:

  • Password reset functionality
  • User notifications
  • OTP delivery for security features

Consider:

  1. Making email required in the user creation payload
  2. Using the existing email validation (@IsEmail()) from UserDto
  3. Removing the null fallback: email: query.email ? query.email : null
🔗 Analysis chain

Verify email handling in Keycloak integration.

Setting email to null might cause issues with Keycloak's user management. Consider:

  1. Making email required if it's mandatory in Keycloak
  2. Validating email format before sending to Keycloak
  3. Removing the email field if it's truly optional

Also, the commented-out groups assignment // groups: [getUserGroup(query.role)] suggests incomplete role management.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if email is required in other user-related files
rg -l "email.*required" --type ts

Length of output: 35

Script:

#!/bin/bash
# Search for Keycloak user-related configurations and types
ast-grep --pattern 'interface $_ {
  $$$
  email
  $$$
}'

# Search for email-related validations
rg "email" -B2 -A2 --type ts

# Look for Keycloak user creation patterns
rg "createUser|addUser|UserRepresentation" --type ts

Length of output: 22430

enabled: "true",
username: query.username,
// groups: [getUserGroup(query.role)],
Expand All @@ -91,7 +84,7 @@ async function createUserInKeyCloak(query, token) {
Authorization: "Bearer " + token,
},
data: data,
};
};
let userResponse;
// try {
// userResponse = await axios(config);
Expand Down
31 changes: 29 additions & 2 deletions src/user/dto/user-create.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
IsUUID,
ValidateNested,
IsOptional,
Length,
IsEnum,
} from "class-validator";
import { User } from "../entities/user-entity";
import { ApiProperty, ApiPropertyOptional } from "@nestjs/swagger";
Expand Down Expand Up @@ -70,9 +72,34 @@ export class UserCreateDto {
@IsNotEmpty()
username: string;

@ApiProperty({ type: () => String })
@ApiProperty({ type: String, description: 'First name of the user', maxLength: 50 })
@Expose()
@IsString()
@Length(1, 50)
firstName: string;

@ApiPropertyOptional({ type: String, description: 'Middle name of the user (optional)', maxLength: 50, required: false })
@Expose()
@IsOptional()
@IsString()
@Length(0, 50)
middleName?: string;

@ApiProperty({ type: String, description: 'Last name of the user', maxLength: 50 })
@Expose()
name: string;
@IsString()
@Length(1, 50)
lastName: string;

@ApiProperty({
type: String,
description: 'Gender of the user',
enum: ['male', 'female', 'transgender']
})
@Expose()
@IsEnum(['male', 'female', 'transgender'])
gender: string;


@ApiPropertyOptional({
type: String,
Expand Down
31 changes: 29 additions & 2 deletions src/user/dto/user-update.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
IsNotEmpty,
IsEnum,
ValidateIf,
Length,
} from "class-validator";
import { Expose, Type } from "class-transformer";
import { UserStatus } from "../entities/user-entity";
Expand All @@ -22,10 +23,36 @@ class UserDataDTO {
@IsOptional()
username: string;

@ApiProperty({ type: () => String })
@ApiProperty({ type: String, description: 'First name of the user', maxLength: 50 })
@Expose()
@IsOptional()
@IsString()
@Length(1, 50)
firstName?: string;

@ApiProperty({ type: String, description: 'Middle name of the user (optional)', maxLength: 50, required: false })
@Expose()
@IsOptional()
@IsString()
@Length(0, 50)
middleName?: string;

@ApiProperty({ type: String, description: 'Last name of the user', maxLength: 50 })
@Expose()
@IsOptional()
@IsString()
@Length(1, 50)
lastName?: string;

@ApiProperty({
type: String,
description: 'Gender of the user',
enum: ['male', 'female', 'transgender']
})
@Expose()
@IsEnum(['male', 'female', 'transgender'])
@IsOptional()
name: string;
gender?: string;

@ApiProperty({ type: () => String })
@IsString()
Expand Down
13 changes: 11 additions & 2 deletions src/user/entities/user-entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,17 @@ export class User {
@Column({ unique: true })
username: string;

@Column()
name: string;
@Column({ type: 'varchar', length: 50, nullable: false })
firstName: string;

@Column({ type: 'varchar', length: 50, nullable: true })
middleName: string;

@Column({ type: 'varchar', length: 50, nullable: false })
lastName: string;

@Column({ type: 'enum', enum: ['male', 'female', 'transgender'], nullable: false })
gender: string;

@Column({ type: "date", nullable: true })
dob: Date;
Expand Down