Skip to content

Commit

Permalink
BED-5066: Return conflict error if SSO provider name conflicts
Browse files Browse the repository at this point in the history
Add better error messaging on UI for this case
  • Loading branch information
wes-mil committed Dec 11, 2024
1 parent a420a32 commit c209d0b
Show file tree
Hide file tree
Showing 13 changed files with 125 additions and 29 deletions.
1 change: 1 addition & 0 deletions cmd/api/src/api/error.go
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"
ErrorResponseDetailsUniqueViolation = "unique constraint was violated"
ErrorResponseDetailsNotImplemented = "All good things to those who wait. Not implemented."

Expand Down
8 changes: 7 additions & 1 deletion cmd/api/src/api/v2/auth/oidc.go
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 @@ -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"
Expand Down Expand Up @@ -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)
}
Expand Down
7 changes: 6 additions & 1 deletion cmd/api/src/api/v2/auth/saml.go
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
5 changes: 3 additions & 2 deletions cmd/api/src/database/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 9 additions & 1 deletion cmd/api/src/database/sso_providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 18 additions & 1 deletion cmd/api/src/test/integration/harnesses/enrollonbehalfof-1.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
19 changes: 18 additions & 1 deletion cmd/api/src/test/integration/harnesses/enrollonbehalfof-2.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
19 changes: 18 additions & 1 deletion cmd/api/src/test/integration/harnesses/enrollonbehalfof-3.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
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
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,22 @@ const UpsertOIDCProviderForm: FC<{
handleSubmit,
reset,
formState: { errors },
setError,
} = useForm<UpsertOIDCProviderRequest>({ 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();
Expand Down Expand Up @@ -113,9 +127,9 @@ const UpsertOIDCProviderForm: FC<{
)}
/>
</Grid>
{error && (
{!!errors.generic && (
<Grid item xs={12}>
<Alert severity='error'>{error}</Alert>
<Alert severity='error'>{errors.generic.message}</Alert>
</Grid>
)}
</Grid>
Expand Down
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
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,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('');
Expand Down Expand Up @@ -126,9 +140,9 @@ const UpsertSAMLProviderForm: FC<{
: 'Upload the Metadata file provided by your SAML Provider'}
</FormHelperText>
</Grid>
{error && (
{!!errors.generic && (
<Grid item xs={12}>
<Alert severity='error'>{error}</Alert>
<Alert severity='error'>{errors.generic.message}</Alert>
</Grid>
)}
</Grid>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const SSOConfiguration: FC = () => {
const [ssoProviderIdToDeleteOrUpdate, setSSOProviderIdToDeleteOrUpdate] = useState<SSOProvider['id'] | undefined>();
const [dialogOpen, setDialogOpen] = useState<'SAML' | 'OIDC' | 'DELETE' | ''>('');
const [nameFilter, setNameFilter] = useState<string>('');
const [upsertProviderError, setUpsertProviderError] = useState<string>('');
const [upsertProviderError, setUpsertProviderError] = useState<any>();
const [typeSortOrder, setTypeSortOrder] = useState<SortOrder>();

const listSSOProvidersQuery = useQuery(['listSSOProviders'], ({ signal }) =>
Expand Down Expand Up @@ -115,7 +115,7 @@ const SSOConfiguration: FC = () => {
};

const closeDialog = () => {
setUpsertProviderError('');
setUpsertProviderError(null);
setDialogOpen('');
setTimeout(() => setSSOProviderIdToDeleteOrUpdate(undefined), 500);
};
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand All @@ -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);
}
};

Expand Down

0 comments on commit c209d0b

Please sign in to comment.