feat: generate secret function and replaced few instances #7810
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request introduces a new generateSecret function to centralize secret management using a single APP_SECRET environment variable, addressing issue #4588.
- Implemented
generateSecretfunction inpackages/twenty-server/src/utils/generate-secret.ts - Modified
token.service.tsto usegenerateSecretfor transient and API key tokens - Replaced individual secret variables with
APP_SECRETin environment files - Added error handling for missing
APP_SECRETingenerate-secret.ts - Updated secret generation logic to include workspace ID and token type
2 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-server/src/engine/core-modules/auth/token/services/token.service.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export const generateSecret = ( | ||
| workspaceId: string, | ||
| type: 'ACCESS' | 'LOGIN' | 'REFRESH' | 'FILE', |
There was a problem hiding this comment.
style: consider using an enum for type parameter
| type: 'ACCESS' | 'LOGIN' | 'REFRESH' | 'FILE', | ||
| ): string => { | ||
| return createHash('sha256') | ||
| .update(`${process.env.APP_SECRET}${workspaceId}${type}`) |
There was a problem hiding this comment.
logic: no separator between APP_SECRET, workspaceId, and type could lead to collisions
| @@ -0,0 +1,14 @@ | |||
| import { createHash } from 'crypto'; | |||
|
|
|||
| if (!process.env.APP_SECRET) { | |||
There was a problem hiding this comment.
We should access env var through the dedicate service not directly through process.env ; you should probably do this in TokenService
There was a problem hiding this comment.
Got it. I'll see that.
|
@FelixMalfait I've pushed a change to get APP_SECRET from the environment service. Can you please open issue? We don't have workspaceId in some function, what do I pass there? e.g. I've attached an image of a function: Thank you very much! |
|
Sorry I hijacked your PR but it was worth cleaning up a bit :) |
|
Haha, no worries. I'm also learning about the new techniques and codebase 🙌 Amazing Job |
|
I guess the PR is complete then? @FelixMalfait Thanks for the help though. I got the understanding of the codebase as well how things are linkedin togther, do you think we can close the PR now? |
|
/award 300 |
|
Awarding Khaan25: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/Khaan25 |
…//github.com/Khaan25/twenty into feat/introduce-app-secret-to-replace-secrets
|
@FelixMalfait, hope I get points for the change as it's for developer docs as per side quest :) Let me knowww |
packages/twenty-website/src/content/developers/self-hosting/self-hosting-var.mdx
Outdated
Show resolved
Hide resolved
packages/twenty-website/src/content/developers/self-hosting/cloud-providers.mdx
Outdated
Show resolved
Hide resolved
packages/twenty-website/src/content/developers/self-hosting/cloud-providers.mdx
Outdated
Show resolved
Hide resolved
|
Awarding Khaan25: 500 points 🕹️ Well done! Check out your new contribution on oss.gg/Khaan25 |
This PR fixes #4588