From 0d79d7d7d334bf87239442f186c2032715daa7e9 Mon Sep 17 00:00:00 2001 From: Tanvir Ahmed Date: Thu, 27 Aug 2026 21:33:48 +0000 Subject: [PATCH 1/2] refactor: Reinstated a configurable upper bound on pagination limit. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8f08c48 removed MaxLimit to allow flexibility, leaving `limit` unbounded on the five Organizations list endpoints. A single request for `?limit=1000000` makes the database materialise the whole table, holds a pooled connection open for the duration, and forces the API process to serialise the result — a denial-of-service vector reachable by any authenticated caller. The flexibility that motivated removing the cap no longer needs an unbounded limit. b702bd2 added unpaginated GetAll… methods at the repository, service and plugin API layers, so embedders who need a whole collection have a first-class escape hatch that is Go-only and never reachable over HTTP. The ceiling is back, but configurable rather than constant: - pagination.DefaultMaxLimit = 100 is the fallback, and Clamp now takes the maximum as a parameter. A maximum of zero or less falls back to DefaultMaxLimit, so a caller with nothing configured still gets a bounded query rather than an unbounded one. - The floor is applied before the ceiling, so a maximum below DefaultLimit also caps the default: with max_page_limit 5, a request with no limit returns 5 rows. - OrganizationsPluginConfig gains MaxPageLimit *int, guarded in ApplyDefaults the same way InvitationsLimit is. - ServiceUtils carries the configured maximum and exposes ClampPagination. It is the one collaborator all five list services already share, and it is constructed once in plugin.go. Because a zero maxPageLimit degrades to DefaultMaxLimit, the existing &ServiceUtils{...} literals across the service tests keep compiling and behave as cap-100. Clamp's signature changed rather than gaining a ClampWithMax sibling, so that every un-migrated caller is a compile error instead of silently returning unbounded results. That is how the direct pagination.Clamp call inside an assertion in organization_team_member_service_test.go surfaced. The repository-layer pageLimit deliberately keeps its floor and gains no ceiling. Repositories take flat page/limit ints and cannot see plugin config, so capping to a constant there would silently override an operator who raised max_page_limit and make the response envelope lie. Every HTTP path reaches a repository through a service that has already clamped; a direct caller is a Go embedder, on the same footing as the GetAll… methods. The reasoning is recorded as a doc comment. Corrects the five list endpoint descriptions, which have documented "no upper bound on limit" since b702bd2, and adds `default` to the page and limit query parameter schemas. No `minimum` or `maximum`: both are JSON Schema assertions, and a validating gateway would reject `?page=0` outright — precisely the 400 that clamping exists to avoid. The ceiling is also deployment-configurable, so a fixed `maximum` in a static spec would be wrong. Bounds stay in prose, where they are advisory. openapi.json is regenerated. Adds docs/pagination.md covering the envelope, the clamping rules, max_page_limit, the created_at DESC, id DESC ordering and its tiebreaker, and when to reach for the unpaginated GetAll… methods. Tests cover the clamp at every layer: the new maximum and its fallbacks in core/pagination, ClampPagination including a nil receiver, ApplyDefaults, and the repository mock expectations in all five service list tests. The SQL-backed cases seed 120 members and prove the configured value — not the constant — reaches the real LIMIT and the response envelope, and that GetAllMembers ignores the ceiling entirely. The handler test asserting that an absurd limit is forwarded unclamped still passes: clamping stays a service concern and handlers stay parse-only. BREAKING CHANGE: pagination.Clamp takes a maxLimit argument and services.NewServiceUtils takes a maxPageLimit argument. Both are exported; embedders calling them must update their call sites, which will fail to compile rather than change behaviour silently. Over HTTP, a client that relied on an unbounded limit to fetch a whole collection in one request now receives at most max_page_limit rows, with the effective value echoed in pagination.limit. Such a client should page on has_more, use GetAll… if embedding in Go, or run against a deployment configured with a higher ceiling. --- core/pagination/pagination.go | 18 ++- core/pagination/pagination_test.go | 56 +++++++- docs/pagination.md | 102 +++++++++++++++ openapi.json | 20 ++- .../handlers/handler_test_helpers.go | 2 +- plugins/organizations/openapi/openapi_docs.go | 10 +- plugins/organizations/plugin.go | 6 +- .../organizations/repositories/pagination.go | 7 + .../organization_invitation_service.go | 2 +- .../organization_invitation_service_test.go | 6 +- .../services/organization_member_service.go | 2 +- .../organization_member_service_test.go | 6 +- .../services/organization_service.go | 2 +- .../services/organization_service_test.go | 6 +- .../organization_team_member_service.go | 2 +- .../organization_team_member_service_test.go | 8 +- .../services/organization_team_service.go | 2 +- .../organization_team_service_test.go | 6 +- .../services/pagination_integration_test.go | 121 +++++++++++++++++- .../organizations/services/service_utils.go | 18 ++- .../services/service_utils_test.go | 75 +++++++++++ plugins/organizations/types/api.go | 20 +-- plugins/organizations/types/config.go | 5 + plugins/organizations/types/config_test.go | 51 ++++++++ .../organizations/usecases/usecases_test.go | 4 +- 25 files changed, 505 insertions(+), 52 deletions(-) create mode 100644 docs/pagination.md create mode 100644 plugins/organizations/types/config_test.go diff --git a/core/pagination/pagination.go b/core/pagination/pagination.go index 9a8b449b..d30d1fbe 100644 --- a/core/pagination/pagination.go +++ b/core/pagination/pagination.go @@ -9,6 +9,9 @@ import ( const ( DefaultPage = 1 DefaultLimit = 10 + // DefaultMaxLimit is the fallback ceiling on a page size. It exists to stop a + // caller-supplied limit from turning a paginated query into a full table scan. + DefaultMaxLimit = 100 ) type Params struct { @@ -24,13 +27,26 @@ type Pagination struct { HasMore bool `json:"has_more" required:"true" nullable:"false"` } -func Clamp(params Params) Params { +// Clamp coerces params into a range that is safe to hand to a database: the page +// starts at 1, and the limit is held between 1 and maxLimit. A maxLimit of zero or +// less falls back to DefaultMaxLimit, so a caller that has nothing configured still +// gets a bounded query rather than an unbounded one. +// +// The floor is applied before the ceiling, so a maxLimit below DefaultLimit also +// caps the default: with maxLimit 5, an unset limit yields 5 rather than 10. +func Clamp(params Params, maxLimit int) Params { + if maxLimit <= 0 { + maxLimit = DefaultMaxLimit + } if params.Page < DefaultPage { params.Page = DefaultPage } if params.Limit <= 0 { params.Limit = DefaultLimit } + if params.Limit > maxLimit { + params.Limit = maxLimit + } return params } diff --git a/core/pagination/pagination_test.go b/core/pagination/pagination_test.go index b9b74d1e..92af5f6a 100644 --- a/core/pagination/pagination_test.go +++ b/core/pagination/pagination_test.go @@ -16,41 +16,91 @@ func TestClamp(t *testing.T) { tests := []struct { name string params pagination.Params + maxLimit int expected pagination.Params }{ { name: "valid params are untouched", params: pagination.Params{Page: 1, Limit: 10}, + maxLimit: pagination.DefaultMaxLimit, expected: pagination.Params{Page: 1, Limit: 10}, }, { name: "page zero becomes the first page", params: pagination.Params{Page: 0, Limit: 10}, + maxLimit: pagination.DefaultMaxLimit, expected: pagination.Params{Page: 1, Limit: 10}, }, { name: "negative page becomes the first page", params: pagination.Params{Page: -5, Limit: 10}, + maxLimit: pagination.DefaultMaxLimit, expected: pagination.Params{Page: 1, Limit: 10}, }, { name: "zero limit falls back to the default limit", params: pagination.Params{Page: 3, Limit: 0}, + maxLimit: pagination.DefaultMaxLimit, expected: pagination.Params{Page: 3, Limit: pagination.DefaultLimit}, }, { name: "negative limit falls back to the default limit", params: pagination.Params{Page: 3, Limit: -1}, + maxLimit: pagination.DefaultMaxLimit, expected: pagination.Params{Page: 3, Limit: pagination.DefaultLimit}, }, { - name: "large limits are left to the caller", + name: "limit exactly at the maximum is not capped", + params: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + maxLimit: pagination.DefaultMaxLimit, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + }, + { + name: "limit above the maximum is capped", + params: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit + 1}, + maxLimit: pagination.DefaultMaxLimit, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + }, + { + name: "an absurdly large limit is capped", + params: pagination.Params{Page: 1, Limit: 1_000_000}, + maxLimit: pagination.DefaultMaxLimit, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + }, + { + name: "a zero maximum falls back to the default maximum", + params: pagination.Params{Page: 1, Limit: 1_000_000}, + maxLimit: 0, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + }, + { + name: "a negative maximum falls back to the default maximum", params: pagination.Params{Page: 1, Limit: 1_000_000}, - expected: pagination.Params{Page: 1, Limit: 1_000_000}, + maxLimit: -1, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + }, + { + name: "a configured maximum above the default is honoured", + params: pagination.Params{Page: 1, Limit: 400}, + maxLimit: 500, + expected: pagination.Params{Page: 1, Limit: 400}, + }, + { + name: "a limit above a configured maximum is capped to it", + params: pagination.Params{Page: 1, Limit: 501}, + maxLimit: 500, + expected: pagination.Params{Page: 1, Limit: 500}, + }, + { + name: "a maximum below the default limit also caps the default", + params: pagination.Params{Page: 1, Limit: 0}, + maxLimit: 5, + expected: pagination.Params{Page: 1, Limit: 5}, }, { name: "high page numbers are legal", params: pagination.Params{Page: 999999, Limit: 10}, + maxLimit: pagination.DefaultMaxLimit, expected: pagination.Params{Page: 999999, Limit: 10}, }, } @@ -59,7 +109,7 @@ func TestClamp(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - require.Equal(t, tt.expected, pagination.Clamp(tt.params)) + require.Equal(t, tt.expected, pagination.Clamp(tt.params, tt.maxLimit)) }) } } diff --git a/docs/pagination.md b/docs/pagination.md new file mode 100644 index 00000000..2ab6e3f7 --- /dev/null +++ b/docs/pagination.md @@ -0,0 +1,102 @@ +# Pagination + +Authula's list endpoints return a page of results plus the metadata needed to walk +the rest. This page describes the contract, the bounds applied to caller input, and +when to reach for an unpaginated call instead. + +The organizations plugin implements this contract on all five of its list endpoints. +Other plugins predate it: `plugins/api-key` uses its own flat envelope and +`plugins/admin` uses cursor pagination. + +## Request + +Two optional query parameters: + +| Parameter | Default | Meaning | +| --- | --- | --- | +| `page` | `1` | 1-indexed page number | +| `limit` | `10` | Rows per page | + +``` +GET /organizations?page=2&limit=25 +``` + +## Response + +Every paginated endpoint returns the same envelope: a `data` array and a +`pagination` object. + +```json +{ + "data": [ ... ], + "pagination": { + "page": 2, + "limit": 25, + "total": 138, + "total_pages": 6, + "has_more": true + } +} +``` + +`total` counts every row matching the query, not just the current page. Iterate +until `has_more` is `false`. + +## Bounds + +Out-of-range input is **clamped, never rejected**. A list endpoint does not return +`400` for a bad `page` or `limit`; it coerces the value into range and serves the +request. The `pagination` object echoes the values actually applied, so a client can +always see what it got. + +| Input | Result | +| --- | --- | +| `page` below 1 | `1` | +| `limit` below 1 | `10` (the default) | +| `limit` above the maximum | the maximum | +| unparseable value | the default for that parameter | + +The ceiling exists to stop a single request from turning a paginated query into a +full table scan — a `limit` of a million would otherwise make the database +materialise the whole table, hold a pooled connection open for the duration, and +force the API process to serialise the result. + +### Configuring the maximum + +The ceiling defaults to **100** and is configurable per deployment: + +```toml +[plugins.organizations] +max_page_limit = 250 +``` + +A value of zero or less is treated as unset and falls back to 100. The floor is +applied before the ceiling, so a `max_page_limit` below 10 also caps the default +page size: with `max_page_limit = 5`, a request with no `limit` returns 5 rows. + +## Ordering + +Rows come back newest first, ordered by `created_at DESC, id DESC`. The `id` +tiebreaker keeps paging stable when several rows share a timestamp — without it, a +row can be skipped or repeated across page boundaries. + +Pending-invitation lookups are the deliberate exception: they order oldest first, +because acceptance resolves role conflicts in favour of the earliest invitation. + +## Fetching a whole collection + +When embedding Authula as a Go library, every paginated `ListAll…` method has an +unpaginated `GetAll…` twin that takes no pagination and returns a plain slice: + +```go +page, err := api.ListAllMembers(ctx, actor, orgID, pagination.Params{Page: 1, Limit: 50}) +all, err := api.GetAllMembers(ctx, actor, orgID) +``` + +The `GetAll…` methods are **not** subject to `max_page_limit` — they are the +sanctioned way to ask for a whole collection, and they skip the `SELECT COUNT(*)` +that the paginated path needs, making them a single round-trip. + +They are Go-only and are not exposed over HTTP. An HTTP client that needs more than +`max_page_limit` rows either pages through the results or runs against a deployment +configured with a higher ceiling. diff --git a/openapi.json b/openapi.json index b4fcbc84..0d6fcda3 100644 --- a/openapi.json +++ b/openapi.json @@ -2404,13 +2404,14 @@ "Organizations" ], "summary": "List organizations", - "description": "Lists every organization the authenticated user can access, both the ones they own and the ones they are a member of, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected.", + "description": "Lists every organization the authenticated user can access, both the ones they own and the ones they are a member of, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied.", "operationId": "listOrganizations", "parameters": [ { "name": "page", "in": "query", "schema": { + "default": 1, "type": "integer" } }, @@ -2418,6 +2419,7 @@ "name": "limit", "in": "query", "schema": { + "default": 10, "type": "integer" } } @@ -2572,13 +2574,14 @@ "Organization Invitations" ], "summary": "List invitations", - "description": "Lists the invitations for an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected.", + "description": "Lists the invitations for an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied.", "operationId": "listOrganizationInvitations", "parameters": [ { "name": "page", "in": "query", "schema": { + "default": 1, "type": "integer" } }, @@ -2586,6 +2589,7 @@ "name": "limit", "in": "query", "schema": { + "default": 10, "type": "integer" } }, @@ -2840,13 +2844,14 @@ "Organization Members" ], "summary": "List members", - "description": "Lists the members of an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected.", + "description": "Lists the members of an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied.", "operationId": "listOrganizationMembers", "parameters": [ { "name": "page", "in": "query", "schema": { + "default": 1, "type": "integer" } }, @@ -2854,6 +2859,7 @@ "name": "limit", "in": "query", "schema": { + "default": 10, "type": "integer" } }, @@ -3090,13 +3096,14 @@ "Organization Teams" ], "summary": "List teams", - "description": "Lists the teams within an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected.", + "description": "Lists the teams within an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied.", "operationId": "listOrganizationTeams", "parameters": [ { "name": "page", "in": "query", "schema": { + "default": 1, "type": "integer" } }, @@ -3104,6 +3111,7 @@ "name": "limit", "in": "query", "schema": { + "default": 10, "type": "integer" } }, @@ -3300,13 +3308,14 @@ "Organization Team Members" ], "summary": "List team members", - "description": "Lists the members of a team, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected.", + "description": "Lists the members of a team, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied.", "operationId": "listOrganizationTeamMembers", "parameters": [ { "name": "page", "in": "query", "schema": { + "default": 1, "type": "integer" } }, @@ -3314,6 +3323,7 @@ "name": "limit", "in": "query", "schema": { + "default": 10, "type": "integer" } }, diff --git a/plugins/organizations/handlers/handler_test_helpers.go b/plugins/organizations/handlers/handler_test_helpers.go index 34fc4108..254fca95 100644 --- a/plugins/organizations/handlers/handler_test_helpers.go +++ b/plugins/organizations/handlers/handler_test_helpers.go @@ -35,7 +35,7 @@ func defaultServiceUtils() *orgservices.ServiceUtils { memberRepo := &orgtests.MockOrganizationMemberRepository{} orgRepo.On("GetByID", mock.Anything, mock.Anything).Return(&orgtypes.Organization{ID: "org-1", OwnerID: "user-1"}, nil).Maybe() memberRepo.On("GetByOrganizationIDAndUserID", mock.Anything, mock.Anything, mock.Anything).Return(&orgtypes.OrganizationMember{ID: "mem-1", OrganizationID: "org-1", UserID: "user-1", Role: "admin"}, nil).Maybe() - return orgservices.NewServiceUtils(orgRepo, memberRepo, nil) + return orgservices.NewServiceUtils(orgRepo, memberRepo, nil, 0) } func defaultAccessControlService() *orgtests.AccessControlServiceStub { diff --git a/plugins/organizations/openapi/openapi_docs.go b/plugins/organizations/openapi/openapi_docs.go index d94cd3df..c01fca3b 100644 --- a/plugins/organizations/openapi/openapi_docs.go +++ b/plugins/organizations/openapi/openapi_docs.go @@ -26,7 +26,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/organizations", openapi.WithOperationID("listOrganizations"), openapi.WithSummary("List organizations"), - openapi.WithDescription("Lists every organization the authenticated user can access, both the ones they own and the ones they are a member of, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected."), + openapi.WithDescription("Lists every organization the authenticated user can access, both the ones they own and the ones they are a member of, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied."), openapi.WithTags("Organizations"), openapi.WithRequest(&types.ListOrganizationsRequest{}), openapi.WithResponseStatus(http.StatusOK, &types.ListOrganizationsResponse{}), @@ -81,7 +81,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/organizations/{organization_id}/invitations", openapi.WithOperationID("listOrganizationInvitations"), openapi.WithSummary("List invitations"), - openapi.WithDescription("Lists the invitations for an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected."), + openapi.WithDescription("Lists the invitations for an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied."), openapi.WithTags("Organization Invitations"), openapi.WithRequest(&types.ListOrganizationInvitationsRequest{}), openapi.WithResponseStatus(http.StatusOK, &types.ListOrganizationInvitationsResponse{}), @@ -144,7 +144,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/organizations/{organization_id}/members", openapi.WithOperationID("listOrganizationMembers"), openapi.WithSummary("List members"), - openapi.WithDescription("Lists the members of an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected."), + openapi.WithDescription("Lists the members of an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied."), openapi.WithTags("Organization Members"), openapi.WithRequest(&types.ListOrganizationMembersRequest{}), openapi.WithResponseStatus(http.StatusOK, &types.ListOrganizationMembersResponse{}), @@ -208,7 +208,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/organizations/{organization_id}/teams", openapi.WithOperationID("listOrganizationTeams"), openapi.WithSummary("List teams"), - openapi.WithDescription("Lists the teams within an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected."), + openapi.WithDescription("Lists the teams within an organization, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied."), openapi.WithTags("Organization Teams"), openapi.WithRequest(&types.ListOrganizationTeamsRequest{}), openapi.WithResponseStatus(http.StatusOK, &types.ListOrganizationTeamsResponse{}), @@ -262,7 +262,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/organizations/{organization_id}/teams/{team_id}/members", openapi.WithOperationID("listOrganizationTeamMembers"), openapi.WithSummary("List team members"), - openapi.WithDescription("Lists the members of a team, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. There is no upper bound on `limit`, so a sufficiently large value returns the whole collection in a single request. Values below the minimum fall back to the defaults rather than being rejected."), + openapi.WithDescription("Lists the members of a team, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied."), openapi.WithTags("Organization Team Members"), openapi.WithRequest(&types.ListOrganizationTeamMembersRequest{}), openapi.WithResponseStatus(http.StatusOK, &types.ListOrganizationTeamMembersResponse{}), diff --git a/plugins/organizations/plugin.go b/plugins/organizations/plugin.go index bfd3abd0..177fd4d5 100644 --- a/plugins/organizations/plugin.go +++ b/plugins/organizations/plugin.go @@ -91,7 +91,11 @@ func (p *OrganizationsPlugin) Init(ctx *models.PluginContext) error { p.teamRepo = repositories.NewBunOrganizationTeamRepository(ctx.DB) p.teamMemberRepo = repositories.NewBunOrganizationTeamMemberRepository(ctx.DB) - p.serviceUtils = services.NewServiceUtils(p.organizationRepo, p.memberRepo, p.teamRepo) + maxPageLimit := 0 + if p.pluginConfig.MaxPageLimit != nil { + maxPageLimit = *p.pluginConfig.MaxPageLimit + } + p.serviceUtils = services.NewServiceUtils(p.organizationRepo, p.memberRepo, p.teamRepo, maxPageLimit) p.organizationService = services.NewOrganizationService(p.organizationRepo, p.memberRepo, p.serviceUtils, accessControlService, p.pluginConfig.OrganizationsLimit, ctx.DB, p.hooksExecutor) emailTemplateManager, err := newOrganizationEmailTemplateManager() if err != nil { diff --git a/plugins/organizations/repositories/pagination.go b/plugins/organizations/repositories/pagination.go index cf73a77d..72e22e47 100644 --- a/plugins/organizations/repositories/pagination.go +++ b/plugins/organizations/repositories/pagination.go @@ -4,6 +4,13 @@ import ( "github.com/Authula/authula/core/pagination" ) +// pageLimit guards only against a non-positive limit producing a LIMIT 0 query. +// There is deliberately no ceiling here: the maximum page size is configurable per +// deployment and lives in the service layer (ServiceUtils.ClampPagination), which is +// the only layer that can see the plugin config. Capping to a constant here would +// silently override an operator who raised max_page_limit. Every HTTP path reaches +// these repositories through a service that has already clamped; a direct caller is +// a Go embedder, on the same footing as the unpaginated GetAll methods. func pageLimit(limit int) int { if limit <= 0 { return pagination.DefaultLimit diff --git a/plugins/organizations/services/organization_invitation_service.go b/plugins/organizations/services/organization_invitation_service.go index d1daa348..954650a1 100644 --- a/plugins/organizations/services/organization_invitation_service.go +++ b/plugins/organizations/services/organization_invitation_service.go @@ -314,7 +314,7 @@ func (s *organizationInvitationService) ListAllOrganizationInvitationsByOrgIDWit return nil, coreerrors.ErrNotFound } - params = pagination.Clamp(params) + params = s.serviceUtils.ClampPagination(params) invitations, total, err := s.orgInvitationRepo.ListAllByOrganizationIDWithOrg(ctx, organizationID, params.Page, params.Limit) if err != nil { diff --git a/plugins/organizations/services/organization_invitation_service_test.go b/plugins/organizations/services/organization_invitation_service_test.go index addab64b..aac7a3e3 100644 --- a/plugins/organizations/services/organization_invitation_service_test.go +++ b/plugins/organizations/services/organization_invitation_service_test.go @@ -751,15 +751,15 @@ func TestOrganizationInvitationService_ListAllOrganizationInvitationsByOrgIDWith expectPagination: pagination.Pagination{Page: 2, Limit: 2, Total: 5, TotalPages: 3, HasMore: true}, }, { - name: "a negative page is clamped before reaching the repository", + name: "a negative page and an oversized limit are clamped before reaching the repository", organizationID: "org-1", params: pagination.Params{Page: -4, Limit: 5000}, setup: func(invRepo *orgtests.MockOrganizationInvitationRepository) { - invRepo.On("ListAllByOrganizationIDWithOrg", mock.Anything, "org-1", 1, 5000). + invRepo.On("ListAllByOrganizationIDWithOrg", mock.Anything, "org-1", 1, pagination.DefaultMaxLimit). Return([]types.GetOrganizationInvitationResponse{}, 0, nil).Once() }, expectLen: 0, - expectPagination: pagination.Pagination{Page: 1, Limit: 5000, Total: 0, TotalPages: 0, HasMore: false}, + expectPagination: pagination.Pagination{Page: 1, Limit: pagination.DefaultMaxLimit, Total: 0, TotalPages: 0, HasMore: false}, }, { name: "nil result is normalised to an empty slice", diff --git a/plugins/organizations/services/organization_member_service.go b/plugins/organizations/services/organization_member_service.go index ce800dde..17076b5a 100644 --- a/plugins/organizations/services/organization_member_service.go +++ b/plugins/organizations/services/organization_member_service.go @@ -120,7 +120,7 @@ func (s *organizationMemberService) ListAllMembers(ctx context.Context, actor *m return nil, err } - params = pagination.Clamp(params) + params = s.serviceUtils.ClampPagination(params) members, total, err := s.orgMemberRepo.ListAllByOrganizationIDWithUser(ctx, organizationID, params.Page, params.Limit) if err != nil { diff --git a/plugins/organizations/services/organization_member_service_test.go b/plugins/organizations/services/organization_member_service_test.go index 65dfc952..217c7f99 100644 --- a/plugins/organizations/services/organization_member_service_test.go +++ b/plugins/organizations/services/organization_member_service_test.go @@ -372,17 +372,17 @@ func TestOrganizationMemberService_ListAllMembers(t *testing.T) { expectPagination: pagination.Pagination{Page: 1, Limit: 10, Total: 25, TotalPages: 3, HasMore: true}, }, { - name: "a negative page is clamped before reaching the repository", + name: "a negative page and an oversized limit are clamped before reaching the repository", actorUserID: "user-1", organizationID: "org-1", params: pagination.Params{Page: -4, Limit: 5000}, setup: func(orgRepo *orgtests.MockOrganizationRepository, memberRepo *orgtests.MockOrganizationMemberRepository) { orgRepo.On("GetByID", mock.Anything, "org-1").Return(&types.Organization{ID: "org-1", OwnerID: "user-1"}, nil).Once() - memberRepo.On("ListAllByOrganizationIDWithUser", mock.Anything, "org-1", 1, 5000). + memberRepo.On("ListAllByOrganizationIDWithUser", mock.Anything, "org-1", 1, pagination.DefaultMaxLimit). Return([]types.OrganizationMemberResponse{}, 0, nil).Once() }, expectLen: 0, - expectPagination: pagination.Pagination{Page: 1, Limit: 5000, Total: 0, TotalPages: 0, HasMore: false}, + expectPagination: pagination.Pagination{Page: 1, Limit: pagination.DefaultMaxLimit, Total: 0, TotalPages: 0, HasMore: false}, }, { name: "nil result is normalised to an empty slice", diff --git a/plugins/organizations/services/organization_service.go b/plugins/organizations/services/organization_service.go index a60ef07d..a4fea78f 100644 --- a/plugins/organizations/services/organization_service.go +++ b/plugins/organizations/services/organization_service.go @@ -184,7 +184,7 @@ func (s *organizationService) ListAllOrganizations(ctx context.Context, actor *m return nil, coreerrors.ErrUnauthorized } - params = pagination.Clamp(params) + params = s.serviceUtils.ClampPagination(params) organizations, total, err := s.orgRepo.ListAllAccessibleByUserID(ctx, actor.ID, params.Page, params.Limit) if err != nil { diff --git a/plugins/organizations/services/organization_service_test.go b/plugins/organizations/services/organization_service_test.go index 0d5cf903..63793beb 100644 --- a/plugins/organizations/services/organization_service_test.go +++ b/plugins/organizations/services/organization_service_test.go @@ -258,15 +258,15 @@ func TestOrganizationService_ListAllOrganizations(t *testing.T) { expectPagination: pagination.Pagination{Page: 1, Limit: 10, Total: 2, TotalPages: 1, HasMore: false}, }, { - name: "a negative page is clamped before reaching the repository", + name: "a negative page and an oversized limit are clamped before reaching the repository", actorUserID: "user-1", params: pagination.Params{Page: -4, Limit: 5000}, setup: func(repo *orgtests.MockOrganizationRepository) { - repo.On("ListAllAccessibleByUserID", mock.Anything, "user-1", 1, 5000). + repo.On("ListAllAccessibleByUserID", mock.Anything, "user-1", 1, pagination.DefaultMaxLimit). Return([]types.Organization{}, 0, nil).Once() }, expectLen: 0, - expectPagination: pagination.Pagination{Page: 1, Limit: 5000, Total: 0, TotalPages: 0, HasMore: false}, + expectPagination: pagination.Pagination{Page: 1, Limit: pagination.DefaultMaxLimit, Total: 0, TotalPages: 0, HasMore: false}, }, { name: "nil result is normalised to an empty slice", diff --git a/plugins/organizations/services/organization_team_member_service.go b/plugins/organizations/services/organization_team_member_service.go index 4fa16433..bb8c951e 100644 --- a/plugins/organizations/services/organization_team_member_service.go +++ b/plugins/organizations/services/organization_team_member_service.go @@ -140,7 +140,7 @@ func (s *organizationTeamMemberService) ListAllTeamMembers(ctx context.Context, return nil, err } - params = pagination.Clamp(params) + params = s.serviceUtils.ClampPagination(params) teamMembers, total, err := s.orgTeamMemberRepo.ListAllByTeamIDWithMemberAndUser(ctx, teamID, params.Page, params.Limit) if err != nil { diff --git a/plugins/organizations/services/organization_team_member_service_test.go b/plugins/organizations/services/organization_team_member_service_test.go index afd53753..f07da947 100644 --- a/plugins/organizations/services/organization_team_member_service_test.go +++ b/plugins/organizations/services/organization_team_member_service_test.go @@ -152,7 +152,7 @@ func TestOrganizationTeamService_ListAllTeamMembers(t *testing.T) { expectCalled: true, }, { - name: "a negative page is clamped before reaching the repository", + name: "a negative page and an oversized limit are clamped before reaching the repository", actorUserID: "user-1", organizationID: "org-1", teamID: "team-1", @@ -161,7 +161,7 @@ func TestOrganizationTeamService_ListAllTeamMembers(t *testing.T) { orgRepo.On("GetByID", mock.Anything, "org-1").Return(&types.Organization{ID: "org-1", OwnerID: "user-1"}, nil).Once() memberRepo.On("GetByOrganizationIDAndUserID", mock.Anything, "org-1", "user-1").Return(&types.OrganizationMember{ID: "owner-member-1", OrganizationID: "org-1", UserID: "user-1", Role: "owner"}, nil).Twice() teamRepo.On("GetByID", mock.Anything, "team-1").Return(&types.OrganizationTeam{ID: "team-1", OrganizationID: "org-1"}, nil).Twice() - teamMemberRepo.On("ListAllByTeamIDWithMemberAndUser", mock.Anything, "team-1", 1, 5000).Return(([]types.OrganizationTeamMemberResponse)(nil), 0, nil).Once() + teamMemberRepo.On("ListAllByTeamIDWithMemberAndUser", mock.Anything, "team-1", 1, pagination.DefaultMaxLimit).Return(([]types.OrganizationTeamMemberResponse)(nil), 0, nil).Once() }, expectLen: 0, expectCalled: true, @@ -200,8 +200,8 @@ func TestOrganizationTeamService_ListAllTeamMembers(t *testing.T) { require.NotNil(t, resp) require.NotNil(t, resp.Data) require.Len(t, resp.Data, tt.expectLen) - require.Equal(t, pagination.Clamp(params).Page, resp.Pagination.Page) - require.Equal(t, pagination.Clamp(params).Limit, resp.Pagination.Limit) + require.Equal(t, svc.serviceUtils.ClampPagination(params).Page, resp.Pagination.Page) + require.Equal(t, svc.serviceUtils.ClampPagination(params).Limit, resp.Pagination.Limit) require.True(t, orgRepo.AssertExpectations(t)) require.True(t, memberRepo.AssertExpectations(t)) require.True(t, teamRepo.AssertExpectations(t)) diff --git a/plugins/organizations/services/organization_team_service.go b/plugins/organizations/services/organization_team_service.go index 94f35b8f..a4cb778c 100644 --- a/plugins/organizations/services/organization_team_service.go +++ b/plugins/organizations/services/organization_team_service.go @@ -156,7 +156,7 @@ func (s *organizationTeamService) ListAllTeams(ctx context.Context, actor *model return nil, err } - params = pagination.Clamp(params) + params = s.serviceUtils.ClampPagination(params) teams, total, err := s.orgTeamRepo.ListAllByOrganizationID(ctx, organizationID, params.Page, params.Limit) if err != nil { diff --git a/plugins/organizations/services/organization_team_service_test.go b/plugins/organizations/services/organization_team_service_test.go index ee5e284c..248da218 100644 --- a/plugins/organizations/services/organization_team_service_test.go +++ b/plugins/organizations/services/organization_team_service_test.go @@ -276,17 +276,17 @@ func TestOrganizationTeamService_ListAllTeams(t *testing.T) { expectPagination: pagination.Pagination{Page: 1, Limit: 10, Total: 1, TotalPages: 1, HasMore: false}, }, { - name: "a negative page is clamped before reaching the repository", + name: "a negative page and an oversized limit are clamped before reaching the repository", actorUserID: "user-1", organizationID: "org-1", params: pagination.Params{Page: -4, Limit: 5000}, setup: func(orgRepo *orgtests.MockOrganizationRepository, teamRepo *orgtests.MockOrganizationTeamRepository, memberRepo *orgtests.MockOrganizationMemberRepository) { orgRepo.On("GetByID", mock.Anything, "org-1").Return(&types.Organization{ID: "org-1", OwnerID: "user-1"}, nil).Once() memberRepo.On("GetByOrganizationIDAndUserID", mock.Anything, "org-1", "user-1").Return(&types.OrganizationMember{ID: "mem-1", OrganizationID: "org-1", UserID: "user-1", Role: "owner"}, nil).Once() - teamRepo.On("ListAllByOrganizationID", mock.Anything, "org-1", 1, 5000).Return(([]types.OrganizationTeam)(nil), 0, nil).Once() + teamRepo.On("ListAllByOrganizationID", mock.Anything, "org-1", 1, pagination.DefaultMaxLimit).Return(([]types.OrganizationTeam)(nil), 0, nil).Once() }, expectLen: 0, - expectPagination: pagination.Pagination{Page: 1, Limit: 5000, Total: 0, TotalPages: 0, HasMore: false}, + expectPagination: pagination.Pagination{Page: 1, Limit: pagination.DefaultMaxLimit, Total: 0, TotalPages: 0, HasMore: false}, }, { name: "unauthorized", diff --git a/plugins/organizations/services/pagination_integration_test.go b/plugins/organizations/services/pagination_integration_test.go index b062497c..3cd09600 100644 --- a/plugins/organizations/services/pagination_integration_test.go +++ b/plugins/organizations/services/pagination_integration_test.go @@ -58,10 +58,10 @@ func TestOrganizationMemberService_ListAllMembersEnforcesLimitsAgainstSQL(t *tes expectPagination: pagination.Pagination{Page: 1, Limit: 10, Total: memberCount, TotalPages: 2, HasMore: true}, }, { - name: "a large limit is honoured", + name: "a large limit is capped at the maximum page size", params: pagination.Params{Page: 1, Limit: 100000}, expectLen: memberCount, - expectPagination: pagination.Pagination{Page: 1, Limit: 100000, Total: memberCount, TotalPages: 1, HasMore: false}, + expectPagination: pagination.Pagination{Page: 1, Limit: pagination.DefaultMaxLimit, Total: memberCount, TotalPages: 1, HasMore: false}, }, { name: "a negative limit does not read the whole table", @@ -187,3 +187,120 @@ func TestOrganizationMemberService_GetAllMembersReturnsEverythingInOneCallAgains require.NotEmpty(t, member.User.Email) } } + +// The ceiling on a page size must survive all the way into the SQL LIMIT, and the +// configured value — not the constant — must be the one that wins. +func TestOrganizationMemberService_ListAllMembersRespectsTheConfiguredMaxPageLimitAgainstSQL(t *testing.T) { + t.Parallel() + + const memberCount = 120 + + setup := func(t *testing.T, maxPageLimit int) (*organizationMemberService, context.Context) { + t.Helper() + + db := orgtests.SetupRepoDB(t) + orgtests.SeedUsers(t, db, memberCount) + orgtests.SeedOrganization(t, db, "org-1", "user-1", "Acme Inc", "acme-inc") + for i := 1; i <= memberCount; i++ { + orgtests.SeedOrganizationMember(t, db, fmt.Sprintf("mem-%03d", i), "org-1", fmt.Sprintf("user-%d", i), "member") + } + + orgRepo := repositories.NewBunOrganizationRepository(db) + memberRepo := repositories.NewBunOrganizationMemberRepository(db) + serviceUtils := &ServiceUtils{orgRepo: orgRepo, orgMemberRepo: memberRepo, maxPageLimit: maxPageLimit} + svc := NewOrganizationMemberService( + &internaltests.MockUserService{}, + orgtests.NewAccessControlServiceStub(), + orgRepo, + memberRepo, + nil, + &orgtests.MockTxRunner{}, + serviceUtils, + ) + + return svc, context.Background() + } + + tests := []struct { + name string + maxPageLimit int + params pagination.Params + expectLen int + expectPagination pagination.Pagination + }{ + { + name: "an unconfigured maximum truncates the page at the default maximum", + maxPageLimit: 0, + params: pagination.Params{Page: 1, Limit: 1_000_000}, + expectLen: pagination.DefaultMaxLimit, + expectPagination: pagination.Pagination{Page: 1, Limit: 100, Total: memberCount, TotalPages: 2, HasMore: true}, + }, + { + name: "a configured maximum below the default maximum wins", + maxPageLimit: 5, + params: pagination.Params{Page: 1, Limit: 1_000_000}, + expectLen: 5, + expectPagination: pagination.Pagination{Page: 1, Limit: 5, Total: memberCount, TotalPages: 24, HasMore: true}, + }, + { + name: "a configured maximum above the default maximum wins", + maxPageLimit: 120, + params: pagination.Params{Page: 1, Limit: 1_000_000}, + expectLen: memberCount, + expectPagination: pagination.Pagination{Page: 1, Limit: 120, Total: memberCount, TotalPages: 1, HasMore: false}, + }, + { + name: "a limit within the configured maximum is untouched", + maxPageLimit: 120, + params: pagination.Params{Page: 2, Limit: 30}, + expectLen: 30, + expectPagination: pagination.Pagination{Page: 2, Limit: 30, Total: memberCount, TotalPages: 4, HasMore: true}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + svc, ctx := setup(t, tt.maxPageLimit) + + resp, err := svc.ListAllMembers(ctx, orgtests.Actor("user-1"), "org-1", tt.params) + require.NoError(t, err) + require.NotNil(t, resp) + require.Len(t, resp.Data, tt.expectLen) + require.Equal(t, tt.expectPagination, resp.Pagination) + }) + } +} + +// The unpaginated sibling is the sanctioned escape hatch for whole collections and +// must stay uncapped, however low the configured page-size ceiling is. +func TestOrganizationMemberService_GetAllMembersIgnoresTheMaxPageLimitAgainstSQL(t *testing.T) { + t.Parallel() + + const memberCount = 120 + + db := orgtests.SetupRepoDB(t) + orgtests.SeedUsers(t, db, memberCount) + orgtests.SeedOrganization(t, db, "org-1", "user-1", "Acme Inc", "acme-inc") + for i := 1; i <= memberCount; i++ { + orgtests.SeedOrganizationMember(t, db, fmt.Sprintf("mem-%03d", i), "org-1", fmt.Sprintf("user-%d", i), "member") + } + + orgRepo := repositories.NewBunOrganizationRepository(db) + memberRepo := repositories.NewBunOrganizationMemberRepository(db) + serviceUtils := &ServiceUtils{orgRepo: orgRepo, orgMemberRepo: memberRepo, maxPageLimit: 5} + svc := NewOrganizationMemberService( + &internaltests.MockUserService{}, + orgtests.NewAccessControlServiceStub(), + orgRepo, + memberRepo, + nil, + &orgtests.MockTxRunner{}, + serviceUtils, + ) + + members, err := svc.GetAllMembers(context.Background(), orgtests.Actor("user-1"), "org-1") + require.NoError(t, err) + require.Len(t, members, memberCount, "a max page limit of 5 must not truncate the unpaginated call") +} diff --git a/plugins/organizations/services/service_utils.go b/plugins/organizations/services/service_utils.go index bd59b329..e9ed0e10 100644 --- a/plugins/organizations/services/service_utils.go +++ b/plugins/organizations/services/service_utils.go @@ -5,26 +5,42 @@ import ( "errors" coreerrors "github.com/Authula/authula/core/errors" + "github.com/Authula/authula/core/pagination" "github.com/Authula/authula/models" "github.com/Authula/authula/plugins/organizations/repositories" "github.com/Authula/authula/plugins/organizations/types" rootservices "github.com/Authula/authula/services" ) +// ServiceUtils carries the policy every organization service shares: the +// authorization checks below, and the configured ceiling on page sizes. type ServiceUtils struct { orgRepo repositories.OrganizationRepository orgMemberRepo repositories.OrganizationMemberRepository orgTeamRepo repositories.OrganizationTeamRepository + maxPageLimit int } -func NewServiceUtils(orgRepo repositories.OrganizationRepository, orgMemberRepo repositories.OrganizationMemberRepository, orgTeamRepo repositories.OrganizationTeamRepository) *ServiceUtils { +func NewServiceUtils(orgRepo repositories.OrganizationRepository, orgMemberRepo repositories.OrganizationMemberRepository, orgTeamRepo repositories.OrganizationTeamRepository, maxPageLimit int) *ServiceUtils { return &ServiceUtils{ orgRepo: orgRepo, orgMemberRepo: orgMemberRepo, orgTeamRepo: orgTeamRepo, + maxPageLimit: maxPageLimit, } } +// ClampPagination bounds caller-supplied pagination against the configured +// maximum page size. A zero or negative maximum falls back to +// pagination.DefaultMaxLimit, so an unconfigured ServiceUtils still yields a +// bounded query. +func (s *ServiceUtils) ClampPagination(params pagination.Params) pagination.Params { + if s == nil { + return pagination.Clamp(params, 0) + } + return pagination.Clamp(params, s.maxPageLimit) +} + func (s *ServiceUtils) authorizeOwner(ctx context.Context, actor *models.Actor, organizationID string) (*types.Organization, error) { if actor == nil || actor.ID == "" || organizationID == "" { return nil, coreerrors.ErrUnauthorized diff --git a/plugins/organizations/services/service_utils_test.go b/plugins/organizations/services/service_utils_test.go index 15735a71..e5ee60e4 100644 --- a/plugins/organizations/services/service_utils_test.go +++ b/plugins/organizations/services/service_utils_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" coreerrors "github.com/Authula/authula/core/errors" + "github.com/Authula/authula/core/pagination" internaltests "github.com/Authula/authula/internal/tests" "github.com/Authula/authula/models" orgconstants "github.com/Authula/authula/plugins/organizations/constants" @@ -605,3 +606,77 @@ func TestServiceUtils_authorizeTeamAccess(t *testing.T) { }) } } + +func TestServiceUtils_ClampPagination(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + maxPageLimit int + params pagination.Params + expected pagination.Params + }{ + { + name: "an unconfigured maximum falls back to the default maximum", + maxPageLimit: 0, + params: pagination.Params{Page: 1, Limit: 1_000_000}, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + }, + { + name: "a negative maximum falls back to the default maximum", + maxPageLimit: -25, + params: pagination.Params{Page: 1, Limit: 1_000_000}, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + }, + { + name: "a configured maximum caps the limit", + maxPageLimit: 25, + params: pagination.Params{Page: 2, Limit: 500}, + expected: pagination.Params{Page: 2, Limit: 25}, + }, + { + name: "a configured maximum above the default is honoured", + maxPageLimit: 500, + params: pagination.Params{Page: 1, Limit: 400}, + expected: pagination.Params{Page: 1, Limit: 400}, + }, + { + name: "a limit within the maximum is untouched", + maxPageLimit: 25, + params: pagination.Params{Page: 1, Limit: 20}, + expected: pagination.Params{Page: 1, Limit: 20}, + }, + { + name: "the page and limit floors are still applied", + maxPageLimit: 25, + params: pagination.Params{Page: -3, Limit: 0}, + expected: pagination.Params{Page: 1, Limit: pagination.DefaultLimit}, + }, + { + name: "a maximum below the default limit also caps the default", + maxPageLimit: 5, + params: pagination.Params{Page: 1, Limit: 0}, + expected: pagination.Params{Page: 1, Limit: 5}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + require.Equal(t, tt.expected, (&ServiceUtils{maxPageLimit: tt.maxPageLimit}).ClampPagination(tt.params)) + }) + } +} + +func TestServiceUtils_ClampPaginationOnANilReceiver(t *testing.T) { + t.Parallel() + + var utils *ServiceUtils + + require.Equal( + t, + pagination.Params{Page: 1, Limit: pagination.DefaultMaxLimit}, + utils.ClampPagination(pagination.Params{Page: 0, Limit: 1_000_000}), + ) +} diff --git a/plugins/organizations/types/api.go b/plugins/organizations/types/api.go index 7fefac11..d10659c9 100644 --- a/plugins/organizations/types/api.go +++ b/plugins/organizations/types/api.go @@ -40,33 +40,33 @@ type TeamMemberID struct { } type ListOrganizationsRequest struct { - Page int `query:"page" json:"page,omitempty" nullable:"false"` - Limit int `query:"limit" json:"limit,omitempty" nullable:"false"` + Page int `query:"page" json:"page,omitempty" nullable:"false" default:"1"` + Limit int `query:"limit" json:"limit,omitempty" nullable:"false" default:"10"` } type ListOrganizationInvitationsRequest struct { OrganizationID string `path:"organization_id"` - Page int `query:"page" json:"page,omitempty" nullable:"false"` - Limit int `query:"limit" json:"limit,omitempty" nullable:"false"` + Page int `query:"page" json:"page,omitempty" nullable:"false" default:"1"` + Limit int `query:"limit" json:"limit,omitempty" nullable:"false" default:"10"` } type ListOrganizationMembersRequest struct { OrganizationID string `path:"organization_id"` - Page int `query:"page" json:"page,omitempty" nullable:"false"` - Limit int `query:"limit" json:"limit,omitempty" nullable:"false"` + Page int `query:"page" json:"page,omitempty" nullable:"false" default:"1"` + Limit int `query:"limit" json:"limit,omitempty" nullable:"false" default:"10"` } type ListOrganizationTeamsRequest struct { OrganizationID string `path:"organization_id"` - Page int `query:"page" json:"page,omitempty" nullable:"false"` - Limit int `query:"limit" json:"limit,omitempty" nullable:"false"` + Page int `query:"page" json:"page,omitempty" nullable:"false" default:"1"` + Limit int `query:"limit" json:"limit,omitempty" nullable:"false" default:"10"` } type ListOrganizationTeamMembersRequest struct { OrganizationID string `path:"organization_id"` TeamID string `path:"team_id"` - Page int `query:"page" json:"page,omitempty" nullable:"false"` - Limit int `query:"limit" json:"limit,omitempty" nullable:"false"` + Page int `query:"page" json:"page,omitempty" nullable:"false" default:"1"` + Limit int `query:"limit" json:"limit,omitempty" nullable:"false" default:"10"` } type ListOrganizationsResponse struct { diff --git a/plugins/organizations/types/config.go b/plugins/organizations/types/config.go index eb161f4a..89ae69ad 100644 --- a/plugins/organizations/types/config.go +++ b/plugins/organizations/types/config.go @@ -4,6 +4,7 @@ import ( "context" "time" + "github.com/Authula/authula/core/pagination" "github.com/Authula/authula/models" ) @@ -12,6 +13,7 @@ type OrganizationsPluginConfig struct { OrganizationsLimit *int `json:"organizations_limit" toml:"organizations_limit"` MembersLimit *int `json:"members_limit" toml:"members_limit"` InvitationsLimit *int `json:"invitations_limit" toml:"invitations_limit"` + MaxPageLimit *int `json:"max_page_limit" toml:"max_page_limit"` InvitationExpiresIn time.Duration `json:"invitation_expires_in" toml:"invitation_expires_in"` RequireEmailVerifiedOnInvitation bool `json:"require_email_verified_on_invitation" toml:"require_email_verified_on_invitation"` @@ -26,6 +28,9 @@ func (config *OrganizationsPluginConfig) ApplyDefaults() { if config.InvitationsLimit == nil || *config.InvitationsLimit <= 0 { config.InvitationsLimit = new(100) } + if config.MaxPageLimit == nil || *config.MaxPageLimit <= 0 { + config.MaxPageLimit = new(pagination.DefaultMaxLimit) + } if config.InvitationExpiresIn == 0 { config.InvitationExpiresIn = 24 * time.Hour } diff --git a/plugins/organizations/types/config_test.go b/plugins/organizations/types/config_test.go new file mode 100644 index 00000000..da9ede87 --- /dev/null +++ b/plugins/organizations/types/config_test.go @@ -0,0 +1,51 @@ +package types_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/Authula/authula/core/pagination" + "github.com/Authula/authula/plugins/organizations/types" +) + +func TestOrganizationsPluginConfig_ApplyDefaultsMaxPageLimit(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + limit *int + expected int + }{ + {name: "an unset maximum falls back to the default", limit: nil, expected: pagination.DefaultMaxLimit}, + {name: "a zero maximum falls back to the default", limit: new(0), expected: pagination.DefaultMaxLimit}, + {name: "a negative maximum falls back to the default", limit: new(-10), expected: pagination.DefaultMaxLimit}, + {name: "a configured maximum is left alone", limit: new(250), expected: 250}, + {name: "a maximum below the default page size is left alone", limit: new(5), expected: 5}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + config := &types.OrganizationsPluginConfig{MaxPageLimit: tt.limit} + config.ApplyDefaults() + + require.NotNil(t, config.MaxPageLimit) + require.Equal(t, tt.expected, *config.MaxPageLimit) + }) + } +} + +func TestOrganizationsPluginConfig_ApplyDefaultsLeavesOtherDefaultsIntact(t *testing.T) { + t.Parallel() + + config := &types.OrganizationsPluginConfig{} + config.ApplyDefaults() + + require.Equal(t, 100, *config.MembersLimit) + require.Equal(t, 100, *config.InvitationsLimit) + require.Equal(t, pagination.DefaultMaxLimit, *config.MaxPageLimit) + require.Equal(t, 24*time.Hour, config.InvitationExpiresIn) +} diff --git a/plugins/organizations/usecases/usecases_test.go b/plugins/organizations/usecases/usecases_test.go index 4af91ce1..2f457af5 100644 --- a/plugins/organizations/usecases/usecases_test.go +++ b/plugins/organizations/usecases/usecases_test.go @@ -20,7 +20,7 @@ import ( func newAuthorizeOrgAccessUseCases(orgRepo *orgtests.MockOrganizationRepository, memberRepo *orgtests.MockOrganizationMemberRepository, accessControl *orgtests.AccessControlServiceStub) *UseCases { return &UseCases{ authorizer: rootservices.NewDefaultAuthorizer(), - serviceUtils: orgservices.NewServiceUtils(orgRepo, memberRepo, nil), + serviceUtils: orgservices.NewServiceUtils(orgRepo, memberRepo, nil, 0), accessControl: accessControl, } } @@ -29,7 +29,7 @@ func newGetInvitationUseCases(invSvc orgservices.OrganizationInvitationService, return &UseCases{ invitationService: invSvc, userService: userSvc, - serviceUtils: orgservices.NewServiceUtils(orgRepo, memberRepo, nil), + serviceUtils: orgservices.NewServiceUtils(orgRepo, memberRepo, nil, 0), authorizer: rootservices.NewDefaultAuthorizer(), accessControl: accessControl, } From 311e6343f220304c92ecc5dd90437cf0e9ac008e Mon Sep 17 00:00:00 2001 From: Tanvir Ahmed Date: Thu, 27 Aug 2026 22:02:31 +0000 Subject: [PATCH 2/2] chore: Removed pagination docs and code comments. --- core/pagination/pagination.go | 13 +-- docs/pagination.md | 102 ------------------ .../organizations/repositories/pagination.go | 7 -- .../services/pagination_integration_test.go | 6 -- .../organizations/services/service_utils.go | 6 -- 5 files changed, 2 insertions(+), 132 deletions(-) delete mode 100644 docs/pagination.md diff --git a/core/pagination/pagination.go b/core/pagination/pagination.go index d30d1fbe..cce5e186 100644 --- a/core/pagination/pagination.go +++ b/core/pagination/pagination.go @@ -7,10 +7,8 @@ import ( ) const ( - DefaultPage = 1 - DefaultLimit = 10 - // DefaultMaxLimit is the fallback ceiling on a page size. It exists to stop a - // caller-supplied limit from turning a paginated query into a full table scan. + DefaultPage = 1 + DefaultLimit = 10 DefaultMaxLimit = 100 ) @@ -27,13 +25,6 @@ type Pagination struct { HasMore bool `json:"has_more" required:"true" nullable:"false"` } -// Clamp coerces params into a range that is safe to hand to a database: the page -// starts at 1, and the limit is held between 1 and maxLimit. A maxLimit of zero or -// less falls back to DefaultMaxLimit, so a caller that has nothing configured still -// gets a bounded query rather than an unbounded one. -// -// The floor is applied before the ceiling, so a maxLimit below DefaultLimit also -// caps the default: with maxLimit 5, an unset limit yields 5 rather than 10. func Clamp(params Params, maxLimit int) Params { if maxLimit <= 0 { maxLimit = DefaultMaxLimit diff --git a/docs/pagination.md b/docs/pagination.md deleted file mode 100644 index 2ab6e3f7..00000000 --- a/docs/pagination.md +++ /dev/null @@ -1,102 +0,0 @@ -# Pagination - -Authula's list endpoints return a page of results plus the metadata needed to walk -the rest. This page describes the contract, the bounds applied to caller input, and -when to reach for an unpaginated call instead. - -The organizations plugin implements this contract on all five of its list endpoints. -Other plugins predate it: `plugins/api-key` uses its own flat envelope and -`plugins/admin` uses cursor pagination. - -## Request - -Two optional query parameters: - -| Parameter | Default | Meaning | -| --- | --- | --- | -| `page` | `1` | 1-indexed page number | -| `limit` | `10` | Rows per page | - -``` -GET /organizations?page=2&limit=25 -``` - -## Response - -Every paginated endpoint returns the same envelope: a `data` array and a -`pagination` object. - -```json -{ - "data": [ ... ], - "pagination": { - "page": 2, - "limit": 25, - "total": 138, - "total_pages": 6, - "has_more": true - } -} -``` - -`total` counts every row matching the query, not just the current page. Iterate -until `has_more` is `false`. - -## Bounds - -Out-of-range input is **clamped, never rejected**. A list endpoint does not return -`400` for a bad `page` or `limit`; it coerces the value into range and serves the -request. The `pagination` object echoes the values actually applied, so a client can -always see what it got. - -| Input | Result | -| --- | --- | -| `page` below 1 | `1` | -| `limit` below 1 | `10` (the default) | -| `limit` above the maximum | the maximum | -| unparseable value | the default for that parameter | - -The ceiling exists to stop a single request from turning a paginated query into a -full table scan — a `limit` of a million would otherwise make the database -materialise the whole table, hold a pooled connection open for the duration, and -force the API process to serialise the result. - -### Configuring the maximum - -The ceiling defaults to **100** and is configurable per deployment: - -```toml -[plugins.organizations] -max_page_limit = 250 -``` - -A value of zero or less is treated as unset and falls back to 100. The floor is -applied before the ceiling, so a `max_page_limit` below 10 also caps the default -page size: with `max_page_limit = 5`, a request with no `limit` returns 5 rows. - -## Ordering - -Rows come back newest first, ordered by `created_at DESC, id DESC`. The `id` -tiebreaker keeps paging stable when several rows share a timestamp — without it, a -row can be skipped or repeated across page boundaries. - -Pending-invitation lookups are the deliberate exception: they order oldest first, -because acceptance resolves role conflicts in favour of the earliest invitation. - -## Fetching a whole collection - -When embedding Authula as a Go library, every paginated `ListAll…` method has an -unpaginated `GetAll…` twin that takes no pagination and returns a plain slice: - -```go -page, err := api.ListAllMembers(ctx, actor, orgID, pagination.Params{Page: 1, Limit: 50}) -all, err := api.GetAllMembers(ctx, actor, orgID) -``` - -The `GetAll…` methods are **not** subject to `max_page_limit` — they are the -sanctioned way to ask for a whole collection, and they skip the `SELECT COUNT(*)` -that the paginated path needs, making them a single round-trip. - -They are Go-only and are not exposed over HTTP. An HTTP client that needs more than -`max_page_limit` rows either pages through the results or runs against a deployment -configured with a higher ceiling. diff --git a/plugins/organizations/repositories/pagination.go b/plugins/organizations/repositories/pagination.go index 72e22e47..cf73a77d 100644 --- a/plugins/organizations/repositories/pagination.go +++ b/plugins/organizations/repositories/pagination.go @@ -4,13 +4,6 @@ import ( "github.com/Authula/authula/core/pagination" ) -// pageLimit guards only against a non-positive limit producing a LIMIT 0 query. -// There is deliberately no ceiling here: the maximum page size is configurable per -// deployment and lives in the service layer (ServiceUtils.ClampPagination), which is -// the only layer that can see the plugin config. Capping to a constant here would -// silently override an operator who raised max_page_limit. Every HTTP path reaches -// these repositories through a service that has already clamped; a direct caller is -// a Go embedder, on the same footing as the unpaginated GetAll methods. func pageLimit(limit int) int { if limit <= 0 { return pagination.DefaultLimit diff --git a/plugins/organizations/services/pagination_integration_test.go b/plugins/organizations/services/pagination_integration_test.go index 3cd09600..8c85e331 100644 --- a/plugins/organizations/services/pagination_integration_test.go +++ b/plugins/organizations/services/pagination_integration_test.go @@ -139,8 +139,6 @@ func TestOrganizationService_QuotaSurvivesPagination(t *testing.T) { require.Len(t, seen, limit, "paging must yield every accessible organization exactly once") } -// The unconstrained sibling of ListAllMembers must return the whole collection in -// one call, against real SQL, where the paginated path needs two pages. func TestOrganizationMemberService_GetAllMembersReturnsEverythingInOneCallAgainstSQL(t *testing.T) { t.Parallel() @@ -188,8 +186,6 @@ func TestOrganizationMemberService_GetAllMembersReturnsEverythingInOneCallAgains } } -// The ceiling on a page size must survive all the way into the SQL LIMIT, and the -// configured value — not the constant — must be the one that wins. func TestOrganizationMemberService_ListAllMembersRespectsTheConfiguredMaxPageLimitAgainstSQL(t *testing.T) { t.Parallel() @@ -273,8 +269,6 @@ func TestOrganizationMemberService_ListAllMembersRespectsTheConfiguredMaxPageLim } } -// The unpaginated sibling is the sanctioned escape hatch for whole collections and -// must stay uncapped, however low the configured page-size ceiling is. func TestOrganizationMemberService_GetAllMembersIgnoresTheMaxPageLimitAgainstSQL(t *testing.T) { t.Parallel() diff --git a/plugins/organizations/services/service_utils.go b/plugins/organizations/services/service_utils.go index e9ed0e10..a028f6f3 100644 --- a/plugins/organizations/services/service_utils.go +++ b/plugins/organizations/services/service_utils.go @@ -12,8 +12,6 @@ import ( rootservices "github.com/Authula/authula/services" ) -// ServiceUtils carries the policy every organization service shares: the -// authorization checks below, and the configured ceiling on page sizes. type ServiceUtils struct { orgRepo repositories.OrganizationRepository orgMemberRepo repositories.OrganizationMemberRepository @@ -30,10 +28,6 @@ func NewServiceUtils(orgRepo repositories.OrganizationRepository, orgMemberRepo } } -// ClampPagination bounds caller-supplied pagination against the configured -// maximum page size. A zero or negative maximum falls back to -// pagination.DefaultMaxLimit, so an unconfigured ServiceUtils still yields a -// bounded query. func (s *ServiceUtils) ClampPagination(params pagination.Params) pagination.Params { if s == nil { return pagination.Clamp(params, 0)