[Feat] Simple Web cookie #17
Conversation
Eliminates the need for redis storage and the socket service, and enables a cookie at login for user authorization
src/api/middlewares/auth.ts
Outdated
|
|
||
| const loggedUser = (req: IResponse, _res: IRequest, next: INext) => { | ||
| const authorization = req.headers?.authorization; | ||
| const cookies = cookie.parse(req.headers.cookie || ''); |
There was a problem hiding this comment.
cookies.parse('') returns an empty object ({}).
src/api/middlewares/auth.ts
Outdated
|
|
||
| const loggedUser = (req: IResponse, _res: IRequest, next: INext) => { | ||
| const authorization = req.headers?.authorization; | ||
| const cookies = cookie.parse(req.headers.cookie || ''); | ||
| const cookieToken = cookies ? cookies[process.env.SESSION_COOKIE_NAME] : null; |
There was a problem hiding this comment.
With the note about cookies.parse(''), {} is truthy, so this will always attempt to access cookies[process.env.SESSION_COOKIE_NAME]. I think this can cause an error when the cookie is falsy for any reason.
There was a problem hiding this comment.
I pushed a change here, right now. Just to prevent the throw error from cookie.parse. if the header doesnt has a cookie the cookies object will be an empty object {}. and cookies[process.env.SESSION_COOKIE_NAME] will be undefined and that's ok.
src/controllers/UserController.ts
Outdated
| return res.json({ email: user.email, token }); | ||
| } | ||
|
|
||
| public async Logout(req: IRequest, res: IResponse): Promise<IResponse> { | ||
| await res.clearCookie(process.env.SESSION_COOKIE_NAME, { path: '*' }); |
There was a problem hiding this comment.
I think the path here needs to match the cookie, which is '/'.
src/controllers/UserController.ts
Outdated
| const isProd = process.env.NODE_ENV === 'production'; | ||
| res.setHeader( | ||
| 'Set-Cookie', | ||
| cookie.serialize(process.env.SESSION_COOKIE_NAME, token, { |
There was a problem hiding this comment.
Is there any reason why this is provided by the environment? It's not a secret, so there's no reason this can't just be 'sessionCookie' (or whatever), right?
There was a problem hiding this comment.
yeah you are right., i changed it to a constant value now.
| sameSite: isProd ? 'none' : null, | ||
| secure: isProd, | ||
| path: '/', | ||
| }), |
There was a problem hiding this comment.
I think we need to set domain: tortugapower.com. As it is now (i.e., not set), the cookie is only available on https://tortugapower.com, and not its subdomains.
There was a problem hiding this comment.
I thought that the idea is that there could be different web apps from different domains that can use the api, so first validate that the domain must exist in our database . That reason is now set to none in production so the cookie will be cross domain. I think there are no problems with subdomains right now as long as the domain exists in our database, but if we are only going to work with the bookplayer domain we can make the change.
There was a problem hiding this comment.
The Domain part of the cookie is for what the server sends and receives. The frontend can be wherever, but this is just what the client will use to decide when to send cookies with requests. The way subdomains works with cookies is kind of strange: if a server foo.example.com sends Set-Cookie to a client, the client will only send that cookie to example.com, not to foo.example.com. If the Domain is set to example.com, then the client will send the cookie to foo.example.com (and bar.example.com, etc.).
|
We'll want to add a refresh mechanism at some point, but I think we need to look at the authentication/authorization situation in general soon anyway. |
|
The more I think about this, the more I think we should rethink how we're doing the auth for the web app. It probably makes more sense to stick to the API server just using JWTs and moving the currently-static web app to it's own thing. That way the web app can handle cookies and just pass JWTs to and from the API server as needed. Although the implementation is pretty simple now, it does start to feel like it's not in the right place. I still think we can go through with this for now, though, and figure out how we want to handle separating those later, if at all. |
Eliminates the need for redis storage, the socket service was not being used, and enables a cookie for user authorization