diff --git a/core/pagination/pagination.go b/core/pagination/pagination.go index 9a8b449b..cce5e186 100644 --- a/core/pagination/pagination.go +++ b/core/pagination/pagination.go @@ -7,8 +7,9 @@ import ( ) const ( - DefaultPage = 1 - DefaultLimit = 10 + DefaultPage = 1 + DefaultLimit = 10 + DefaultMaxLimit = 100 ) type Params struct { @@ -24,13 +25,19 @@ type Pagination struct { HasMore bool `json:"has_more" required:"true" nullable:"false"` } -func Clamp(params Params) Params { +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/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/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..8c85e331 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", @@ -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() @@ -187,3 +185,116 @@ func TestOrganizationMemberService_GetAllMembersReturnsEverythingInOneCallAgains require.NotEmpty(t, member.User.Email) } } + +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) + }) + } +} + +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..a028f6f3 100644 --- a/plugins/organizations/services/service_utils.go +++ b/plugins/organizations/services/service_utils.go @@ -5,6 +5,7 @@ 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" @@ -15,16 +16,25 @@ 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, } } +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, }