From 58c9df8796825bb838ced0c5ac35c59af0f610c9 Mon Sep 17 00:00:00 2001 From: Magnus Jensen Date: Mon, 6 Oct 2025 18:32:29 -0300 Subject: [PATCH] Smallstep qa fixes (#33885) --- ee/server/service/certificate_authorities_test.go | 10 +++++----- .../components/AddCertAuthorityModal/helpers.tsx | 8 ++++++-- .../components/EditCertAuthorityModal/helpers.tsx | 10 +++++++--- .../components/SmallstepForm/helpers.ts | 3 --- server/fleet/certificate_authorities.go | 12 ++---------- 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/ee/server/service/certificate_authorities_test.go b/ee/server/service/certificate_authorities_test.go index 7ca0301db0..8f62996dc4 100644 --- a/ee/server/service/certificate_authorities_test.go +++ b/ee/server/service/certificate_authorities_test.go @@ -1852,7 +1852,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) { require.EqualError(t, err, "Couldn't edit certificate authority. Invalid challenge URL or credentials. Please correct and try again.") }) - t.Run("Requires all fields when updating URL", func(t *testing.T) { + t.Run("Requires password when updating SCEP URL", func(t *testing.T) { svc, ctx := baseSetupForCATests() payload := fleet.CertificateAuthorityUpdatePayload{ SmallstepSCEPProxyCAUpdatePayload: &fleet.SmallstepSCEPProxyCAUpdatePayload{ @@ -1861,10 +1861,10 @@ func TestUpdatingCertificateAuthorities(t *testing.T) { } err := svc.UpdateCertificateAuthority(ctx, smallstepID, payload) - require.EqualError(t, err, "Couldn't edit certificate authority. \"challenge_url\", \"username\" and \"password\" must be set when modifying \"url\" of an existing certificate authority: Smallstep CA.") + require.EqualError(t, err, "Couldn't edit certificate authority. \"password\" must be set when modifying \"url\", \"challenge_url\", or \"username\" of an existing certificate authority: Smallstep CA") }) - t.Run("Requires password and username when updating challenge URL", func(t *testing.T) { + t.Run("Requires password when updating challenge URL", func(t *testing.T) { svc, ctx := baseSetupForCATests() payload := fleet.CertificateAuthorityUpdatePayload{ SmallstepSCEPProxyCAUpdatePayload: &fleet.SmallstepSCEPProxyCAUpdatePayload{ @@ -1873,7 +1873,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) { } err := svc.UpdateCertificateAuthority(ctx, smallstepID, payload) - require.EqualError(t, err, "Couldn't edit certificate authority. \"username\" and \"password\" must be set when modifying \"challenge_url\" of an existing certificate authority: Smallstep CA.") + require.EqualError(t, err, "Couldn't edit certificate authority. \"password\" must be set when modifying \"url\", \"challenge_url\", or \"username\" of an existing certificate authority: Smallstep CA") }) t.Run("Requires password when updating username", func(t *testing.T) { @@ -1885,7 +1885,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) { } err := svc.UpdateCertificateAuthority(ctx, smallstepID, payload) - require.EqualError(t, err, "Couldn't edit certificate authority. \"password\" must be set when modifying \"username\" of an existing certificate authority: Smallstep CA.") + require.EqualError(t, err, "Couldn't edit certificate authority. \"password\" must be set when modifying \"url\", \"challenge_url\", or \"username\" of an existing certificate authority: Smallstep CA") }) t.Run("Validates password and username on update", func(t *testing.T) { diff --git a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/AddCertAuthorityModal/helpers.tsx b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/AddCertAuthorityModal/helpers.tsx index 3aacb547ed..9cd3aa0fbf 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/AddCertAuthorityModal/helpers.tsx +++ b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/AddCertAuthorityModal/helpers.tsx @@ -185,12 +185,14 @@ const NDES_PASSWORD_CACHE_FULL_ERROR = "The NDES password cache is full. Please increase the number of cached passwords in NDES and try again."; const INVALID_CHALLENGE_ERROR = "Invalid challenge. Please correct and try again."; +const INVALID_CHALLENGE_URL_OR_CREDENTIALS_ERROR = + "Invalid challenge URL or credentials. Please correct and try again."; /** * Gets the error message we want to display from the api error message. * This is used in both add and edit certificate authority flows. */ -export const getDisplayErrMessage = (err: unknown) => { +export const getDisplayErrMessage = (err: unknown): string | JSX.Element => { let message: string | JSX.Element = DEFAULT_ERROR; const reason = getErrorReason(err).toLowerCase(); @@ -211,6 +213,8 @@ export const getDisplayErrMessage = (err: unknown) => { message = INVALID_ADMIN_URL_OR_CREDENTIALS_ERROR; } else if (reason.includes("password cache is full")) { message = NDES_PASSWORD_CACHE_FULL_ERROR; + } else if (reason.includes("invalid challenge url")) { + message = INVALID_CHALLENGE_URL_OR_CREDENTIALS_ERROR; } else if (reason.includes("invalid challenge")) { message = INVALID_CHALLENGE_ERROR; } else { @@ -220,7 +224,7 @@ export const getDisplayErrMessage = (err: unknown) => { return message; }; -export const getErrorMessage = (err: unknown) => { +export const getErrorMessage = (err: unknown): JSX.Element => { return ( <>Couldn't add certificate authority. {getDisplayErrMessage(err)} ); diff --git a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/EditCertAuthorityModal/helpers.tsx b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/EditCertAuthorityModal/helpers.tsx index 5cc17404cb..b76497eccc 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/EditCertAuthorityModal/helpers.tsx +++ b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/EditCertAuthorityModal/helpers.tsx @@ -1,3 +1,5 @@ +import React from "react"; + import { IEditCertAuthorityBody } from "services/entities/certificates"; import { ICertificateAuthority, @@ -150,7 +152,7 @@ export const generateEditCertAuthorityData = ( smallstep: deepDifference( { name: smallstepName, - scep_url: smallstepURL, + url: smallstepURL, challenge_url: smallstepChallengeURL, username: smallstepUsername, password: smallstepPassword, @@ -255,6 +257,8 @@ export const updateFormData = ( return newData; }; -export const getErrorMessage = (err: unknown) => { - return `Couldn't edit certificate authority. ${getDisplayErrMessage(err)}`; +export const getErrorMessage = (err: unknown): JSX.Element => { + return ( + <>Couldn't edit certificate authority. {getDisplayErrMessage(err)} + ); }; diff --git a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/SmallstepForm/helpers.ts b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/SmallstepForm/helpers.ts index fbc3ed6a9e..33dfcb5201 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/SmallstepForm/helpers.ts +++ b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/SmallstepForm/helpers.ts @@ -160,9 +160,6 @@ export const validateFormData = ( message: getErrorMessage(formData, failedValidation.message), }; } - - console.log("objKey", objKey); - console.log("formValidation", formValidation); }); return formValidation; diff --git a/server/fleet/certificate_authorities.go b/server/fleet/certificate_authorities.go index 8f5473c6dd..099201b74e 100644 --- a/server/fleet/certificate_authorities.go +++ b/server/fleet/certificate_authorities.go @@ -385,16 +385,8 @@ func (sscepp SmallstepSCEPProxyCAUpdatePayload) IsEmpty() bool { // ValidateRelatedFields verifies that fields that are related to each other are set correctly. // For example if the Name is provided then the Challenge URL, Username and Password must also be provided func (sscepp *SmallstepSCEPProxyCAUpdatePayload) ValidateRelatedFields(errPrefix string, certName string) error { - if sscepp.URL != nil && (sscepp.ChallengeURL == nil || sscepp.Username == nil || sscepp.Password == nil) { - return &BadRequestError{Message: fmt.Sprintf(`%s"challenge_url", "username" and "password" must be set when modifying "url" of an existing certificate authority: %s.`, errPrefix, certName)} - } - - if sscepp.ChallengeURL != nil && sscepp.Username == nil && sscepp.Password == nil { - return &BadRequestError{Message: fmt.Sprintf(`%s"username" and "password" must be set when modifying "challenge_url" of an existing certificate authority: %s.`, errPrefix, certName)} - } - - if sscepp.ChallengeURL == nil && sscepp.URL == nil && sscepp.Username != nil && sscepp.Password == nil { - return &BadRequestError{Message: fmt.Sprintf(`%s"password" must be set when modifying "username" of an existing certificate authority: %s.`, errPrefix, certName)} + if (sscepp.URL != nil || sscepp.ChallengeURL != nil || sscepp.Username != nil) && sscepp.Password == nil { + return &BadRequestError{Message: fmt.Sprintf(`%s"password" must be set when modifying "url", "challenge_url", or "username" of an existing certificate authority: %s`, errPrefix, certName)} } return nil