From c209d0b2e050889f3b6807d773d26df1b3244889 Mon Sep 17 00:00:00 2001 From: Wes Date: Wed, 11 Dec 2024 15:41:22 -0500 Subject: [PATCH] BED-5066: Return conflict error if SSO provider name conflicts Add better error messaging on UI for this case --- cmd/api/src/api/error.go | 1 + cmd/api/src/api/v2/auth/oidc.go | 8 ++++++- cmd/api/src/api/v2/auth/saml.go | 7 +++++- cmd/api/src/database/db.go | 5 +++-- cmd/api/src/database/sso_providers.go | 10 ++++++++- .../harnesses/enrollonbehalfof-1.svg | 19 +++++++++++++++- .../harnesses/enrollonbehalfof-2.svg | 19 +++++++++++++++- .../harnesses/enrollonbehalfof-3.svg | 19 +++++++++++++++- .../components/UpsertOIDCProviderDialog.tsx | 2 +- .../src/components/UpsertOIDCProviderForm.tsx | 22 +++++++++++++++---- .../UpsertSAMLProviderDialog.tsx | 2 +- .../UpsertSAMLProviderForm.tsx | 22 +++++++++++++++---- .../SSOConfiguration/SSOConfiguration.tsx | 18 ++++++--------- 13 files changed, 125 insertions(+), 29 deletions(-) diff --git a/cmd/api/src/api/error.go b/cmd/api/src/api/error.go index 4827d1031a..e2f4c9ae38 100644 --- a/cmd/api/src/api/error.go +++ b/cmd/api/src/api/error.go @@ -66,6 +66,7 @@ const ( ErrorResponseAGNameTagEmpty = "asset group name or tag must not be empty" ErrorResponseAGDuplicateName = "asset group name must be unique" ErrorResponseAGDuplicateTag = "asset group tag must be unique" + ErrorResponseSSOProviderDuplicateName = "sso provider name must be unique" ErrorResponseDetailsUniqueViolation = "unique constraint was violated" ErrorResponseDetailsNotImplemented = "All good things to those who wait. Not implemented." diff --git a/cmd/api/src/api/v2/auth/oidc.go b/cmd/api/src/api/v2/auth/oidc.go index 76bd695ccd..b8f1b21f41 100644 --- a/cmd/api/src/api/v2/auth/oidc.go +++ b/cmd/api/src/api/v2/auth/oidc.go @@ -17,6 +17,7 @@ package auth import ( + "errors" "fmt" "net/http" "time" @@ -26,6 +27,7 @@ import ( "github.com/specterops/bloodhound/src/api" "github.com/specterops/bloodhound/src/config" "github.com/specterops/bloodhound/src/ctx" + "github.com/specterops/bloodhound/src/database" "github.com/specterops/bloodhound/src/model" "github.com/specterops/bloodhound/src/utils/validation" "golang.org/x/oauth2" @@ -82,7 +84,11 @@ func (s ManagementResource) CreateOIDCProvider(response http.ResponseWriter, req api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, validated.Error(), request), response) } else { if oidcProvider, err := s.db.CreateOIDCProvider(request.Context(), upsertReq.Name, upsertReq.Issuer, upsertReq.ClientID); err != nil { - api.HandleDatabaseError(request, response, err) + if errors.Is(err, database.ErrDuplicateSSOProviderName) { + api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusConflict, api.ErrorResponseSSOProviderDuplicateName, request), response) + } else { + api.HandleDatabaseError(request, response, err) + } } else { api.WriteBasicResponse(request.Context(), oidcProvider, http.StatusCreated, response) } diff --git a/cmd/api/src/api/v2/auth/saml.go b/cmd/api/src/api/v2/auth/saml.go index 41827e3f6c..e35ade652a 100644 --- a/cmd/api/src/api/v2/auth/saml.go +++ b/cmd/api/src/api/v2/auth/saml.go @@ -36,6 +36,7 @@ import ( v2 "github.com/specterops/bloodhound/src/api/v2" "github.com/specterops/bloodhound/src/auth" "github.com/specterops/bloodhound/src/ctx" + "github.com/specterops/bloodhound/src/database" "github.com/specterops/bloodhound/src/model" ) @@ -156,7 +157,11 @@ func (s ManagementResource) CreateSAMLProviderMultipart(response http.ResponseWr samlIdentityProvider.SingleSignOnURI = ssoURL if newSAMLProvider, err := s.db.CreateSAMLIdentityProvider(request.Context(), samlIdentityProvider); err != nil { - api.HandleDatabaseError(request, response, err) + if errors.Is(err, database.ErrDuplicateSSOProviderName) { + api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusConflict, api.ErrorResponseSSOProviderDuplicateName, request), response) + } else { + api.HandleDatabaseError(request, response, err) + } } else { api.WriteBasicResponse(request.Context(), newSAMLProvider, http.StatusOK, response) } diff --git a/cmd/api/src/database/db.go b/cmd/api/src/database/db.go index 19c792f2c5..61e35bbb1e 100644 --- a/cmd/api/src/database/db.go +++ b/cmd/api/src/database/db.go @@ -44,8 +44,9 @@ const ( ) var ( - ErrDuplicateAGName = errors.New("duplicate asset group name") - ErrDuplicateAGTag = errors.New("duplicate asset group tag") + ErrDuplicateAGName = errors.New("duplicate asset group name") + ErrDuplicateAGTag = errors.New("duplicate asset group tag") + ErrDuplicateSSOProviderName = errors.New("duplicate sso provider name") ) func IsUnexpectedDatabaseError(err error) bool { diff --git a/cmd/api/src/database/sso_providers.go b/cmd/api/src/database/sso_providers.go index 9aa22375ce..87b083e50e 100644 --- a/cmd/api/src/database/sso_providers.go +++ b/cmd/api/src/database/sso_providers.go @@ -60,7 +60,15 @@ func (s *BloodhoundDB) CreateSSOProvider(ctx context.Context, name string, authP ) err := s.AuditableTransaction(ctx, auditEntry, func(tx *gorm.DB) error { - return CheckError(tx.Table(ssoProviderTableName).Create(&provider)) + result := tx.Table(ssoProviderTableName).Create(&provider) + + if result.Error != nil { + if strings.Contains(result.Error.Error(), "duplicate key value violates unique constraint \"sso_providers_name_key\"") { + return fmt.Errorf("%w: %v", ErrDuplicateSSOProviderName, tx.Error) + } + } + + return CheckError(result) }) return provider, err diff --git a/cmd/api/src/test/integration/harnesses/enrollonbehalfof-1.svg b/cmd/api/src/test/integration/harnesses/enrollonbehalfof-1.svg index 6fcfa6838d..4f22563a8b 100644 --- a/cmd/api/src/test/integration/harnesses/enrollonbehalfof-1.svg +++ b/cmd/api/src/test/integration/harnesses/enrollonbehalfof-1.svg @@ -1 +1,18 @@ -NTAuthStoreForTrustedForNTAuthPublishedToPublishedToPublishedToRootCAForEnterpriseCAForEnrollOnBehalfOfEnrollOnBehalfOfEnrollOnBehalfOfDomain1CertTemplate1-1schemaversion:2effectiveekus:["2.5.29.37.0"]NTAuthStore1EnterpriseCA1CertTemplate1-2schemaversion:1effectiveekus:["2.5.29.37.0"]CertTemplate1-3schemaversion:2effectiveekus:["2.5.29.37.0"]RootCA1 \ No newline at end of file + +NTAuthStoreForTrustedForNTAuthPublishedToPublishedToPublishedToRootCAForEnterpriseCAForEnrollOnBehalfOfEnrollOnBehalfOfEnrollOnBehalfOfDomain1CertTemplate1-1schemaversion:2effectiveekus:["2.5.29.37.0"]NTAuthStore1EnterpriseCA1CertTemplate1-2schemaversion:1effectiveekus:["2.5.29.37.0"]CertTemplate1-3schemaversion:2effectiveekus:["2.5.29.37.0"]RootCA1 diff --git a/cmd/api/src/test/integration/harnesses/enrollonbehalfof-2.svg b/cmd/api/src/test/integration/harnesses/enrollonbehalfof-2.svg index d8d8ec0fcb..96181905f2 100644 --- a/cmd/api/src/test/integration/harnesses/enrollonbehalfof-2.svg +++ b/cmd/api/src/test/integration/harnesses/enrollonbehalfof-2.svg @@ -1 +1,18 @@ -NTAuthStoreForTrustedForNTAuthRootCAForEnterpriseCAForPublishedToPublishedToPublishedToPublishedToEnrollOnBehalfOfDomain2NTAuthStore2EnterpriseCA2RootCA2CertTemplate2-1effectiveekus:["1.3.6.1.4.1.311.20.2.1"]schemaversion:2CertTemplate2-2effectiveekus:["1.3.6.1.4.1.311.20.2.1", "2.5.29.37.0"]schemaversion:2CertTemplate2-3effectiveekus:[]schemaversion:2authorizedsignatures:1applicationpolicies:["1.3.6.1.4.1.311.20.2.1"]CertTemplate2-4effectiveekus:[]schemaversion:2authorizedsignatures:1applicationpolicies:[] \ No newline at end of file + +NTAuthStoreForTrustedForNTAuthRootCAForEnterpriseCAForPublishedToPublishedToPublishedToPublishedToEnrollOnBehalfOfDomain2NTAuthStore2EnterpriseCA2RootCA2CertTemplate2-1effectiveekus:["1.3.6.1.4.1.311.20.2.1"]schemaversion:2CertTemplate2-2effectiveekus:["1.3.6.1.4.1.311.20.2.1", "2.5.29.37.0"]schemaversion:2CertTemplate2-3effectiveekus:[]schemaversion:2authorizedsignatures:1applicationpolicies:["1.3.6.1.4.1.311.20.2.1"]CertTemplate2-4effectiveekus:[]schemaversion:2authorizedsignatures:1applicationpolicies:[] diff --git a/cmd/api/src/test/integration/harnesses/enrollonbehalfof-3.svg b/cmd/api/src/test/integration/harnesses/enrollonbehalfof-3.svg index d7192c6074..42dced49a9 100644 --- a/cmd/api/src/test/integration/harnesses/enrollonbehalfof-3.svg +++ b/cmd/api/src/test/integration/harnesses/enrollonbehalfof-3.svg @@ -1 +1,18 @@ -NTAuthStoreForTrustedForNTAuthPublishedToPublishedToRootCAForEnterpriseCAForEnrollOnBehalfOfPublishedToEnterpriseCAForEnrollOnBehalfOfDomain1CertTemplate1-1schemaversion:2effectiveekus:["2.5.29.37.0"]NTAuthStore1EnterpriseCA1CertTemplate1-2schemaversion:1effectiveekus:["2.5.29.37.0"]CertTemplate1-3schemaversion:2effectiveekus:["2.5.29.37.0"]RootCA1EnterpriseCA2 \ No newline at end of file + +NTAuthStoreForTrustedForNTAuthPublishedToPublishedToRootCAForEnterpriseCAForEnrollOnBehalfOfPublishedToEnterpriseCAForEnrollOnBehalfOfDomain1CertTemplate1-1schemaversion:2effectiveekus:["2.5.29.37.0"]NTAuthStore1EnterpriseCA1CertTemplate1-2schemaversion:1effectiveekus:["2.5.29.37.0"]CertTemplate1-3schemaversion:2effectiveekus:["2.5.29.37.0"]RootCA1EnterpriseCA2 diff --git a/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderDialog.tsx b/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderDialog.tsx index 3cda8ded0e..11ab94b40f 100644 --- a/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderDialog.tsx +++ b/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderDialog.tsx @@ -20,7 +20,7 @@ import UpsertOIDCProviderForm from './UpsertOIDCProviderForm'; const UpsertOIDCProviderDialog: React.FC<{ open: boolean; - error?: string; + error: any; oldSSOProvider?: SSOProvider; onClose: () => void; onSubmit: (data: UpsertOIDCProviderRequest) => void; diff --git a/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderForm.tsx b/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderForm.tsx index b99c8ec04b..6bcc4ebdf7 100644 --- a/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderForm.tsx +++ b/packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderForm.tsx @@ -16,12 +16,12 @@ import { Button } from '@bloodhoundenterprise/doodleui'; import { Alert, DialogContent, DialogActions, Grid, TextField } from '@mui/material'; -import { FC } from 'react'; +import { useEffect, FC } from 'react'; import { Controller, useForm } from 'react-hook-form'; import { OIDCProviderInfo, SSOProvider, UpsertOIDCProviderRequest } from 'js-client-library'; const UpsertOIDCProviderForm: FC<{ - error?: string; + error: any; oldSSOProvider?: SSOProvider; onClose: () => void; onSubmit: (data: UpsertOIDCProviderRequest) => void; @@ -37,8 +37,22 @@ const UpsertOIDCProviderForm: FC<{ handleSubmit, reset, formState: { errors }, + setError, } = useForm({ defaultValues }); + useEffect(() => { + if (error) { + if (error.response?.data?.errors[0]?.message == 'sso provider name must be unique') { + setError('name', { type: 'custom', message: 'SSO Provider Name is already in use.' }); + } else { + setError('generic', { + type: 'custom', + message: `Unable to ${oldSSOProvider ? 'update' : 'create new'} SAML Provider configuration. Please try again.`, + }); + } + } + }, [error, setError, oldSSOProvider]); + const handleClose = () => { onClose(); reset(); @@ -113,9 +127,9 @@ const UpsertOIDCProviderForm: FC<{ )} /> - {error && ( + {!!errors.generic && ( - {error} + {errors.generic.message} )} diff --git a/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderDialog/UpsertSAMLProviderDialog.tsx b/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderDialog/UpsertSAMLProviderDialog.tsx index cfc85ed2e1..7885e67105 100644 --- a/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderDialog/UpsertSAMLProviderDialog.tsx +++ b/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderDialog/UpsertSAMLProviderDialog.tsx @@ -20,7 +20,7 @@ import UpsertSAMLProviderForm from '../UpsertSAMLProviderForm'; const UpsertSAMLProviderDialog: React.FC<{ open: boolean; - error?: string; + error: any; oldSSOProvider?: SSOProvider; onClose: () => void; onSubmit: (data: UpsertSAMLProviderFormInputs) => void; diff --git a/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderForm/UpsertSAMLProviderForm.tsx b/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderForm/UpsertSAMLProviderForm.tsx index af8d98fef6..102120d7b4 100644 --- a/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderForm/UpsertSAMLProviderForm.tsx +++ b/packages/javascript/bh-shared-ui/src/components/UpsertSAMLProviderForm/UpsertSAMLProviderForm.tsx @@ -26,12 +26,12 @@ import { Typography, useTheme, } from '@mui/material'; -import { useState, FC } from 'react'; +import { useState, useEffect, FC } from 'react'; import { Controller, useForm } from 'react-hook-form'; import { SSOProvider, UpsertSAMLProviderFormInputs } from 'js-client-library'; const UpsertSAMLProviderForm: FC<{ - error?: string; + error: any; oldSSOProvider?: SSOProvider; onClose: () => void; onSubmit: (data: UpsertSAMLProviderFormInputs) => void; @@ -42,6 +42,7 @@ const UpsertSAMLProviderForm: FC<{ handleSubmit, reset, formState: { errors }, + setError, } = useForm({ defaultValues: { name: oldSSOProvider?.name ?? '', @@ -50,6 +51,19 @@ const UpsertSAMLProviderForm: FC<{ }); const [fileValue, setFileValue] = useState(''); // small workaround to use the file input + useEffect(() => { + if (error) { + if (error.response?.data?.errors[0]?.message == 'sso provider name must be unique') { + setError('name', { type: 'custom', message: 'SSO Provider Name is already in use.' }); + } else { + setError('generic', { + type: 'custom', + message: `Unable to ${oldSSOProvider ? 'update' : 'create new'} SAML Provider configuration. Please try again.`, + }); + } + } + }, [error, setError, oldSSOProvider]); + const handleClose = () => { onClose(); setFileValue(''); @@ -126,9 +140,9 @@ const UpsertSAMLProviderForm: FC<{ : 'Upload the Metadata file provided by your SAML Provider'} - {error && ( + {!!errors.generic && ( - {error} + {errors.generic.message} )} diff --git a/packages/javascript/bh-shared-ui/src/views/SSOConfiguration/SSOConfiguration.tsx b/packages/javascript/bh-shared-ui/src/views/SSOConfiguration/SSOConfiguration.tsx index cfcdd40915..1b60793575 100644 --- a/packages/javascript/bh-shared-ui/src/views/SSOConfiguration/SSOConfiguration.tsx +++ b/packages/javascript/bh-shared-ui/src/views/SSOConfiguration/SSOConfiguration.tsx @@ -44,7 +44,7 @@ const SSOConfiguration: FC = () => { const [ssoProviderIdToDeleteOrUpdate, setSSOProviderIdToDeleteOrUpdate] = useState(); const [dialogOpen, setDialogOpen] = useState<'SAML' | 'OIDC' | 'DELETE' | ''>(''); const [nameFilter, setNameFilter] = useState(''); - const [upsertProviderError, setUpsertProviderError] = useState(''); + const [upsertProviderError, setUpsertProviderError] = useState(); const [typeSortOrder, setTypeSortOrder] = useState(); const listSSOProvidersQuery = useQuery(['listSSOProviders'], ({ signal }) => @@ -115,7 +115,7 @@ const SSOConfiguration: FC = () => { }; const closeDialog = () => { - setUpsertProviderError(''); + setUpsertProviderError(null); setDialogOpen(''); setTimeout(() => setSSOProviderIdToDeleteOrUpdate(undefined), 500); }; @@ -171,7 +171,7 @@ const SSOConfiguration: FC = () => { }; const upsertSAMLProvider = async (samlProvider: UpsertSAMLProviderFormInputs) => { - setUpsertProviderError(''); + setUpsertProviderError(null); try { const payload = { name: samlProvider.name, metadata: samlProvider.metadata && samlProvider.metadata[0] }; if (ssoProviderIdToDeleteOrUpdate) { @@ -183,16 +183,14 @@ const SSOConfiguration: FC = () => { } listSSOProvidersQuery.refetch(); closeDialog(); - } catch (error) { + } catch (error: any) { console.error(error); - setUpsertProviderError( - `Unable to ${ssoProviderIdToDeleteOrUpdate ? 'update' : 'create new'} SAML Provider configuration. Please try again.` - ); + setUpsertProviderError(error); } }; const upsertOIDCProvider = async (oidcProvider: UpsertOIDCProviderRequest) => { - setUpsertProviderError(''); + setUpsertProviderError(null); try { if (ssoProviderIdToDeleteOrUpdate) { await apiClient.updateOIDCProvider(ssoProviderIdToDeleteOrUpdate, oidcProvider); @@ -209,9 +207,7 @@ const SSOConfiguration: FC = () => { closeDialog(); } catch (error) { console.error(error); - setUpsertProviderError( - `Unable to ${ssoProviderIdToDeleteOrUpdate ? 'update' : 'create new'} OIDC Provider configuration. Please try again.` - ); + setUpsertProviderError(error); } };