From 78d4e23c74d19f57321ac67287db429327894c47 Mon Sep 17 00:00:00 2001 From: Mahendra Paipuri Date: Thu, 11 Jul 2024 12:22:47 +0200 Subject: [PATCH] feat: Incremental improvements on server * When queried fields are invalid, return error * Allow listing all users using admin endpoint * Add e2e tests validating new features Signed-off-by: Mahendra Paipuri --- pkg/api/http/error.go | 7 +++-- pkg/api/http/querier.go | 6 ---- pkg/api/http/server.go | 28 +++++++++++++++++-- ...2e-test-api-server-units-invalid-query.txt | 1 + ...e-test-api-server-user-admin-all-query.txt | 1 + scripts/e2e-test.sh | 14 ++++++++++ 6 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 pkg/api/testdata/output/e2e-test-api-server-units-invalid-query.txt create mode 100644 pkg/api/testdata/output/e2e-test-api-server-user-admin-all-query.txt diff --git a/pkg/api/http/error.go b/pkg/api/http/error.go index 16c7206d..0327c748 100644 --- a/pkg/api/http/error.go +++ b/pkg/api/http/error.go @@ -47,9 +47,10 @@ const ( // Custom errors var ( - errNoUser = errors.New("no user identified") - errNoPrivs = errors.New("current user does not have admin privileges") - errInvalidRequest = errors.New("invalid request") + errNoUser = errors.New("no user identified") + errNoPrivs = errors.New("current user does not have admin privileges") + errInvalidRequest = errors.New("invalid request") + errInvalidQueryField = errors.New("invalid query fields") ) // Return error response for by setting errorString and errorType in response diff --git a/pkg/api/http/querier.go b/pkg/api/http/querier.go index efef1c87..5b544c63 100644 --- a/pkg/api/http/querier.go +++ b/pkg/api/http/querier.go @@ -108,12 +108,6 @@ func countRows(dbConn *sql.DB, query Query) (int, error) { } defer countStmt.Close() - queryStmt, err := dbConn.Prepare(queryString) - if err != nil { - return 0, err - } - defer queryStmt.Close() - // queryParams has to be an inteface. Do casting here qParams := make([]interface{}, len(queryParams)) for i, v := range queryParams { diff --git a/pkg/api/http/server.go b/pkg/api/http/server.go index 539a7c48..ec01a697 100644 --- a/pkg/api/http/server.go +++ b/pkg/api/http/server.go @@ -431,6 +431,11 @@ func (s *CEEMSServer) unitsQuerier( // Get fields query parameters if any queriedFields := s.getQueriedFields(r.URL.Query(), base.UnitsDBTableColNames) + if len(queriedFields) == 0 { + level.Error(s.logger).Log("msg", "Invalid query fields", "loggedUser", loggedUser, "err", errInvalidQueryField) + errorResponse[any](w, &apiError{errorBadData, errInvalidQueryField}, s.logger, nil) + return + } // Initialise query builder q := Query{} @@ -748,8 +753,14 @@ func (s *CEEMSServer) usersQuerier(users []string, w http.ResponseWriter, r *htt // Make query q := Query{} q.query(fmt.Sprintf("SELECT * FROM %s", base.UsersDBTableName)) - q.query(" WHERE name IN ") - q.param(users) + // If no user is queried, return all users. This can happen only for admin + // end points + if len(users) == 0 { + q.query(" WHERE name LIKE '%' ") + } else { + q.query(" WHERE name IN ") + q.param(users) + } // Get cluster_id query parameters if any if clusterIDs := r.URL.Query()["cluster_id"]; len(clusterIDs) > 0 { @@ -1147,6 +1158,11 @@ func (s *CEEMSServer) usage(w http.ResponseWriter, r *http.Request) { // Get fields query parameters if any queriedFields := s.getQueriedFields(r.URL.Query(), base.UsageDBTableColNames) + if len(queriedFields) == 0 { + level.Error(s.logger).Log("msg", "Invalid query fields", "loggedUser", dashboardUser) + errorResponse[any](w, &apiError{errorBadData, errInvalidQueryField}, s.logger, nil) + return + } // handle current usage query if mode == "current" { @@ -1211,6 +1227,9 @@ func (s *CEEMSServer) usageAdmin(w http.ResponseWriter, r *http.Request) { // Set headers s.setHeaders(w) + // Get current user from header + _, dashboardUser := s.getUser(r) + // Get path parameter type var mode string var exists bool @@ -1221,6 +1240,11 @@ func (s *CEEMSServer) usageAdmin(w http.ResponseWriter, r *http.Request) { // Get fields query parameters if any queriedFields := s.getQueriedFields(r.URL.Query(), base.UsageDBTableColNames) + if len(queriedFields) == 0 { + level.Error(s.logger).Log("msg", "Invalid query fields", "loggedUser", dashboardUser) + errorResponse[any](w, &apiError{errorBadData, errInvalidQueryField}, s.logger, nil) + return + } // handle current usage query if mode == "current" { diff --git a/pkg/api/testdata/output/e2e-test-api-server-units-invalid-query.txt b/pkg/api/testdata/output/e2e-test-api-server-units-invalid-query.txt new file mode 100644 index 00000000..46f769ff --- /dev/null +++ b/pkg/api/testdata/output/e2e-test-api-server-units-invalid-query.txt @@ -0,0 +1 @@ +{"status":"error","data":null,"errorType":"bad_data","error":"invalid query fields"} diff --git a/pkg/api/testdata/output/e2e-test-api-server-user-admin-all-query.txt b/pkg/api/testdata/output/e2e-test-api-server-user-admin-all-query.txt new file mode 100644 index 00000000..6aa78555 --- /dev/null +++ b/pkg/api/testdata/output/e2e-test-api-server-user-admin-all-query.txt @@ -0,0 +1 @@ +{"status":"success","data":[{"cluster_id":"slurm-0","resource_manager":"slurm","name":"testusr","projects":["testacc"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr1","projects":["acc1","acc2"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr15","projects":["acc1"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr2","projects":["acc2"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr3","projects":["acc3"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr4","projects":["acc4"]},{"cluster_id":"slurm-0","resource_manager":"slurm","name":"usr8","projects":["acc1"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"testusr","projects":["testacc"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr1","projects":["acc1","acc2"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr15","projects":["acc1"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr2","projects":["acc2"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr3","projects":["acc3"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr4","projects":["acc4"]},{"cluster_id":"slurm-1","resource_manager":"slurm","name":"usr8","projects":["acc1"]}]} diff --git a/scripts/e2e-test.sh b/scripts/e2e-test.sh index a653a174..035d358c 100755 --- a/scripts/e2e-test.sh +++ b/scripts/e2e-test.sh @@ -104,6 +104,10 @@ then then desc="/users/admin end point test" fixture='pkg/api/testdata/output/e2e-test-api-server-user-admin-query.txt' + elif [ "${scenario}" = "api-user-admin-all-query" ] + then + desc="/users/admin end point test that queries all users" + fixture='pkg/api/testdata/output/e2e-test-api-server-user-admin-all-query.txt' elif [ "${scenario}" = "api-cluster-admin-query" ] then desc="/clusters/admin end point test" @@ -112,6 +116,10 @@ then then desc="/units end point test with uuid query param" fixture='pkg/api/testdata/output/e2e-test-api-server-uuid-query.txt' + elif [ "${scenario}" = "api-units-invalid-query" ] + then + desc="/units end point test with invalid field query" + fixture='pkg/api/testdata/output/e2e-test-api-server-units-invalid-query.txt' elif [ "${scenario}" = "api-running-query" ] then desc="/units end point test with running query param" @@ -425,12 +433,18 @@ then elif [ "${scenario}" = "api-user-admin-query" ] then get -H "X-Grafana-User: grafana" "127.0.0.1:${port}/api/${api_version}/users/admin?user=usr1" > "${fixture_output}" + elif [ "${scenario}" = "api-user-admin-all-query" ] + then + get -H "X-Grafana-User: grafana" "127.0.0.1:${port}/api/${api_version}/users/admin" > "${fixture_output}" elif [ "${scenario}" = "api-cluster-admin-query" ] then get -H "X-Ceems-User: usr1" "127.0.0.1:${port}/api/${api_version}/clusters/admin" > "${fixture_output}" elif [ "${scenario}" = "api-uuid-query" ] then get -H "X-Grafana-User: usr2" "127.0.0.1:${port}/api/${api_version}/units?uuid=1481508&project=acc2&cluster_id=slurm-0" > "${fixture_output}" + elif [ "${scenario}" = "api-units-invalid-query" ] + then + get -H "X-Grafana-User: usr2" "127.0.0.1:${port}/api/${api_version}/units?cluster_id=slurm-0&from=1676934000&to=1677538800&field=uuiid" > "${fixture_output}" elif [ "${scenario}" = "api-running-query" ] then get -H "X-Grafana-User: usr3" "127.0.0.1:${port}/api/${api_version}/units?running&cluster_id=slurm-1&from=1676934000&to=1677538800&field=uuid&field=state&field=started_at&field=allocation&field=tags" > "${fixture_output}"