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.

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

feat(auth): implement Secure Remote Password (SRP) authentication #386

Merged
merged 7 commits into from
Feb 22, 2025
Merged
Show file tree
Hide file tree
Changes from 6 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
2 changes: 0 additions & 2 deletions .github/workflows/test-coverage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ jobs:
postgresql-version: [latest]
elasticsearch-version: [8.12.2]
node-version: [18.x]
pnpm-version: [8]

env:
TEST_REPEAT: 1 # Tests will be repeated
Expand Down Expand Up @@ -101,7 +100,6 @@ jobs:
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: ${{ matrix.pnpm-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

run_install: false

- name: Install Dependencies
Expand Down
2 changes: 0 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ jobs:
postgresql-version: [latest, 12]
elasticsearch-version: [8.12.2, 8.0.0]
node-version: [18.x, 20.x]
pnpm-version: [8]
repeat: [1, 2]

env:
Expand Down Expand Up @@ -102,7 +101,6 @@ jobs:
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: ${{ matrix.pnpm-version }}
run_install: false

- name: Install Dependencies
Expand Down
13 changes: 12 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"qrcode": "^1.5.4",
"reflect-metadata": "^0.2.2",
"rxjs": "^7.8.1",
"secure-remote-password": "^0.3.1",
"uuid": "^10.0.0"
},
"devDependencies": {
Expand Down Expand Up @@ -102,5 +103,15 @@
"ts-node": "^10.9.2",
"tsconfig-paths": "^4.2.0",
"typescript": "^5.7.2"
}
},
"pnpm": {
"onlyBuiltDependencies": [
"@nestjs/core",
"@prisma/client",
"@prisma/engines",
"bignum",
"prisma"
]
},
"packageManager": "pnpm@10.4.1+sha512.c753b6c3ad7afa13af388fa6d808035a008e30ea9993f58c6663e2bc5ff21679aa834db094987129aa4d488b86df57f7b634981b2f827cdcacc698cc0cfb88af"
}
53 changes: 53 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions prisma/migrations/20250219084632_add_srp_fields/migration.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- AlterTable
ALTER TABLE "user" ADD COLUMN "srp_salt" VARCHAR(500),
ADD COLUMN "srp_upgraded" BOOLEAN NOT NULL DEFAULT false,
ADD COLUMN "srp_verifier" VARCHAR(1000);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- AlterTable
ALTER TABLE "user" ALTER COLUMN "hashed_password" DROP NOT NULL;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- AlterTable
ALTER TABLE "user" ADD COLUMN "last_password_changed_at" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP;
6 changes: 5 additions & 1 deletion prisma/schema.prisma
Original file line number Diff line number Diff line change
Expand Up @@ -611,11 +611,15 @@ model TopicSearchLog {
model User {
id Int @id(map: "PK_cace4a159ff9f2512dd42373760") @default(autoincrement())
username String @unique(map: "IDX_78a916df40e02a9deb1c4b75ed") @db.VarChar
hashedPassword String @map("hashed_password") @db.VarChar
hashedPassword String? @map("hashed_password") @db.VarChar
srpSalt String? @map("srp_salt") @db.VarChar(500)
srpVerifier String? @map("srp_verifier") @db.VarChar(1000)
srpUpgraded Boolean @default(false) @map("srp_upgraded")
email String @unique(map: "IDX_e12875dfb3b1d92d7d7c5377e2") @db.VarChar
createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6)
updatedAt DateTime @default(now()) @updatedAt @map("updated_at") @db.Timestamptz(6)
deletedAt DateTime? @map("deleted_at") @db.Timestamptz(6)
lastPasswordChangedAt DateTime @default(now()) @map("last_password_changed_at") @db.Timestamptz(6)
answer Answer[]
answerDeleteLog AnswerDeleteLog[]
answerFavoritedByUser AnswerFavoritedByUser[]
Expand Down
12 changes: 12 additions & 0 deletions sample.env
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
APP_NAME=Cheese

# The port that the app will listen to
PORT=3000

Expand Down Expand Up @@ -70,3 +72,13 @@ EMAILTEST_RECEIVER=developer@example.com

# Use real ip or X-Forwarded-For header
TRUST_X_FORWARDED_FOR=false

REDIS_HOST=valkey
REDIS_PORT=6379
# REDIS_USERNAME=
# REDIS_PASSWORD=

WEB_AUTHN_RP_NAME=Cheese Community
WEB_AUTHN_RP_ID=localhost
WEB_AUTHN_ORIGIN=http://localhost:3000
TOTP_ENCRYPTION_KEY=sm2vSXU3SudEuBd2r6ewGiap1LbqGbjf
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 hardcoded TOTP encryption key.

The hardcoded encryption key in the sample environment file poses a security risk if accidentally used in production.

Replace the hardcoded key with a placeholder and add instructions for generating a secure key:

-TOTP_ENCRYPTION_KEY=sm2vSXU3SudEuBd2r6ewGiap1LbqGbjf
+# Generate a secure 32-byte key for production using:
+# node -e "console.log(require('crypto').randomBytes(32).toString('base64'))"
+TOTP_ENCRYPTION_KEY=your_secure_random_key_here
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
TOTP_ENCRYPTION_KEY=sm2vSXU3SudEuBd2r6ewGiap1LbqGbjf
# Generate a secure 32-byte key for production using:
# node -e "console.log(require('crypto').randomBytes(32).toString('base64'))"
TOTP_ENCRYPTION_KEY=your_secure_random_key_here

1 change: 1 addition & 0 deletions src/auth/definitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export interface Authorization {
userId: number;
permissions: Permission[];
sudoUntil?: number; // sudo 模式过期时间戳
username?: string; // 用户名,用于密码重置等场景
}

export class TokenPayload {
Expand Down
7 changes: 5 additions & 2 deletions src/auth/token-payload.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@
},
"type": "array"
},
"userId": {
"sudoUntil": {
"type": "number"
},
"sudoUntil": {
"userId": {
"type": "number"
},
"username": {
"type": "string"
}
},
"required": [
Expand Down
12 changes: 12 additions & 0 deletions src/users/DTO/change-password.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { IsString } from 'class-validator';
import { BaseResponseDto } from '../../common/DTO/base-response.dto';

export class ChangePasswordRequestDto {
@IsString()
srpSalt: string; // 新密码的 SRP 凭证

@IsString()
srpVerifier: string; // 新密码的 SRP 凭证
}

export class ChangePasswordResponseDto extends BaseResponseDto {}
4 changes: 4 additions & 0 deletions src/users/DTO/passkey.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ export class PasskeyRegistrationVerifyRequestDto {
export class PasskeyRegistrationVerifyResponseDto extends BaseResponseDto {}

// Authentication DTOs
export class PasskeyAuthenticationOptionsRequestDto {
userId?: number;
}
Comment on lines +24 to +26
Copy link

@coderabbitai coderabbitai bot Feb 21, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation decorator for optional number field.

The userId field should be validated as an optional number:

 export class PasskeyAuthenticationOptionsRequestDto {
+  @IsNumber()
+  @IsOptional()
   userId?: number;
 }

Import the decorator:

+ import { IsNumber, IsOptional } from 'class-validator';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export class PasskeyAuthenticationOptionsRequestDto {
userId?: number;
}
import { IsNumber, IsOptional } from 'class-validator';
export class PasskeyAuthenticationOptionsRequestDto {
@IsNumber()
@IsOptional()
userId?: number;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@HuanCheng65 See this.

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


export class PasskeyAuthenticationOptionsResponseDto extends BaseResponseDto {
data: {
options: PublicKeyCredentialRequestOptionsJSON;
Expand Down
19 changes: 17 additions & 2 deletions src/users/DTO/register.dto.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { IsString } from 'class-validator';
import { IsBoolean, IsOptional, IsString } from 'class-validator';
import { LoginResponseDto } from './login.dto';

export class RegisterRequestDto {
Expand All @@ -9,13 +9,28 @@ export class RegisterRequestDto {
nickname: string;

@IsString()
password: string;
@IsOptional()
srpSalt?: string;

@IsString()
@IsOptional()
srpVerifier?: string;

@IsString()
email: string;

@IsString()
emailCode: string;

// 可选的传统密码字段,仅用于测试
@IsString()
@IsOptional()
password?: string;

// 是否使用传统认证方式,仅用于测试
@IsBoolean()
@IsOptional()
isLegacyAuth?: boolean;
}

export class RegisterResponseDto extends LoginResponseDto {}
5 changes: 4 additions & 1 deletion src/users/DTO/reset-password.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ export class ResetPasswordVerifyRequestDto {
token: string;

@IsString()
new_password: string;
srpSalt: string;

@IsString()
srpVerifier: string;
}

export class ResetPasswordVerifyResponseDto extends BaseResponseDto {}
33 changes: 33 additions & 0 deletions src/users/DTO/srp.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Description: This file defines the DTOs for SRP authentication.
*/

import { BaseResponseDto } from '../../common/DTO/base-response.dto';
import { UserDto } from '../../users/DTO/user.dto';

export class SrpInitRequestDto {
username: string;
clientPublicEphemeral: string;
}
Comment on lines +8 to +11
Copy link

@coderabbitai coderabbitai bot Feb 21, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation decorators for required fields.

The fields are required but lack validation decorators. Consider adding @IsString() decorators:

 export class SrpInitRequestDto {
+  @IsString()
   username: string;
+  @IsString()
   clientPublicEphemeral: string;
 }

Import the decorator:

+ import { IsString } from 'class-validator';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export class SrpInitRequestDto {
username: string;
clientPublicEphemeral: string;
}
import { IsString } from 'class-validator';
export class SrpInitRequestDto {
@IsString()
username: string;
@IsString()
clientPublicEphemeral: string;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this.

Copy link

Choose a reason for hiding this comment

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

@andylizf Got it! I'll mark this suggestion as accepted.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


export class SrpInitResponseDto extends BaseResponseDto {
data: {
salt: string;
serverPublicEphemeral: string;
};
}

export class SrpVerifyRequestDto {
username: string;
clientProof: string;
}
Comment on lines +20 to +23
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation decorators for required fields.

The fields are required but lack validation decorators. Consider adding @IsString() decorators:

 export class SrpVerifyRequestDto {
+  @IsString()
   username: string;
+  @IsString()
   clientProof: string;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export class SrpVerifyRequestDto {
username: string;
clientProof: string;
}
export class SrpVerifyRequestDto {
@IsString()
username: string;
@IsString()
clientProof: string;
}


export class SrpVerifyResponseDto extends BaseResponseDto {
data: {
serverProof: string;
accessToken: string;
requires2FA: boolean;
tempToken?: string;
user?: UserDto;
};
}
Loading
Loading