Fix/auth decorator status code #178
Conversation
sync upstream
403 with roles and 401 without
src/driver/express/ExpressDriver.ts
Outdated
|
|
||
| } else { | ||
| next(); | ||
| } | ||
| }); | ||
| } else { | ||
| if (!checkResult) { | ||
| this.handleError(new AccessDeniedError(action), actionMetadata, action); | ||
| this.handleError((actionMetadata.authorizedRoles.length === 0 |
There was a problem hiding this comment.
all these code blocks become messy... maybe we should avoid using ternary if and beatify code?
There was a problem hiding this comment.
ternary is faster, so if the libs want's to be fast, this should stay with other tail call optimizations 😜
There was a problem hiding this comment.
so if the libs want's to be fast
I see you need time to heal from the traumatic experience of that conversation 😄
There was a problem hiding this comment.
The only traumatic thing is the hypocrisy:
Promise.resolveremoves half on duplicated code block but it's slower, so we stick with messy code because "its important to make it performant as we can"- now "all these code blocks become messy", so we should remove ternary and tail call optimization to "beatify code"
So please be consistent - peformance at any cost or readable and maintainable code 😄
There was a problem hiding this comment.
@19majkel94 you should investigate a bit how Promise.resolve actually works, its not as simple as let vs const. We make decisions based on pros and cons of everything, its impossible to be consistent in everything, so make judging based on each scenario individually.
| } else { | ||
| if (!checkResult) { | ||
| this.handleError(new AccessDeniedError(action), actionMetadata, action); | ||
| const handleError = (result: any) => { |
There was a problem hiding this comment.
So, do you like this one?
src/driver/express/ExpressDriver.ts
Outdated
| this.handleError(new AccessDeniedError(action), actionMetadata, action); | ||
| const handleError = (result: any) => { | ||
| if (!result) { | ||
| return this.handleError(( |
There was a problem hiding this comment.
multiline params this way do not look good, simplify this. extract error class in a separate variable if needed
|
everyone thinks its good to merge (not only performance and code style considerations 😆 ) ? |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Let
@Authorized()raising401and@Authorized("some_role")raising403.