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

Introduce APP_SECRET to replace SECRETS #4588

Closed
FelixMalfait opened this issue Mar 20, 2024 · 44 comments · Fixed by #7810
Closed

Introduce APP_SECRET to replace SECRETS #4588

FelixMalfait opened this issue Mar 20, 2024 · 44 comments · Fixed by #7810
Assignees
Labels

Comments

@FelixMalfait
Copy link
Member

FelixMalfait commented Mar 20, 2024

Context

We currently have 4 environment variables to specify our SECRETS in packages/twenty-server/.env:

  • ACCESS_TOKEN_SECRET
  • LOGIN_TOKEN_SECRET
  • REFRESH_TOKEN_SECRET
  • FILE_TOKEN_SECRET

We would like to replace those by a unique APP_SECRET env variable and to generate those 4 when needed as:

  • ACCESS_TOKEN_SECRET=SHA256({APP_SECRET}+{workspaceId}+"ACCESS")
  • LOGIN_TOKEN_SECRET=SHA256({APP_SECRET}+{workspaceId}+"FILE")
  • ...

What & How

  • Remove those 4 secrets from the code base
  • introduce APP_SECRET instead (this needs to be introduced in several places, I would recommend look for ACCESS_TOKEN_SECRET and to mimick it)
  • compute accessTokenSecret based on the formula above whenever it's needed
@charlesBochet charlesBochet added the good first issue Good for newcomers label Oct 9, 2024
@charlesBochet charlesBochet changed the title Workspace specific secret key Introduce APP_SECRET to replace SECRETS Oct 9, 2024
@twentyhq twentyhq deleted a comment from FelixMalfait Oct 9, 2024
@charlesBochet
Copy link
Member

I have updated the tickets desc, ready to be picked!

@charlesBochet
Copy link
Member

/oss.gg 500

Copy link

oss-gg bot commented Oct 9, 2024

Thanks for opening an issue! It's live on oss.gg!

@charlesBochet
Copy link
Member

@Devessier Let's go with sha as discussed together!

@vishnud05
Copy link

/assign

Copy link

oss-gg bot commented Oct 10, 2024

Assigned to @vishnud05! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@Fahad-Dezloper
Copy link

/assign

Copy link

oss-gg bot commented Oct 10, 2024

This issue is already assigned to another person. Please find more issues here.

@riazahmedshah
Copy link

/assign

Copy link

oss-gg bot commented Oct 10, 2024

This issue is already assigned to another person. Please find more issues here.

Copy link

oss-gg bot commented Oct 11, 2024

@vishnud05, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

@AbhishekKhot
Copy link

/assign

Copy link

oss-gg bot commented Oct 12, 2024

This issue is already assigned to another person. Please find more issues here.

@BasitShafiq
Copy link

/assign

Copy link

oss-gg bot commented Oct 12, 2024

This issue is already assigned to another person. Please find more issues here.

@mr-rayeen
Copy link

/assign

Copy link

oss-gg bot commented Oct 13, 2024

This issue is already assigned to another person. Please find more issues here.

@amit-y11
Copy link

/assign

Copy link

oss-gg bot commented Oct 13, 2024

This issue is already assigned to another person. Please find more issues here.

@charlesBochet
Copy link
Member

/unassign

Copy link

oss-gg bot commented Oct 13, 2024

Issue unassigned.

Copy link

oss-gg bot commented Oct 14, 2024

Issue unassigned.

@oss-gg oss-gg bot unassigned amit-y11 Oct 14, 2024
@HarshithAlfred
Copy link

/assign

Copy link

oss-gg bot commented Oct 14, 2024

Assigned to @HarshithAlfred! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

Copy link

oss-gg bot commented Oct 15, 2024

@vishnud05, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

Copy link

oss-gg bot commented Oct 16, 2024

@HarshithAlfred, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

Copy link

oss-gg bot commented Oct 16, 2024

@amit-y11, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

@charlesBochet
Copy link
Member

/unassign

Copy link

oss-gg bot commented Oct 16, 2024

Issue unassigned.

@Khaan25
Copy link
Contributor

Khaan25 commented Oct 16, 2024

/assign

Copy link

oss-gg bot commented Oct 16, 2024

You already have an open issue assigned to you here. Once that's closed or unassigned, only then we recommend you to take up more.

@Khaan25
Copy link
Contributor

Khaan25 commented Oct 16, 2024

If I'm not wrong, we'll have a function.

function generateSecret(workspaceId: string, type: 'ACCESS' | 'LOGIN' | 'REFRESH' | 'FILE'): string

Which is gonna return ${appSecret}${workspaceId}${type}

where appSecret = process.env.APP_SECRET
where workspaceId = we will pass it?

Please correct me if I'm wrong.

@FelixMalfait @Bonapara

@FelixMalfait
Copy link
Member Author

@Khaan25 that's correct! Thank you

@devhiteshk
Copy link

/assign

Copy link

oss-gg bot commented Oct 17, 2024

This issue is already assigned to another person. Please find more issues here.

@Khaan25
Copy link
Contributor

Khaan25 commented Oct 18, 2024

One quick question, @FelixMalfait,

I see some functions
image

They don't have workspaceId as prop, I believe we're gonna modify the function params to include it? Right?

@Khaan25
Copy link
Contributor

Khaan25 commented Oct 18, 2024

Maybe we can make the workspaceId optional?
Could you please guide me here?

@FelixMalfait
Copy link
Member Author

@Khaan25 no we don't want to make workspaceId optional, you should usually be able to retrieve it in the context (it's available at the resolver level usually since most API calls require to be logged in). Thanks

@charlesBochet
Copy link
Member

@Khaan25 in some sign-in / sign-up cases, we indeed don't have the workspaceId available.

@FelixMalfait, we could either append the userId in this case or try to get the defaultWorkspaceId

@Khaan25
Copy link
Contributor

Khaan25 commented Oct 18, 2024

Well, let's do defaultWorkspaceId, how do we get that?

there's workspaceRespository.getId() function but not sure if that's correct to use. I'll dig into further.
Is there any function or anything to get or we can pass userId?

@charlesBochet

@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Product development ✅ Oct 22, 2024
@FelixMalfait FelixMalfait assigned FelixMalfait and unassigned Khaan25 Oct 22, 2024
charlesBochet added a commit that referenced this issue Oct 30, 2024
This PR fixes #4588

---------

Co-authored-by: Félix Malfait <felix@twenty.com>
Co-authored-by: Charles Bochet <charles@twenty.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.