From 0a9d39173206c3eb115c0812abb0629fd1b05dad Mon Sep 17 00:00:00 2001 From: x032205 Date: Tue, 21 Oct 2025 19:47:58 -0400 Subject: [PATCH] greptile review comment --- .../20251015042917_pam-account-rotation.ts | 34 ++-- .../services/pam-account/pam-account-dal.ts | 1 + .../pam-account/pam-account-service.ts | 154 ++++++++++-------- frontend/src/consts/pam.ts | 0 frontend/src/hooks/api/pam/constants.ts | 1 + .../PamAccountForm/PostgresAccountForm.tsx | 3 +- .../shared/SqlAccountFields.tsx | 16 +- .../PamResourceForm/PostgresResourceForm.tsx | 3 +- .../shared/SqlRotateAccountFields.tsx | 16 +- 9 files changed, 135 insertions(+), 93 deletions(-) create mode 100644 frontend/src/consts/pam.ts create mode 100644 frontend/src/hooks/api/pam/constants.ts diff --git a/backend/src/db/migrations/20251015042917_pam-account-rotation.ts b/backend/src/db/migrations/20251015042917_pam-account-rotation.ts index 4e9fc90d4..b83dae0ae 100644 --- a/backend/src/db/migrations/20251015042917_pam-account-rotation.ts +++ b/backend/src/db/migrations/20251015042917_pam-account-rotation.ts @@ -3,18 +3,21 @@ import { Knex } from "knex"; import { TableName } from "../schemas"; export async function up(knex: Knex): Promise { - if ( - !(await knex.schema.hasColumn(TableName.PamAccount, "rotationEnabled")) && - !(await knex.schema.hasColumn(TableName.PamAccount, "rotationIntervalSeconds")) && - !(await knex.schema.hasColumn(TableName.PamAccount, "lastRotatedAt")) - ) { + if (!(await knex.schema.hasColumn(TableName.PamAccount, "rotationEnabled"))) { await knex.schema.alterTable(TableName.PamAccount, (t) => { t.boolean("rotationEnabled").notNullable().defaultTo(false); + }); + } + if (!(await knex.schema.hasColumn(TableName.PamAccount, "rotationIntervalSeconds"))) { + await knex.schema.alterTable(TableName.PamAccount, (t) => { t.integer("rotationIntervalSeconds").nullable(); + }); + } + if (!(await knex.schema.hasColumn(TableName.PamAccount, "lastRotatedAt"))) { + await knex.schema.alterTable(TableName.PamAccount, (t) => { t.timestamp("lastRotatedAt").nullable(); }); } - if (!(await knex.schema.hasColumn(TableName.PamResource, "encryptedRotationAccountCredentials"))) { await knex.schema.alterTable(TableName.PamResource, (t) => { t.binary("encryptedRotationAccountCredentials").nullable(); @@ -28,16 +31,19 @@ export async function down(knex: Knex): Promise { t.dropColumn("encryptedRotationAccountCredentials"); }); } - - if ( - (await knex.schema.hasColumn(TableName.PamAccount, "rotationEnabled")) && - (await knex.schema.hasColumn(TableName.PamAccount, "rotationIntervalSeconds")) && - (await knex.schema.hasColumn(TableName.PamAccount, "lastRotatedAt")) - ) { + if (await knex.schema.hasColumn(TableName.PamAccount, "rotationEnabled")) { await knex.schema.alterTable(TableName.PamAccount, (t) => { - t.dropColumn("lastRotatedAt"); - t.dropColumn("rotationIntervalSeconds"); t.dropColumn("rotationEnabled"); }); } + if (await knex.schema.hasColumn(TableName.PamAccount, "rotationIntervalSeconds")) { + await knex.schema.alterTable(TableName.PamAccount, (t) => { + t.dropColumn("rotationIntervalSeconds"); + }); + } + if (await knex.schema.hasColumn(TableName.PamAccount, "lastRotatedAt")) { + await knex.schema.alterTable(TableName.PamAccount, (t) => { + t.dropColumn("lastRotatedAt"); + }); + } } diff --git a/backend/src/ee/services/pam-account/pam-account-dal.ts b/backend/src/ee/services/pam-account/pam-account-dal.ts index 4c8f5136a..6ef7df76e 100644 --- a/backend/src/ee/services/pam-account/pam-account-dal.ts +++ b/backend/src/ee/services/pam-account/pam-account-dal.ts @@ -50,6 +50,7 @@ export const pamAccountDALFactory = (db: TDbClient) => { .innerJoin(TableName.PamResource, `${TableName.PamAccount}.resourceId`, `${TableName.PamResource}.id`) .whereNotNull(`${TableName.PamResource}.encryptedRotationAccountCredentials`) .whereNotNull(`${TableName.PamAccount}.rotationIntervalSeconds`) + .where(`${TableName.PamAccount}.rotationEnabled`, true) .whereRaw( `COALESCE("${TableName.PamAccount}"."lastRotatedAt", "${TableName.PamAccount}"."createdAt") + "${TableName.PamAccount}"."rotationIntervalSeconds" * interval '1 second' < NOW()` ) diff --git a/backend/src/ee/services/pam-account/pam-account-service.ts b/backend/src/ee/services/pam-account/pam-account-service.ts index 2dfdec196..2dfc3bade 100644 --- a/backend/src/ee/services/pam-account/pam-account-service.ts +++ b/backend/src/ee/services/pam-account/pam-account-service.ts @@ -85,6 +85,12 @@ export const pamAccountServiceFactory = ({ }); } + if (rotationEnabled && (rotationIntervalSeconds === undefined || rotationIntervalSeconds === null)) { + throw new BadRequestError({ + message: "Rotation interval must be defined when rotation is enabled." + }); + } + const resource = await pamResourceDAL.findById(resourceId); if (!resource) throw new NotFoundError({ message: `Resource with ID '${resourceId}' not found` }); @@ -568,87 +574,97 @@ export const pamAccountServiceFactory = ({ for (let i = 0; i < accounts.length; i += ROTATION_CONCURRENCY_LIMIT) { const batch = accounts.slice(i, i + ROTATION_CONCURRENCY_LIMIT); - const rotationPromises = batch.map(async (account) => { - let logResourceType = "unknown"; - try { - const resource = await pamResourceDAL.findById(account.resourceId); - if (!resource || !resource.encryptedRotationAccountCredentials) return; - logResourceType = resource.resourceType; + const rotationPromises = batch.map(async (account) => + pamAccountDAL.transaction(async (tx) => { + let logResourceType = "unknown"; + try { + const resource = await pamResourceDAL.findById(account.resourceId, tx); + if (!resource || !resource.encryptedRotationAccountCredentials) return; + logResourceType = resource.resourceType; - const { connectionDetails, rotationAccountCredentials, gatewayId, resourceType } = await decryptResource( - resource, - account.projectId, - kmsService - ); + const { connectionDetails, rotationAccountCredentials, gatewayId, resourceType } = await decryptResource( + resource, + account.projectId, + kmsService + ); - if (!rotationAccountCredentials) return; + if (!rotationAccountCredentials) return; - const accountCredentials = await decryptAccountCredentials({ - encryptedCredentials: account.encryptedCredentials, - projectId: account.projectId, - kmsService - }); + const accountCredentials = await decryptAccountCredentials({ + encryptedCredentials: account.encryptedCredentials, + projectId: account.projectId, + kmsService + }); - const factory = PAM_RESOURCE_FACTORY_MAP[resourceType as PamResource]( - resourceType as PamResource, - connectionDetails, - gatewayId, - gatewayV2Service - ); + const factory = PAM_RESOURCE_FACTORY_MAP[resourceType as PamResource]( + resourceType as PamResource, + connectionDetails, + gatewayId, + gatewayV2Service + ); - const newCredentials = await factory.rotateAccountCredentials(rotationAccountCredentials, accountCredentials); + const newCredentials = await factory.rotateAccountCredentials( + rotationAccountCredentials, + accountCredentials + ); - const encryptedCredentials = await encryptAccountCredentials({ - credentials: newCredentials, - projectId: account.projectId, - kmsService - }); + const encryptedCredentials = await encryptAccountCredentials({ + credentials: newCredentials, + projectId: account.projectId, + kmsService + }); - await pamAccountDAL.updateById(account.id, { - encryptedCredentials, - lastRotatedAt: new Date() - }); + await pamAccountDAL.updateById( + account.id, + { + encryptedCredentials, + lastRotatedAt: new Date() + }, + tx + ); - await auditLogService.createAuditLog({ - projectId: account.projectId, - actor: { - type: ActorType.PLATFORM, - metadata: {} - }, - event: { - type: EventType.PAM_ACCOUNT_CREDENTIAL_ROTATION, - metadata: { - accountId: account.id, - accountName: account.name, - resourceId: resource.id, - resourceType: logResourceType + await auditLogService.createAuditLog({ + projectId: account.projectId, + actor: { + type: ActorType.PLATFORM, + metadata: {} + }, + event: { + type: EventType.PAM_ACCOUNT_CREDENTIAL_ROTATION, + metadata: { + accountId: account.id, + accountName: account.name, + resourceId: resource.id, + resourceType: logResourceType + } } - } - }); - } catch (error) { - logger.error(error, `Failed to rotate credentials for account [accountId=${account.id}]`); + }); + } catch (error) { + logger.error(error, `Failed to rotate credentials for account [accountId=${account.id}]`); - const errorMessage = error instanceof Error ? error.message : "An unknown error occurred"; + const errorMessage = error instanceof Error ? error.message : "An unknown error occurred"; - await auditLogService.createAuditLog({ - projectId: account.projectId, - actor: { - type: ActorType.PLATFORM, - metadata: {} - }, - event: { - type: EventType.PAM_ACCOUNT_CREDENTIAL_ROTATION_FAILED, - metadata: { - accountId: account.id, - accountName: account.name, - resourceId: account.resourceId, - resourceType: logResourceType, - errorMessage + await auditLogService.createAuditLog({ + projectId: account.projectId, + actor: { + type: ActorType.PLATFORM, + metadata: {} + }, + event: { + type: EventType.PAM_ACCOUNT_CREDENTIAL_ROTATION_FAILED, + metadata: { + accountId: account.id, + accountName: account.name, + resourceId: account.resourceId, + resourceType: logResourceType, + errorMessage + } } - } - }); - } - }); + }); + throw error; // Rollback transaction + } + }) + ); // eslint-disable-next-line no-await-in-loop await Promise.all(rotationPromises); diff --git a/frontend/src/consts/pam.ts b/frontend/src/consts/pam.ts new file mode 100644 index 000000000..e69de29bb diff --git a/frontend/src/hooks/api/pam/constants.ts b/frontend/src/hooks/api/pam/constants.ts new file mode 100644 index 000000000..8cdbd3324 --- /dev/null +++ b/frontend/src/hooks/api/pam/constants.ts @@ -0,0 +1 @@ +export const UNCHANGED_PASSWORD_SENTINEL = "__INFISICAL_UNCHANGED__"; diff --git a/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/PostgresAccountForm.tsx b/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/PostgresAccountForm.tsx index 17d68bdf0..3e5994344 100644 --- a/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/PostgresAccountForm.tsx +++ b/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/PostgresAccountForm.tsx @@ -5,6 +5,7 @@ import { z } from "zod"; import { Button, ModalClose } from "@app/components/v2"; import { PamResourceType, TPostgresAccount, useGetPamResourceById } from "@app/hooks/api/pam"; +import { UNCHANGED_PASSWORD_SENTINEL } from "@app/hooks/api/pam/constants"; import { BaseSqlAccountSchema } from "./shared/sql-account-schemas"; import { SqlAccountFields } from "./shared/SqlAccountFields"; @@ -34,7 +35,7 @@ export const PostgresAccountForm = ({ account, resourceId, resourceType, onSubmi ...account, credentials: { ...account.credentials, - password: "__INFISICAL_UNCHANGED__" + password: UNCHANGED_PASSWORD_SENTINEL } } : undefined diff --git a/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/shared/SqlAccountFields.tsx b/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/shared/SqlAccountFields.tsx index 0fd5760f3..8d507c01a 100644 --- a/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/shared/SqlAccountFields.tsx +++ b/frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/shared/SqlAccountFields.tsx @@ -1,11 +1,19 @@ -import { useState } from "react"; -import { Controller, useFormContext } from "react-hook-form"; +import { useEffect, useState } from "react"; +import { Controller, useFormContext, useWatch } from "react-hook-form"; import { FormControl, Input } from "@app/components/v2"; +import { UNCHANGED_PASSWORD_SENTINEL } from "@app/hooks/api/pam/constants"; export const SqlAccountFields = ({ isUpdate }: { isUpdate: boolean }) => { const { control } = useFormContext(); const [showPassword, setShowPassword] = useState(false); + const password = useWatch({ control, name: "credentials.password" }); + + useEffect(() => { + if (password === UNCHANGED_PASSWORD_SENTINEL) { + setShowPassword(false); + } + }, [password]); return (
@@ -38,14 +46,14 @@ export const SqlAccountFields = ({ isUpdate }: { isUpdate: boolean }) => { type={showPassword ? "text" : "password"} autoComplete="new-password" onFocus={() => { - if (isUpdate && field.value === "__INFISICAL_UNCHANGED__") { + if (isUpdate && field.value === UNCHANGED_PASSWORD_SENTINEL) { field.onChange(""); } setShowPassword(true); }} onBlur={() => { if (isUpdate && field.value === "") { - field.onChange("__INFISICAL_UNCHANGED__"); + field.onChange(UNCHANGED_PASSWORD_SENTINEL); } setShowPassword(false); }} diff --git a/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/PostgresResourceForm.tsx b/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/PostgresResourceForm.tsx index 2c53c89df..7e96fffda 100644 --- a/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/PostgresResourceForm.tsx +++ b/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/PostgresResourceForm.tsx @@ -5,6 +5,7 @@ import { z } from "zod"; import { Button, ModalClose } from "@app/components/v2"; import { PamResourceType, TPostgresResource } from "@app/hooks/api/pam"; +import { UNCHANGED_PASSWORD_SENTINEL } from "@app/hooks/api/pam/constants"; import { BaseSqlAccountSchema } from "@app/pages/pam/PamAccountsPage/components/PamAccountForm/shared/sql-account-schemas"; import { BaseSqlResourceSchema } from "./shared/sql-resource-schemas"; @@ -37,7 +38,7 @@ export const PostgresResourceForm = ({ resource, onSubmit }: Props) => { rotationAccountCredentials: resource.rotationAccountCredentials ? { ...resource.rotationAccountCredentials, - password: "__INFISICAL_UNCHANGED__" + password: UNCHANGED_PASSWORD_SENTINEL } : resource.rotationAccountCredentials } diff --git a/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/shared/SqlRotateAccountFields.tsx b/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/shared/SqlRotateAccountFields.tsx index 6ce92b8e6..e7e9023dd 100644 --- a/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/shared/SqlRotateAccountFields.tsx +++ b/frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/shared/SqlRotateAccountFields.tsx @@ -1,5 +1,5 @@ -import { useState } from "react"; -import { Controller, useFormContext } from "react-hook-form"; +import { useEffect, useState } from "react"; +import { Controller, useFormContext, useWatch } from "react-hook-form"; import { Accordion, @@ -9,10 +9,18 @@ import { FormControl, Input } from "@app/components/v2"; +import { UNCHANGED_PASSWORD_SENTINEL } from "@app/hooks/api/pam/constants"; export const SqlRotateAccountFields = ({ isUpdate }: { isUpdate: boolean }) => { const { control } = useFormContext(); const [showPassword, setShowPassword] = useState(false); + const password = useWatch({ control, name: "credentials.password" }); + + useEffect(() => { + if (password === UNCHANGED_PASSWORD_SENTINEL) { + setShowPassword(false); + } + }, [password]); return ( @@ -55,14 +63,14 @@ export const SqlRotateAccountFields = ({ isUpdate }: { isUpdate: boolean }) => { type={showPassword ? "text" : "password"} autoComplete="new-password" onFocus={() => { - if (isUpdate && field.value === "__INFISICAL_UNCHANGED__") { + if (isUpdate && field.value === UNCHANGED_PASSWORD_SENTINEL) { field.onChange(""); } setShowPassword(true); }} onBlur={() => { if (isUpdate && field.value === "") { - field.onChange("__INFISICAL_UNCHANGED__"); + field.onChange(UNCHANGED_PASSWORD_SENTINEL); } setShowPassword(false); }}