From fd1a3d5d123fc77917c5768204afd907d8f31043 Mon Sep 17 00:00:00 2001 From: Piyush Gupta Date: Fri, 5 Dec 2025 19:39:21 +0530 Subject: [PATCH] fix: review changes --- .../services/external-kms/providers/model.ts | 7 +- frontend/src/hooks/api/kms/types.ts | 2 +- .../OrgEncryptionTab/AwsKmsForm.tsx | 255 ++++++++++-------- .../EditExternalKmsCredentialsModal.tsx | 13 +- .../EditExternalKmsDetailsModal.tsx | 110 ++------ .../OrgEncryptionTab/GcpKmsForm.tsx | 228 +++++++++------- 6 files changed, 309 insertions(+), 306 deletions(-) diff --git a/backend/src/ee/services/external-kms/providers/model.ts b/backend/src/ee/services/external-kms/providers/model.ts index 13d374b26..08a9a3fc7 100644 --- a/backend/src/ee/services/external-kms/providers/model.ts +++ b/backend/src/ee/services/external-kms/providers/model.ts @@ -26,7 +26,7 @@ const AwsConnectionAssumeRoleCredentialsSchema = z.object({ .trim() .min(1) .optional() - .describe("AWS assume role external id for furthur security in authentication") + .describe("AWS assume role external id for further security in authentication") }); const AwsConnectionAccessTokenCredentialsSchema = z.object({ @@ -64,7 +64,10 @@ export const SanitizedExternalKmsAwsSchema = ExternalKmsAwsSchema.extend({ }), z.object({ type: z.literal(KmsAwsCredentialType.AssumeRole), - data: AwsConnectionAssumeRoleCredentialsSchema.pick({}) + data: AwsConnectionAssumeRoleCredentialsSchema.pick({ + assumeRoleArn: true, + externalId: true + }) }) ]) }); diff --git a/frontend/src/hooks/api/kms/types.ts b/frontend/src/hooks/api/kms/types.ts index a5b514bfe..32c2fa32a 100644 --- a/frontend/src/hooks/api/kms/types.ts +++ b/frontend/src/hooks/api/kms/types.ts @@ -131,7 +131,7 @@ export type AddExternalKmsType = z.infer; // we need separate schema for update because the credential field is not required on GCP export const ExternalKmsUpdateInputSchema = z.discriminatedUnion("type", [ - z.object({ type: z.literal(ExternalKmsProvider.Aws), inputs: ExternalKmsAwsSchema }), + z.object({ type: z.literal(ExternalKmsProvider.Aws), inputs: ExternalKmsAwsSchema.partial() }), z.object({ type: z.literal(ExternalKmsProvider.Gcp), inputs: ExternalKmsGcpSchema.pick({ gcpRegion: true, keyName: true }) diff --git a/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/AwsKmsForm.tsx b/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/AwsKmsForm.tsx index 980e098cd..fe063d88a 100644 --- a/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/AwsKmsForm.tsx +++ b/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/AwsKmsForm.tsx @@ -11,7 +11,8 @@ import { AddExternalKmsType, ExternalKmsProvider, Kms, - KmsAwsCredentialType + KmsAwsCredentialType, + UpdateExternalKmsSchema } from "@app/hooks/api/kms/types"; const AWS_REGIONS = [ @@ -50,10 +51,12 @@ type Props = { onCompleted: () => void; onCancel: () => void; kms?: Kms; - mode?: "full" | "credentials"; + mode?: "full" | "credentials" | "details"; }; export const AwsKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) => { + const validationSchema = kms ? UpdateExternalKmsSchema : AddExternalKmsSchema; + const { control, handleSubmit, @@ -61,24 +64,35 @@ export const AwsKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) setValue, formState: { isSubmitting } } = useForm({ - resolver: zodResolver(AddExternalKmsSchema), + resolver: zodResolver(validationSchema), defaultValues: { name: kms?.name, description: kms?.description ?? "", configuration: { type: ExternalKmsProvider.Aws, inputs: { - credential: { - type: kms?.externalKms?.configuration?.credential?.type, - data: { - accessKey: kms?.externalKms?.configuration?.credential?.data?.accessKey, - secretKey: kms?.externalKms?.configuration?.credential?.data?.secretKey, - assumeRoleArn: kms?.externalKms?.configuration?.credential?.data?.assumeRoleArn, - externalId: kms?.externalKms?.configuration?.credential?.data?.externalId - } - }, - awsRegion: kms?.externalKms?.configuration?.awsRegion, - kmsKeyId: kms?.externalKms?.configuration?.kmsKeyId + ...(mode !== "details" && + kms?.externalKms?.configuration?.credential?.type && + kms.externalKms.configuration.credential.data + ? { + credential: { + type: kms.externalKms.configuration.credential.type, + data: { + accessKey: kms.externalKms.configuration.credential.data?.accessKey ?? "", + secretKey: kms.externalKms.configuration.credential.data?.secretKey ?? "", + assumeRoleArn: + kms.externalKms.configuration.credential.data?.assumeRoleArn ?? "", + externalId: kms.externalKms.configuration.credential.data?.externalId ?? "" + } + } + } + : {}), + ...(mode !== "credentials" + ? { + awsRegion: kms?.externalKms?.configuration?.awsRegion ?? "", + kmsKeyId: kms?.externalKms?.configuration?.kmsKeyId ?? "" + } + : {}) } } } @@ -97,16 +111,33 @@ export const AwsKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) const { name, description, configuration } = data; try { if (kms) { + if (configuration.type !== ExternalKmsProvider.Aws) { + throw new Error("Invalid configuration type"); + } + const awsInputs = configuration.inputs; + if (mode === "credentials") { await updateAwsExternalKms({ kmsId: kms.id, - configuration + configuration: { + type: ExternalKmsProvider.Aws, + inputs: { + credential: { ...awsInputs.credential } + } + } }); } else { await updateAwsExternalKms({ kmsId: kms.id, name, - description + description, + configuration: { + type: ExternalKmsProvider.Aws, + inputs: { + awsRegion: awsInputs.awsRegion, + kmsKeyId: awsInputs.kmsKeyId + } + } }); } @@ -114,7 +145,7 @@ export const AwsKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) text: mode === "credentials" ? "Successfully updated AWS External KMS credentials" - : "Successfully updated AWS External KMS", + : "Successfully updated AWS External KMS Details", type: "success" }); } else { @@ -138,7 +169,7 @@ export const AwsKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) return (
- {mode === "full" && ( + {(mode === "full" || mode === "details") && ( <> )} - ( - - - - )} - /> - - {selectedAwsAuthType === KmsAwsCredentialType.AccessKey ? ( + {(mode === "full" || mode === "credentials") && ( <> ( + name="configuration.inputs.credential.type" + defaultValue={KmsAwsCredentialType.AssumeRole} + render={({ field: { onChange, ...field }, fieldState: { error } }) => ( - - - )} - /> - ( - - + )} /> + + {selectedAwsAuthType === KmsAwsCredentialType.AccessKey ? ( + <> + ( + + + + )} + /> + ( + + + + )} + /> + + ) : ( + <> + ( + + + + )} + /> + ( + + + + )} + /> + + )} - ) : ( + )} + {(mode === "full" || mode === "details") && ( <> ( - - + name="configuration.inputs.awsRegion" + render={({ field: { onChange, ...field }, fieldState: { error } }) => ( + + )} /> ( @@ -249,35 +321,6 @@ export const AwsKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) /> )} - ( - - - - )} - /> - ( - - - - )} - />
- - - -
- - - ); -}; - export const EditExternalKmsDetailsModal = ({ isOpen, onOpenChange, kmsId, provider }: Props) => { - const { data: kms, isPending } = useGetExternalKmsById({ kmsId, provider }); + const { data: kms } = useGetExternalKmsById({ kmsId, provider }); return ( - {isPending && } - {kms && onOpenChange(false)} />} + {kms?.externalKms?.provider === ExternalKmsProvider.Aws && ( + onOpenChange(false)} + onCompleted={() => onOpenChange(false)} + /> + )} + {kms?.externalKms?.provider === ExternalKmsProvider.Gcp && ( + onOpenChange(false)} + onCompleted={() => onOpenChange(false)} + /> + )} ); diff --git a/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/GcpKmsForm.tsx b/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/GcpKmsForm.tsx index d120bb658..7b80a33d2 100644 --- a/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/GcpKmsForm.tsx +++ b/frontend/src/pages/organization/SettingsPage/components/OrgEncryptionTab/GcpKmsForm.tsx @@ -24,7 +24,7 @@ type Props = { onCompleted: () => void; onCancel: () => void; kms?: Kms; - mode?: "full" | "credentials"; + mode?: "full" | "credentials" | "details"; }; const GCP_REGIONS = [ @@ -145,53 +145,70 @@ export const GcpKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) // handles the form submission const handleGcpKmsFormSubmit = async (data: AddExternalKmsGcpFormSchemaType) => { const { name, description, gcpRegion: gcpRegionObject, keyObject } = data; - const gcpRegion = gcpRegionObject.value; - if (!keys.find((k) => k.value === keyObject?.value)) { - setError("keyObject", { - message: "Please select a valid key." - }); - resetField("keyObject"); - return; - } try { if (kms) { - if (mode === "credentials") { - await updateGcpExternalKms({ - kmsId: kms.id, - name: kms.name, - description: kms.description, - configuration: { - type: ExternalKmsProvider.Gcp, - inputs: { - gcpRegion, - keyName: keyObject?.value - } - } - }); - } else { + if (mode === "details") { await updateGcpExternalKms({ kmsId: kms.id, name, - description, + description + }); + + createNotification({ + text: "Successfully updated GCP External KMS Details", + type: "success" + }); + } else if (mode === "credentials") { + const gcpRegion = gcpRegionObject?.value; + if (!gcpRegion) { + setError("gcpRegion", { + message: "Please select a GCP region." + }); + return; + } + + if (keyObject && !keys.find((k) => k.value === keyObject.value)) { + setError("keyObject", { + message: "Please select a valid key." + }); + resetField("keyObject"); + return; + } + + await updateGcpExternalKms({ + kmsId: kms.id, configuration: { type: ExternalKmsProvider.Gcp, inputs: { gcpRegion, - keyName: keyObject?.value + keyName: keyObject?.value ?? kms.externalKms.configuration.keyName } } }); + + createNotification({ + text: "Successfully updated GCP External KMS configuration", + type: "success" + }); + } + } else { + const gcpRegion = gcpRegionObject?.value; + if (!gcpRegion) { + setError("gcpRegion", { + message: "Please select a GCP region." + }); + return; + } + + if (!keys.find((k) => k.value === keyObject?.value)) { + setError("keyObject", { + message: "Please select a valid key." + }); + resetField("keyObject"); + return; } - createNotification({ - text: - mode === "credentials" - ? "Successfully updated GCP External KMS configuration" - : "Successfully updated GCP External KMS", - type: "success" - }); - } else { const credentialJson = await getCredentialFileJson(); if (!credentialJson) { return; @@ -230,8 +247,9 @@ export const GcpKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) if (!kms && !credentialJson) { return; } - const gcpRegion = getValues("gcpRegion").value; - if (!gcpRegion.length) { + const gcpRegionObject = getValues("gcpRegion"); + const gcpRegion = gcpRegionObject?.value; + if (!gcpRegion) { setError("gcpRegion", { message: "Please select a GCP region to fetch GCP Keys." }); @@ -284,7 +302,7 @@ export const GcpKmsForm = ({ onCompleted, onCancel, kms, mode = "full" }: Props) return (
- {mode === "full" && ( + {(mode === "full" || mode === "details") && ( <> )} - ( - - { - resetField("keyObject"); - field.onChange(e); - fetchGCPKeys(); - }} - formatOptionLabel={formatOptionLabel} + {(mode === "full" || mode === "credentials") && ( + <> + ( + + { + resetField("keyObject"); + field.onChange(e); + fetchGCPKeys(); + }} + formatOptionLabel={formatOptionLabel} + /> + + )} + /> + {!kms && ( + ( + + { + onChange(e.target.files); + fetchGCPKeys(); + }} + /> + + )} /> - - )} - /> - {!kms && ( - ( - - { - onChange(e.target.files); - fetchGCPKeys(); - }} - /> - )} - /> - )} - ( - - - - )} - /> - {kms && ( - - To change your GCP credentials, create a new external KMS and assign it to project you - want to use it with. - + ( + + + + )} + /> + {kms && ( + + To change your GCP credentials, create a new external KMS and assign it to project you + want to use it with. + + )} + )}