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

Chore(auth): store refresh token in cookie #240

Closed
wants to merge 6 commits into from

Conversation

kloV148
Copy link
Contributor

@kloV148 kloV148 commented Apr 4, 2024

Now, we store refresh token as http only cookie

Copy link

github-actions bot commented Apr 4, 2024

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 86.21% (🎯 80%)
⬆️ +0.04%
7087 / 8220
🟢 Statements 86.21% (🎯 80%)
⬆️ +0.04%
7087 / 8220
🔴 Functions 79.32% (🎯 80%)
⬆️ +0.05%
234 / 295
🟢 Branches 85.92% (🎯 80%)
⬆️ +0.24%
354 / 412
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/domain/service/auth.ts 100% 100% 100% 100%
src/presentation/http/http-api.ts 95.9% 90.32% 93.75% 95.9% 95-103, 115-116, 306-307, 331-332
src/presentation/http/router/auth.ts 88.77% 100% 100% 88.77% 83-93
src/presentation/http/router/oauth.ts 48.27% 100% 100% 48.27% 38-82
Generated in workflow #681

})
.send({
accessToken,
refreshToken,
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to send rt via json then

@@ -71,7 +71,7 @@ export default class AuthService {
* @param userId - user to sign refresh token for
* @returns {Promise<string>} refresh token
Copy link
Member

Choose a reason for hiding this comment

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

update doc as well

@@ -23,6 +23,11 @@ interface AuthRouterOptions {
* Auth service instance
*/
authService: AuthService,

/**
* Cookie domain for refresh and access tokens
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Cookie domain for refresh and access tokens
* Cookie domain for refresh token

refreshToken,
});
return reply
.setCookie('refreshToken', refreshToken, {
Copy link
Member

Choose a reason for hiding this comment

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

maybe call it "rt" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this naming is not clear enough

Copy link
Member

Choose a reason for hiding this comment

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

camel case is not a common practice for cookie names, as I know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe use snake case instead? refresh_token for example

Copy link
Member

Choose a reason for hiding this comment

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

yep

@neSpecc neSpecc marked this pull request as draft April 25, 2024 17:01
@kloV148 kloV148 force-pushed the chore/http-cookie-refresh-token branch from fcef5b5 to 49c2a8b Compare June 30, 2024 13:45
@kloV148 kloV148 closed this Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants