feat: Add cookie authentication #45
Conversation
gabrielmachin
requested review from
mlvieras,
lauta2912,
MBerguer and
chaba11
as code owners
src/controllers/auth.ts
Outdated
| if (JWTEnabled) return authReturn; | ||
| return null; |
There was a problem hiding this comment.
Shouldn't we still return something when using cookie auth?
There was a problem hiding this comment.
We should return and serialize the user right?
There was a problem hiding this comment.
If we delete the JWTEnabled and cookieEnabled, as discussed previously with @mlvieras, should we always return the authReturn alongside the serialized user?
src/utils/auth.ts
Outdated
| export const cookieConfig = { | ||
| signed: true, | ||
| httpOnly: true, | ||
| maxAge: config.cookieExpirationSeconds * SECONDS_TO_MILLISECONDS, | ||
| secure: isProduction, | ||
| } as CookieOptions; |
There was a problem hiding this comment.
Is there a reason we're not using the CookieOptions type explicitly here?
| export const cookieConfig = { | |
| signed: true, | |
| httpOnly: true, | |
| maxAge: config.cookieExpirationSeconds * SECONDS_TO_MILLISECONDS, | |
| secure: isProduction, | |
| } as CookieOptions; | |
| export const cookieConfig : CookieOptions = { | |
| signed: true, | |
| httpOnly: true, | |
| maxAge: config.cookieExpirationSeconds * SECONDS_TO_MILLISECONDS, | |
| secure: isProduction, | |
| }; |
src/utils/auth.ts
Outdated
| secure: isProduction, | ||
| } as CookieOptions; | ||
|
|
||
| export const verifyCookie = async (signedCookies: any) => { |
There was a problem hiding this comment.
Why is signedCookies of any type?
src/controllers/auth.ts
Outdated
| if (JWTEnabled) return authReturn; | ||
| return null; |
There was a problem hiding this comment.
We should return and serialize the user right?
gabrielmachin
force-pushed
the
feat/Add-cookies-implementation
branch
from
558197f
to
875eddf
Compare
| const { sessionId, ...authReturn } = await AuthService.register(user); | ||
|
|
||
| const { res } = req; | ||
| res?.cookie(COOKIE_NAME, sessionId, cookieConfig); |
There was a problem hiding this comment.
Shouldn't this only be done if the authentication scheme is cookies?
There was a problem hiding this comment.
If we want to differentiate between cases for settings and clearing cookies, I'd rather have different endpoints for cookies and tokens than having the user tell me what method it's using, what do you think about it?
There was a problem hiding this comment.
Don't you know what method the client is using? Meaning there is either a token or a cookie.
There was a problem hiding this comment.
In the case of a register or login endpoint we don't know what method will be used, in other cases we might me able to deduce it based on the authentication method, but in this case we don't know , maybe I'm overlooking something. @chaba11 do you have any idea regarding this?
| @Security('jwt') | ||
| public async logout(@Request() req: AuthenticatedRequest): Promise<void> { | ||
| await AuthService.logout(req.user.token); | ||
| const { res } = req; | ||
| res?.clearCookie(COOKIE_NAME); |
| await UserService.destroy(id); | ||
| if (user.id === id) res?.clearCookie(COOKIE_NAME); |
There was a problem hiding this comment.
I don't think this line is needed. You might want to destroy all sessions of the user though.
NOTION Ticket
Type of change