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")) }) }