-
Notifications
You must be signed in to change notification settings - Fork 0
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
OV-8: protect routing #33
OV-8: protect routing #33
Conversation
…V-8-protect-routing
public async findById(userId: number): Promise<UserEntity | null> { | ||
return await this.userRepository.findById(userId); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modify method find instead of adding this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only in user service or in user repository as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified only in user service for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also modify it in the repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const isRouteInWhiteList = options.routesWhiteList.includes( | ||
request.url, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also check for method in whitelist because each route can have multiple methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
package.json
Outdated
@@ -50,6 +50,7 @@ | |||
"commit-msg": "npx commitlint --edit $1" | |||
}, | |||
"dependencies": { | |||
"fastify-plugin": "4.5.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add it as backend dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…V-8-protect-routing
@@ -1,5 +1,5 @@ | |||
type Repository<T = unknown> = { | |||
find(): Promise<T>; | |||
find(userId: number): Promise<T | null>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find(userId: number): Promise<T | null>; | |
find(id: number): Promise<T | null>; |
we would use this method not only in user repo if we want to find something by id
@@ -1,5 +1,5 @@ | |||
type Service<T = unknown> = { | |||
find(): Promise<T>; | |||
find(userId: number): Promise<T | null>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -20,6 +22,12 @@ class UserRepository implements Repository { | |||
return user ? UserEntity.initialize(user) : null; | |||
} | |||
|
|||
// public async findById(userId: number): Promise<UserEntity | null> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought I deleted that already
b44cde0
to
145f206
Compare
080bcb2
to
1ce3a2e
Compare
@@ -1,5 +1,5 @@ | |||
type Repository<T = unknown> = { | |||
find(): Promise<T>; | |||
find(id: number): Promise<T | null>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should accept any params here, or we should rename it to findById
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it to findById
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check comment
In this pr: