Skip to content

Commit

Permalink
BED-5066: Better error messaging when creating SSO providers (#1010)
Browse files Browse the repository at this point in the history 
* BED-5066: Return conflict error if SSO provider name conflicts
Add better error messaging on UI for this case

* BED-5066: Add error check to SSO update routes
Make front-end error check more robust

* BED-5055: Back change the way the errors are handled on the front-end

* BED-5066: Fix error check
Run prepare for codereview

* BED-5066: resolve TS errors
Raise name error on duplicate SSO slug

* BED-5066: Add duplicate slug check to UpdateSSOProvider as well
  • Loading branch information
@wes-mil
wes-mil authored Dec 20, 2024
1 parent b4fcdc9 commit c062f0f
Show file tree
Hide file tree
Showing 13 changed files with 145 additions and 42 deletions.
1 change: 1 addition & 0 deletions cmd/api/src/api/error.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
ErrorResponseUserDuplicatePrincipal = "principal name must be unique"
ErrorResponseDetailsUniqueViolation = "unique constraint was violated"
ErrorResponseDetailsNotImplemented = "All good things to those who wait. Not implemented."
Expand Down
14 changes: 12 additions & 2 deletions cmd/api/src/api/v2/auth/oidc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package auth

import (
"errors"
"fmt"
"net/http"
"time"
Expand All @@ -28,6 +29,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"
Expand Down Expand Up @@ -67,7 +69,11 @@ func (s ManagementResource) UpdateOIDCProviderRequest(response http.ResponseWrit
}

if oidcProvider, err := s.db.UpdateOIDCProvider(request.Context(), ssoProvider); 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.StatusOK, response)
}
Expand All @@ -84,7 +90,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)
}
Expand Down
13 changes: 11 additions & 2 deletions cmd/api/src/api/v2/auth/saml.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -249,7 +254,11 @@ func (s ManagementResource) UpdateSAMLProviderRequest(response http.ResponseWrit
}

if newSAMLProvider, err := s.db.UpdateSAMLIdentityProvider(request.Context(), ssoProvider); 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)
}
Expand Down
7 changes: 4 additions & 3 deletions cmd/api/src/database/db.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ const (
)

var (
ErrDuplicateAGName = errors.New("duplicate asset group name")
ErrDuplicateAGTag = errors.New("duplicate asset group tag")
ErrDuplicateUserPrincipal = errors.New("duplicate user principal name")
ErrDuplicateAGName = errors.New("duplicate asset group name")
ErrDuplicateAGTag = errors.New("duplicate asset group tag")
ErrDuplicateSSOProviderName = errors.New("duplicate sso provider name")
ErrDuplicateUserPrincipal = errors.New("duplicate user principal name")
)

func IsUnexpectedDatabaseError(err error) bool {
Expand Down
24 changes: 22 additions & 2 deletions cmd/api/src/database/sso_providers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,17 @@ 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)
} else if strings.Contains(result.Error.Error(), "duplicate key value violates unique constraint \"sso_providers_slug_key\"") {
return fmt.Errorf("%w: %v", ErrDuplicateSSOProviderName, tx.Error)
}
}

return CheckError(result)
})

return provider, err
Expand Down Expand Up @@ -183,7 +193,17 @@ func (s *BloodhoundDB) UpdateSSOProvider(ctx context.Context, ssoProvider model.
}

err := s.AuditableTransaction(ctx, auditEntry, func(tx *gorm.DB) error {
return CheckError(tx.WithContext(ctx).Exec(fmt.Sprintf("UPDATE %s SET name = ?, slug = ?, updated_at = ? WHERE id = ?;", ssoProviderTableName), ssoProvider.Name, ssoProvider.Slug, time.Now().UTC(), ssoProvider.ID))
result := tx.WithContext(ctx).Exec(fmt.Sprintf("UPDATE %s SET name = ?, slug = ?, updated_at = ? WHERE id = ?;", ssoProviderTableName), ssoProvider.Name, ssoProvider.Slug, time.Now().UTC(), ssoProvider.ID)

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)
} else if strings.Contains(result.Error.Error(), "duplicate key value violates unique constraint \"sso_providers_slug_key\"") {
return fmt.Errorf("%w: %v", ErrDuplicateSSOProviderName, tx.Error)
}
}

return CheckError(result)
})

return ssoProvider, err
Expand Down
17 changes: 12 additions & 5 deletions packages/javascript/bh-shared-ui/src/components/CreateUserForm/CreateUserForm.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,17 @@ const CreateUserForm: React.FC<{
}

if (error) {
if (error.response?.data?.errors[0]?.message == 'principal name must be unique') {
setError('principal', { type: 'custom', message: 'Principal name is already in use.' });
if (error?.response?.status === 409) {
if (error.response?.data?.errors[0]?.message.toLowerCase().includes('principal name')) {
setError('principal', { type: 'custom', message: 'Principal name is already in use.' });
} else {
setError('root.generic', { type: 'custom', message: `A conflict has occured.` });
}
} else {
setError('generic', { type: 'custom', message: 'An unexpected error occurred. Please try again.' });
setError('root.generic', {
type: 'custom',
message: 'An unexpected error occurred. Please try again.',
});
}
}
}, [authenticationMethod, setValue, error, setError]);
Expand Down Expand Up @@ -345,9 +352,9 @@ const CreateUserForm: React.FC<{
)}
/>
</Grid>
{!!errors.generic && (
{!!errors.root?.generic && (
<Grid item xs={12}>
<Alert severity='error'>{errors.generic.message}</Alert>
<Alert severity='error'>{errors.root.generic.message}</Alert>
</Grid>
)}
</Grid>
Expand Down
1 change: 1 addition & 0 deletions packages/javascript/bh-shared-ui/src/components/UpdateUserDialog/UpdateUserDialog.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ describe('UpdateUserDialog', () => {
onSave={testOnSave}
isLoading={options?.renderLoading || false}
error={options?.renderErrors}
hasSelectedSelf={false}
/>
);

Expand Down
30 changes: 23 additions & 7 deletions packages/javascript/bh-shared-ui/src/components/UpdateUserForm/UpdateUserForm.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@

import { Button } from '@bloodhoundenterprise/doodleui';
import {
Alert,
DialogActions,
DialogContent,
DialogContentText,
FormControl,
FormHelperText,
Grid,
InputLabel,
MenuItem,
Expand Down Expand Up @@ -141,6 +141,7 @@ const UpdateUserFormInner: React.FC<{
handleSubmit,
setValue,
formState: { errors },
setError,
watch,
} = useForm<UpdateUserRequestForm & { authenticationMethod: 'sso' | 'password' }>({
defaultValues: {
Expand All @@ -155,7 +156,22 @@ const UpdateUserFormInner: React.FC<{
if (authenticationMethod === 'password') {
setValue('SSOProviderId', undefined);
}
}, [authenticationMethod, setValue]);

if (error) {
if (error?.response?.status === 409) {
if (error.response?.data?.errors[0]?.message.toLowerCase().includes('principal name')) {
setError('principal', { type: 'custom', message: 'Principal name is already in use.' });
} else {
setError('root.generic', { type: 'custom', message: `A conflict has occured.` });
}
} else {
setError('root.generic', {
type: 'custom',
message: 'An unexpected error occurred. Please try again.',
});
}
}
}, [authenticationMethod, setValue, error, setError]);

return (
<form autoComplete='off' onSubmit={handleSubmit(onSubmit)}>
Expand Down Expand Up @@ -359,14 +375,14 @@ const UpdateUserFormInner: React.FC<{
)}
/>
</Grid>
{!!errors.root?.generic && (
<Grid item xs={12}>
<Alert severity='error'>{errors.root.generic.message}</Alert>
</Grid>
)}
</Grid>
</DialogContent>
<DialogActions>
{error && (
<FormHelperText error style={{ margin: 0 }}>
An unexpected error occurred. Please try again.
</FormHelperText>
)}
<Button
type='button'
variant={'tertiary'}
Expand Down
2 changes: 1 addition & 1 deletion packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
29 changes: 25 additions & 4 deletions packages/javascript/bh-shared-ui/src/components/UpsertOIDCProviderForm.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -37,8 +37,29 @@ const UpsertOIDCProviderForm: FC<{
handleSubmit,
reset,
formState: { errors },
setError,
} = useForm<UpsertOIDCProviderRequest>({ defaultValues });

useEffect(() => {
if (error) {
if (error?.response?.status === 409) {
if (error.response?.data?.errors[0]?.message.toLowerCase().includes('sso provider name')) {
setError('name', { type: 'custom', message: 'SSO Provider Name is already in use.' });
} else {
setError('root.generic', {
type: 'custom',
message: 'A conflict has occured.',
});
}
} else {
setError('root.generic', {
type: 'custom',
message: `Unable to ${oldSSOProvider ? 'update' : 'create new'} OIDC Provider configuration. Please try again.`,
});
}
}
}, [error, setError, oldSSOProvider]);

const handleClose = () => {
onClose();
reset();
Expand Down Expand Up @@ -113,9 +134,9 @@ const UpsertOIDCProviderForm: FC<{
)}
/>
</Grid>
{error && (
{!!errors.root?.generic && (
<Grid item xs={12}>
<Alert severity='error'>{error}</Alert>
<Alert severity='error'>{errors.root.generic.message}</Alert>
</Grid>
)}
</Grid>
Expand Down
2 changes: 1 addition & 1 deletion ...ascript/bh-shared-ui/src/components/UpsertSAMLProviderDialog/UpsertSAMLProviderDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
29 changes: 25 additions & 4 deletions .../javascript/bh-shared-ui/src/components/UpsertSAMLProviderForm/UpsertSAMLProviderForm.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,6 +42,7 @@ const UpsertSAMLProviderForm: FC<{
handleSubmit,
reset,
formState: { errors },
setError,
} = useForm<UpsertSAMLProviderFormInputs>({
defaultValues: {
name: oldSSOProvider?.name ?? '',
Expand All @@ -50,6 +51,26 @@ const UpsertSAMLProviderForm: FC<{
});
const [fileValue, setFileValue] = useState(''); // small workaround to use the file input

useEffect(() => {
if (error) {
if (error?.response?.status === 409) {
if (error.response?.data?.errors[0]?.message.toLowerCase().includes('sso provider name')) {
setError('name', { type: 'custom', message: 'SSO Provider Name is already in use.' });
} else {
setError('root.generic', {
type: 'custom',
message: `A conflict has occured.`,
});
}
} else {
setError('root.generic', {
type: 'custom',
message: `Unable to ${oldSSOProvider ? 'update' : 'create new'} SAML Provider configuration. Please try again.`,
});
}
}
}, [error, setError, oldSSOProvider]);

const handleClose = () => {
onClose();
setFileValue('');
Expand Down Expand Up @@ -126,9 +147,9 @@ const UpsertSAMLProviderForm: FC<{
: 'Upload the Metadata file provided by your SAML Provider'}
</FormHelperText>
</Grid>
{error && (
{!!errors.root?.generic && (
<Grid item xs={12}>
<Alert severity='error'>{error}</Alert>
<Alert severity='error'>{errors.root.generic.message}</Alert>
</Grid>
)}
</Grid>
Expand Down
Loading

0 comments on commit c062f0f

Please sign in to comment.