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

Add auth update endpoint #48

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
278ce7d
feat(schemas): add UpdateCredentialsSchema
Mathieuka Oct 28, 2024
dacae2b
feat(api): add update credentials endpoint
Mathieuka Oct 28, 2024
926db83
feat(schemas): add UpdateCredentialsSchema
Mathieuka Oct 29, 2024
571fa57
refactor(auth): remove update endpoint
Mathieuka Oct 29, 2024
c407db6
style(src): update users schema format
Mathieuka Oct 29, 2024
ac8f341
feat(api): add endpoint to update user password
Mathieuka Oct 29, 2024
2f98620
chore(routes): update tags in users route
Mathieuka Oct 29, 2024
58fbfb5
fix(api): remove transaction to simplify the code
Mathieuka Oct 29, 2024
0b68ee0
chore(api): rename user to singular
Mathieuka Oct 29, 2024
1dbd8f3
feat(api): add rate limiting to update-password route
Mathieuka Oct 30, 2024
ba0e63e
refactor: Implement early return if user or password is falsy
Mathieuka Oct 30, 2024
90ec6b7
fix(routes): prevent setting new password same as current password
Mathieuka Oct 30, 2024
4fc8904
chore(schemas): update password validation pattern
Mathieuka Oct 30, 2024
8919cf6
fix(api): improve error handling
Mathieuka Oct 30, 2024
fc7ee3d
refactor: password validation logic
Mathieuka Oct 30, 2024
a584645
fix(auth): fix password case to match the password pattern
Mathieuka Oct 31, 2024
b1bc64d
fix(schemas): update password pattern validation
Mathieuka Oct 31, 2024
9a6e66a
feat: validate current password before allowing password update
Mathieuka Oct 31, 2024
3af77a7
feat(api): add test cases for user password update
Mathieuka Oct 31, 2024
41d5a78
test: newPassword should match the required pattern
Mathieuka Oct 31, 2024
afe2f5a
test: refactor
Mathieuka Oct 31, 2024
50bf928
test: should update the password successfully
Mathieuka Oct 31, 2024
b719403
feat: remove comment
Mathieuka Oct 31, 2024
d55b31c
fix(api): update unauthorized response
Mathieuka Oct 31, 2024
3ebe20f
fix(user): update user.test.ts to include new test cases
Mathieuka Oct 31, 2024
1ef8c8a
test: isolate test by seeding users
Mathieuka Oct 31, 2024
e4cabd6
test: add unit test to verify rate limiting for password update requests
Mathieuka Nov 3, 2024
cb017ea
refactor(test): refactor user creation loop
Mathieuka Nov 3, 2024
0e68537
test: improve variable naming
Mathieuka Nov 3, 2024
259bbf7
fix(api): refactor user.test.ts
Mathieuka Nov 3, 2024
27ae3e2
refactor: rename helper function for updating password injection
Mathieuka Nov 3, 2024
dec5bc3
refactor: remove errorResponseBuilder from user route
Mathieuka Nov 6, 2024
213e4b8
refactor: simplify password pattern regex
Mathieuka Nov 6, 2024
486488a
refactor: rename Password to PasswordSchema
Mathieuka Nov 6, 2024
29cc091
test: use scryptHash
Mathieuka Nov 6, 2024
5ca3969
test: create and delete user at the test level
Mathieuka Nov 6, 2024
ffe5522
test: add abstraction to rate limiting testing
Mathieuka Nov 6, 2024
07fd4c2
chore: ajv-errors integration code example
Mathieuka Nov 9, 2024
f10522b
feat: Remove ajv-errors and related implementation
Mathieuka Nov 16, 2024
57da679
feat(routes): add rate limiting to home route
Mathieuka Nov 20, 2024
56d775b
refactor(api): remove redundant return statement
Mathieuka Nov 20, 2024
9326a67
refactor(api): rename user to users
Mathieuka Nov 20, 2024
5b456a6
refactor(routes): remove rate limiting configuration
Mathieuka Nov 21, 2024
6059bae
fix(test): update rate limit test to match new rate limit
Mathieuka Nov 21, 2024
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: 1 addition & 1 deletion scripts/seed-database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ async function truncateTables (connection: Connection) {

async function seedUsers (connection: Connection) {
const usernames = ['basic', 'moderator', 'admin']
const hash = await scryptHash('password123$')
const hash = await scryptHash('Password123$')

// The goal here is to create a role hierarchy
// E.g. an admin should have all the roles
Expand Down
74 changes: 74 additions & 0 deletions src/routes/api/users/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import {
FastifyPluginAsyncTypebox,
Type
} from '@fastify/type-provider-typebox'
import { Auth } from '../../../schemas/auth.js'
import { UpdateCredentialsSchema } from '../../../schemas/users.js'

const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
fastify.put(
'/update-password',
{
config: {
rateLimit: {
max: 3,
timeWindow: '1 minute'
}
},
schema: {
body: UpdateCredentialsSchema,
response: {
200: Type.Object({
message: Type.String()
}),
401: Type.Object({
message: Type.String()
})
},
tags: ['Users']
}
},
async function (request, reply) {
const { newPassword, currentPassword } = request.body
const username = request.session.user.username

try {
const user = await fastify.knex<Auth>('users')
.select('username', 'password')
.where({ username })
.first()

if (!user) {
return reply.code(401).send({ message: 'User does not exist.' })
}

const isPasswordValid = await fastify.compare(
currentPassword,
user.password
)

if (!isPasswordValid) {
return reply.code(401).send({ message: 'Invalid current password.' })
}

if (newPassword === currentPassword) {
reply.status(400)
return { message: 'New password cannot be the same as the current password.' }
}

const hashedPassword = await fastify.hash(newPassword)
await fastify.knex('users')
.update({
password: hashedPassword
})
.where({ username })

return { message: 'Password updated successfully' }
} catch (error) {
reply.internalServerError('An error occurred while updating the password.')
}
}
)
}

export default plugin
14 changes: 14 additions & 0 deletions src/schemas/users.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { Type } from '@sinclair/typebox'

const passwordPattern = '^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).*$'

const PasswordSchema = Type.String({
pattern: passwordPattern,
Copy link
Author

@Mathieuka Mathieuka Nov 6, 2024

Choose a reason for hiding this comment

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

@climba03003 @jean-michelet

Currently I don't think there is a way to customize the error message in case the payload doesn't match the pattern directly via the Type Builder Typebox.

I managed to do this via implementing fastify.setErrorHandler in the password update endpoint configuration, have you ever encountered this problem ?

Is it correct to manage the error message in a custom way at the endpoint ?

Copy link
Member

Choose a reason for hiding this comment

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

Is it correct to manage the error message in a custom way at the endpoint ?

You can using ajv-errors, so the message can be customize per schema.
https://www.npmjs.com/package/ajv-errors

Copy link
Author

@Mathieuka Mathieuka Nov 9, 2024

Choose a reason for hiding this comment

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

@climba03003

config:
locally Node v20.12.2
ajv-errors 3.0.0

I have installed and attempted to integrate ajv-errors with Fastify's AJV plugin, but it seems there is an issue when we add the errorMessage keyword in the TypeBox or AJV schema.

My research is based on the Fastify documentation

And I think an obsolete implementation example in the documentation, more than 3 years example

// Excerpt from the documentation
const fastify = Fastify({
  ajv: {
    customOptions: {
      jsonPointers: true,  // `jsonPointers` is not still part of `AjvOptions` in Fastify is replaced by `jsPropertySyntax` 
      allErrors: true
    },
    plugins: [
      require('ajv-errors')
    ]
  }
})

@jean-michelet If this is the case, I suggest creating an issue to update this part of documentation.

I have create a specific commit as an example of implementation a304da3

// Error thrown by the server
FastifyError [Error]: Failed building the validation schema for PUT: /api/user/update-password, due to error strict mode: unknown keyword: "errorMessage"

Copy link
Author

@Mathieuka Mathieuka Nov 16, 2024

Choose a reason for hiding this comment

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

@jean-michelet

Regarding the custom error message for AJV formatting, I haven't found a solution, and it seems that @climba03003 hasn't either ? as they approved the PR following my message #48 (comment)

As for SonarQube's warning about duplication, that's code I don't want to abstract and SonarQube doesn't know the difference

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We can review on the dashboard and claim it is fixed or safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Eomm
Is it possible to give these rights to the Fastify members that collaborates on this repo?
Besides me, I am aware of @climba03003 and @Fdawgs.

Copy link
Member

Choose a reason for hiding this comment

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

I believe it can identify the Github account and give the correct permission?
Because I have updated the sonarcloud issue when I using the Github OAuth.

minLength: 8

})

export const UpdateCredentialsSchema = Type.Object({
currentPassword: PasswordSchema,
newPassword: PasswordSchema
})
2 changes: 1 addition & 1 deletion test/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ async function login (this: FastifyInstance, username: string) {
url: '/api/auth/login',
payload: {
username,
password: 'password123$'
password: 'Password123$'
}
})

Expand Down
4 changes: 2 additions & 2 deletions test/routes/api/auth/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ test('Transaction should rollback on error', async (t) => {
url: '/api/auth/login',
payload: {
username: 'basic',
password: 'password123$'
password: 'Password123$'
}
})

Expand All @@ -39,7 +39,7 @@ test('POST /api/auth/login with valid credentials', async (t) => {
url: '/api/auth/login',
payload: {
username: 'basic',
password: 'password123$'
password: 'Password123$'
}
})

Expand Down
150 changes: 150 additions & 0 deletions test/routes/api/users/users.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
import { it, describe, beforeEach, afterEach } from 'node:test'
import assert from 'node:assert'
import { build } from '../../../helper.js'
import { FastifyInstance } from 'fastify'
import { scryptHash } from '../../../../src/plugins/custom/scrypt.js'

async function createUser (app: FastifyInstance, userData: Partial<{ username: string; password: string }>) {
const [id] = await app.knex('users').insert(userData)
return id
}

async function deleteUser (app: FastifyInstance, username: string) {
await app.knex('users').delete().where({ username })
}

async function updatePasswordWithLoginInjection (app: FastifyInstance, username: string, payload: { currentPassword: string; newPassword: string }) {
return await app.injectWithLogin(username, {
method: 'PUT',
url: '/api/users/update-password',
payload
})
}

describe('Users API', async () => {
const hash = await scryptHash('Password123$')
let app: FastifyInstance

beforeEach(async () => {
app = await build()
})

afterEach(async () => {
await app.close()
})

it('Should update the password successfully', async () => {
await createUser(app, { username: 'random-user-0', password: hash })
const res = await updatePasswordWithLoginInjection(app, 'random-user-0', {
currentPassword: 'Password123$',
newPassword: 'NewPassword123$'
})

assert.strictEqual(res.statusCode, 200)
assert.deepStrictEqual(JSON.parse(res.payload), { message: 'Password updated successfully' })

await deleteUser(app, 'random-user-0')
})

it('Should return 400 if the new password is the same as current password', async () => {
await createUser(app, { username: 'random-user-1', password: hash })
const res = await updatePasswordWithLoginInjection(app, 'random-user-1', {
currentPassword: 'Password123$',
newPassword: 'Password123$'
})

assert.strictEqual(res.statusCode, 400)
assert.deepStrictEqual(JSON.parse(res.payload), { message: 'New password cannot be the same as the current password.' })

await deleteUser(app, 'random-user-1')
})

it('Should return 400 if the newPassword password not match the required pattern', async () => {
await createUser(app, { username: 'random-user-2', password: hash })
const res = await updatePasswordWithLoginInjection(app, 'random-user-2', {
currentPassword: 'Password123$',
newPassword: 'password123$'
})

assert.strictEqual(res.statusCode, 400)
assert.deepStrictEqual(JSON.parse(res.payload), { message: 'body/newPassword must match pattern "^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).*$"' })

await deleteUser(app, 'random-user-2')
})

it('Should return 401 the current password is incorrect', async () => {
await createUser(app, { username: 'random-user-3', password: hash })
const res = await updatePasswordWithLoginInjection(app, 'random-user-3', {
currentPassword: 'WrongPassword123$',
newPassword: 'Password123$'
})

assert.strictEqual(res.statusCode, 401)
assert.deepStrictEqual(JSON.parse(res.payload), { message: 'Invalid current password.' })

await deleteUser(app, 'random-user-3')
})

it('Should return 401 if user does not exist in the database', async () => {
await createUser(app, { username: 'random-user-4', password: hash })
const loginResponse = await app.injectWithLogin('random-user-4', {
method: 'POST',
url: '/api/auth/login',
payload: {
username: 'random-user-4',
password: 'Password123$'
}
})

assert.strictEqual(loginResponse.statusCode, 200)

await deleteUser(app, 'random-user-4')

app.config = {
...app.config,
COOKIE_SECRET: loginResponse.cookies[0].value
}

const res = await app.inject({
method: 'PUT',
url: '/api/users/update-password',
payload: {
currentPassword: 'Password123$',
newPassword: 'NewPassword123$'
},
cookies: {
[app.config.COOKIE_NAME]: loginResponse.cookies[0].value
}
})

assert.strictEqual(res.statusCode, 401)
assert.deepStrictEqual(JSON.parse(res.payload), { message: 'User does not exist.' })
await deleteUser(app, 'random-user-4')
})

it('Should enforce rate limiting by returning a 429 status after exceeding 3 password update attempts within 1 minute', async () => {
const updatePassword = async () => {
return await updatePasswordWithLoginInjection(app, 'random-user-5', {
currentPassword: 'WrongPassword123$',
newPassword: 'Password123$'
})
}

const performMultiplePasswordUpdates = async () => {
for (let i = 0; i < 3; i++) {
await updatePassword()
}
}

await createUser(app, { username: 'random-user-5', password: hash })

await performMultiplePasswordUpdates()

const res = await updatePassword()

assert.strictEqual(res.statusCode, 429)
assert.equal(res.payload, '{"message":"Rate limit exceeded, retry in 1 minute"}')

await deleteUser(app, 'random-user-5')
})
})
Loading