Feat/is 21 repo privatisation #806

alexanderleegs · 2023-06-21T03:58:38Z

Problem

This PR introduces a repo privatisation feature. To be reviewed in conjunction with PR #1316 on the isomercms-frontend repo.

This PR adds new fields to the sites and deployments table - isPrivate and encryptedPassword, encryptionIv, passwordDate respectively. The new endpoints have been added to the settings router and related services, with the deployment modification logic added to DeploymentsService.

Tests

See frontend for related tests - for private repos, the is_private param for the sites entry should be true and the encrypted_password, encryption_iv and password_date in deployments should contain the appropriate entries

Deploy Notes

Migrations will need to be run on staging/prod before merging this PR.

New environment variables:

SITE_PASSWORD_SECRET_KEY : Encryption key for stored passwords, should match the equivalent key on the backend
NETLIFY_ACCESS_TOKEN: this is the access token we use to access our account. We should be using the one generated for isomeradmin

seaerchin · 2023-06-26T08:41:32Z

src/database/migrations/20230615070344-add-password-and-iv.js

+        queryInterface.addColumn(
+          "deployments", // name of Source model
+          "encryption_iv", // name of column we're adding
+          {
+            type: Sequelize.TEXT,
+            allowNull: true,
+            transaction: t,
+          }
+        ),
+        queryInterface.addColumn(
+          "deployments", // name of Source model
+          "password_date", // name of column we're adding
+          {
+            type: Sequelize.DATE,
+            allowNull: true,
+            transaction: t,
+          }


could i get more information/context on what encryption_iv + password_date implies? i was under the impression that we'd only need encrypted_password?

encryption_iv is the initialisation vector used for encryption - this is needed so that similar patterns of text when encrypted don't give rise to similar ciphertexts! This information needs to be stored to retrieve the original unencrypted value

password_date is an additional field which will be helpful for a future feature - we eventually want to be able to tell when a repo password has not been changed for a certain amount of time, so this is useful for that, though it's currently unused

seaerchin

some comments; also, should we update 1pw env vars?

seaerchin · 2023-06-26T08:46:28Z

src/services/configServices/SettingsService.js

+    const deploymentInfo = await this.deploymentsService.getDeploymentInfoFromSiteId(
+      id
+    )
+    if (deploymentInfo.isErr()) return deploymentInfo


why not just throw here rather than returning and throwing? also, can't help but feel that using neverthrow in .js files is asking for trouble .-.

seaerchin · 2023-06-26T08:47:19Z

src/routes/v2/authenticatedSites/settings.js

@@ -77,11 +80,58 @@ class SettingsRouter {
    return next()
  }

+  async getRepoPassword(req, res, next) {


nit: maybe can prefix with underscore/ignore the unused stuff

seaerchin · 2023-06-26T08:48:28Z

src/routes/v2/authenticatedSites/settings.js

+      attachReadRouteHandlerWrapper(this.getRepoPassword)
+    )
+    router.post(
+      "/repoPassword",
+      attachRollbackRouteHandlerWrapper(this.updateRepoPassword)


probably should kebab-case sir

seaerchin · 2023-06-26T08:48:45Z

src/routes/v2/authenticatedSites/settings.js

+    res.status(200).send("OK")
+    return next()


is there a difference between this and return res.status(200).send("OK")?

Yes for these cases - we have a notification middleware which can potentially trigger, so we need to move on to the next router instead of returning immediately

seaerchin · 2023-06-26T08:53:15Z

src/services/configServices/__tests__/SettingsService.spec.js

seaerchin · 2023-06-26T09:21:49Z

src/services/identity/DeploymentsService.ts

+      return errAsync(`Deployment for ${repoName} does not exist`)
+    const { id, hostingId: appId } = deploymentInfo
+    let updateAppInput
+    if (!enablePassword) {


could we just branch here and hide the rest of the logic into 2 separate methods?

very confusing to figure out where/what is used, especially because the purpose of the two is diametrically opposed (1 sets password, 1 removes)

Yeah that makes sense, f13e3f2

seaerchin · 2023-06-26T09:22:30Z

src/utils/crypto-utils.ts

+
+import { config } from "@config/config"
+
+const ALGORITHM = "aes-256-cbc"


Suggested change

const ALGORITHM = "aes-256-cbc"

const DECRYPTION_ALGORITHM = "aes-256-cbc"

changed in d1f2659!

seaerchin · 2023-06-26T09:27:36Z

src/utils/crypto-utils.ts

+  const decipher = createDecipheriv(
+    ALGORITHM,
+    SECRET_KEY,
+    Buffer.from(iv, "hex")
+  )
+  let decrypted = decipher.update(encryptedPassword, "hex", "utf8")
+  decrypted += decipher.final("utf8")
+  return decrypted


sorry, but this is abit confusing to me - it seems to me that we are decrypting first and thereafter, converting from hex to utf8 before calling a finalizer of utf8 and appending it to the updated value (in effect, decrypted === decipher<utf8> * 2) isn't it?)

am i misunderstanding something? shouldn't it be either

// update -> final decipher.update(...) return decipher.final(...) // just return straight return decipher.final(...)

why have a repeated output string?

This is the usage as given by the documentation - i'm not sure what to call the intermediate steps either tbh, so I used their naming. But .update alone doesn't actually provide the final decrypted string, and .final is needed to cut off the decipher usage, but doesn't actually provide any decryption

seaerchin · 2023-06-26T09:29:44Z

src/utils/crypto-utils.ts

+
+const ALGORITHM = "aes-256-cbc"
+
+export const decryptPassword = (encryptedPassword: string, iv: string) => {


highly suggest typing + branding the return and enforcing this on downstream callers. doesn't have to be here if the logic here is correct.

// type-def of decrypted password, prevents assigning string to it type DecryptedPassword = Brand<string, "DecryptedPassword"> export const decryptPassword = (): DecryptedPassword

Updated in cea7e0e!

seaerchin · 2023-06-26T09:30:39Z

src/validators/RequestSchema.js

+const UpdateRepoPasswordRequestSchema = Joi.object().keys({
+  encryptedPassword: Joi.string(),
+  iv: Joi.string(),
+  enablePassword: Joi.boolean(),
+})


is this all or nothing? ie, do we accept partial inputs like encryptedPassword + iv only

No partial input - always specify whether we're enabling or disabling the password

kishore03109

Hey mostly lgtm, but I have some questions regarding the implementation of this.

does FE generate the IV?
Do we regenerate the IV everytime the user wishes to change the password?
did we consider only FE not knowing the IV and secret key? Ie, we send unencrypted pass over secure HTTPS, but this way only BE is aware of the symmetric encryption + has the relevant keys to achieve this functionality?

kishore03109 · 2023-06-26T08:34:45Z

src/config/config.ts

+        doc: "Secret key used to encrypt password",
+        env: "SITE_PASSWORD_SECRET_KEY",
+        format: String,
+        default: "",


Sanity check here: should this not be required string here? Having an empty string here should not be a sane value for decryption right?

Good catch, 0c5622f

kishore03109 · 2023-06-26T09:04:26Z

src/services/configServices/SettingsService.js

@@ -43,6 +50,37 @@ class SettingsService {
    }
  }

+  async getEncryptedPassword(sessionData) {


nit: could we enforce a return type here so that it doesnt resolve to Promise<any>?

It's a js file, I don't think we can enforce the return type :(

kishore03109 · 2023-06-26T09:06:04Z

src/services/configServices/SettingsService.js

+    const deploymentInfo = await this.deploymentsService.getDeploymentInfoFromSiteId(
+      id
+    )
+    if (deploymentInfo.isErr()) return deploymentInfo


Sanity check here, shouldnt we reuturn some sort of error here instead?

This returns the wrapped error from getDeploymentInfoFromSiteId - we're not modifying it here

kishore03109 · 2023-06-26T09:07:21Z

src/routes/v2/authenticatedSites/settings.js

+    )
+
+    if (passwordRes.isErr()) {
+      throw passwordRes.error


sanity check here: should we return some sort of 404 rather than thorwing an error here?

Had a chat with Chin about this - given that it's difficult to step through to retrieve all the possible errors here now, I'm leaning towards leaving this as throw and fixing it after the mvp is out (by refactoring to ts) - do you have concerns about this?

fixing it after the mvp

let's add a ticket for the refactor

@alexanderleegs am ok with this, but are we sure that there is is no errors meant for internal consumption thrown back to the end user?

Verified offline - the thrown isomer errors have user facing error messages, and any other error gets caught by our error handler to throw a generic error and message

kishore03109 · 2023-06-26T09:08:24Z

src/routes/v2/authenticatedSites/settings.js

+    const { userWithSiteSessionData } = res.locals
+
+    const { error } = UpdateRepoPasswordRequestSchema.validate(req.body)
+    if (error) throw new BadRequestError(error.message)


Similar comment as above, should we be returning 404 rather than throwing here?

kishore03109 · 2023-06-26T09:13:22Z

src/utils/crypto-utils.ts

+    Buffer.from(iv, "hex")
+  )
+  let decrypted = decipher.update(encryptedPassword, "hex", "utf8")
+  decrypted += decipher.final("utf8")


why do we need to do an update + final here ah?

See above on Chin's comment!

kishore03109 · 2023-06-26T09:17:13Z

src/services/identity/DeploymentsService.ts

@@ -115,6 +118,83 @@ class DeploymentsService {
          .map(() => amplifyInfo)
      })
  }
+
+  getDeploymentInfoFromSiteId = async (repoId: string) => {


could we rename repoId to siteId? we have a repos and a sites table, might be better to make a distinction here

Good catch, forgot to rename the var - d255ec4

kishore03109 · 2023-06-26T09:19:09Z

src/services/identity/DeploymentsService.ts

+    iv: string,
+    enablePassword: boolean
+  ) => {
+    const deploymentInfo = await this.repository.findOne({


hey, this can be made simpler right?
eg:

const deploymentInfo = await this.repository.findOne({ where: { name: repoName, }, })

repoName only exists in Repo though, so we need to do the joins!

[nit]: rename for repository to deploymentRepository

src/services/identity/DeploymentsService.ts

kishore03109 · 2023-06-26T09:27:32Z

src/services/identity/DeploymentClient.ts

@@ -88,6 +96,28 @@ class DeploymentClient {
      JEKYLL_ENV: branchName === "master" ? "production" : "staging",
    },
  })
+
+  generateUpdatePasswordInput = (


nit: could we separate out the concerns of with two functions of
generateUpdatePasswordInput
and
generateDeletePasswordInput?

Sure, changed in 592ab87

harishv7 · 2023-06-30T09:51:59Z

src/routes/v2/authenticatedSites/settings.js

+    )
+
+    if (passwordRes.isErr()) {
+      throw passwordRes.error


fixing it after the mvp

let's add a ticket for the refactor

harishv7 · 2023-06-30T09:55:59Z

src/services/configServices/SettingsService.js

+    if (siteInfo.isErr()) {
+      // Missing site indicating netlify site - return special result
+      return okAsync({
+        password: "",


would it be possible to return this without the password when isAmplifySite is false? the password only applies if isAmplifySite is true right?

Yeah that makes sense, have updated it in af96a20

harishv7 · 2023-06-30T09:58:27Z

src/utils/crypto-utils.ts

+  encryptedPassword: string,
+  iv: string
+): DecryptedPassword => {
+  const SECRET_KEY = Buffer.from(


shouldn't this be a argument to the method too? decryptPassword is a generic util to decrypt an encrypted pass with all necessary parameters given to it

Sure, have made the change to both decrypt and encrypt in 67a47f2

kishore03109

sadly cicd failign ya

kishore03109 · 2023-07-05T02:41:22Z

src/services/identity/DeploymentsService.ts

+    iv: string,
+    enablePassword: boolean
+  ) => {
+    const deploymentInfo = await this.repository.findOne({


[nit]: rename for repository to deploymentRepository

kishore03109

@alexanderleegs not sure why but there are sadly alot of extra commits
Could I get your help to rebase this?
[Style] When we are making stacked diffs, shall we we only merge them in when the parent PR is approved? abit hard to reason about a larger pr in one sitting :(

kishore03109 · 2023-07-06T08:32:16Z

CHANGELOG.md

- fix(test cases): fix failing tests [`c843865`](https://github.com/isomerpages/isomercms-backend/commit/c84386549a28602e1af10cd2eda9f056212afaa7)
-
-#### [v0.24.0](https://github.com/isomerpages/isomercms-backend/compare/v0.23.1...v0.24.0)
+#### [v0.24.1](https://github.com/isomerpages/isomercms-backend/compare/v0.23.1...v0.24.1)


hey i think these got committed by accident?

@alexanderleegs i think need to rebase

Yup fixed on rebase!

* chore: update site fixtures * test: add githubService tests * test: add settings router test * Fix: settings tests * chore: update env vars for tests * chore: update fixtures * fix: requestSchema * fix: use writehandler instead of rollback handler * chore: update tests to use new dto * feat: add crypto-utils test * feat: add integration test * fix: update updatePassword schema * fix: update crypto-utils tests * chore: remove unused imports * chore: update names of imported fixtures * feat: add new test case for invalid req * chore: update const name * fix: test input for new dto * chore: add validator * chore: update tests * nit: update test names

alexanderleegs · 2023-07-07T01:41:30Z

@alexanderleegs not sure why but there are sadly alot of extra commits Could I get your help to rebase this? [Style] When we are making stacked diffs, shall we we only merge them in when the parent PR is approved? abit hard to reason about a larger pr in one sitting :(

Sorry I thought the base PR was also approved already!

kishore03109

mostly ok pending 2 concerns:

this pr contains throw .error. Have we made sure that the errors thrown back to the users are not meant for internal consumption? Eg, an axios error from github should not be returned to the user.
This is a stacked diff, have we made sanity checks on the child diff? Specifically ensuring that

all non-email sites dont have access to this
all the email sites is able to privatise their repos?

kishore03109 · 2023-07-19T02:02:06Z

src/routes/v2/authenticatedSites/settings.js

+    )
+
+    if (passwordRes.isErr()) {
+      throw passwordRes.error


@alexanderleegs am ok with this, but are we sure that there is is no errors meant for internal consumption thrown back to the end user?

kishore03109 · 2023-07-19T02:15:56Z

src/services/db/GitHubService.js

+
+  async changeRepoPrivacy(sessionData, shouldMakePrivate) {
+    const { siteName, isomerUserId } = sessionData
+    const endpoint = `${siteName}`


sanity check here, wouldnt endpoint refer to repos/<repoName> and siteName refer to just repoName? should we just use the variable siteName instead?

This is referring to the endpoint still! The actual endpoint to call is /repos/<orgname>/<repoName>, but our axios instance prefixes with /repos/<orgname>/, so the remaining endpoint is just <repoName> (referred to as siteName here

* chore: add new env var for netlify access token * feat: add netlify privatisation * chore: update .env.test * chore: add return type * chore: updates stop_builds type * chore: cast error * chore: move netlify conditional out separately * fix: check for error type * chore: shift and rename file * Fix: refactor axios-utils and add isAxiosError * refactor: pass only password to updateNetlifySite * fix: use isAxiosError * fix: revert to require * chore: swap to @utils * chore: modify method name * chore: rename netlifyService to netlifyApi * Fix/restrict privatisation to email login (#840) * fix: add verifyIsEmailUser wrapper to password endpoints * chore: update tests

alexanderleegs mentioned this pull request Jun 21, 2023

Feat/repo privatisation isomerpages/isomercms-frontend#1316

Merged

8 tasks

alexanderleegs requested a review from a team June 21, 2023 09:22

alexanderleegs force-pushed the develop branch from 9eeee28 to 32ef92c Compare June 22, 2023 08:41

alexanderleegs force-pushed the feat/IS-21-repo-privatisation branch from 94cb9f0 to 478aca0 Compare June 26, 2023 01:36

seaerchin reviewed Jun 26, 2023

View reviewed changes

kishore03109 reviewed Jun 26, 2023

View reviewed changes

alexanderleegs requested review from kishore03109 and seaerchin June 30, 2023 05:46

harishv7 reviewed Jun 30, 2023

View reviewed changes

kishore03109 reviewed Jul 5, 2023

View reviewed changes

alexanderleegs requested review from kishore03109 and harishv7 July 5, 2023 05:40

kishore03109 reviewed Jul 6, 2023

View reviewed changes

alexanderleegs added 16 commits July 7, 2023 09:37

chore: add env var and reformat env-example

43abd3b

feat: modify db and add migrations

a2e153d

feat: add util methods for password decryption

d748aa7

feat: add changeRepoPrivacy

c2c24ea

feat: add deploymentClient updateBranch methods

8713719

feat: add deploymentService methods

10ef732

feat: add methods to get and retrieve password

5f109a3

feat: add get and update password endpoints

6df41a9

chore: add schema

a887e91

chore: update initialisation

5036990

chore: update db schema to include date of password

c16cbeb

feat: add functionality to remove password

cfbba42

fix: change api to send/receive password directly

c946efa

chore: swap to kebab-case

bbbaef9

chore: underscore unused vars

f0d7bff

chore: fix name

8606da7

alexanderleegs added 15 commits July 7, 2023 09:37

chore: update comment

5e045cf

chore: update wrap

58adcab

chore: separate update and delete password

d2b315a

chore: split delete path into separate method

7aaf4dd

fix: set secret key to required

9223ac5

chore: rename arg

2d2146a

Fix: handle case where no password change necessary

90bbbf6

chore: enforce that decrypted password is not modified

6cdaa38

chore: update error type

ba263e9

fix: revert casting of amplify methods

a4fe9f4

chore: pass key as argument

7b127ce

chore: update DTO for netlify sites

c0c89cb

nit: rename repository to deploymentsRepository

230977e

fix: var name

58fa9d5

alexanderleegs force-pushed the feat/IS-21-repo-privatisation branch from 98a17a0 to 58fa9d5 Compare July 7, 2023 01:38

alexanderleegs requested a review from kishore03109 July 10, 2023 07:18

kishore03109 reviewed Jul 19, 2023

View reviewed changes

kishore03109 self-requested a review July 19, 2023 04:38

kishore03109 approved these changes Jul 19, 2023

View reviewed changes

alexanderleegs merged commit 4c120ed into develop Jul 19, 2023

mergify bot deleted the feat/IS-21-repo-privatisation branch July 19, 2023 06:13

kishore03109 mentioned this pull request Jul 19, 2023

Release/0.35.0 #845

Merged


		import { config } from "@config/config"

		const ALGORITHM = "aes-256-cbc"

	const ALGORITHM = "aes-256-cbc"
	const DECRYPTION_ALGORITHM = "aes-256-cbc"


		const ALGORITHM = "aes-256-cbc"

		export const decryptPassword = (encryptedPassword: string, iv: string) => {

Feat/is 21 repo privatisation #806

Feat/is 21 repo privatisation #806

Conversation

alexanderleegs commented Jun 21, 2023 • edited Loading

Problem

Tests

Deploy Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaerchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderleegs commented Jul 7, 2023

kishore03109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderleegs commented Jun 21, 2023 •

edited

Loading

kishore03109 left a comment •

edited

Loading