Bed 4582 (#787)

* added initial logic for delete checking admin and public * finished logic. added unit tests * running prepare-for-codereview * removed commented code * refactored code from pr * updated unit tests
SpecterOps · Aug 14, 2024 · 40724e4 · 40724e4
1 parent ccf7f44
commit 40724e4
Show file tree

Hide file tree

Showing 56 changed files with 286 additions and 66 deletions.
diff --git a/cmd/api/src/api/v2/saved_queries.go b/cmd/api/src/api/v2/saved_queries.go
@@ -229,16 +229,30 @@ func (s Resources) DeleteSavedQuery(response http.ResponseWriter, request *http.
 		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusNotFound, "query does not exist", request), response)
 	} else if err != nil {
 		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, api.ErrorResponseDetailsInternalServerError, request), response)
-	} else if !savedQueryBelongsToUser {
-		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, "invalid saved_query_id supplied", request), response)
-	} else if err := s.DB.DeleteSavedQuery(request.Context(), savedQueryID); errors.Is(err, database.ErrNotFound) {
-		// This is an edge case and can only occur if the database has a concurrent operation that deletes the saved query
-		// after the check at s.DB.SavedQueryBelongsToUser but before getting here.
-		// Still, adding in the same check for good measure.
-		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusNotFound, "query does not exist", request), response)
-	} else if err != nil {
-		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, api.ErrorResponseDetailsInternalServerError, request), response)
 	} else {
-		response.WriteHeader(http.StatusNoContent)
+		if !savedQueryBelongsToUser {
+			if _, isAdmin := user.Roles.FindByName(auth.RoleAdministrator); !isAdmin {
+				api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusForbidden, "User does not have permission to delete this query", request), response)
+				return
+			} else if isPublicQuery, err := s.DB.IsSavedQueryPublic(request.Context(), int64(savedQueryID)); err != nil {
+				api.HandleDatabaseError(request, response, err)
+				return
+			} else if !isPublicQuery {
+				api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusForbidden, "User does not have permission to delete this query", request), response)
+				return
+			}
+		}
+
+		if err := s.DB.DeleteSavedQuery(request.Context(), savedQueryID); errors.Is(err, database.ErrNotFound) {
+			// This is an edge case and can only occur if the database has a concurrent operation that deletes the saved query
+			// after the check at s.DB.SavedQueryBelongsToUser but before getting here.
+			// Still, adding in the same check for good measure.
+			api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusNotFound, "query does not exist", request), response)
+		} else if err != nil {
+			api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, api.ErrorResponseDetailsInternalServerError, request), response)
+		} else {
+			response.WriteHeader(http.StatusNoContent)
+		}
+
 	}
 }
diff --git a/cmd/api/src/api/v2/saved_queries_test.go b/cmd/api/src/api/v2/saved_queries_test.go
@@ -1236,7 +1236,7 @@ func TestResources_DeleteSavedQuery_DBError(t *testing.T) {
 	require.Equal(t, http.StatusInternalServerError, response.Code)
 }
 
-func TestResources_DeleteSavedQuery_QueryDoesNotBelongToUser(t *testing.T) {
+func TestResources_DeleteSavedQuery_UserNotAdmin(t *testing.T) {
 	var (
 		mockCtrl  = gomock.NewController(t)
 		mockDB    = mocks.NewMockDatabase(mockCtrl)
@@ -1262,8 +1262,69 @@ func TestResources_DeleteSavedQuery_QueryDoesNotBelongToUser(t *testing.T) {
 	handler := http.HandlerFunc(resources.DeleteSavedQuery)
 
 	handler.ServeHTTP(response, req)
-	require.Equal(t, http.StatusBadRequest, response.Code)
-	require.Contains(t, response.Body.String(), api.URIPathVariableSavedQueryID)
+	assert.Equal(t, http.StatusForbidden, response.Code)
+	assert.Contains(t, response.Body.String(), "User does not have permission to delete this query")
+}
+
+func TestResources_DeleteSavedQuery_IsPublicSavedQueryDBError(t *testing.T) {
+	var (
+		mockCtrl  = gomock.NewController(t)
+		mockDB    = mocks.NewMockDatabase(mockCtrl)
+		resources = v2.Resources{DB: mockDB}
+	)
+	defer mockCtrl.Finish()
+
+	userId, err := uuid2.NewV4()
+	require.Nil(t, err)
+
+	endpoint := "/api/v2/saved-queries/%s"
+	savedQueryId := "1"
+
+	mockDB.EXPECT().SavedQueryBelongsToUser(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
+	mockDB.EXPECT().IsSavedQueryPublic(gomock.Any(), gomock.Any()).Return(false, fmt.Errorf("error"))
+
+	req, err := http.NewRequestWithContext(createContextWithAdminOwnerId(userId), "DELETE", fmt.Sprintf(endpoint, savedQueryId), nil)
+	require.Nil(t, err)
+
+	req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+	req = mux.SetURLVars(req, map[string]string{api.URIPathVariableSavedQueryID: savedQueryId})
+
+	response := httptest.NewRecorder()
+	handler := http.HandlerFunc(resources.DeleteSavedQuery)
+
+	handler.ServeHTTP(response, req)
+	assert.Equal(t, http.StatusInternalServerError, response.Code)
+}
+
+func TestResources_DeleteSavedQuery_NotPublicQueryAndUserIsAdmin(t *testing.T) {
+	var (
+		mockCtrl  = gomock.NewController(t)
+		mockDB    = mocks.NewMockDatabase(mockCtrl)
+		resources = v2.Resources{DB: mockDB}
+	)
+	defer mockCtrl.Finish()
+
+	userId, err := uuid2.NewV4()
+	require.Nil(t, err)
+
+	endpoint := "/api/v2/saved-queries/%s"
+	savedQueryId := "1"
+
+	mockDB.EXPECT().SavedQueryBelongsToUser(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
+	mockDB.EXPECT().IsSavedQueryPublic(gomock.Any(), gomock.Any()).Return(false, nil)
+
+	req, err := http.NewRequestWithContext(createContextWithAdminOwnerId(userId), "DELETE", fmt.Sprintf(endpoint, savedQueryId), nil)
+	require.Nil(t, err)
+
+	req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+	req = mux.SetURLVars(req, map[string]string{api.URIPathVariableSavedQueryID: savedQueryId})
+
+	response := httptest.NewRecorder()
+	handler := http.HandlerFunc(resources.DeleteSavedQuery)
+
+	handler.ServeHTTP(response, req)
+	assert.Equal(t, http.StatusForbidden, response.Code)
+	assert.Contains(t, response.Body.String(), "User does not have permission to delete this query")
 }
 
 func TestResources_DeleteSavedQuery_RecordNotFound(t *testing.T) {
@@ -1358,6 +1419,37 @@ func TestResources_DeleteSavedQuery_DeleteError(t *testing.T) {
 	require.Contains(t, response.Body.String(), api.ErrorResponseDetailsInternalServerError)
 }
 
+func TestResources_DeleteSavedQuery_PublicQueryAndUserIsAdmin(t *testing.T) {
+	var (
+		mockCtrl  = gomock.NewController(t)
+		mockDB    = mocks.NewMockDatabase(mockCtrl)
+		resources = v2.Resources{DB: mockDB}
+	)
+	defer mockCtrl.Finish()
+
+	userId, err := uuid2.NewV4()
+	require.Nil(t, err)
+
+	endpoint := "/api/v2/saved-queries/%s"
+	savedQueryId := "1"
+
+	mockDB.EXPECT().SavedQueryBelongsToUser(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
+	mockDB.EXPECT().IsSavedQueryPublic(gomock.Any(), gomock.Any()).Return(true, nil)
+	mockDB.EXPECT().DeleteSavedQuery(gomock.Any(), gomock.Any()).Return(nil)
+
+	req, err := http.NewRequestWithContext(createContextWithAdminOwnerId(userId), "DELETE", fmt.Sprintf(endpoint, savedQueryId), nil)
+	require.Nil(t, err)
+
+	req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+	req = mux.SetURLVars(req, map[string]string{api.URIPathVariableSavedQueryID: savedQueryId})
+
+	response := httptest.NewRecorder()
+	handler := http.HandlerFunc(resources.DeleteSavedQuery)
+
+	handler.ServeHTTP(response, req)
+	require.Equal(t, http.StatusNoContent, response.Code)
+}
+
 func TestResources_DeleteSavedQuery(t *testing.T) {
 	var (
 		mockCtrl  = gomock.NewController(t)

diff --git a/cmd/api/src/queries/graph.go b/cmd/api/src/queries/graph.go
@@ -154,9 +154,9 @@ type GraphQuery struct {
 	Cache                        cache.Cache
 	SlowQueryThreshold           int64 // Threshold in milliseconds
 	DisableCypherComplexityLimit bool
-	EnableCypherMutations bool
-	cypherEmitter         format.Emitter
-	strippedCypherEmitter format.Emitter
+	EnableCypherMutations        bool
+	cypherEmitter                format.Emitter
+	strippedCypherEmitter        format.Emitter
 }
 
 func NewGraphQuery(graphDB graph.Database, cache cache.Cache, cfg config.Configuration) *GraphQuery {

diff --git a/cmd/api/src/test/fixtures/fixtures/expected_ingest.go b/cmd/api/src/test/fixtures/fixtures/expected_ingest.go
@@ -18,6 +18,7 @@ package fixtures
 
 import (
 	"bytes"
+
 	"github.com/specterops/bloodhound/cypher/models/cypher/format"
 
 	"github.com/specterops/bloodhound/cypher/models/cypher"

diff --git a/cmd/ui/src/utils.ts b/cmd/ui/src/utils.ts
@@ -25,7 +25,7 @@ import { isLink, isNode } from 'src/ducks/graph/utils';
 import { Glyph } from 'src/rendering/programs/node.glyphs';
 import { store } from 'src/store';
 
-const IGNORE_401_LOGOUT = ['/api/v2/login', '/api/v2/logout', '/api/v2/features']
+const IGNORE_401_LOGOUT = ['/api/v2/login', '/api/v2/logout', '/api/v2/features'];
 
 export const getDatesInRange = (startDate: Date, endDate: Date) => {
     const date = new Date(startDate.getTime());

diff --git a/packages/go/cypher/analyzer/analyzer.go b/packages/go/cypher/analyzer/analyzer.go
@@ -18,6 +18,7 @@ package analyzer
 
 import (
 	"errors"
+
 	"github.com/specterops/bloodhound/cypher/models/cypher"
 	"github.com/specterops/bloodhound/dawgs/graph"
 )

diff --git a/packages/go/cypher/analyzer/measure.go b/packages/go/cypher/analyzer/measure.go
@@ -18,6 +18,7 @@ package analyzer
 
 import (
 	"fmt"
+
 	"github.com/specterops/bloodhound/cypher/models/cypher"
 
 	"github.com/specterops/bloodhound/dawgs/graph"

diff --git a/packages/go/cypher/frontend/comparison.go b/packages/go/cypher/frontend/comparison.go
@@ -18,6 +18,7 @@ package frontend
 
 import (
 	"fmt"
+
 	"github.com/specterops/bloodhound/cypher/models/cypher"
 
 	"github.com/antlr4-go/antlr/v4"

diff --git a/packages/go/cypher/frontend/expression.go b/packages/go/cypher/frontend/expression.go
@@ -17,9 +17,10 @@
 package frontend
 
 import (
-	"github.com/specterops/bloodhound/cypher/models/cypher"
 	"strings"
 
+	"github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/antlr4-go/antlr/v4"
 	"github.com/specterops/bloodhound/cypher/parser"
 	"github.com/specterops/bloodhound/dawgs/graph"

diff --git a/packages/go/cypher/frontend/literal.go b/packages/go/cypher/frontend/literal.go
@@ -18,9 +18,10 @@ package frontend
 
 import (
 	"fmt"
-	"github.com/specterops/bloodhound/cypher/models/cypher"
 	"strconv"
 
+	"github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/specterops/bloodhound/cypher/parser"
 )
 

diff --git a/packages/go/cypher/frontend/parse.go b/packages/go/cypher/frontend/parse.go
@@ -19,9 +19,10 @@ package frontend
 import (
 	"bytes"
 	"errors"
-	"github.com/specterops/bloodhound/cypher/models/cypher"
 	"strings"
 
+	"github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/antlr4-go/antlr/v4"
 	"github.com/specterops/bloodhound/cypher/parser"
 )

diff --git a/packages/go/cypher/frontend/pattern.go b/packages/go/cypher/frontend/pattern.go
@@ -18,9 +18,10 @@ package frontend
 
 import (
 	"fmt"
-	"github.com/specterops/bloodhound/cypher/models/cypher"
 	"strconv"
 
+	"github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/antlr4-go/antlr/v4"
 	"github.com/specterops/bloodhound/cypher/parser"
 	"github.com/specterops/bloodhound/dawgs/graph"

diff --git a/packages/go/cypher/frontend/query.go b/packages/go/cypher/frontend/query.go
@@ -18,9 +18,10 @@ package frontend
 
 import (
 	"fmt"
-	"github.com/specterops/bloodhound/cypher/models/cypher"
 	"strconv"
 
+	"github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/specterops/bloodhound/cypher/parser"
 	"github.com/specterops/bloodhound/dawgs/graph"
 )

diff --git a/packages/go/cypher/models/cypher/copy_test.go b/packages/go/cypher/models/cypher/copy_test.go
@@ -17,9 +17,10 @@
 package cypher_test
 
 import (
-	model2 "github.com/specterops/bloodhound/cypher/models/cypher"
 	"testing"
 
+	model2 "github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/specterops/bloodhound/dawgs/graph"
 	"github.com/stretchr/testify/require"
 )

diff --git a/packages/go/cypher/models/cypher/format/format.go b/packages/go/cypher/models/cypher/format/format.go
@@ -18,11 +18,12 @@ package format
 
 import (
 	"fmt"
-	"github.com/specterops/bloodhound/cypher/models/cypher"
 	"io"
 	"strconv"
 	"strings"
 
+	"github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/specterops/bloodhound/dawgs/graph"
 )
 
@@ -844,7 +845,6 @@ func (s Emitter) formatRemove(output io.Writer, remove *cypher.Remove) error {
 			}
 		}
 
-
 		if removeItem.KindMatcher != nil {
 			if err := s.WriteExpression(output, removeItem.KindMatcher.Reference); err != nil {
 				return err

diff --git a/packages/go/cypher/models/cypher/format/format_test.go b/packages/go/cypher/models/cypher/format/format_test.go
@@ -18,9 +18,10 @@ package format_test
 
 import (
 	"bytes"
-	"github.com/specterops/bloodhound/cypher/models/cypher/format"
 	"testing"
 
+	"github.com/specterops/bloodhound/cypher/models/cypher/format"
+
 	"github.com/specterops/bloodhound/cypher/frontend"
 	"github.com/stretchr/testify/require"
 

diff --git a/packages/go/cypher/models/cypher/walk_test.go b/packages/go/cypher/models/cypher/walk_test.go
@@ -17,9 +17,10 @@
 package cypher_test
 
 import (
-	model2 "github.com/specterops/bloodhound/cypher/models/cypher"
 	"testing"
 
+	model2 "github.com/specterops/bloodhound/cypher/models/cypher"
+
 	"github.com/specterops/bloodhound/cypher/frontend"
 	"github.com/specterops/bloodhound/cypher/test"
 	"github.com/stretchr/testify/require"

diff --git a/packages/go/cypher/models/pgsql/format/format_test.go b/packages/go/cypher/models/pgsql/format/format_test.go
@@ -18,11 +18,12 @@ package format_test
 
 import (
 	"fmt"
+	"testing"
+
 	"github.com/specterops/bloodhound/cypher/models"
 	"github.com/specterops/bloodhound/cypher/models/pgsql"
 	"github.com/specterops/bloodhound/cypher/models/pgsql/format"
 	"github.com/stretchr/testify/require"
-	"testing"
 )
 
 func mustAsLiteral(value any) pgsql.Literal {

diff --git a/packages/go/cypher/models/pgsql/identifiers.go b/packages/go/cypher/models/pgsql/identifiers.go
@@ -17,9 +17,10 @@
 package pgsql
 
 import (
-	"github.com/specterops/bloodhound/cypher/models"
 	"slices"
 	"strings"
+
+	"github.com/specterops/bloodhound/cypher/models"
 )
 
 const (

diff --git a/packages/go/cypher/models/pgsql/identifiers_test.go b/packages/go/cypher/models/pgsql/identifiers_test.go
@@ -1,8 +1,25 @@
+// Copyright 2024 Specter Ops, Inc.
+//
+// Licensed under the Apache License, Version 2.0
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// SPDX-License-Identifier: Apache-2.0
+
 package pgsql
 
 import (
-	"github.com/stretchr/testify/assert"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestIdentifierSet_CombinedKey(t *testing.T) {

diff --git a/packages/go/cypher/models/pgsql/model.go b/packages/go/cypher/models/pgsql/model.go
@@ -17,9 +17,10 @@
 package pgsql
 
 import (
-	"github.com/specterops/bloodhound/dawgs/graph"
 	"strings"
 
+	"github.com/specterops/bloodhound/dawgs/graph"
+
 	"github.com/specterops/bloodhound/cypher/models"
 )