From 552f6b807bf04130b3b63b351a5ef2d48da4a679 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Thu, 3 Sep 2026 18:38:58 -0300 Subject: [PATCH] refactor(server): give the List shape the query contract it serves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourteen routes answered with a page of a collection, and thirteen opened by hand-rolling the same preamble: bind, normalize the paginator, normalize the sorter, unmarshal the base64 filter, validate the filter fields, validate the sort fields, answer 400. One contract, written thirteen times, in four different orders, with three of them skipping field validation altogether. The order is now fixed, and it is the wrapper's: normalize the page, normalize the sort applying the resource's default, decode the filter, hold the filter to the contract, hold the sort to the contract, then the struct validation that was always last. Each step's failure was a 400 before and is a 400 now, so reordering them changes no status. The 400 gains a body: the invalid-entity error the converted device list already returned, which the shared OpenAPI 400 component has documented all along. That is the deliberate behaviour change. Three others ride along with the List shape and are worth naming: - the thirteen routes now require an actor, because the shape's wrapper resolves one. Every production caller carries X-ID; three test files did not, and now do. - /namespaces and /users/invitations declare an unbounded scope. Both answer across namespaces for a caller who may have selected none, so a bounded scope would 403 them. - the SSH identity list's 401 gains a body, having been a bodiless NoContent. ListSSHIdentities keeps a check on actor.ID that looks dead next to the wrapper's own: the wrapper refuses a *zero* actor, and an API-key actor is not zero — it names a namespace with no person behind it, so its user ID is empty. The route is open to API keys, so the check is live. It reads the role through gateway.RoleFromContext rather than from the actor, because a role is a membership's, and an actor is not yet a member of anything. Access policies, service accounts and SSH identities set X-Total-Count to the length of the page they were about to return, because their services discarded the count the store had already computed. The count now comes from the store. For the first two that is the same number today; the point is that it stops being the handler's to decide the day the route paginates. The service-account store still derives its count from the slice, which is now the store's business and not the route's. list_validation_test.go is deleted in the same change. It was a linter written as a 171-line go/ast test, and it had already gone blind: it matched handlers by their gateway-context parameter, so the device list left its coverage the moment #6941 converted that route, and it kept passing. The rule it enforced now reads the route table, where a route appears whatever shape its handler has. This breaks cloud, which reaches across the module replace for services.NamespaceFilterFields. That is the next rung, as A-cloud was; it cannot start until shellhub-io/cloud#2534 lands. Implements the B rung of shellhub-io/shellhub#7032. Fixes: shellhub-io/shellhub#7034 --- pkg/api/query/contract.go | 34 ++ pkg/api/query/filter.go | 12 + server/api/pkg/gateway/gateway.go | 13 + server/api/pkg/gateway/route.go | 64 +++- server/api/pkg/gateway/route_test.go | 137 ++++++++ server/api/routes/access-policy.go | 20 +- server/api/routes/api-key.go | 41 +-- server/api/routes/api-key_test.go | 4 + server/api/routes/device.go | 22 -- server/api/routes/device_handler_test.go | 118 +------ server/api/routes/install-key.go | 76 +---- server/api/routes/invitation.go | 83 +---- .../api/routes/list_query_rejection_test.go | 318 +++++++++++++----- server/api/routes/list_validation_test.go | 171 ---------- server/api/routes/nsadm.go | 63 +--- server/api/routes/route_table_test.go | 35 ++ server/api/routes/routes.go | 32 +- server/api/routes/service-account.go | 24 +- server/api/routes/session.go | 44 +-- server/api/routes/session_test.go | 10 +- server/api/routes/ssh-identity.go | 34 +- server/api/routes/sshkeys.go | 40 +-- server/api/routes/sshkeys_test.go | 2 +- server/api/routes/tags.go | 45 +-- server/api/services/access-policy.go | 22 +- server/api/services/access-policy_test.go | 24 ++ server/api/services/api-key.go | 20 +- server/api/services/device.go | 52 ++- server/api/services/install-key.go | 50 +-- server/api/services/invitation.go | 53 +-- server/api/services/invitation_test.go | 4 +- server/api/services/mocks/mock_service.go | 100 +++--- server/api/services/namespace.go | 21 +- server/api/services/namespace_test.go | 16 +- server/api/services/service-account.go | 24 +- server/api/services/service-account_test.go | 26 ++ server/api/services/session.go | 19 +- server/api/services/ssh-identity.go | 20 +- server/api/services/ssh-identity_test.go | 13 +- server/api/services/sshkeys.go | 13 +- server/api/services/tags.go | 23 +- server/api/services/tags_test.go | 24 +- 42 files changed, 939 insertions(+), 1027 deletions(-) create mode 100644 pkg/api/query/contract.go delete mode 100644 server/api/routes/list_validation_test.go diff --git a/pkg/api/query/contract.go b/pkg/api/query/contract.go new file mode 100644 index 00000000000..c22e29a5778 --- /dev/null +++ b/pkg/api/query/contract.go @@ -0,0 +1,34 @@ +package query + +// Contract is what one resource's list endpoint accepts from a client: the filter fields with the +// operators valid against each, the fields it may be ordered by, and the order it is served in when +// the client names none. +// +// The zero value accepts no filter and no sort, which is what a resource taking neither declares — +// an omitted contract is indistinguishable from a forgotten one, so there is no such thing. +type Contract struct { + Filter FieldConstraints + Sort FieldSet + + // DefaultSort is applied to a sorter the client left blank. It is applied before the sort is + // validated, so a default naming a field outside Sort is refused exactly as a client's would be. + DefaultSort Sorter +} + +// NormalizeSorter fills from the contract's default whatever the client left blank, then holds the +// order to asc or desc. A zero contract leaves the sorter as [Sorter.Normalize] would. +func (c Contract) NormalizeSorter(sorter *Sorter) { + if sorter == nil { + return + } + + if sorter.By == "" { + sorter.By = c.DefaultSort.By + } + + if sorter.Order == "" { + sorter.Order = c.DefaultSort.Order + } + + sorter.Normalize() +} diff --git a/pkg/api/query/filter.go b/pkg/api/query/filter.go index 4449f079d5f..79e73258a18 100644 --- a/pkg/api/query/filter.go +++ b/pkg/api/query/filter.go @@ -65,6 +65,18 @@ func (fs *Filters) Unmarshal() error { return nil } +// Filtered is a request that carries a [Filters]. It is the filtering counterpart of [Paginated] +// and [Sorted], and exists for the same reason: it lets a caller holding only the request decode +// the filter without knowing the request's concrete type. +type Filtered interface { + GetFilters() *Filters +} + +// GetFilters returns the filters themselves, satisfying [Filtered] for every type that embeds it. +func (fs *Filters) GetFilters() *Filters { + return fs +} + // Filter is one node of a query filter: a tagged union whose Type picks the shape of Params. // Unmarshal one rather than building it by hand, or Params holds a map instead of a params struct. type Filter struct { diff --git a/server/api/pkg/gateway/gateway.go b/server/api/pkg/gateway/gateway.go index adda36c1f62..8eb4f94fd48 100644 --- a/server/api/pkg/gateway/gateway.go +++ b/server/api/pkg/gateway/gateway.go @@ -7,6 +7,7 @@ package gateway import ( "context" + "github.com/shellhub-io/shellhub/pkg/api/authorizer" "github.com/shellhub-io/shellhub/pkg/models" ) @@ -25,6 +26,18 @@ func TenantFromContext(ctx context.Context) *models.Tenant { return nil } +// RoleFromContext returns the role the request authenticated with, or the zero role when it carries +// none. It is what a handler taking no gateway [Context] reads to widen or narrow what it serves by +// the caller's authority — a decision the route's permission cannot express, because the route is +// reachable either way. +func RoleFromContext(ctx context.Context) authorizer.Role { + if c, ok := ctx.Value("ctx").(*Context); ok { + return c.Role() + } + + return authorizer.RoleInvalid +} + // UsernameFromContext returns the authenticated username, or nil when the request is // anonymous or authenticated by an API key. func UsernameFromContext(ctx context.Context) *models.Username { diff --git a/server/api/pkg/gateway/route.go b/server/api/pkg/gateway/route.go index 4911e734d88..8b25e37010b 100644 --- a/server/api/pkg/gateway/route.go +++ b/server/api/pkg/gateway/route.go @@ -152,12 +152,8 @@ func prepare[T any](c *echo.Context, declaration Declaration) (inputs[T], error) return inputs[T]{}, err } - if paginated, ok := any(req).(query.Paginated); ok { - paginated.GetPaginator().Normalize() - } - - if sorted, ok := any(req).(query.Sorted); ok { - sorted.GetSorter().Normalize() + if err := applyQuery(req, declaration); err != nil { + return inputs[T]{}, err } if err := c.Validate(req); err != nil { @@ -177,6 +173,42 @@ func prepare[T any](c *echo.Context, declaration Declaration) (inputs[T], error) return inputs[T]{ctx: gCtx.Ctx(), scope: sc, actor: actor, req: req}, nil } +func applyQuery[T any](req *T, declaration Declaration) error { + if paginated, ok := any(req).(query.Paginated); ok { + paginated.GetPaginator().Normalize() + } + + sorted, sorts := any(req).(query.Sorted) + if sorts { + declaration.Query.NormalizeSorter(sorted.GetSorter()) + } + + if !declaration.AcceptsQuery { + return nil + } + + if filtered, ok := any(req).(query.Filtered); ok { + filters := filtered.GetFilters() + + if err := filters.Unmarshal(); err != nil { + return routes.NewErrInvalidEntity(map[string]string{"filter": "cannot be decoded"}) + } + + if err := query.ValidateFilters(filters, declaration.Query.Filter); err != nil { + return routes.NewErrInvalidEntity(map[string]string{"filter": "is not valid"}) + } + } + + if sorts { + sorter := sorted.GetSorter() + if err := query.ValidateSorter(sorter, declaration.Query.Sort); err != nil { + return routes.NewErrInvalidEntity(map[string]string{"sort_by": sorter.By}) + } + } + + return nil +} + // RouteOption states one claim a route's registration makes, and returns the guard that enforces // it — or nil when the claim is enforced by the wrapper rather than by a middleware. Options are // applied in the order they are written, and their guards run in that same order. @@ -230,6 +262,20 @@ func NoAPIKey() RouteOption { } } +// Accepts declares the query contract a list route holds a client's filter and sort to, and +// installs no guard: the wrapper enforces it, before the handler is called and in one fixed order. +// +// A resource that accepts neither a filter nor a sort names a contract allowing nothing rather than +// leaving the option off, because an omission cannot be told from a forgotten one — which is the +// failure the route table's invariant exists to catch. +func Accepts(contract query.Contract) RouteOption { + return func(d *Declaration) echo.MiddlewareFunc { + d.Query, d.AcceptsQuery = contract, true + + return nil + } +} + // Guard installs a middleware the declaration says nothing about. It is what the guards that are // not claims — the tenant check, the legacy authorize middleware — are written with, and it runs // in the position it is written in, among the guards the other options install. @@ -265,6 +311,12 @@ type Declaration struct { // route's permission is refused all the same. BlocksAPIKey bool + // Query is the filter and sort fields the route accepts from a client, and the order it serves + // when the client names none. The zero value accepts neither, so AcceptsQuery is what tells a + // route deliberately taking no query from one that named no contract at all. + Query query.Contract + AcceptsQuery bool + Unbounded bool UnboundedReason string diff --git a/server/api/pkg/gateway/route_test.go b/server/api/pkg/gateway/route_test.go index 5aa1f89db98..9315c686cfd 100644 --- a/server/api/pkg/gateway/route_test.go +++ b/server/api/pkg/gateway/route_test.go @@ -2,12 +2,16 @@ package gateway_test import ( "context" + "encoding/base64" + "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/labstack/echo/v5" "github.com/shellhub-io/shellhub/pkg/api/query" + "github.com/shellhub-io/shellhub/pkg/api/responses" "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/pkg/errors" "github.com/shellhub-io/shellhub/server/api/pkg/echo/handlers" @@ -23,6 +27,7 @@ type probeRequest struct { Name string `query:"name" validate:"omitempty,min=3"` query.Paginator query.Sorter + query.Filters } type probeCall struct { @@ -372,3 +377,135 @@ func TestDeclarationsRecordEveryClaim(t *testing.T) { assert.True(t, found, "the wrapper recorded no declaration for the probe route") } + +// TestListHoldsTheQueryToTheContractItsRegistrationNamed drives the contract mechanism once, +// through a mounted route: the wrapper is what decodes the filter and refuses a field the +// resource does not allow, so no handler opens with a validation preamble. +func TestListHoldsTheQueryToTheContractItsRegistrationNamed(t *testing.T) { + contract := query.Contract{ + Filter: query.NewFieldConstraints(map[string][]string{"name": {"contains"}}), + Sort: query.NewFieldSet("name", "created_at"), + DefaultSort: query.Sorter{By: "created_at", Order: query.OrderAsc}, + } + + cases := []struct { + description string + target string + expectedStatus int + expectedFields map[string]string + assert func(*testing.T, *probeCall) + }{ + { + description: "refuses a filter naming a field the contract does not allow", + target: "/probe?filter=" + encodeProbeFilter(t, "signature", "contains"), + expectedStatus: http.StatusBadRequest, + expectedFields: map[string]string{"filter": "is not valid"}, + }, + { + description: "refuses a filter naming an operator the contract does not allow", + target: "/probe?filter=" + encodeProbeFilter(t, "name", "eq"), + expectedStatus: http.StatusBadRequest, + expectedFields: map[string]string{"filter": "is not valid"}, + }, + { + description: "refuses a filter that is not base64", + target: "/probe?filter=not-base64!!", + expectedStatus: http.StatusBadRequest, + expectedFields: map[string]string{"filter": "cannot be decoded"}, + }, + { + description: "refuses a filter larger than the cap", + target: "/probe?filter=" + strings.Repeat("A", query.MaxFilterRawBytes+1), + expectedStatus: http.StatusBadRequest, + expectedFields: map[string]string{"filter": "cannot be decoded"}, + }, + { + description: "refuses a sort naming a field the contract does not allow", + target: "/probe?sort_by=secret", + expectedStatus: http.StatusBadRequest, + expectedFields: map[string]string{"sort_by": "secret"}, + }, + { + description: "hands the handler a decoded filter, a normalized page and the contract's default sort", + target: "/probe?page=0&per_page=999&filter=" + encodeProbeFilter(t, "name", "contains"), + expectedStatus: http.StatusOK, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + require.Len(t, call.req.Filters.Data, 1) + assert.Equal(t, &query.FilterProperty{Name: "name", Operator: "contains", Value: "value"}, call.req.Filters.Data[0].Params) + assert.Equal(t, query.MinPage, call.req.Paginator.Page) + assert.Equal(t, query.MaxPerPage, call.req.Paginator.PerPage) + assert.Equal(t, "created_at", call.req.Sorter.By) + assert.Equal(t, query.OrderAsc, call.req.Sorter.Order) + }, + }, + { + description: "leaves a sort the client asked for alone when the contract allows it", + target: "/probe?sort_by=name&order_by=asc", + expectedStatus: http.StatusOK, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.Equal(t, "name", call.req.Sorter.By) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + call := new(probeCall) + + e := probeRouter(t, true) + gateway.GET(rootOf(e), "/probe", gateway.List(probeHandler(call, []string{"item"}, 1, nil)), gateway.Accepts(contract)) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tc.target, nil) + req.Header.Set("X-Tenant-ID", probeTenant) + req.Header.Set("X-ID", "user-id") + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + require.Equal(t, tc.expectedStatus, rec.Code, rec.Body.String()) + require.Equal(t, tc.expectedStatus == http.StatusOK, call.called) + + if tc.expectedFields != nil { + var body responses.Error + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + assert.Equal(t, tc.expectedFields, body.Fields) + } + + if tc.assert != nil { + tc.assert(t, call) + } + }) + } +} + +// TestAcceptsRecordsTheContractOnTheDeclaration is what the route table's invariant reads: a claim +// the wrapper enforces still has to be visible to an audit of the declarations. +func TestAcceptsRecordsTheContractOnTheDeclaration(t *testing.T) { + e := probeRouter(t, true) + gateway.GET(rootOf(e), "/accepting", gateway.List(probeHandler(new(probeCall), nil, 0, nil)), + gateway.Accepts(query.Contract{Sort: query.NewFieldSet("name")})) + gateway.GET(rootOf(e), "/silent", gateway.List(probeHandler(new(probeCall), nil, 0, nil))) + + accepts := make(map[string]bool) + for _, declaration := range gateway.Declarations(e) { + accepts[declaration.Path] = declaration.AcceptsQuery + } + + assert.True(t, accepts["/accepting"]) + assert.False(t, accepts["/silent"]) +} + +func encodeProbeFilter(t *testing.T, name, operator string) string { + t.Helper() + + encoded, err := json.Marshal([]query.Filter{ + {Type: query.FilterTypeProperty, Params: &query.FilterProperty{Name: name, Operator: operator, Value: "value"}}, + }) + require.NoError(t, err) + + return base64.RawURLEncoding.EncodeToString(encoded) +} diff --git a/server/api/routes/access-policy.go b/server/api/routes/access-policy.go index 14f45d99c6a..04b6398d8b7 100644 --- a/server/api/routes/access-policy.go +++ b/server/api/routes/access-policy.go @@ -1,10 +1,12 @@ package routes import ( + "context" "net/http" - "strconv" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" ) @@ -18,20 +20,8 @@ const ( ) // ListAccessPolicies serves the policy list for the caller's namespace. -func (h *Handler) ListAccessPolicies(c *gateway.Context) error { - var tenant string - if c.Tenant() != nil { - tenant = c.Tenant().ID - } - - list, err := h.service.ListAccessPolicies(c.Ctx(), tenant) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(len(list))) - - return c.JSON(http.StatusOK, list) +func (h *Handler) ListAccessPolicies(ctx context.Context, sc scope.Scope, _ gateway.Actor, _ *requests.AccessPolicyList) ([]models.AccessPolicy, int, error) { + return h.service.ListAccessPolicies(ctx, sc.TenantID()) } // GetAccessPolicy serves a single policy by id. diff --git a/server/api/routes/api-key.go b/server/api/routes/api-key.go index 433da9ec89a..3e0295da870 100644 --- a/server/api/routes/api-key.go +++ b/server/api/routes/api-key.go @@ -1,13 +1,13 @@ package routes import ( + "context" "net/http" - "strconv" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - "github.com/shellhub-io/shellhub/server/api/services" ) // The API key routes, relative to the API's base path. @@ -40,39 +40,8 @@ func (h *Handler) CreateAPIKey(c *gateway.Context) error { } // ListAPIKeys serves the namespace's keys, without their plaintext. -func (h *Handler) ListAPIKeys(c *gateway.Context) error { - req := new(requests.ListAPIKey) - - if err := c.Bind(req); err != nil { - return err - } - - req.Paginator.Normalize() - - if req.Sorter.By == "" { - req.Sorter.By = "expires_in" - } - - if req.Sorter.Order == "" { - req.Sorter.Order = "desc" - } - - if err := query.ValidateSorter(&req.Sorter, services.APIKeySortFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := c.Validate(req); err != nil { - return err - } - - res, count, err := h.service.ListAPIKeys(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - return c.JSON(http.StatusOK, res) +func (h *Handler) ListAPIKeys(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.ListAPIKey) ([]models.APIKey, int, error) { + return h.service.ListAPIKeys(ctx, req) } // UpdateAPIKey renames a key or changes the role it acts with. diff --git a/server/api/routes/api-key_test.go b/server/api/routes/api-key_test.go index 9210d3a8bf1..5683debde8b 100644 --- a/server/api/routes/api-key_test.go +++ b/server/api/routes/api-key_test.go @@ -307,6 +307,7 @@ func TestListAPIKey(t *testing.T) { description: "success", headers: map[string]string{ "Content-Type": "application/json", + "X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000", "X-Role": "owner", }, @@ -365,6 +366,7 @@ func TestListAPIKey(t *testing.T) { description: "success when page and per_page are invalid", headers: map[string]string{ "Content-Type": "application/json", + "X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000", "X-Role": "owner", }, @@ -423,6 +425,7 @@ func TestListAPIKey(t *testing.T) { description: "success when order_by is an empty string", headers: map[string]string{ "Content-Type": "application/json", + "X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000", "X-Role": "owner", }, @@ -479,6 +482,7 @@ func TestListAPIKey(t *testing.T) { description: "success when sort_by is an empty string", headers: map[string]string{ "Content-Type": "application/json", + "X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000", "X-Role": "owner", }, diff --git a/server/api/routes/device.go b/server/api/routes/device.go index d212507192a..a979458c789 100644 --- a/server/api/routes/device.go +++ b/server/api/routes/device.go @@ -4,14 +4,10 @@ import ( "context" "net/http" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - errs "github.com/shellhub-io/shellhub/server/api/routes/errors" - "github.com/shellhub-io/shellhub/server/api/services" - log "github.com/sirupsen/logrus" ) // The device routes, relative to the API's base path. @@ -36,24 +32,6 @@ const ( // GetDeviceList serves the namespace's devices, filtered, sorted and paginated as requested. func (h *Handler) GetDeviceList(ctx context.Context, sc scope.Scope, _ gateway.Actor, req *requests.DeviceList) ([]models.Device, int, error) { - if err := query.ValidateSorter(&req.Sorter, services.DeviceSortFields); err != nil { - log.WithError(err).WithField("sort_by", req.Sorter.By).Warn("failed to validate device list sorter") - - return nil, 0, errs.NewErrInvalidEntity(map[string]string{"sort_by": req.Sorter.By}) - } - - if err := req.Filters.Unmarshal(); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode device list filter") - - return nil, 0, errs.NewErrInvalidEntity(map[string]string{"filter": "cannot be decoded"}) - } - - if err := query.ValidateFilters(&req.Filters, services.DeviceFilterFields); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to validate device list filter") - - return nil, 0, errs.NewErrInvalidEntity(map[string]string{"filter": "is not valid"}) - } - return h.service.ListDevices(ctx, sc, req) } diff --git a/server/api/routes/device_handler_test.go b/server/api/routes/device_handler_test.go index 2dc6651043c..636f98a2557 100644 --- a/server/api/routes/device_handler_test.go +++ b/server/api/routes/device_handler_test.go @@ -1,17 +1,12 @@ package routes import ( - "encoding/base64" - "encoding/json" "testing" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" "github.com/shellhub-io/shellhub/pkg/api/scope" - "github.com/shellhub-io/shellhub/pkg/errors" "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - errs "github.com/shellhub-io/shellhub/server/api/routes/errors" svc "github.com/shellhub-io/shellhub/server/api/services" "github.com/shellhub-io/shellhub/server/api/services/mocks" "github.com/stretchr/testify/assert" @@ -72,109 +67,24 @@ func TestGetDeviceHandler(t *testing.T) { } } -// TestGetDeviceListHandler covers what the device list handler still decides now that the wrapper -// owns the ceremony: the sort field, the caller's encoded filter, and nothing else. +// TestGetDeviceListHandler drives what the device list handler still decides now that the wrapper +// owns the whole query ceremony: nothing but which service call the scope and the request go to. +// The refusals it used to make are the wrapper's, and are driven there. func TestGetDeviceListHandler(t *testing.T) { const tenantID = "00000000-0000-4000-0000-000000000000" - encode := func(t *testing.T, filters []query.Filter) string { - t.Helper() + req := &requests.DeviceList{TenantID: tenantID, Connector: true} - raw, err := json.Marshal(filters) - require.NoError(t, err) + service := mocks.NewMockService(t) + service. + On("ListDevices", gomock.Anything, scope.MustBounded(tenantID), req). + Return([]models.Device{{UID: "uid"}}, 7, nil). + Once() - return base64.StdEncoding.EncodeToString(raw) - } - - cases := []struct { - description string - req func(*testing.T) *requests.DeviceList - requiredMocks func(*mocks.MockService) - expectedFields map[string]string - }{ - { - description: "refuses a sort field the device list does not accept", - req: func(*testing.T) *requests.DeviceList { - return &requests.DeviceList{Sorter: query.Sorter{By: "not_a_column", Order: query.OrderAsc}} - }, - requiredMocks: func(*mocks.MockService) {}, - expectedFields: map[string]string{"sort_by": "not_a_column"}, - }, - { - description: "refuses a filter that is not valid base64", - req: func(*testing.T) *requests.DeviceList { - return &requests.DeviceList{Filters: query.Filters{Raw: "!!!not-base64!!!"}} - }, - requiredMocks: func(*mocks.MockService) {}, - expectedFields: map[string]string{"filter": "cannot be decoded"}, - }, - { - description: "refuses a filter naming a field the device list does not know", - req: func(t *testing.T) *requests.DeviceList { - t.Helper() - - raw := encode(t, []query.Filter{{ - Type: query.FilterTypeProperty, - Params: &query.FilterProperty{Name: "nonexistent_field", Operator: "eq", Value: "foo"}, - }}) + handler := NewHandler(service, nil) - return &requests.DeviceList{Filters: query.Filters{Raw: raw}} - }, - requiredMocks: func(*mocks.MockService) {}, - expectedFields: map[string]string{"filter": "is not valid"}, - }, - { - description: "hands the service the decoded filter and the caller's connector intent", - req: func(t *testing.T) *requests.DeviceList { - t.Helper() - - raw := encode(t, []query.Filter{{ - Type: query.FilterTypeProperty, - Params: &query.FilterProperty{Name: "name", Operator: "contains", Value: "foo"}, - }}) - - return &requests.DeviceList{TenantID: tenantID, Connector: true, Filters: query.Filters{Raw: raw}} - }, - requiredMocks: func(service *mocks.MockService) { - service. - On("ListDevices", gomock.Anything, scope.MustBounded(tenantID), gomock.MatchedBy(func(req *requests.DeviceList) bool { - if !req.Connector || len(req.Filters.Data) != 1 { - return false - } - - property, ok := req.Filters.Data[0].Params.(*query.FilterProperty) - - return ok && property.Name == "name" && property.Value == "foo" - })). - Return([]models.Device{}, 0, nil). - Once() - }, - }, - } - - for _, tc := range cases { - t.Run(tc.description, func(t *testing.T) { - service := mocks.NewMockService(t) - tc.requiredMocks(service) - - handler := NewHandler(service, nil) - - _, _, err := handler.GetDeviceList(t.Context(), scope.MustBounded(tenantID), gateway.Actor{ID: "user-id"}, tc.req(t)) - - if tc.expectedFields != nil { - require.Error(t, err) - - var wrapped errors.Error - require.ErrorAs(t, err, &wrapped, "a refusal must be a ShellHub error") - - data, ok := wrapped.Data.(errs.ErrDataInvalidEntity) - require.True(t, ok, "a refusal must carry the fields the caller can act on, got %v", wrapped.Data) - assert.Equal(t, tc.expectedFields, data.Fields) - - return - } - - require.NoError(t, err) - }) - } + devices, count, err := handler.GetDeviceList(t.Context(), scope.MustBounded(tenantID), gateway.Actor{ID: "user-id"}, req) + require.NoError(t, err) + assert.Equal(t, []models.Device{{UID: "uid"}}, devices) + assert.Equal(t, 7, count) } diff --git a/server/api/routes/install-key.go b/server/api/routes/install-key.go index 356bd82d518..a1ad2864ce6 100644 --- a/server/api/routes/install-key.go +++ b/server/api/routes/install-key.go @@ -1,14 +1,14 @@ package routes import ( + "context" "net/http" - "strconv" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" "github.com/shellhub-io/shellhub/pkg/api/responses" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - "github.com/shellhub-io/shellhub/server/api/services" ) // The install key routes, relative to the API's base path. @@ -45,39 +45,8 @@ func (h *Handler) CreateInstallKey(c *gateway.Context) error { } // ListInstallKeys serves the namespace's install keys, without their plaintext. -func (h *Handler) ListInstallKeys(c *gateway.Context) error { - req := new(requests.ListInstallKey) - - if err := c.Bind(req); err != nil { - return err - } - - req.Paginator.Normalize() - - if req.Sorter.By == "" { - req.Sorter.By = "created_at" - } - - if req.Sorter.Order == "" { - req.Sorter.Order = "desc" - } - - if err := query.ValidateSorter(&req.Sorter, services.InstallKeySortFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := c.Validate(req); err != nil { - return err - } - - res, count, err := h.service.ListInstallKeys(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - return c.JSON(http.StatusOK, res) +func (h *Handler) ListInstallKeys(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.ListInstallKey) ([]models.InstallKey, int, error) { + return h.service.ListInstallKeys(ctx, req) } // UpdateInstallKey changes a key's name, expiry or the device attributes it pre-assigns. @@ -140,37 +109,6 @@ func (h *Handler) EnrollmentCallback(c *gateway.Context) error { } // HistoryInstallKey serves the record of what a key has been used for. -func (h *Handler) HistoryInstallKey(c *gateway.Context) error { - req := new(requests.ListInstallKeyEvents) - - if err := c.Bind(req); err != nil { - return err - } - - req.Paginator.Normalize() - - if req.Sorter.By == "" { - req.Sorter.By = "created_at" - } - - if req.Sorter.Order == "" { - req.Sorter.Order = "desc" - } - - if err := query.ValidateSorter(&req.Sorter, services.InstallKeyEventSortFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := c.Validate(req); err != nil { - return err - } - - events, count, err := h.service.ListInstallKeyEvents(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - return c.JSON(http.StatusOK, events) +func (h *Handler) HistoryInstallKey(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.ListInstallKeyEvents) ([]models.InstallKeyEvent, int, error) { + return h.service.ListInstallKeyEvents(ctx, req) } diff --git a/server/api/routes/invitation.go b/server/api/routes/invitation.go index 17c97ec5676..1260b443709 100644 --- a/server/api/routes/invitation.go +++ b/server/api/routes/invitation.go @@ -1,14 +1,13 @@ package routes import ( + "context" "net/http" - "strconv" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - "github.com/shellhub-io/shellhub/server/api/services" - log "github.com/sirupsen/logrus" + "github.com/shellhub-io/shellhub/server/api/pkg/responses" ) // The registration and invitation routes, relative to the API's base path. @@ -110,81 +109,13 @@ func (h *Handler) AcceptInvite(c *gateway.Context) error { } // GetUserMembershipInvitationList serves the invitations awaiting the caller. -func (h *Handler) GetUserMembershipInvitationList(c *gateway.Context) error { - req := new(requests.UserMembershipInvitationList) - - if err := c.Bind(req); err != nil { - return err - } - - req.Paginator.Normalize() - req.Sorter.Normalize() - - if err := req.Filters.Unmarshal(); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode user membership invitation list filter") - - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateFilters(&req.Filters, services.MembershipInvitationFilterFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateSorter(&req.Sorter, services.MembershipInvitationSortFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := c.Validate(req); err != nil { - return err - } - - invitations, count, err := h.service.UserMembershipInvitationList(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.FormatInt(count, 10)) - - return c.JSON(http.StatusOK, invitations) +func (h *Handler) GetUserMembershipInvitationList(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.UserMembershipInvitationList) ([]responses.MembershipInvitation, int, error) { + return h.service.UserMembershipInvitationList(ctx, req) } // GetNamespaceMembershipInvitationList serves the invitations a namespace has outstanding. -func (h *Handler) GetNamespaceMembershipInvitationList(c *gateway.Context) error { - req := new(requests.NamespaceMembershipInvitationList) - - if err := c.Bind(req); err != nil { - return err - } - - req.Paginator.Normalize() - req.Sorter.Normalize() - - if err := req.Filters.Unmarshal(); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode namespace membership invitation list filter") - - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateFilters(&req.Filters, services.MembershipInvitationFilterFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateSorter(&req.Sorter, services.MembershipInvitationSortFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := c.Validate(req); err != nil { - return err - } - - invitations, count, err := h.service.NamespaceMembershipInvitationList(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.FormatInt(count, 10)) - - return c.JSON(http.StatusOK, invitations) +func (h *Handler) GetNamespaceMembershipInvitationList(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.NamespaceMembershipInvitationList) ([]responses.MembershipInvitation, int, error) { + return h.service.NamespaceMembershipInvitationList(ctx, req) } // CancelMembershipInvitation withdraws an invitation, invalidating its code. diff --git a/server/api/routes/list_query_rejection_test.go b/server/api/routes/list_query_rejection_test.go index 83fec933392..940bdf33b10 100644 --- a/server/api/routes/list_query_rejection_test.go +++ b/server/api/routes/list_query_rejection_test.go @@ -1,126 +1,274 @@ package routes import ( + "encoding/json" "net/http" "net/http/httptest" "net/url" "testing" "github.com/shellhub-io/shellhub/pkg/api/query" + "github.com/shellhub-io/shellhub/pkg/api/responses" servicemock "github.com/shellhub-io/shellhub/server/api/services/mocks" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) -func TestListEndpointsRejectAnUnapprovedFilterField(t *testing.T) { - cases := []struct { - description string - path string - filter string - uncalledFunc string - }{ +const listQueryTenant = "00000000-0000-4000-0000-000000000000" + +type listRoute struct { + description string + path string + serviceCall string + serviceArguments int + + refusedFilterField string + refusedSortField string + acceptedFilterField string + acceptedFilterOp string + acceptedSortField string +} + +func listRoutes() []listRoute { + return []listRoute{ { - description: "refuses to filter a user's invitations by the invite signature", - path: "/api/users/invitations", - filter: "sig", - uncalledFunc: "UserMembershipInvitationList", + description: "devices", + path: "/api/devices", + serviceCall: "ListDevices", + serviceArguments: 3, + refusedFilterField: "custom_fields.token", + refusedSortField: "id", + acceptedFilterField: "name", + acceptedFilterOp: "contains", + acceptedSortField: "last_seen", }, { - description: "refuses to filter a user's invitations by an unexposed column", - path: "/api/users/invitations", - filter: "invited_by", - uncalledFunc: "UserMembershipInvitationList", + description: "api keys", + path: "/api/namespaces/api-key", + serviceCall: "ListAPIKeys", + serviceArguments: 2, + refusedSortField: "key_digest", + acceptedSortField: "expires_in", }, { - description: "refuses to filter a namespace's invitations by the invite signature", - path: "/api/namespaces/00000000-0000-4000-0000-000000000000/invitations", - filter: "sig", - uncalledFunc: "NamespaceMembershipInvitationList", + description: "install keys", + path: "/api/namespaces/install-key", + serviceCall: "ListInstallKeys", + serviceArguments: 2, + refusedSortField: "webhook_secret", + acceptedSortField: "used_times", + }, + { + description: "install key history", + path: "/api/namespaces/install-key/abc/history", + serviceCall: "ListInstallKeyEvents", + serviceArguments: 2, + refusedSortField: "public_key", + acceptedSortField: "decided_at", }, - } - - for _, tc := range cases { - t.Run(tc.description, func(t *testing.T) { - svcMock := servicemock.NewMockService(t) - - values := url.Values{} - values.Set("filter", encodeFilter(t, []query.Filter{ - { - Type: query.FilterTypeProperty, - Params: &query.FilterProperty{Name: tc.filter, Operator: "contains", Value: "A"}, - }, - })) - - req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tc.path+"?"+values.Encode(), nil) - req.Header.Set("X-ID", "000000000000000000000000") - req.Header.Set("X-Tenant-ID", "00000000-0000-4000-0000-000000000000") - req.Header.Set("X-Role", "owner") - - rec := httptest.NewRecorder() - NewRouter(svcMock).ServeHTTP(rec, req) - - assert.Equal(t, http.StatusBadRequest, rec.Result().StatusCode) - svcMock.AssertNotCalled(t, tc.uncalledFunc) - }) - } -} - -func TestListEndpointsRejectAnUnapprovedSortField(t *testing.T) { - cases := []struct { - description string - path string - sortBy string - uncalledFunc string - }{ { - description: "refuses to order install keys by the webhook signing secret", - path: "/api/namespaces/install-key", - sortBy: "webhook_secret", - uncalledFunc: "ListInstallKeys", + description: "a user's invitations", + path: "/api/users/invitations", + serviceCall: "UserMembershipInvitationList", + serviceArguments: 2, + refusedFilterField: "sig", + refusedSortField: "sig", + acceptedFilterField: "status", + acceptedFilterOp: "eq", + acceptedSortField: "expires_at", }, { - description: "refuses to order install keys by the key ciphertext", - path: "/api/namespaces/install-key", - sortBy: "key_encrypted", - uncalledFunc: "ListInstallKeys", + description: "a namespace's invitations", + path: "/api/namespaces/" + listQueryTenant + "/invitations", + serviceCall: "NamespaceMembershipInvitationList", + serviceArguments: 2, + refusedFilterField: "sig", + refusedSortField: "sig", + acceptedFilterField: "role", + acceptedFilterOp: "eq", + acceptedSortField: "expires_at", }, { - description: "refuses to order install key history by an unexposed column", - path: "/api/namespaces/install-key/abc/history", - sortBy: "public_key", - uncalledFunc: "ListInstallKeyEvents", + description: "tags", + path: "/api/tags", + serviceCall: "ListTags", + serviceArguments: 2, + refusedFilterField: "name", + refusedSortField: "id", + acceptedSortField: "updated_at", }, { - description: "refuses to order API keys by the key digest", - path: "/api/namespaces/api-key", - sortBy: "key_digest", - uncalledFunc: "ListAPIKeys", + description: "tags under the deprecated namespace path", + path: "/api/namespaces/" + listQueryTenant + "/tags", + serviceCall: "ListTags", + serviceArguments: 2, + refusedFilterField: "name", + refusedSortField: "id", + acceptedSortField: "updated_at", }, { - description: "refuses to order a user's invitations by the invite signature", - path: "/api/users/invitations", - sortBy: "sig", - uncalledFunc: "UserMembershipInvitationList", + description: "sessions", + path: "/api/sessions", + serviceCall: "ListSessions", + serviceArguments: 3, + refusedFilterField: "username", + acceptedFilterField: "device_uid", + acceptedFilterOp: "eq", + }, + { + description: "public keys", + path: "/api/sshkeys/public-keys", + serviceCall: "ListPublicKeys", + serviceArguments: 2, + refusedFilterField: "data", + acceptedFilterField: "fingerprint", + acceptedFilterOp: "contains", + }, + { + description: "namespaces", + path: "/api/namespaces", + serviceCall: "ListNamespaces", + serviceArguments: 2, + refusedFilterField: "tenant_id", + acceptedFilterField: "name", + acceptedFilterOp: "contains", + }, + { + description: "namespace members", + path: "/api/namespaces/" + listQueryTenant + "/members", + serviceCall: "ListNamespaceMembers", + serviceArguments: 2, + }, + { + description: "access policies", + path: "/api/access-policies", + serviceCall: "ListAccessPolicies", + serviceArguments: 2, + }, + { + description: "ssh identities", + path: "/api/ssh-identities", + serviceCall: "ListSSHIdentities", + serviceArguments: 2, + }, + { + description: "service accounts", + path: "/api/service-accounts", + serviceCall: "ListServiceAccounts", + serviceArguments: 2, }, } +} + +func filterOn(t *testing.T, name, operator string) string { + t.Helper() + + return encodeFilter(t, []query.Filter{ + { + Type: query.FilterTypeProperty, + Params: &query.FilterProperty{Name: name, Operator: operator, Value: "A"}, + }, + }) +} + +func serveList(t *testing.T, svcMock *servicemock.MockService, path string, values url.Values) *httptest.ResponseRecorder { + t.Helper() + + target := path + if encoded := values.Encode(); encoded != "" { + target += "?" + encoded + } + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, target, nil) + req.Header.Set("X-ID", "000000000000000000000000") + req.Header.Set("X-Tenant-ID", listQueryTenant) + req.Header.Set("X-Role", "owner") + + rec := httptest.NewRecorder() + NewRouter(svcMock).ServeHTTP(rec, req) + + return rec +} + +func assertRefusedField(t *testing.T, rec *httptest.ResponseRecorder, field string) { + t.Helper() + + require.Equal(t, http.StatusBadRequest, rec.Result().StatusCode, rec.Body.String()) + + var body responses.Error + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + assert.Contains(t, body.Fields, field, "the refusal must name what the client got wrong") +} + +func TestListEndpointsRejectAnUnapprovedFilterField(t *testing.T) { + for _, tc := range listRoutes() { + if tc.refusedFilterField == "" { + continue + } + + t.Run(tc.description, func(t *testing.T) { + svcMock := servicemock.NewMockService(t) + + values := url.Values{"filter": {filterOn(t, tc.refusedFilterField, "contains")}} + + assertRefusedField(t, serveList(t, svcMock, tc.path, values), "filter") + svcMock.AssertNumberOfCalls(t, tc.serviceCall, 0) + }) + } +} + +func TestListEndpointsRejectAnUnapprovedSortField(t *testing.T) { + for _, tc := range listRoutes() { + if tc.refusedSortField == "" { + continue + } - for _, tc := range cases { t.Run(tc.description, func(t *testing.T) { svcMock := servicemock.NewMockService(t) + values := url.Values{"sort_by": {tc.refusedSortField}, "order_by": {"asc"}} + + assertRefusedField(t, serveList(t, svcMock, tc.path, values), "sort_by") + svcMock.AssertNumberOfCalls(t, tc.serviceCall, 0) + }) + } +} + +// TestListEndpointsAcceptTheirOwnContract is the direction the route table's invariant cannot check. +// That invariant proves a list route names a contract; only driving the route with a field its own +// resource allows proves it names the right one, and a route wired to a neighbour's contract fails +// here rather than in production. +// +// The four lists that take no query at all — members, access policies, SSH identities and service +// accounts — have nothing to send, so for them this only reaches the service and the header. Their +// contract is pinned by the rejection tests above being inapplicable, not by this one. +func TestListEndpointsAcceptTheirOwnContract(t *testing.T) { + for _, tc := range listRoutes() { + t.Run(tc.description, func(t *testing.T) { + svcMock := servicemock.NewMockService(t) + + arguments := make([]any, tc.serviceArguments) + for i := range arguments { + arguments[i] = mock.Anything + } + + svcMock.On(tc.serviceCall, arguments...).Return(nil, 0, nil).Once() + values := url.Values{} - values.Set("sort_by", tc.sortBy) - values.Set("order_by", "asc") + if tc.acceptedFilterField != "" { + values.Set("filter", filterOn(t, tc.acceptedFilterField, tc.acceptedFilterOp)) + } - req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tc.path+"?"+values.Encode(), nil) - req.Header.Set("X-ID", "000000000000000000000000") - req.Header.Set("X-Tenant-ID", "00000000-0000-4000-0000-000000000000") - req.Header.Set("X-Role", "owner") + if tc.acceptedSortField != "" { + values.Set("sort_by", tc.acceptedSortField) + } - rec := httptest.NewRecorder() - NewRouter(svcMock).ServeHTTP(rec, req) + rec := serveList(t, svcMock, tc.path, values) - assert.Equal(t, http.StatusBadRequest, rec.Result().StatusCode) - svcMock.AssertNotCalled(t, tc.uncalledFunc) + require.Equal(t, http.StatusOK, rec.Result().StatusCode, rec.Body.String()) + assert.Equal(t, "0", rec.Header().Get("X-Total-Count")) + svcMock.AssertExpectations(t) }) } } diff --git a/server/api/routes/list_validation_test.go b/server/api/routes/list_validation_test.go deleted file mode 100644 index be815a7c691..00000000000 --- a/server/api/routes/list_validation_test.go +++ /dev/null @@ -1,171 +0,0 @@ -package routes - -import ( - "go/ast" - "go/parser" - "go/token" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/stretchr/testify/require" -) - -const requestsPkgDir = "../../../pkg/api/requests" - -type queryEmbeds struct { - filters bool - sorter bool -} - -func parsePackage(t *testing.T, dir string) []*ast.File { - t.Helper() - - entries, err := os.ReadDir(dir) - require.NoError(t, err) - - fset := token.NewFileSet() - files := make([]*ast.File, 0, len(entries)) - - for _, entry := range entries { - name := entry.Name() - if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { - continue - } - - file, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, 0) - require.NoError(t, err) - - files = append(files, file) - } - - return files -} - -func selectorPath(expr ast.Expr) string { - sel, ok := expr.(*ast.SelectorExpr) - if !ok { - return "" - } - - pkg, ok := sel.X.(*ast.Ident) - if !ok { - return "" - } - - return pkg.Name + "." + sel.Sel.Name -} - -func embeddedQueryTypes(t *testing.T) map[string]queryEmbeds { - t.Helper() - - embeds := make(map[string]queryEmbeds) - - for _, file := range parsePackage(t, requestsPkgDir) { - ast.Inspect(file, func(n ast.Node) bool { - spec, ok := n.(*ast.TypeSpec) - if !ok { - return true - } - - structType, ok := spec.Type.(*ast.StructType) - if !ok { - return true - } - - var found queryEmbeds - for _, field := range structType.Fields.List { - if len(field.Names) > 0 { - continue - } - - switch selectorPath(field.Type) { - case "query.Filters": - found.filters = true - case "query.Sorter": - found.sorter = true - } - } - - if found.filters || found.sorter { - embeds[spec.Name.Name] = found - } - - return true - }) - } - - return embeds -} - -func isHandler(fn *ast.FuncDecl) bool { - if fn.Recv == nil || fn.Body == nil { - return false - } - - for _, param := range fn.Type.Params.List { - if star, ok := param.Type.(*ast.StarExpr); ok && selectorPath(star.X) == "gateway.Context" { - return true - } - } - - return false -} - -func handlerQueryUsage(fn *ast.FuncDecl, embeds map[string]queryEmbeds) (queryEmbeds, map[string]bool) { - var needs queryEmbeds - calls := make(map[string]bool) - - ast.Inspect(fn.Body, func(n ast.Node) bool { - switch node := n.(type) { - case *ast.SelectorExpr: - if pkg, ok := node.X.(*ast.Ident); ok && pkg.Name == "requests" { - if e, ok := embeds[node.Sel.Name]; ok { - needs.filters = needs.filters || e.filters - needs.sorter = needs.sorter || e.sorter - } - } - case *ast.CallExpr: - if path := selectorPath(node.Fun); path != "" { - calls[path] = true - } - } - - return true - }) - - return needs, calls -} - -func TestListHandlersValidateEveryQueryFieldTheyAccept(t *testing.T) { - embeds := embeddedQueryTypes(t) - require.NotEmpty(t, embeds) - - for _, file := range parsePackage(t, ".") { - for _, decl := range file.Decls { - fn, ok := decl.(*ast.FuncDecl) - if !ok || !isHandler(fn) { - continue - } - - needs, calls := handlerQueryUsage(fn, embeds) - - if needs.filters && !calls["query.ValidateFilters"] { - t.Errorf( - "%s binds a request carrying a client filter but never calls query.ValidateFilters: "+ - "the filter then names any column on the table, including ones the response omits", - fn.Name.Name, - ) - } - - if needs.sorter && !calls["query.ValidateSorter"] { - t.Errorf( - "%s binds a request carrying a client sort_by but never calls query.ValidateSorter: "+ - "the sort then orders by any column on the table, including ones the response omits", - fn.Name.Name, - ) - } - } - } -} diff --git a/server/api/routes/nsadm.go b/server/api/routes/nsadm.go index d437db7c979..af55404c354 100644 --- a/server/api/routes/nsadm.go +++ b/server/api/routes/nsadm.go @@ -1,14 +1,13 @@ package routes import ( + "context" "net/http" - "strconv" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - "github.com/shellhub-io/shellhub/server/api/services" - log "github.com/sirupsen/logrus" ) // The namespace and membership routes, relative to the API's base path. @@ -34,37 +33,8 @@ const ( ) // GetNamespaceList serves the namespaces the caller belongs to. -func (h *Handler) GetNamespaceList(c *gateway.Context) error { - req := new(requests.NamespaceList) - - if err := c.Bind(req); err != nil { - return err - } - - req.Normalize() - - if err := req.Unmarshal(); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode namespace list filter") - - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateFilters(&req.Filters, services.NamespaceFilterFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := c.Validate(req); err != nil { - return err - } - - namespaces, count, err := h.service.ListNamespaces(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - return c.JSON(http.StatusOK, namespaces) +func (h *Handler) GetNamespaceList(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.NamespaceList) ([]models.Namespace, int, error) { + return h.service.ListNamespaces(ctx, req) } // CreateNamespace creates a namespace owned by the caller. @@ -118,27 +88,8 @@ func (h *Handler) GetNamespace(c *gateway.Context) error { } // ListNamespaceMembers serves who belongs to a namespace and in what role. -func (h *Handler) ListNamespaceMembers(c *gateway.Context) error { - req := new(requests.MemberList) - - if err := c.Bind(req); err != nil { - return err - } - - req.Paginator.Normalize() - - if err := c.Validate(req); err != nil { - return err - } - - members, count, err := h.service.ListNamespaceMembers(c.Ctx(), req) - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - if err != nil { - return err - } - - return c.JSON(http.StatusOK, members) +func (h *Handler) ListNamespaceMembers(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.MemberList) ([]models.MemberView, int, error) { + return h.service.ListNamespaceMembers(ctx, req) } // DeleteNamespace removes a namespace and everything scoped to it. diff --git a/server/api/routes/route_table_test.go b/server/api/routes/route_table_test.go index 1c5f0361722..5585b2754ba 100644 --- a/server/api/routes/route_table_test.go +++ b/server/api/routes/route_table_test.go @@ -178,6 +178,20 @@ func staleExemptions(registered map[string]struct{}, exempt map[string]string) [ return stale } +func unqueriedListRoutes(declarations []gateway.Declaration) []string { + unqueried := make([]string, 0) + + for _, declaration := range declarations { + if declaration.Shape == gateway.ShapeList && !declaration.AcceptsQuery { + unqueried = append(unqueried, declaration.Address()+" serves a page and names no query contract") + } + } + + sort.Strings(unqueried) + + return unqueried +} + // TestRouteTableHoldsItsClaims reads the whole route table of a fully built router against the // claims its registrations made. Every check is one predicate over that table, and each has a // companion below feeding it a known-bad input, so a passing run means the predicate looked. @@ -209,6 +223,10 @@ func TestRouteTableHoldsItsClaims(t *testing.T) { assert.Empty(t, anonymityMismatches(declarations, authn.AnonymousRoutes())) }) + t.Run("every list route names a query contract", func(t *testing.T) { + assert.Empty(t, unqueriedListRoutes(declarations)) + }) + t.Run("every exemption names a mounted route and states why", func(t *testing.T) { assert.Empty(t, staleExemptions(registered, gatewayExemptRoutes)) }) @@ -335,3 +353,20 @@ func TestStaleExemptionsCatchesAnExemptionNothingMounts(t *testing.T) { "GET /silent is exempt and states no reason", }, stale) } + +// TestUnqueriedListRoutesCatchesAListThatNamesNoContract replaces the go/ast test that used to +// enforce this rule by reading the requests and routes packages as source text. That test matched a +// handler by its gateway-context parameter, so a route dropped out of its coverage the moment its +// handler changed shape — and it kept passing. A route is in the table whatever shape its handler +// has, so a list route cannot leave this check by changing its signature. A route that still writes +// its own response is a ShapeLegacy one, and is covered when it converts. +func TestUnqueriedListRoutesCatchesAListThatNamesNoContract(t *testing.T) { + unqueried := unqueriedListRoutes([]gateway.Declaration{ + {Method: "GET", Path: "/named", Shape: gateway.ShapeList, AcceptsQuery: true}, + {Method: "GET", Path: "/silent", Shape: gateway.ShapeList}, + {Method: "GET", Path: "/single", Shape: gateway.ShapeOne}, + {Method: "GET", Path: "/legacy", Shape: gateway.ShapeLegacy}, + }) + + assert.Equal(t, []string{"GET /silent serves a page and names no query contract"}, unqueried) +} diff --git a/server/api/routes/routes.go b/server/api/routes/routes.go index 46d9c6c96bb..964b2053259 100644 --- a/server/api/routes/routes.go +++ b/server/api/routes/routes.go @@ -136,15 +136,15 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { gateway.POST(publicAPI, AuthPublicKeyURL, gateway.Handler(handler.AuthPublicKey)) gateway.POST(publicAPI, CreateAPIKeyURL, gateway.Handler(handler.CreateAPIKey), gateway.NoAPIKey(), gateway.Requires(authorizer.APIKeyCreate)) - gateway.GET(publicAPI, ListAPIKeysURL, gateway.Handler(handler.ListAPIKeys), gateway.NoAPIKey()) + gateway.GET(publicAPI, ListAPIKeysURL, gateway.List(handler.ListAPIKeys), gateway.Accepts(services.APIKeyQuery), gateway.NoAPIKey()) gateway.PATCH(publicAPI, UpdateAPIKeyURL, gateway.Handler(handler.UpdateAPIKey), gateway.NoAPIKey(), gateway.Requires(authorizer.APIKeyUpdate)) gateway.DELETE(publicAPI, DeleteAPIKeyURL, gateway.Handler(handler.DeleteAPIKey), gateway.NoAPIKey(), gateway.Requires(authorizer.APIKeyDelete)) gateway.POST(publicAPI, CreateInstallKeyURL, gateway.Handler(handler.CreateInstallKey), gateway.NoAPIKey(), gateway.Requires(authorizer.InstallKeyCreate)) - gateway.GET(publicAPI, ListInstallKeysURL, gateway.Handler(handler.ListInstallKeys), gateway.NoAPIKey(), gateway.Requires(authorizer.InstallKeyList)) + gateway.GET(publicAPI, ListInstallKeysURL, gateway.List(handler.ListInstallKeys), gateway.Accepts(services.InstallKeyQuery), gateway.NoAPIKey(), gateway.Requires(authorizer.InstallKeyList)) gateway.PATCH(publicAPI, UpdateInstallKeyURL, gateway.Handler(handler.UpdateInstallKey), gateway.NoAPIKey(), gateway.Requires(authorizer.InstallKeyUpdate)) gateway.GET(publicAPI, RevealInstallKeyURL, gateway.Handler(handler.RevealInstallKey), gateway.NoAPIKey(), gateway.Requires(authorizer.InstallKeyReveal)) - gateway.GET(publicAPI, HistoryInstallKeyURL, gateway.Handler(handler.HistoryInstallKey), gateway.NoAPIKey(), gateway.Requires(authorizer.InstallKeyList)) + gateway.GET(publicAPI, HistoryInstallKeyURL, gateway.List(handler.HistoryInstallKey), gateway.Accepts(services.InstallKeyEventQuery), gateway.NoAPIKey(), gateway.Requires(authorizer.InstallKeyList)) gateway.PATCH(publicAPI, URLUpdateUser, gateway.Handler(handler.UpdateUser), gateway.NoAPIKey()) gateway.PATCH(publicAPI, URLDeprecatedUpdateUser, gateway.Handler(handler.UpdateUser), gateway.NoAPIKey()) // WARN: DEPRECATED. @@ -156,11 +156,12 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { gateway.Anonymous("an invitee follows the link before holding an account, and the signed invitation is the credential")) gateway.POST(publicAPI, URLGenerateInvitationLink, gateway.Handler(handler.GenerateInvitationLink), gateway.NoAPIKey(), gateway.Requires(authorizer.NamespaceAddMember)) gateway.PATCH(publicAPI, URLAcceptInvite, gateway.Handler(handler.AcceptInvite), gateway.NoAPIKey()) - gateway.GET(publicAPI, URLUserMembershipInvitationList, gateway.Handler(handler.GetUserMembershipInvitationList)) - gateway.GET(publicAPI, URLNamespaceMembershipInvitationList, gateway.Handler(handler.GetNamespaceMembershipInvitationList), gateway.Requires(authorizer.NamespaceEditMember)) + gateway.GET(publicAPI, URLUserMembershipInvitationList, gateway.List(handler.GetUserMembershipInvitationList), gateway.Accepts(services.MembershipInvitationQuery), + gateway.Unbounded("an invitation is addressed to a person across namespaces, and the invitee may belong to none yet")) + gateway.GET(publicAPI, URLNamespaceMembershipInvitationList, gateway.List(handler.GetNamespaceMembershipInvitationList), gateway.Accepts(services.MembershipInvitationQuery), gateway.Requires(authorizer.NamespaceEditMember)) gateway.DELETE(publicAPI, URLCancelMembershipInvitation, gateway.Handler(handler.CancelMembershipInvitation), gateway.Requires(authorizer.NamespaceRemoveMember)) - gateway.GET(publicAPI, GetDeviceListURL, gateway.List(handler.GetDeviceList), gateway.Guard(routesmiddleware.Authorize)) + gateway.GET(publicAPI, GetDeviceListURL, gateway.List(handler.GetDeviceList), gateway.Accepts(services.DeviceQuery), gateway.Guard(routesmiddleware.Authorize)) gateway.GET(publicAPI, GetDeviceURL, gateway.One(handler.GetDevice), gateway.Guard(routesmiddleware.Authorize)) gateway.GET(publicAPI, ResolveDeviceURL, gateway.Handler(handler.ResolveDevice), gateway.Guard(routesmiddleware.Authorize)) gateway.PUT(publicAPI, UpdateDevice, gateway.Handler(handler.UpdateDevice), gateway.Requires(authorizer.DeviceUpdate)) @@ -185,21 +186,21 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { gateway.PUT(publicAPI, SetDeviceCustomFieldURL, gateway.Handler(handler.SetDeviceCustomField), gateway.Requires(authorizer.DeviceCustomFieldUpdate)) gateway.DELETE(publicAPI, DeleteDeviceCustomFieldURL, gateway.Handler(handler.DeleteDeviceCustomField), gateway.Requires(authorizer.DeviceCustomFieldUpdate)) - gateway.GET(publicAPI, URLGetTags, gateway.Handler(handler.GetTags)) + gateway.GET(publicAPI, URLGetTags, gateway.List(handler.GetTags), gateway.Accepts(services.TagQuery)) gateway.POST(publicAPI, URLCreateTag, gateway.Handler(handler.CreateTag), gateway.Requires(authorizer.TagCreate)) gateway.PATCH(publicAPI, URLUpdateTag, gateway.Handler(handler.UpdateTag), gateway.Requires(authorizer.TagUpdate)) gateway.DELETE(publicAPI, URLDeleteTag, gateway.Handler(handler.DeleteTag), gateway.Requires(authorizer.TagDelete)) gateway.POST(publicAPI, URLPushTagToDevice, gateway.Handler(handler.PushTagToDevice), gateway.Requires(authorizer.TagCreate)) gateway.DELETE(publicAPI, URLPullTagFromDevice, gateway.Handler(handler.PullTagFromDevice), gateway.Requires(authorizer.TagDelete)) - gateway.GET(publicAPI, URLOldGetTags, gateway.Handler(handler.GetTags)) + gateway.GET(publicAPI, URLOldGetTags, gateway.List(handler.GetTags), gateway.Accepts(services.TagQuery)) gateway.POST(publicAPI, URLOldCreateTag, gateway.Handler(handler.CreateTag), gateway.Requires(authorizer.TagCreate)) gateway.PATCH(publicAPI, URLOldUpdateTag, gateway.Handler(handler.UpdateTag), gateway.Requires(authorizer.TagUpdate)) gateway.DELETE(publicAPI, URLOldDeleteTag, gateway.Handler(handler.DeleteTag), gateway.Requires(authorizer.TagDelete)) gateway.POST(publicAPI, URLOldPushTagToDevice, gateway.Handler(handler.PushTagToDevice), gateway.Requires(authorizer.TagCreate)) gateway.DELETE(publicAPI, URLOldPullTagFromDevice, gateway.Handler(handler.PullTagFromDevice), gateway.Requires(authorizer.TagDelete)) - gateway.GET(publicAPI, GetSessionsURL, gateway.Handler(handler.GetSessionList), gateway.Guard(routesmiddleware.Authorize)) + gateway.GET(publicAPI, GetSessionsURL, gateway.List(handler.GetSessionList), gateway.Accepts(services.SessionQuery), gateway.Guard(routesmiddleware.Authorize)) gateway.GET(publicAPI, GetSessionURL, gateway.Handler(handler.GetSession), gateway.Guard(routesmiddleware.Authorize)) gateway.GET(publicAPI, GetStatsURL, gateway.Handler(handler.GetStats), gateway.Guard(routesmiddleware.Authorize)) @@ -209,7 +210,7 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { gateway.Anonymous("the install script is fetched by a shell on a machine that holds no credential")) gateway.POST(publicAPI, CreatePublicKeyURL, gateway.Handler(handler.CreatePublicKey), gateway.Requires(authorizer.PublicKeyCreate)) - gateway.GET(publicAPI, GetPublicKeysURL, gateway.Handler(handler.GetPublicKeys)) + gateway.GET(publicAPI, GetPublicKeysURL, gateway.List(handler.GetPublicKeys), gateway.Accepts(services.PublicKeyQuery)) gateway.PUT(publicAPI, UpdatePublicKeyURL, gateway.Handler(handler.UpdatePublicKey), gateway.Requires(authorizer.PublicKeyEdit)) gateway.DELETE(publicAPI, DeletePublicKeyURL, gateway.Handler(handler.DeletePublicKey), gateway.Requires(authorizer.PublicKeyRemove)) @@ -217,11 +218,12 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { gateway.POST(publicAPI, CreateNamespaceURL, gateway.Handler(handler.CreateNamespace), gateway.NoAPIKey()) } gateway.GET(publicAPI, GetNamespaceURL, gateway.Handler(handler.GetNamespace), gateway.Guard(routesmiddleware.RequiresTenant(ParamNamespaceTenant))) - gateway.GET(publicAPI, ListNamespaceURL, gateway.Handler(handler.GetNamespaceList), gateway.NoAPIKey()) + gateway.GET(publicAPI, ListNamespaceURL, gateway.List(handler.GetNamespaceList), gateway.Accepts(services.NamespaceQuery), gateway.NoAPIKey(), + gateway.Unbounded("the list answers which namespaces the caller belongs to, and the caller may have selected none")) gateway.PUT(publicAPI, EditNamespaceURL, gateway.Handler(handler.EditNamespace), gateway.Guard(routesmiddleware.RequiresTenant(ParamNamespaceTenant)), gateway.Requires(authorizer.NamespaceUpdate)) gateway.DELETE(publicAPI, DeleteNamespaceURL, gateway.Handler(handler.DeleteNamespace), gateway.Guard(routesmiddleware.RequiresTenant(ParamNamespaceTenant)), gateway.Requires(authorizer.NamespaceDelete)) - gateway.GET(publicAPI, ListNamespaceMembersURL, gateway.Handler(handler.ListNamespaceMembers), gateway.Guard(routesmiddleware.RequiresTenant(ParamNamespaceTenant))) + gateway.GET(publicAPI, ListNamespaceMembersURL, gateway.List(handler.ListNamespaceMembers), gateway.Accepts(services.MemberQuery), gateway.Guard(routesmiddleware.RequiresTenant(ParamNamespaceTenant))) gateway.POST(publicAPI, AddNamespaceMemberURL, gateway.Handler(handler.AddNamespaceMember), gateway.Requires(authorizer.NamespaceAddMember)) gateway.PATCH(publicAPI, EditNamespaceMemberURL, gateway.Handler(handler.EditNamespaceMember), gateway.Requires(authorizer.NamespaceEditMember)) gateway.DELETE(publicAPI, RemoveNamespaceMemberURL, gateway.Handler(handler.RemoveNamespaceMember), gateway.Requires(authorizer.NamespaceRemoveMember)) @@ -230,20 +232,20 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { gateway.PUT(publicAPI, EditSessionRecordStatusURL, gateway.Handler(handler.EditSessionRecordStatus), gateway.Guard(routesmiddleware.RequiresTenant(ParamNamespaceTenant)), gateway.Requires(authorizer.NamespaceEnableSessionRecord)) gateway.PUT(publicAPI, EditSSHAccessModeURL, gateway.Handler(handler.EditSSHAccessMode), gateway.Guard(routesmiddleware.RequiresTenant(ParamNamespaceTenant)), gateway.Requires(authorizer.NamespaceUpdate)) - gateway.GET(publicAPI, ListAccessPoliciesURL, gateway.Handler(handler.ListAccessPolicies), gateway.Requires(authorizer.AccessPolicyManage)) + gateway.GET(publicAPI, ListAccessPoliciesURL, gateway.List(handler.ListAccessPolicies), gateway.Accepts(services.AccessPolicyQuery), gateway.Requires(authorizer.AccessPolicyManage)) gateway.POST(publicAPI, CreateAccessPolicyURL, gateway.Handler(handler.CreateAccessPolicy), gateway.Requires(authorizer.AccessPolicyManage)) gateway.GET(publicAPI, GetAccessPolicyURL, gateway.Handler(handler.GetAccessPolicy), gateway.Requires(authorizer.AccessPolicyManage)) gateway.PUT(publicAPI, UpdateAccessPolicyURL, gateway.Handler(handler.UpdateAccessPolicy), gateway.Requires(authorizer.AccessPolicyManage)) gateway.DELETE(publicAPI, DeleteAccessPolicyURL, gateway.Handler(handler.DeleteAccessPolicy), gateway.Requires(authorizer.AccessPolicyManage)) - gateway.GET(publicAPI, ListSSHIdentitiesURL, gateway.Handler(handler.ListSSHIdentities)) + gateway.GET(publicAPI, ListSSHIdentitiesURL, gateway.List(handler.ListSSHIdentities), gateway.Accepts(services.SSHIdentityQuery)) gateway.POST(publicAPI, CreateSSHIdentityURL, gateway.Handler(handler.CreateSSHIdentity), gateway.Requires(authorizer.SSHIdentityAdd)) gateway.PATCH(publicAPI, UpdateSSHIdentityURL, gateway.Handler(handler.UpdateSSHIdentity), gateway.Requires(authorizer.SSHIdentityAdd)) gateway.DELETE(publicAPI, DeleteSSHIdentityURL, gateway.Handler(handler.DeleteSSHIdentity)) gateway.POST(publicAPI, WebReauthURL, gateway.Handler(handler.WebReauthVerify)) - gateway.GET(publicAPI, ListServiceAccountsURL, gateway.Handler(handler.ListServiceAccounts), gateway.Requires(authorizer.NamespaceAddMember)) + gateway.GET(publicAPI, ListServiceAccountsURL, gateway.List(handler.ListServiceAccounts), gateway.Accepts(services.ServiceAccountQuery), gateway.Requires(authorizer.NamespaceAddMember)) gateway.POST(publicAPI, CreateServiceAccountURL, gateway.Handler(handler.CreateServiceAccount), gateway.Requires(authorizer.NamespaceAddMember)) gateway.DELETE(publicAPI, DeleteServiceAccountURL, gateway.Handler(handler.DeleteServiceAccount), gateway.Requires(authorizer.NamespaceAddMember)) diff --git a/server/api/routes/service-account.go b/server/api/routes/service-account.go index 12d1fb074bf..624bb46b215 100644 --- a/server/api/routes/service-account.go +++ b/server/api/routes/service-account.go @@ -1,10 +1,12 @@ package routes import ( + "context" "net/http" - "strconv" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" ) @@ -16,24 +18,10 @@ const ( ) // ListServiceAccounts returns the namespace's service accounts with their identities. -func (h *Handler) ListServiceAccounts(c *gateway.Context) error { - req := new(requests.ServiceAccountList) - if err := c.Bind(req); err != nil { - return err - } - - if c.Tenant() != nil { - req.TenantID = c.Tenant().ID - } - - list, err := h.service.ListServiceAccounts(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(len(list))) +func (h *Handler) ListServiceAccounts(ctx context.Context, sc scope.Scope, _ gateway.Actor, req *requests.ServiceAccountList) ([]models.ServiceAccount, int, error) { + req.TenantID = sc.TenantID() - return c.JSON(http.StatusOK, list) + return h.service.ListServiceAccounts(ctx, req) } // CreateServiceAccount creates a service account from a display name and an OpenSSH diff --git a/server/api/routes/session.go b/server/api/routes/session.go index 164d7bc4210..59873f82010 100644 --- a/server/api/routes/session.go +++ b/server/api/routes/session.go @@ -1,15 +1,13 @@ package routes import ( + "context" "net/http" - "strconv" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - "github.com/shellhub-io/shellhub/server/api/services" - log "github.com/sirupsen/logrus" ) // The session routes, relative to the API's base path. @@ -24,42 +22,8 @@ const ( ) // GetSessionList serves the namespace's sessions, filtered and paginated as requested. -func (h *Handler) GetSessionList(c *gateway.Context) error { - req := new(requests.ListSessions) - - if err := c.Bind(req); err != nil { - return err - } - - if err := c.Validate(req); err != nil { - return err - } - - req.Paginator.Normalize() - - if err := req.Filters.Unmarshal(); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode session list filter") - - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateFilters(&req.Filters, services.SessionFilterFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - sc, err := c.AdminOrScope() - if err != nil { - return err - } - - sessions, count, err := h.service.ListSessions(c.Ctx(), sc, req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - return c.JSON(http.StatusOK, sessions) +func (h *Handler) GetSessionList(ctx context.Context, sc scope.Scope, _ gateway.Actor, req *requests.ListSessions) ([]models.Session, int, error) { + return h.service.ListSessions(ctx, sc, req) } // GetSession serves one session by UID. diff --git a/server/api/routes/session_test.go b/server/api/routes/session_test.go index c11d2110d11..86e09e63b6a 100644 --- a/server/api/routes/session_test.go +++ b/server/api/routes/session_test.go @@ -42,7 +42,7 @@ func TestGetSessionList(t *testing.T) { { description: "fails when try to searching a session list of a existing session", paginator: query.Paginator{Page: 1, PerPage: 10}, - headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, + headers: map[string]string{"X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, requiredMocks: func() { mock. On("ListSessions", gomock.Anything, gomock.Anything, &requests.ListSessions{Paginator: query.Paginator{Page: 1, PerPage: 10}, TenantID: "00000000-0000-4000-0000-000000000000"}). @@ -57,7 +57,7 @@ func TestGetSessionList(t *testing.T) { { description: "success when try to searching a session list of a existing session", paginator: query.Paginator{Page: 2, PerPage: 5}, - headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, + headers: map[string]string{"X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, requiredMocks: func() { mock. On("ListSessions", gomock.Anything, gomock.Anything, &requests.ListSessions{Paginator: query.Paginator{Page: 2, PerPage: 5}, TenantID: "00000000-0000-4000-0000-000000000000"}). @@ -89,7 +89,7 @@ func TestGetSessionList(t *testing.T) { return base64.StdEncoding.EncodeToString(b) }(), - headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, + headers: map[string]string{"X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, requiredMocks: func() {}, expected: Expected{ expectedSession: nil, @@ -100,7 +100,7 @@ func TestGetSessionList(t *testing.T) { description: "returns 400 when filter is malformed non-base64", paginator: query.Paginator{Page: 1, PerPage: 10}, filter: "!!!not-base64!!!", - headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, + headers: map[string]string{"X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, requiredMocks: func() {}, expected: Expected{ expectedSession: nil, @@ -126,7 +126,7 @@ func TestGetSessionList(t *testing.T) { return base64.StdEncoding.EncodeToString(b) }(), - headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, + headers: map[string]string{"X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, requiredMocks: func() { mock. On("ListSessions", gomock.Anything, gomock.Anything, gomock.MatchedBy(func(req *requests.ListSessions) bool { diff --git a/server/api/routes/ssh-identity.go b/server/api/routes/ssh-identity.go index d57d8104a36..8605d4b21ff 100644 --- a/server/api/routes/ssh-identity.go +++ b/server/api/routes/ssh-identity.go @@ -1,12 +1,15 @@ package routes import ( + "context" "net/http" - "strconv" "github.com/shellhub-io/shellhub/pkg/api/authorizer" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" + errs "github.com/shellhub-io/shellhub/server/api/routes/errors" ) // The SSH identity routes, relative to the API's base path. @@ -20,34 +23,19 @@ const ( // ListSSHIdentities returns the caller's enrolled SSH identities in the current // namespace. With ?all=true (and the manage permission) it returns every // member's, for offboarding. -func (h *Handler) ListSSHIdentities(c *gateway.Context) error { - req := new(requests.SSHIdentityList) - if err := c.Bind(req); err != nil { - return err - } - - userID, ok := c.GetID() - if !ok { - return c.NoContent(http.StatusUnauthorized) +func (h *Handler) ListSSHIdentities(ctx context.Context, sc scope.Scope, actor gateway.Actor, req *requests.SSHIdentityList) ([]models.SSHIdentity, int, error) { + if actor.ID == "" { + return nil, 0, errs.NewErrUnauthorized(nil) } - req.UserID = userID - if c.Tenant() != nil { - req.TenantID = c.Tenant().ID - } + req.UserID = actor.ID + req.TenantID = sc.TenantID() - if req.All && !c.Role().HasPermission(authorizer.SSHIdentityManage) { + if req.All && !gateway.RoleFromContext(ctx).HasPermission(authorizer.SSHIdentityManage) { req.All = false } - list, err := h.service.ListSSHIdentities(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(len(list))) - - return c.JSON(http.StatusOK, list) + return h.service.ListSSHIdentities(ctx, req) } // CreateSSHIdentity manually enrolls a pasted OpenSSH public key for the caller. diff --git a/server/api/routes/sshkeys.go b/server/api/routes/sshkeys.go index 714e1117d64..60587169887 100644 --- a/server/api/routes/sshkeys.go +++ b/server/api/routes/sshkeys.go @@ -1,14 +1,13 @@ package routes import ( + "context" "net/http" - "strconv" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - "github.com/shellhub-io/shellhub/server/api/services" - log "github.com/sirupsen/logrus" ) // The public key routes, relative to the API's base path. @@ -25,37 +24,8 @@ const ( ) // GetPublicKeys serves the namespace's public keys. -func (h *Handler) GetPublicKeys(c *gateway.Context) error { - req := new(requests.ListPublicKeys) - - if err := c.Bind(req); err != nil { - return err - } - - if err := c.Validate(req); err != nil { - return err - } - - if err := req.Filters.Unmarshal(); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode public keys list filter") - - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateFilters(&req.Filters, services.PublicKeyFilterFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - req.Paginator.Normalize() - - list, count, err := h.service.ListPublicKeys(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - return c.JSON(http.StatusOK, list) +func (h *Handler) GetPublicKeys(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.ListPublicKeys) ([]models.PublicKey, int, error) { + return h.service.ListPublicKeys(ctx, req) } // CreatePublicKey adds a public key, with the device and username rules restricting it. diff --git a/server/api/routes/sshkeys_test.go b/server/api/routes/sshkeys_test.go index 6e0c4029fdd..2b912f3edd3 100644 --- a/server/api/routes/sshkeys_test.go +++ b/server/api/routes/sshkeys_test.go @@ -37,7 +37,7 @@ func TestGetPublicKeys(t *testing.T) { { description: "success when try to list a publics keys exists", paginator: query.Paginator{Page: 1, PerPage: 10}, - headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, + headers: map[string]string{"X-ID": "000000000000000000000000", "X-Tenant-ID": "00000000-0000-4000-0000-000000000000"}, requiredMocks: func() { mock. On("ListPublicKeys", gomock.Anything, &requests.ListPublicKeys{Paginator: query.Paginator{Page: 1, PerPage: 10}, TenantID: "00000000-0000-4000-0000-000000000000"}). diff --git a/server/api/routes/tags.go b/server/api/routes/tags.go index 7115e7969eb..3b7ffb8b5d8 100644 --- a/server/api/routes/tags.go +++ b/server/api/routes/tags.go @@ -1,15 +1,14 @@ package routes import ( + "context" "net/http" - "strconv" - "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" - "github.com/shellhub-io/shellhub/server/api/services" "github.com/shellhub-io/shellhub/server/api/store" - log "github.com/sirupsen/logrus" ) // The tag routes, relative to the API's base path. The URLOld* spellings are kept because @@ -53,42 +52,8 @@ func (h *Handler) CreateTag(c *gateway.Context) error { } // GetTags serves the namespace's tags. -func (h *Handler) GetTags(c *gateway.Context) error { - req := new(requests.ListTags) - - if err := c.Bind(req); err != nil { - return err - } - - if err := c.Validate(req); err != nil { - return err - } - - if err := req.Unmarshal(); err != nil { - log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode tags list filter") - - return c.NoContent(http.StatusBadRequest) - } - - if err := query.ValidateFilters(&req.Filters, services.TagFilterFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - req.Paginator.Normalize() - req.Sorter.Normalize() - - if err := query.ValidateSorter(&req.Sorter, services.TagSortFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - tags, totalCount, err := h.service.ListTags(c.Ctx(), req) - if err != nil { - return err - } - - c.Response().Header().Set("X-Total-Count", strconv.Itoa(totalCount)) - - return c.JSON(http.StatusOK, tags) +func (h *Handler) GetTags(ctx context.Context, _ scope.Scope, _ gateway.Actor, req *requests.ListTags) ([]models.Tag, int, error) { + return h.service.ListTags(ctx, req) } // UpdateTag renames a tag, which renames it everywhere it is attached. diff --git a/server/api/services/access-policy.go b/server/api/services/access-policy.go index 96aaa6e0361..28ae771c1bc 100644 --- a/server/api/services/access-policy.go +++ b/server/api/services/access-policy.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/shellhub-io/shellhub/pkg/api/authorizer" + "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/pkg/models" @@ -14,6 +15,10 @@ import ( log "github.com/sirupsen/logrus" ) +// AccessPolicyQuery is the query contract the access policy list accepts, which is nothing: the +// list serves every policy in the namespace and offers neither a filter nor a sort. +var AccessPolicyQuery = query.Contract{} + // AccessPolicyService answers whether a namespace's Access Policies permit a connection. type AccessPolicyService interface { // Authorize decides whether the user may reach the device as the given login, @@ -24,8 +29,9 @@ type AccessPolicyService interface { // ephemeral-key mint point. Authorize(ctx context.Context, tenantID, userID, deviceUID, login, sourceIP string) (*models.Decision, error) - // ListAccessPolicies returns every access policy in the namespace. - ListAccessPolicies(ctx context.Context, tenantID string) ([]models.AccessPolicy, error) + // ListAccessPolicies returns every access policy in the namespace, and the size of the whole + // collection as the store counted it. + ListAccessPolicies(ctx context.Context, tenantID string) ([]models.AccessPolicy, int, error) // NamespaceHasAccessPolicies reports whether the namespace has any access // policy. The gateway uses it to refuse an identity-mode login before minting @@ -266,22 +272,22 @@ func stricterReauthPeriod(a, b *int) *int { return a } -func (s *service) ListAccessPolicies(ctx context.Context, tenantID string) ([]models.AccessPolicy, error) { +func (s *service) ListAccessPolicies(ctx context.Context, tenantID string) ([]models.AccessPolicy, int, error) { sc, err := BoundTo(tenantID) if err != nil { - return nil, err + return nil, 0, err } if _, err := s.store.NamespaceResolve(ctx, store.NamespaceTenantIDResolver, tenantID); err != nil { - return nil, NewErrNamespaceNotFound(tenantID, err) + return nil, 0, NewErrNamespaceNotFound(tenantID, err) } - policies, _, err := s.store.AccessPolicyList(ctx, sc) + policies, count, err := s.store.AccessPolicyList(ctx, sc) if err != nil { - return nil, err + return nil, 0, err } - return policies, nil + return policies, count, nil } func (s *service) GetAccessPolicy(ctx context.Context, req *requests.AccessPolicyGet) (*models.AccessPolicy, error) { diff --git a/server/api/services/access-policy_test.go b/server/api/services/access-policy_test.go index d49a87868d4..f6e92954a8c 100644 --- a/server/api/services/access-policy_test.go +++ b/server/api/services/access-policy_test.go @@ -1000,3 +1000,27 @@ func TestStricterReauthPeriod(t *testing.T) { }) } } + +// TestListAccessPoliciesCarriesTheStoreCount pins the count the header is written from as the +// store's, not the page's. The mock returns a count that disagrees with the slice length, which is +// the only way to tell one from the other while the list is unpaginated. +func TestListAccessPoliciesCarriesTheStoreCount(t *testing.T) { + ctx := context.TODO() + + const tenantID = "00000000-0000-4000-0000-000000000000" + + storeMock := new(storemock.MockStore) + storeMock.On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, tenantID). + Return(&models.Namespace{TenantID: tenantID}, nil).Once() + storeMock.On("AccessPolicyList", ctx, mock.Anything). + Return([]models.AccessPolicy{{ID: "policy1"}}, 7, nil).Once() + + service := NewService(storeMock, privateKey, publicKey, nil) + + policies, count, err := service.ListAccessPolicies(ctx, tenantID) + require.NoError(t, err) + require.Len(t, policies, 1) + require.Equal(t, 7, count) + + storeMock.AssertExpectations(t) +} diff --git a/server/api/services/api-key.go b/server/api/services/api-key.go index 9bda10be479..6eb35833f19 100644 --- a/server/api/services/api-key.go +++ b/server/api/services/api-key.go @@ -15,15 +15,17 @@ import ( "github.com/shellhub-io/shellhub/server/api/store" ) -// APIKeySortFields is the set of field names accepted in the sort_by query parameter when listing -// API keys. The row also holds the key's digest, which the response omits and a sort must not -// order by. -var APIKeySortFields = query.NewFieldSet( - "name", - "created_at", - "updated_at", - "expires_in", -) +// APIKeyQuery is the query contract the API key list accepts. The row also holds the key's digest, +// which the response omits and a sort must not order by; keys take no filter at all. +var APIKeyQuery = query.Contract{ + Sort: query.NewFieldSet( + "name", + "created_at", + "updated_at", + "expires_in", + ), + DefaultSort: query.Sorter{By: "expires_in", Order: query.OrderDesc}, +} // APIKeyService manages the keys that authenticate a namespace rather than a person. A key's // plaintext is returned once, at creation, and only its hash is kept. diff --git a/server/api/services/device.go b/server/api/services/device.go index 7a7e2bd1396..e8b736a3abc 100644 --- a/server/api/services/device.go +++ b/server/api/services/device.go @@ -20,33 +20,31 @@ import ( // because the store and the API both compare against the wire value. const StatusAccepted = "accepted" -// DeviceFilterFields maps each filter field the device list endpoint accepts -// to the set of operators valid for it. Operators that the database rejects -// on a given column type (e.g. ILIKE on the status enum) are omitted so the -// handler returns HTTP 400 instead of letting the store produce a 500. -var DeviceFilterFields = query.NewFieldConstraints(map[string][]string{ - "name": {"contains", "eq", "ne"}, - "status": {"eq", "ne"}, - "mac": {"contains", "eq", "ne"}, - "platform": {"contains", "eq", "ne"}, - "tags.name": {"contains", "eq"}, - "online": {"bool", "eq"}, - "custom_fields": {"contains"}, - - "info.platform": {"contains", "eq", "ne"}, - "identity.mac": {"contains", "eq", "ne"}, -}, - "online", -) - -// DeviceSortFields is the set of field names accepted in the sort_by query -// parameter when listing devices. -var DeviceSortFields = query.NewFieldSet( - "name", - "status", - "last_seen", - "created_at", -) +// DeviceQuery is the query contract the device list accepts. Operators the database rejects on a +// given column type (e.g. ILIKE on the status enum) are omitted, so the route answers 400 instead of +// letting the store produce a 500. +var DeviceQuery = query.Contract{ + Filter: query.NewFieldConstraints(map[string][]string{ + "name": {"contains", "eq", "ne"}, + "status": {"eq", "ne"}, + "mac": {"contains", "eq", "ne"}, + "platform": {"contains", "eq", "ne"}, + "tags.name": {"contains", "eq"}, + "online": {"bool", "eq"}, + "custom_fields": {"contains"}, + + "info.platform": {"contains", "eq", "ne"}, + "identity.mac": {"contains", "eq", "ne"}, + }, + "online", + ), + Sort: query.NewFieldSet( + "name", + "status", + "last_seen", + "created_at", + ), +} // DeviceService owns the device lifecycle: enrolment, acceptance, renaming, tagging and // removal, all within a namespace scope. diff --git a/server/api/services/install-key.go b/server/api/services/install-key.go index 78b7754b94b..50efe771a6e 100644 --- a/server/api/services/install-key.go +++ b/server/api/services/install-key.go @@ -28,29 +28,35 @@ const ( installKeyMaxEphemeralTimeout = 10 ) -// InstallKeySortFields is the set of field names accepted in the sort_by query parameter when -// listing install keys. The row also holds the key ciphertext and the webhook signing secret, -// neither of which the response carries and neither of which a sort must order by. -var InstallKeySortFields = query.NewFieldSet( - "name", - "mode", - "type", - "used_times", - "last_used_at", - "created_at", - "updated_at", - "expires_at", -) +// InstallKeyQuery is the query contract the install key list accepts. The row also holds the key +// ciphertext and the webhook signing secret, neither of which the response carries and neither of +// which a sort must order by. +var InstallKeyQuery = query.Contract{ + Sort: query.NewFieldSet( + "name", + "mode", + "type", + "used_times", + "last_used_at", + "created_at", + "updated_at", + "expires_at", + ), + DefaultSort: query.Sorter{By: "created_at", Order: query.OrderDesc}, +} -// InstallKeyEventSortFields is the set of field names accepted in the sort_by query parameter -// when listing an install key's history. -var InstallKeyEventSortFields = query.NewFieldSet( - "hostname", - "source_ip", - "decided_status", - "decided_at", - "created_at", -) +// InstallKeyEventQuery is the query contract an install key's history accepts. The history takes no +// filter, and the row holds nothing the response omits. +var InstallKeyEventQuery = query.Contract{ + Sort: query.NewFieldSet( + "hostname", + "source_ip", + "decided_status", + "decided_at", + "created_at", + ), + DefaultSort: query.Sorter{By: "created_at", Order: query.OrderDesc}, +} func installKeyExpiry(days *int) *time.Time { if days == nil { diff --git a/server/api/services/invitation.go b/server/api/services/invitation.go index 3a19b33f24a..80a624410c4 100644 --- a/server/api/services/invitation.go +++ b/server/api/services/invitation.go @@ -16,23 +16,23 @@ import ( log "github.com/sirupsen/logrus" ) -// MembershipInvitationFilterFields maps each filter field the invitation list endpoints accept to -// the set of operators valid for it. It names only fields the response already carries: the row -// also holds the invitation's signature, which the response omits and a filter must not reach. -var MembershipInvitationFilterFields = query.NewFieldConstraints(map[string][]string{ - "status": {"eq", "ne"}, - "role": {"eq", "ne"}, -}) - -// MembershipInvitationSortFields is the set of field names accepted in the sort_by query -// parameter when listing invitations. -var MembershipInvitationSortFields = query.NewFieldSet( - "status", - "role", - "created_at", - "updated_at", - "expires_at", -) +// MembershipInvitationQuery is the query contract both invitation lists accept — the user's and the +// namespace's, which face opposite directions across the same rows. It names only fields the +// response already carries: the row also holds the invitation's signature, which the response omits +// and neither a filter nor a sort must reach. +var MembershipInvitationQuery = query.Contract{ + Filter: query.NewFieldConstraints(map[string][]string{ + "status": {"eq", "ne"}, + "role": {"eq", "ne"}, + }), + Sort: query.NewFieldSet( + "status", + "role", + "created_at", + "updated_at", + "expires_at", + ), +} // InvitationService owns membership invitations, from issuing an invite code through to the // invitee accepting it, whether or not they already have an account. @@ -51,11 +51,14 @@ type InvitationService interface { // enabled (enterprise), the member is added directly and an empty link is returned. GenerateInvitationLink(ctx context.Context, req *requests.GenerateInvitationLink) (string, error) - // UserMembershipInvitationList lists membership invitations for a user. - UserMembershipInvitationList(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses.MembershipInvitation, int64, error) + // UserMembershipInvitationList lists membership invitations for a user, and the size of the whole + // collection. The store counts in int64; the count is narrowed here so the route layer writes the + // same header for invitations as for every other list. + UserMembershipInvitationList(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses.MembershipInvitation, int, error) - // NamespaceMembershipInvitationList lists membership invitations for a namespace. - NamespaceMembershipInvitationList(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses.MembershipInvitation, int64, error) + // NamespaceMembershipInvitationList lists membership invitations for a namespace, and the size of + // the whole collection, narrowed from the store's int64 as [InvitationService.UserMembershipInvitationList] is. + NamespaceMembershipInvitationList(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses.MembershipInvitation, int, error) // CancelMembershipInvitation cancels a pending membership invitation. CancelMembershipInvitation(ctx context.Context, req *requests.CancelMembershipInvitation) error @@ -160,7 +163,7 @@ func buildInviteURL(forwardedProto, forwardedHost, sig string) string { return scheme + "://" + forwardedHost + "/accept-invite?" + query.Encode() } -func (s *service) UserMembershipInvitationList(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses.MembershipInvitation, int64, error) { +func (s *service) UserMembershipInvitationList(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses.MembershipInvitation, int, error) { invitations, count, err := s.store.UserMembershipInvitationList( ctx, req.UserID, @@ -177,10 +180,10 @@ func (s *service) UserMembershipInvitationList(ctx context.Context, req *request res[i] = *responses.MembershipInvitationFromModel(&invitations[i]) } - return res, count, nil + return res, int(count), nil } -func (s *service) NamespaceMembershipInvitationList(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses.MembershipInvitation, int64, error) { +func (s *service) NamespaceMembershipInvitationList(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses.MembershipInvitation, int, error) { if _, _, err := s.resolveActingMember(ctx, req.TenantID, req.UserID, authorizer.RoleAdministrator); err != nil { return nil, 0, err } @@ -209,7 +212,7 @@ func (s *service) NamespaceMembershipInvitationList(ctx context.Context, req *re } } - return res, count, nil + return res, int(count), nil } func (s *service) CancelMembershipInvitation(ctx context.Context, req *requests.CancelMembershipInvitation) error { diff --git a/server/api/services/invitation_test.go b/server/api/services/invitation_test.go index 51f425cb38f..ce5261ac035 100644 --- a/server/api/services/invitation_test.go +++ b/server/api/services/invitation_test.go @@ -426,7 +426,7 @@ func TestService_UserMembershipInvitationList(t *testing.T) { type Expected struct { invitations []responses.MembershipInvitation - count int64 + count int err error } @@ -503,7 +503,7 @@ func TestService_NamespaceMembershipInvitationList(t *testing.T) { type Expected struct { invitations []responses.MembershipInvitation - count int64 + count int err error } diff --git a/server/api/services/mocks/mock_service.go b/server/api/services/mocks/mock_service.go index d88799463e3..84916f615c3 100644 --- a/server/api/services/mocks/mock_service.go +++ b/server/api/services/mocks/mock_service.go @@ -4349,7 +4349,7 @@ func (_c *MockService_ListAPIKeys_Call) RunAndReturn(run func(ctx context.Contex } // ListAccessPolicies provides a mock function for the type MockService -func (_mock *MockService) ListAccessPolicies(ctx context.Context, tenantID string) ([]models.AccessPolicy, error) { +func (_mock *MockService) ListAccessPolicies(ctx context.Context, tenantID string) ([]models.AccessPolicy, int, error) { ret := _mock.Called(ctx, tenantID) if len(ret) == 0 { @@ -4357,8 +4357,9 @@ func (_mock *MockService) ListAccessPolicies(ctx context.Context, tenantID strin } var r0 []models.AccessPolicy - var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string) ([]models.AccessPolicy, error)); ok { + var r1 int + var r2 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string) ([]models.AccessPolicy, int, error)); ok { return returnFunc(ctx, tenantID) } if returnFunc, ok := ret.Get(0).(func(context.Context, string) []models.AccessPolicy); ok { @@ -4368,12 +4369,17 @@ func (_mock *MockService) ListAccessPolicies(ctx context.Context, tenantID strin r0 = ret.Get(0).([]models.AccessPolicy) } } - if returnFunc, ok := ret.Get(1).(func(context.Context, string) error); ok { + if returnFunc, ok := ret.Get(1).(func(context.Context, string) int); ok { r1 = returnFunc(ctx, tenantID) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(int) } - return r0, r1 + if returnFunc, ok := ret.Get(2).(func(context.Context, string) error); ok { + r2 = returnFunc(ctx, tenantID) + } else { + r2 = ret.Error(2) + } + return r0, r1, r2 } // MockService_ListAccessPolicies_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListAccessPolicies' @@ -4406,12 +4412,12 @@ func (_c *MockService_ListAccessPolicies_Call) Run(run func(ctx context.Context, return _c } -func (_c *MockService_ListAccessPolicies_Call) Return(accessPolicys []models.AccessPolicy, err error) *MockService_ListAccessPolicies_Call { - _c.Call.Return(accessPolicys, err) +func (_c *MockService_ListAccessPolicies_Call) Return(accessPolicys []models.AccessPolicy, n int, err error) *MockService_ListAccessPolicies_Call { + _c.Call.Return(accessPolicys, n, err) return _c } -func (_c *MockService_ListAccessPolicies_Call) RunAndReturn(run func(ctx context.Context, tenantID string) ([]models.AccessPolicy, error)) *MockService_ListAccessPolicies_Call { +func (_c *MockService_ListAccessPolicies_Call) RunAndReturn(run func(ctx context.Context, tenantID string) ([]models.AccessPolicy, int, error)) *MockService_ListAccessPolicies_Call { _c.Call.Return(run) return _c } @@ -4941,7 +4947,7 @@ func (_c *MockService_ListPublicKeys_Call) RunAndReturn(run func(ctx context.Con } // ListSSHIdentities provides a mock function for the type MockService -func (_mock *MockService) ListSSHIdentities(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, error) { +func (_mock *MockService) ListSSHIdentities(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, int, error) { ret := _mock.Called(ctx, req) if len(ret) == 0 { @@ -4949,8 +4955,9 @@ func (_mock *MockService) ListSSHIdentities(ctx context.Context, req *requests.S } var r0 []models.SSHIdentity - var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.SSHIdentityList) ([]models.SSHIdentity, error)); ok { + var r1 int + var r2 error + if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.SSHIdentityList) ([]models.SSHIdentity, int, error)); ok { return returnFunc(ctx, req) } if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.SSHIdentityList) []models.SSHIdentity); ok { @@ -4960,12 +4967,17 @@ func (_mock *MockService) ListSSHIdentities(ctx context.Context, req *requests.S r0 = ret.Get(0).([]models.SSHIdentity) } } - if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.SSHIdentityList) error); ok { + if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.SSHIdentityList) int); ok { r1 = returnFunc(ctx, req) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(int) } - return r0, r1 + if returnFunc, ok := ret.Get(2).(func(context.Context, *requests.SSHIdentityList) error); ok { + r2 = returnFunc(ctx, req) + } else { + r2 = ret.Error(2) + } + return r0, r1, r2 } // MockService_ListSSHIdentities_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListSSHIdentities' @@ -4998,18 +5010,18 @@ func (_c *MockService_ListSSHIdentities_Call) Run(run func(ctx context.Context, return _c } -func (_c *MockService_ListSSHIdentities_Call) Return(sSHIdentitys []models.SSHIdentity, err error) *MockService_ListSSHIdentities_Call { - _c.Call.Return(sSHIdentitys, err) +func (_c *MockService_ListSSHIdentities_Call) Return(sSHIdentitys []models.SSHIdentity, n int, err error) *MockService_ListSSHIdentities_Call { + _c.Call.Return(sSHIdentitys, n, err) return _c } -func (_c *MockService_ListSSHIdentities_Call) RunAndReturn(run func(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, error)) *MockService_ListSSHIdentities_Call { +func (_c *MockService_ListSSHIdentities_Call) RunAndReturn(run func(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, int, error)) *MockService_ListSSHIdentities_Call { _c.Call.Return(run) return _c } // ListServiceAccounts provides a mock function for the type MockService -func (_mock *MockService) ListServiceAccounts(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, error) { +func (_mock *MockService) ListServiceAccounts(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, int, error) { ret := _mock.Called(ctx, req) if len(ret) == 0 { @@ -5017,8 +5029,9 @@ func (_mock *MockService) ListServiceAccounts(ctx context.Context, req *requests } var r0 []models.ServiceAccount - var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.ServiceAccountList) ([]models.ServiceAccount, error)); ok { + var r1 int + var r2 error + if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.ServiceAccountList) ([]models.ServiceAccount, int, error)); ok { return returnFunc(ctx, req) } if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.ServiceAccountList) []models.ServiceAccount); ok { @@ -5028,12 +5041,17 @@ func (_mock *MockService) ListServiceAccounts(ctx context.Context, req *requests r0 = ret.Get(0).([]models.ServiceAccount) } } - if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.ServiceAccountList) error); ok { + if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.ServiceAccountList) int); ok { r1 = returnFunc(ctx, req) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(int) } - return r0, r1 + if returnFunc, ok := ret.Get(2).(func(context.Context, *requests.ServiceAccountList) error); ok { + r2 = returnFunc(ctx, req) + } else { + r2 = ret.Error(2) + } + return r0, r1, r2 } // MockService_ListServiceAccounts_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListServiceAccounts' @@ -5066,12 +5084,12 @@ func (_c *MockService_ListServiceAccounts_Call) Run(run func(ctx context.Context return _c } -func (_c *MockService_ListServiceAccounts_Call) Return(serviceAccounts []models.ServiceAccount, err error) *MockService_ListServiceAccounts_Call { - _c.Call.Return(serviceAccounts, err) +func (_c *MockService_ListServiceAccounts_Call) Return(serviceAccounts []models.ServiceAccount, n int, err error) *MockService_ListServiceAccounts_Call { + _c.Call.Return(serviceAccounts, n, err) return _c } -func (_c *MockService_ListServiceAccounts_Call) RunAndReturn(run func(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, error)) *MockService_ListServiceAccounts_Call { +func (_c *MockService_ListServiceAccounts_Call) RunAndReturn(run func(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, int, error)) *MockService_ListServiceAccounts_Call { _c.Call.Return(run) return _c } @@ -5371,7 +5389,7 @@ func (_c *MockService_NamespaceHasAccessPolicies_Call) RunAndReturn(run func(ctx } // NamespaceMembershipInvitationList provides a mock function for the type MockService -func (_mock *MockService) NamespaceMembershipInvitationList(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses0.MembershipInvitation, int64, error) { +func (_mock *MockService) NamespaceMembershipInvitationList(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses0.MembershipInvitation, int, error) { ret := _mock.Called(ctx, req) if len(ret) == 0 { @@ -5379,9 +5397,9 @@ func (_mock *MockService) NamespaceMembershipInvitationList(ctx context.Context, } var r0 []responses0.MembershipInvitation - var r1 int64 + var r1 int var r2 error - if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.NamespaceMembershipInvitationList) ([]responses0.MembershipInvitation, int64, error)); ok { + if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.NamespaceMembershipInvitationList) ([]responses0.MembershipInvitation, int, error)); ok { return returnFunc(ctx, req) } if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.NamespaceMembershipInvitationList) []responses0.MembershipInvitation); ok { @@ -5391,10 +5409,10 @@ func (_mock *MockService) NamespaceMembershipInvitationList(ctx context.Context, r0 = ret.Get(0).([]responses0.MembershipInvitation) } } - if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.NamespaceMembershipInvitationList) int64); ok { + if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.NamespaceMembershipInvitationList) int); ok { r1 = returnFunc(ctx, req) } else { - r1 = ret.Get(1).(int64) + r1 = ret.Get(1).(int) } if returnFunc, ok := ret.Get(2).(func(context.Context, *requests.NamespaceMembershipInvitationList) error); ok { r2 = returnFunc(ctx, req) @@ -5434,12 +5452,12 @@ func (_c *MockService_NamespaceMembershipInvitationList_Call) Run(run func(ctx c return _c } -func (_c *MockService_NamespaceMembershipInvitationList_Call) Return(membershipInvitations []responses0.MembershipInvitation, n int64, err error) *MockService_NamespaceMembershipInvitationList_Call { +func (_c *MockService_NamespaceMembershipInvitationList_Call) Return(membershipInvitations []responses0.MembershipInvitation, n int, err error) *MockService_NamespaceMembershipInvitationList_Call { _c.Call.Return(membershipInvitations, n, err) return _c } -func (_c *MockService_NamespaceMembershipInvitationList_Call) RunAndReturn(run func(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses0.MembershipInvitation, int64, error)) *MockService_NamespaceMembershipInvitationList_Call { +func (_c *MockService_NamespaceMembershipInvitationList_Call) RunAndReturn(run func(ctx context.Context, req *requests.NamespaceMembershipInvitationList) ([]responses0.MembershipInvitation, int, error)) *MockService_NamespaceMembershipInvitationList_Call { _c.Call.Return(run) return _c } @@ -7568,7 +7586,7 @@ func (_c *MockService_UpdateUser_Call) RunAndReturn(run func(ctx context.Context } // UserMembershipInvitationList provides a mock function for the type MockService -func (_mock *MockService) UserMembershipInvitationList(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses0.MembershipInvitation, int64, error) { +func (_mock *MockService) UserMembershipInvitationList(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses0.MembershipInvitation, int, error) { ret := _mock.Called(ctx, req) if len(ret) == 0 { @@ -7576,9 +7594,9 @@ func (_mock *MockService) UserMembershipInvitationList(ctx context.Context, req } var r0 []responses0.MembershipInvitation - var r1 int64 + var r1 int var r2 error - if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.UserMembershipInvitationList) ([]responses0.MembershipInvitation, int64, error)); ok { + if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.UserMembershipInvitationList) ([]responses0.MembershipInvitation, int, error)); ok { return returnFunc(ctx, req) } if returnFunc, ok := ret.Get(0).(func(context.Context, *requests.UserMembershipInvitationList) []responses0.MembershipInvitation); ok { @@ -7588,10 +7606,10 @@ func (_mock *MockService) UserMembershipInvitationList(ctx context.Context, req r0 = ret.Get(0).([]responses0.MembershipInvitation) } } - if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.UserMembershipInvitationList) int64); ok { + if returnFunc, ok := ret.Get(1).(func(context.Context, *requests.UserMembershipInvitationList) int); ok { r1 = returnFunc(ctx, req) } else { - r1 = ret.Get(1).(int64) + r1 = ret.Get(1).(int) } if returnFunc, ok := ret.Get(2).(func(context.Context, *requests.UserMembershipInvitationList) error); ok { r2 = returnFunc(ctx, req) @@ -7631,12 +7649,12 @@ func (_c *MockService_UserMembershipInvitationList_Call) Run(run func(ctx contex return _c } -func (_c *MockService_UserMembershipInvitationList_Call) Return(membershipInvitations []responses0.MembershipInvitation, n int64, err error) *MockService_UserMembershipInvitationList_Call { +func (_c *MockService_UserMembershipInvitationList_Call) Return(membershipInvitations []responses0.MembershipInvitation, n int, err error) *MockService_UserMembershipInvitationList_Call { _c.Call.Return(membershipInvitations, n, err) return _c } -func (_c *MockService_UserMembershipInvitationList_Call) RunAndReturn(run func(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses0.MembershipInvitation, int64, error)) *MockService_UserMembershipInvitationList_Call { +func (_c *MockService_UserMembershipInvitationList_Call) RunAndReturn(run func(ctx context.Context, req *requests.UserMembershipInvitationList) ([]responses0.MembershipInvitation, int, error)) *MockService_UserMembershipInvitationList_Call { _c.Call.Return(run) return _c } diff --git a/server/api/services/namespace.go b/server/api/services/namespace.go index d4d7546529d..7a06ce46ea1 100644 --- a/server/api/services/namespace.go +++ b/server/api/services/namespace.go @@ -15,14 +15,19 @@ import ( "github.com/shellhub-io/shellhub/server/api/store" ) -// NamespaceFilterFields maps each filter field the namespace list endpoint accepts -// to the set of operators valid for it. The "type" field maps to the "scope" column -// in the database (see namespaceFilterColumns) and only supports equality operators -// because it is an enum column. -var NamespaceFilterFields = query.NewFieldConstraints(map[string][]string{ - "name": {"contains", "eq", "ne"}, - "type": {"eq", "ne"}, -}) +// NamespaceQuery is the query contract the namespace list accepts. The "type" field maps to the +// "scope" column in the database (see namespaceFilterColumns) and only supports equality operators +// because it is an enum column. The list takes no sort. +var NamespaceQuery = query.Contract{ + Filter: query.NewFieldConstraints(map[string][]string{ + "name": {"contains", "eq", "ne"}, + "type": {"eq", "ne"}, + }), +} + +// MemberQuery is the query contract the namespace member list accepts, which is nothing: the list +// pages through a namespace's members and offers neither a filter nor a sort. +var MemberQuery = query.Contract{} var namespaceFilterColumns = map[string]string{ "type": "scope", diff --git a/server/api/services/namespace_test.go b/server/api/services/namespace_test.go index 7906b429703..384b66e6f2c 100644 --- a/server/api/services/namespace_test.go +++ b/server/api/services/namespace_test.go @@ -1602,24 +1602,24 @@ func TestDeleteNamespace(t *testing.T) { storeMock.AssertExpectations(t) } -func TestNamespaceFilterFields(t *testing.T) { +func TestNamespaceQuery(t *testing.T) { t.Run("name field allows contains, eq and ne operators", func(t *testing.T) { - assert.True(t, NamespaceFilterFields.Allows("name", "contains")) - assert.True(t, NamespaceFilterFields.Allows("name", "eq")) - assert.True(t, NamespaceFilterFields.Allows("name", "ne")) + assert.True(t, NamespaceQuery.Filter.Allows("name", "contains")) + assert.True(t, NamespaceQuery.Filter.Allows("name", "eq")) + assert.True(t, NamespaceQuery.Filter.Allows("name", "ne")) }) t.Run("type field allows eq and ne operators", func(t *testing.T) { - assert.True(t, NamespaceFilterFields.Allows("type", "eq")) - assert.True(t, NamespaceFilterFields.Allows("type", "ne")) + assert.True(t, NamespaceQuery.Filter.Allows("type", "eq")) + assert.True(t, NamespaceQuery.Filter.Allows("type", "ne")) }) t.Run("type field does not allow contains operator", func(t *testing.T) { - assert.False(t, NamespaceFilterFields.Allows("type", "contains")) + assert.False(t, NamespaceQuery.Filter.Allows("type", "contains")) }) t.Run("unknown field is rejected", func(t *testing.T) { - assert.False(t, NamespaceFilterFields.Allows("unknown", "eq")) + assert.False(t, NamespaceQuery.Filter.Allows("unknown", "eq")) }) t.Run("namespaceFilterColumns maps type to scope", func(t *testing.T) { diff --git a/server/api/services/service-account.go b/server/api/services/service-account.go index 1e22b65f19b..71b966861a7 100644 --- a/server/api/services/service-account.go +++ b/server/api/services/service-account.go @@ -4,6 +4,7 @@ import ( "context" "github.com/shellhub-io/shellhub/pkg/api/authorizer" + "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" "github.com/shellhub-io/shellhub/pkg/clock" "github.com/shellhub-io/shellhub/pkg/models" @@ -12,6 +13,10 @@ import ( "golang.org/x/crypto/ssh" ) +// ServiceAccountQuery is the query contract the service account list accepts, which is nothing: the +// list serves every account in the namespace and offers neither a filter nor a sort. +var ServiceAccountQuery = query.Contract{} + // ServiceAccountService manages accounts that exist only to hold an SSH identity. They never // sign in and are not API principals. type ServiceAccountService interface { @@ -20,8 +25,9 @@ type ServiceAccountService interface { // key, all atomically. The account never signs in and is not an API principal. CreateServiceAccount(ctx context.Context, req *requests.ServiceAccountCreate) (*models.ServiceAccount, error) - // ListServiceAccounts returns the namespace's service accounts with their identities. - ListServiceAccounts(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, error) + // ListServiceAccounts returns the namespace's service accounts with their identities, and the + // size of the whole collection as the store counted it. + ListServiceAccounts(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, int, error) // DeleteServiceAccount removes a service account. Deleting the account cascades to its // membership and every SSH identity it holds. @@ -106,24 +112,24 @@ func (s *service) CreateServiceAccount(ctx context.Context, req *requests.Servic return account, nil } -func (s *service) ListServiceAccounts(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, error) { +func (s *service) ListServiceAccounts(ctx context.Context, req *requests.ServiceAccountList) ([]models.ServiceAccount, int, error) { sc, err := BoundTo(req.TenantID) if err != nil { - return nil, err + return nil, 0, err } if _, err := s.store.NamespaceResolve(ctx, store.NamespaceTenantIDResolver, req.TenantID); err != nil { - return nil, NewErrNamespaceNotFound(req.TenantID, err) + return nil, 0, NewErrNamespaceNotFound(req.TenantID, err) } - accounts, _, err := s.store.ServiceAccountList(ctx, req.TenantID) + accounts, count, err := s.store.ServiceAccountList(ctx, req.TenantID) if err != nil { - return nil, err + return nil, 0, err } identities, _, err := s.store.SSHIdentityList(ctx, sc) if err != nil { - return nil, err + return nil, 0, err } byUser := make(map[string][]models.SSHIdentity, len(accounts)) @@ -135,7 +141,7 @@ func (s *service) ListServiceAccounts(ctx context.Context, req *requests.Service accounts[i].Identities = byUser[accounts[i].ID] } - return accounts, nil + return accounts, count, nil } func (s *service) DeleteServiceAccount(ctx context.Context, req *requests.ServiceAccountDelete) error { diff --git a/server/api/services/service-account_test.go b/server/api/services/service-account_test.go index e90e069897d..9ddba1cf620 100644 --- a/server/api/services/service-account_test.go +++ b/server/api/services/service-account_test.go @@ -9,6 +9,7 @@ import ( "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/store" storemock "github.com/shellhub-io/shellhub/server/api/store/mocks" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -87,3 +88,28 @@ func TestDeleteServiceAccount(t *testing.T) { }) } } + +// TestListServiceAccountsCarriesTheStoreCount pins the count the header is written from as the +// account store's, not the page's nor the identity list's. +func TestListServiceAccountsCarriesTheStoreCount(t *testing.T) { + ctx := context.TODO() + + const tenantID = "00000000-0000-4000-0000-000000000000" + + storeMock := new(storemock.MockStore) + storeMock.On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, tenantID). + Return(&models.Namespace{TenantID: tenantID}, nil).Once() + storeMock.On("ServiceAccountList", ctx, tenantID). + Return([]models.ServiceAccount{{ID: "account1"}}, 7, nil).Once() + storeMock.On("SSHIdentityList", ctx, mock.Anything). + Return([]models.SSHIdentity{{ID: "id1", PrincipalID: "account1"}, {ID: "id2", PrincipalID: "account1"}}, 2, nil).Once() + + service := NewService(storeMock, privateKey, publicKey, nil) + + accounts, count, err := service.ListServiceAccounts(ctx, &requests.ServiceAccountList{TenantID: tenantID}) + require.NoError(t, err) + require.Len(t, accounts, 1) + require.Equal(t, 7, count) + + storeMock.AssertExpectations(t) +} diff --git a/server/api/services/session.go b/server/api/services/session.go index a6d2b3245b5..7f79cb28ab5 100644 --- a/server/api/services/session.go +++ b/server/api/services/session.go @@ -13,20 +13,21 @@ import ( log "github.com/sirupsen/logrus" ) -// SessionFilterFields maps each filter field the session list endpoint accepts -// to the set of operators valid for it. +// SessionQuery is the query contract the session list accepts. The list takes no sort. // // "closed" and "active" are boolean-typed; only the "bool" operator is // permitted. Allowing "eq" on a boolean column lets a string value // (e.g. "true") bypass validation but fail at the Postgres level with // "operator does not exist: boolean = text", producing a 500 instead of 400. -var SessionFilterFields = query.NewFieldConstraints(map[string][]string{ - "device_uid": {"eq", "ne"}, - "closed": {"bool"}, - "active": {"bool"}, -}, - "active", -) +var SessionQuery = query.Contract{ + Filter: query.NewFieldConstraints(map[string][]string{ + "device_uid": {"eq", "ne"}, + "closed": {"bool"}, + "active": {"bool"}, + }, + "active", + ), +} // SessionService owns SSH session records and their recordings — the history of who reached // which device, and what was seen on screen. diff --git a/server/api/services/ssh-identity.go b/server/api/services/ssh-identity.go index ed3bd5e88f5..ed0499a8f69 100644 --- a/server/api/services/ssh-identity.go +++ b/server/api/services/ssh-identity.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" "github.com/shellhub-io/shellhub/pkg/clock" "github.com/shellhub-io/shellhub/pkg/models" @@ -42,6 +43,10 @@ func sshIdentityExpiry(days *int) *time.Time { return &at } +// SSHIdentityQuery is the query contract the SSH identity list accepts, which is nothing: what the +// list serves is chosen by the all flag and the caller's permission, not by a filter or a sort. +var SSHIdentityQuery = query.Contract{} + // SSHIdentityService owns the enrolled public keys that identify a person to a device, as // opposed to the namespace-wide keys in [SSHKeysService]. type SSHIdentityService interface { @@ -56,10 +61,11 @@ type SSHIdentityService interface { // concurrent session already consumed the key and the caller must be denied. ConsumeSSHIdentity(ctx context.Context, tenantID, fingerprint string) (bool, error) - // ListSSHIdentities returns the caller's enrolled identities in the namespace. + // ListSSHIdentities returns the caller's enrolled identities in the namespace, and the size of + // the whole collection as the store counted it. // When all is true it returns every member's (the caller must hold // SSHIdentityManage, enforced at the handler). - ListSSHIdentities(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, error) + ListSSHIdentities(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, int, error) // CreateSSHIdentity manually enrolls a pasted OpenSSH public key for the // caller and returns the stored identity. @@ -168,10 +174,10 @@ func (s *service) persistSSHIdentity(ctx context.Context, identity *models.SSHId return identity, nil } -func (s *service) ListSSHIdentities(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, error) { +func (s *service) ListSSHIdentities(ctx context.Context, req *requests.SSHIdentityList) ([]models.SSHIdentity, int, error) { sc, err := BoundTo(req.TenantID) if err != nil { - return nil, err + return nil, 0, err } var opts []store.QueryOption @@ -179,12 +185,12 @@ func (s *service) ListSSHIdentities(ctx context.Context, req *requests.SSHIdenti opts = append(opts, s.store.Options().WithUserID(req.UserID)) } - identities, _, err := s.store.SSHIdentityList(ctx, sc, opts...) + identities, count, err := s.store.SSHIdentityList(ctx, sc, opts...) if err != nil { - return nil, err + return nil, 0, err } - return identities, nil + return identities, count, nil } func (s *service) CreateSSHIdentity(ctx context.Context, req *requests.SSHIdentityCreate) (*models.SSHIdentity, error) { diff --git a/server/api/services/ssh-identity_test.go b/server/api/services/ssh-identity_test.go index 14eaa4239cf..db35181d9f0 100644 --- a/server/api/services/ssh-identity_test.go +++ b/server/api/services/ssh-identity_test.go @@ -491,6 +491,9 @@ func TestDeleteSSHIdentity(t *testing.T) { } } +// TestListSSHIdentities pins the count the header is written from as the store's, not the page's. +// Each store mock returns a count that disagrees with the slice length, which is the only way to +// tell one from the other while the list is unpaginated. func TestListSSHIdentities(t *testing.T) { ctx := context.TODO() @@ -505,13 +508,14 @@ func TestListSSHIdentities(t *testing.T) { storeMock.On("Options").Return(queryOptionsMock).Maybe() queryOptionsMock.On("WithUserID", userID).Return(nil).Once() storeMock.On("SSHIdentityList", ctx, mock.Anything, mock.Anything). - Return([]models.SSHIdentity{{ID: "id1", PrincipalID: userID}}, 1, nil).Once() + Return([]models.SSHIdentity{{ID: "id1", PrincipalID: userID}}, 7, nil).Once() service := NewService(storeMock, privateKey, publicKey, nil) - list, err := service.ListSSHIdentities(ctx, &requests.SSHIdentityList{TenantID: tenantID, UserID: userID, All: false}) + list, count, err := service.ListSSHIdentities(ctx, &requests.SSHIdentityList{TenantID: tenantID, UserID: userID, All: false}) require.NoError(t, err) require.Len(t, list, 1) + require.Equal(t, 7, count) storeMock.AssertExpectations(t) }) @@ -521,13 +525,14 @@ func TestListSSHIdentities(t *testing.T) { queryOptionsMock := new(storemock.MockQueryOptions) storeMock.On("Options").Return(queryOptionsMock).Maybe() storeMock.On("SSHIdentityList", ctx, mock.Anything). - Return([]models.SSHIdentity{{ID: "id1", PrincipalID: userID}, {ID: "id2", PrincipalID: "user2"}}, 2, nil).Once() + Return([]models.SSHIdentity{{ID: "id1", PrincipalID: userID}, {ID: "id2", PrincipalID: "user2"}}, 9, nil).Once() service := NewService(storeMock, privateKey, publicKey, nil) - list, err := service.ListSSHIdentities(ctx, &requests.SSHIdentityList{TenantID: tenantID, UserID: userID, All: true}) + list, count, err := service.ListSSHIdentities(ctx, &requests.SSHIdentityList{TenantID: tenantID, UserID: userID, All: true}) require.NoError(t, err) require.Len(t, list, 2) + require.Equal(t, 9, count) queryOptionsMock.AssertNotCalled(t, "WithUserID", mock.Anything) diff --git a/server/api/services/sshkeys.go b/server/api/services/sshkeys.go index 2d5a8185805..3da22576d97 100644 --- a/server/api/services/sshkeys.go +++ b/server/api/services/sshkeys.go @@ -17,12 +17,13 @@ import ( "golang.org/x/crypto/ssh" ) -// PublicKeyFilterFields maps each filter field the public key list endpoint -// accepts to the set of operators valid for it. -var PublicKeyFilterFields = query.NewFieldConstraints(map[string][]string{ - "name": {"contains", "eq", "ne"}, - "fingerprint": {"contains", "eq", "ne"}, -}) +// PublicKeyQuery is the query contract the public key list accepts. The list takes no sort. +var PublicKeyQuery = query.Contract{ + Filter: query.NewFieldConstraints(map[string][]string{ + "name": {"contains", "eq", "ne"}, + "fingerprint": {"contains", "eq", "ne"}, + }), +} // SSHKeysService owns a namespace's public keys and the rules restricting which devices and // usernames each may reach. diff --git a/server/api/services/tags.go b/server/api/services/tags.go index 530f8ecad01..f1d0a8916f3 100644 --- a/server/api/services/tags.go +++ b/server/api/services/tags.go @@ -10,19 +10,16 @@ import ( "github.com/shellhub-io/shellhub/server/api/store" ) -// TagFilterFields maps each filter field the tag list endpoint accepts to the -// set of operators valid for it. Tags currently have no filterable fields, so -// this is an empty FieldConstraints that causes all filter attempts to be -// rejected at the handler level. -var TagFilterFields = query.NewFieldConstraints(map[string][]string{}) - -// TagSortFields is the set of field names accepted in the sort_by query -// parameter when listing tags. -var TagSortFields = query.NewFieldSet( - "name", - "created_at", - "updated_at", -) +// TagQuery is the query contract the tag list accepts. Tags have no filterable fields, so every +// filter a client sends is refused. +var TagQuery = query.Contract{ + Filter: query.NewFieldConstraints(map[string][]string{}), + Sort: query.NewFieldSet( + "name", + "created_at", + "updated_at", + ), +} // TagsService owns tags, which are namespace-scoped labels attached to devices and keys. type TagsService interface { diff --git a/server/api/services/tags_test.go b/server/api/services/tags_test.go index 3120fef5963..81356a5132f 100644 --- a/server/api/services/tags_test.go +++ b/server/api/services/tags_test.go @@ -838,25 +838,25 @@ func TestService_DeleteTag(t *testing.T) { } func TestListTags(t *testing.T) { - t.Run("TagFilterFields rejects any field and operator", func(t *testing.T) { - assert.False(t, TagFilterFields.Allows("name", "eq")) - assert.False(t, TagFilterFields.Allows("name", "contains")) - assert.False(t, TagFilterFields.Allows("unknown", "eq")) + t.Run("the filter contract rejects any field and operator", func(t *testing.T) { + assert.False(t, TagQuery.Filter.Allows("name", "eq")) + assert.False(t, TagQuery.Filter.Allows("name", "contains")) + assert.False(t, TagQuery.Filter.Allows("unknown", "eq")) }) - t.Run("TagSortFields allows name", func(t *testing.T) { - assert.True(t, TagSortFields.Allows("name")) + t.Run("the sort contract allows name", func(t *testing.T) { + assert.True(t, TagQuery.Sort.Allows("name")) }) - t.Run("TagSortFields allows created_at", func(t *testing.T) { - assert.True(t, TagSortFields.Allows("created_at")) + t.Run("the sort contract allows created_at", func(t *testing.T) { + assert.True(t, TagQuery.Sort.Allows("created_at")) }) - t.Run("TagSortFields allows updated_at", func(t *testing.T) { - assert.True(t, TagSortFields.Allows("updated_at")) + t.Run("the sort contract allows updated_at", func(t *testing.T) { + assert.True(t, TagQuery.Sort.Allows("updated_at")) }) - t.Run("TagSortFields rejects unknown field", func(t *testing.T) { - assert.False(t, TagSortFields.Allows("unknown")) + t.Run("the sort contract rejects unknown field", func(t *testing.T) { + assert.False(t, TagQuery.Sort.Allows("unknown")) }) }