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 2 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
59 changes: 37 additions & 22 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.userId = userCreateDto?.userId),
(user.username = userCreateDto?.username),
(user.firstName = userCreateDto?.firstName),
(user.middleName = userCreateDto?.middleName),
(user.lastName = userCreateDto?.lastName),
(user.gender = userCreateDto?.gender),
Sourav-Tekdi marked this conversation as resolved.
Show resolved Hide resolved
(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.createdBy = userCreateDto?.createdBy || userCreateDto?.createdBy);

if (userCreateDto?.dob) {
user.dob = new Date(userCreateDto.dob);
Expand Down
16 changes: 4 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,14 @@ 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,
enabled: "true",
username: query.username,
// groups: [getUserGroup(query.role)],
Expand All @@ -91,7 +83,7 @@ async function createUserInKeyCloak(query, token) {
Authorization: "Bearer " + token,
},
data: data,
};
};
let userResponse;
// try {
// userResponse = await axios(config);
Expand Down
34 changes: 32 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,37 @@ 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;

@Expose()
name:string;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove redundant 'name' field.

The name field is redundant since we now have separate fields for firstName, middleName, and lastName.

-  @Expose()
-  name:string;
🧰 Tools
🪛 eslint

[error] 95-95: Insert ·

(prettier/prettier)


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

Choose a reason for hiding this comment

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

💡 Codebase verification

Breaking Change: Comprehensive Updates Required for Name Field Restructuring

The name field restructuring requires coordinated changes across multiple system components:

  • Database queries in user-adapter.ts need updating to use the new name fields
  • Authentication payload mapping in keycloak.strategy.ts must be adjusted
  • Email template system uses the current name field for notifications
  • JOIN operations and result mapping in database queries need modification
🔗 Analysis chain

Verify field changes across the application.

The introduction of new name fields and gender requires verification across the application to ensure consistency.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining references to the old 'name' field
echo "Checking for old 'name' field usage..."
rg -l "\.name\b" --type ts

# Check for consistency in field usage
echo "Checking field usage consistency..."
ast-grep --pattern 'firstName: string' --lang ts
ast-grep --pattern 'lastName: string' --lang ts
ast-grep --pattern 'middleName: string' --lang ts
ast-grep --pattern 'gender: ' --lang ts

Length of output: 845

Script:

#!/bin/bash
# Check the context of name usage in key files
echo "Checking name usage in user adapter..."
rg "\.name" -B 2 -A 2 src/adapters/postgres/user-adapter.ts

echo "\nChecking name usage in keycloak strategy..."
rg "\.name" -B 2 -A 2 src/common/guards/keycloak.strategy.ts

echo "\nChecking user-related interfaces and types..."
fd -e ts -e js --type f -X rg "interface.*User|type.*User" -A 5

Length of output: 5010

🧰 Tools
🪛 eslint

[error] 75-75: Replace ·type:·String,·description:·'First·name·of·the·user',·maxLength:·50 with ⏎····type:·String,⏎····description:·"First·name·of·the·user",⏎····maxLength:·50,⏎·

(prettier/prettier)

[error] 81-81: Replace ·type:·String,·description:·'Middle·name·of·the·user·(optional)',·maxLength:·50,·required:·false with ⏎····type:·String,⏎····description:·"Middle·name·of·the·user·(optional)",⏎····maxLength:·50,⏎····required:·false,⏎·

(prettier/prettier)

[error] 88-88: Replace ·type:·String,·description:·'Last·name·of·the·user',·maxLength:·50 with ⏎····type:·String,⏎····description:·"Last·name·of·the·user",⏎····maxLength:·50,⏎·

(prettier/prettier)

[error] 95-95: Insert ·

(prettier/prettier)

[error] 96-96: Delete ··

(prettier/prettier)

[error] 97-97: Delete ·

(prettier/prettier)

[error] 98-98: Delete ·

(prettier/prettier)

[error] 99-99: Replace 'Gender·of·the·user',· with "Gender·of·the·user",

(prettier/prettier)

[error] 100-100: Replace 'male',·'female',·'transgender']· with "male",·"female",·"transgender"],

(prettier/prettier)

[error] 103-103: Replace 'male',·'female',·'transgender' with "male",·"female",·"transgender"

(prettier/prettier)



@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