Skip to content

Commit

Permalink
refactor: Move ownership verify into separate file
Browse files Browse the repository at this point in the history
* Add test that uses same data as lb

* Add and update test fixture outputs

Signed-off-by: Mahendra Paipuri <[email protected]>
  • Loading branch information
mahendrapaipuri committed Apr 20, 2024
1 parent 96b6386 commit 034ca18
Show file tree
Hide file tree
Showing 14 changed files with 261 additions and 131 deletions.
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ test-e2e: $(PROMTOOL) build pkg/collector/testdata/sys/.unpacked pkg/collector/t
./scripts/e2e-test.sh -s api-current-usage-admin-query
./scripts/e2e-test.sh -s api-global-usage-admin-query
./scripts/e2e-test.sh -s api-current-usage-admin-denied-query
./scripts/e2e-test.sh -s api-verify-pass-query
./scripts/e2e-test.sh -s api-verify-fail-query
@env GOBIN=$(FIRST_GOPATH) ./scripts/e2e-test.sh -s lb-basic
@env GOBIN=$(FIRST_GOPATH) ./scripts/e2e-test.sh -s lb-forbid-user-query-db
@env GOBIN=$(FIRST_GOPATH) ./scripts/e2e-test.sh -s lb-allow-user-query-db
Expand Down Expand Up @@ -188,6 +190,8 @@ test-e2e-update: $(PROMTOOL) build pkg/collector/testdata/sys/.unpacked pkg/coll
./scripts/e2e-test.sh -s api-current-usage-admin-query -u || true
./scripts/e2e-test.sh -s api-global-usage-admin-query -u || true
./scripts/e2e-test.sh -s api-current-usage-admin-denied-query -u || true
./scripts/e2e-test.sh -s api-verify-pass-query -u || true
./scripts/e2e-test.sh -s api-verify-fail-query -u || true
@env GOBIN=$(FIRST_GOPATH) ./scripts/e2e-test.sh -s lb-basic -u || true
@env GOBIN=$(FIRST_GOPATH) ./scripts/e2e-test.sh -s lb-forbid-user-query-db -u || true
@env GOBIN=$(FIRST_GOPATH) ./scripts/e2e-test.sh -s lb-allow-user-query-db -u || true
Expand Down
118 changes: 17 additions & 101 deletions pkg/api/http/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
_ "net/http/pprof"
"net/url"
"path/filepath"
"reflect"
"slices"
"strconv"
"strings"
Expand Down Expand Up @@ -99,99 +98,6 @@ func init() {
}
}

// Convert a slice of types into slice of interfaces
func islice(x interface{}) []interface{} {
xv := reflect.ValueOf(x)
out := make([]interface{}, xv.Len())
for i := range out {
out[i] = xv.Index(i).Interface()
}
return out
}

// UnitOwnership returns true if user is the owner of queried units
func UnitOwnership(user string, uuids []string, db *sql.DB, logger log.Logger) bool {
// If there is no active DB conn or if uuids is empty, return
if db == nil || len(uuids) == 0 {
return true
}

level.Debug(logger).
Log("msg", "UUIDs in query", "user", user, "queried_uuids", strings.Join(uuids, ","))

// First get a list of projects that user is part of
rows, err := db.Query(
fmt.Sprintf("SELECT DISTINCT project FROM %s WHERE usr = ?", base.UsageDBTableName),
user,
)
if err != nil {
level.Warn(logger).
Log("msg", "Failed to get user projects. Allowing query", "user", user,
"queried_uuids", strings.Join(uuids, ","), "err", err,
)
return false
}

// Scan project rows
var projects []string
var project string
for rows.Next() {
if err := rows.Scan(&project); err != nil {
continue
}
projects = append(projects, project)
}

// If no projects found, return. This should not be the case and if it happens
// something is wrong. Spit a warning log
if len(projects) == 0 {
level.Warn(logger).
Log("msg", "No user projects found. Query unauthorized", "user", user,
"queried_uuids", strings.Join(uuids, ","), "err", err,
)
return false
}

// Make a query and query args. Query args must be converted to slice of interfaces
// and it is sql driver's responsibility to cast them to proper types
query := fmt.Sprintf(
"SELECT uuid FROM %s WHERE project IN (%s) AND uuid IN (%s)",
base.UnitsDBTableName,
strings.Join(strings.Split(strings.Repeat("?", len(projects)), ""), ","),
strings.Join(strings.Split(strings.Repeat("?", len(uuids)), ""), ","),
)
queryData := islice(append(projects, uuids...))

// Make query. If query fails for any reason, we allow request to avoid false negatives
rows, err = db.Query(query, queryData...)
if err != nil {
level.Warn(logger).
Log("msg", "Failed to check uuid ownership. Query unauthorized", "user", user, "query", query,
"user_projects", strings.Join(projects, ","), "queried_uuids", strings.Join(uuids, ","),
"err", err,
)
return false
}
defer rows.Close()

// Get number of rows returned by query
uuidCount := 0
for rows.Next() {
uuidCount++
}

// If returned number of UUIDs is not same as queried UUIDs, user is attempting
// to query for jobs of other user
if uuidCount != len(uuids) {
level.Debug(logger).
Log("msg", "Unauthorized query", "user", user, "user_projects", strings.Join(projects, ","),
"queried_uuids", len(uuids), "found_uuids", uuidCount,
)
return false
}
return true
}

// Ping DB for connection test
func getDBStatus(dbConn *sql.DB, logger log.Logger) bool {
if err := dbConn.Ping(); err != nil {
Expand Down Expand Up @@ -247,7 +153,7 @@ func NewCEEMSServer(c *Config) (*CEEMSServer, func(), error) {
Methods("GET")
router.HandleFunc(fmt.Sprintf("/api/%s/{query:(?:current|global)}/admin", usageResourceName), server.usageAdmin).
Methods("GET")
router.HandleFunc(fmt.Sprintf("/api/%s/validate", unitsResourceName), server.unitsValidation).Methods("GET")
router.HandleFunc(fmt.Sprintf("/api/%s/verify", unitsResourceName), server.verifyUnitsOwnership).Methods("GET")

// pprof debug end points
router.PathPrefix("/debug/").Handler(http.DefaultServeMux)
Expand Down Expand Up @@ -500,9 +406,9 @@ func (s *CEEMSServer) units(w http.ResponseWriter, r *http.Request) {
s.unitsQuerier([]string{dashboardUser}, w, r)
}

// GET /api/units/validate
// GET /api/units/verify
// Verify the user ownership for queried units
func (s *CEEMSServer) unitsValidation(w http.ResponseWriter, r *http.Request) {
func (s *CEEMSServer) verifyUnitsOwnership(w http.ResponseWriter, r *http.Request) {
// Set headers
s.setHeaders(w)

Expand All @@ -513,12 +419,22 @@ func (s *CEEMSServer) unitsValidation(w http.ResponseWriter, r *http.Request) {
uuids := r.URL.Query()["uuid"]

// Check if user is owner of the queries uuids
if UnitOwnership(dashboardUser, uuids, s.db, s.logger) {
if VerifyOwnership(dashboardUser, uuids, s.db, s.logger) {
w.WriteHeader(http.StatusOK)
w.Write([]byte("success"))
response := Response{
Status: "success",
Data: map[string]interface{}{
"user": dashboardUser,
"uuids": uuids,
"verfiy": true,
},
}
if err := json.NewEncoder(w).Encode(&response); err != nil {
level.Error(s.logger).Log("msg", "Failed to encode response", "err", err)
w.Write([]byte("KO"))
}
} else {
w.WriteHeader(http.StatusUnauthorized)
w.Write([]byte("fail"))
errorResponse(w, &apiError{errorUnauthorized, fmt.Errorf("user do not have permissions on uuids")}, s.logger, nil)
}
}

Expand Down
105 changes: 105 additions & 0 deletions pkg/api/http/validation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package http

import (
"database/sql"
"fmt"
"reflect"
"strings"

"github.com/go-kit/log"
"github.com/go-kit/log/level"
"github.com/mahendrapaipuri/ceems/pkg/api/base"
)

// VerifyOwnership returns true if user is the owner of queried units
func VerifyOwnership(user string, uuids []string, db *sql.DB, logger log.Logger) bool {
// If there is no active DB conn or if uuids is empty, return
if db == nil || len(uuids) == 0 {
return true
}

level.Debug(logger).
Log("msg", "UUIDs in query", "user", user, "queried_uuids", strings.Join(uuids, ","))

// First get a list of projects that user is part of
rows, err := db.Query(
fmt.Sprintf("SELECT DISTINCT project FROM %s WHERE usr = ?", base.UsageDBTableName),
user,
)
if err != nil {
level.Warn(logger).
Log("msg", "Failed to get user projects. Allowing query", "user", user,
"queried_uuids", strings.Join(uuids, ","), "err", err,
)
return false
}

// Scan project rows
var projects []string
var project string
for rows.Next() {
if err := rows.Scan(&project); err != nil {
continue
}
projects = append(projects, project)
}

// If no projects found, return. This should not be the case and if it happens
// something is wrong. Spit a warning log
if len(projects) == 0 {
level.Warn(logger).
Log("msg", "No user projects found. Query unauthorized", "user", user,
"queried_uuids", strings.Join(uuids, ","), "err", err,
)
return false
}

// Make a query and query args. Query args must be converted to slice of interfaces
// and it is sql driver's responsibility to cast them to proper types
query := fmt.Sprintf(
"SELECT uuid FROM %s WHERE project IN (%s) AND uuid IN (%s)",
base.UnitsDBTableName,
strings.Join(strings.Split(strings.Repeat("?", len(projects)), ""), ","),
strings.Join(strings.Split(strings.Repeat("?", len(uuids)), ""), ","),
)
queryData := islice(append(projects, uuids...))

// Make query. If query fails for any reason, we allow request to avoid false negatives
rows, err = db.Query(query, queryData...)
if err != nil {
level.Warn(logger).
Log("msg", "Failed to check uuid ownership. Query unauthorized", "user", user, "query", query,
"user_projects", strings.Join(projects, ","), "queried_uuids", strings.Join(uuids, ","),
"err", err,
)
return false
}
defer rows.Close()

// Get number of rows returned by query
uuidCount := 0
for rows.Next() {
uuidCount++
}

// If returned number of UUIDs is not same as queried UUIDs, user is attempting
// to query for jobs of other user
if uuidCount != len(uuids) {
level.Debug(logger).
Log("msg", "Unauthorized query", "user", user, "user_projects", strings.Join(projects, ","),
"queried_uuids", len(uuids), "found_uuids", uuidCount,
)
return false
}
return true
}

// Convert a slice of types into slice of interfaces
func islice(x interface{}) []interface{} {
xv := reflect.ValueOf(x)
out := make([]interface{}, xv.Len())
for i := range out {
out[i] = xv.Index(i).Interface()
}
return out
}
105 changes: 105 additions & 0 deletions pkg/api/http/validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package http

import (
"database/sql"
"fmt"
"path/filepath"
"testing"

"github.com/go-kit/log"
)

// Same as the one in lb/frontend/middleware_test.go
func setupMockDB(d string) (*sql.DB, string) {
dbPath := filepath.Join(d, "test.db")
db, err := sql.Open("sqlite3", dbPath)
if err != nil {
fmt.Printf("failed to create DB")
}

stmts := `
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE units (
"id" integer not null primary key,
"uuid" text,
"project" text,
"usr" text
);
INSERT INTO units VALUES(1,'1479763', 'prj1', 'usr1');
INSERT INTO units VALUES(2,'1481508', 'prj1', 'usr2');
INSERT INTO units VALUES(3,'1479765', 'prj2', 'usr2');
INSERT INTO units VALUES(4,'1481510', 'prj3', 'usr3');
CREATE TABLE usage (
"id" integer not null primary key,
"project" text,
"usr" text
);
INSERT INTO usage VALUES(1, 'prj1', 'usr1');
INSERT INTO usage VALUES(2, 'prj1', 'usr2');
INSERT INTO usage VALUES(3, 'prj2', 'usr2');
INSERT INTO usage VALUES(4, 'prj3', 'usr3');
COMMIT;
`
_, err = db.Exec(stmts)
if err != nil {
fmt.Printf("failed to insert mock data into DB: %s", err)
}
return db, dbPath
}

func TestVerifyOwnership(t *testing.T) {
db, _ := setupMockDB(t.TempDir())

tests := []struct {
name string
uuids []string
user string
verify bool
}{
{
name: "forbid due to mismatch uuid",
uuids: []string{"1479765", "1481510"},
user: "usr1",
verify: false,
},
{
name: "forbid due to missing project",
uuids: []string{"123", "345"},
user: "usr1",
verify: false,
},
{
name: "forbid due to missing header",
uuids: []string{"123", "345"},
user: "",
verify: false,
},
{
name: "pass due to correct uuid",
uuids: []string{"1479763"},
user: "usr1",
verify: true,
},
{
name: "pass due to uuid from same project",
uuids: []string{"1481508"},
user: "usr1",
verify: true,
},
{
name: "pass due to no uuid",
uuids: []string{},
user: "usr3",
verify: true,
},
}

for _, test := range tests {
result := VerifyOwnership(test.user, test.uuids, db, log.NewNopLogger())

if result != test.verify {
t.Errorf("%s: expected %t, got %t", test.name, test.verify, result)
}
}
}

This file was deleted.

This file was deleted.

1 change: 1 addition & 0 deletions pkg/api/testdata/output/e2e-test-api-verify-fail-query.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"status":"error","errorType":"unauthorized","error":"user do not have permissions on uuids"}
1 change: 1 addition & 0 deletions pkg/api/testdata/output/e2e-test-api-verify-pass-query.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"status":"success","data":{"user":"usr1","uuids":["1479763","1479765"],"verfiy":true}}
Loading

0 comments on commit 034ca18

Please sign in to comment.