chore: implement sane default pagination limit for audit logs (coder#13676)

Emyrk · web-flow · commit 3cc86cf62da1 · 2024-06-28T07:38:04.000-05:00
* chore: implement sane default pagination limit for audit logs
diff --git a/coderd/audit.go b/coderd/audit.go
@@ -53,8 +53,8 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) {
 		})
 		return
 	}
-	filter.Offset = int32(page.Offset)
-	filter.Limit = int32(page.Limit)
+	filter.OffsetOpt = int32(page.Offset)
+	filter.LimitOpt = int32(page.Limit)
 
 	if filter.Username == "me" {
 		filter.UserID = apiKey.UserID
diff --git a/coderd/audit_test.go b/coderd/audit_test.go
@@ -343,9 +343,6 @@ func TestAuditLogsFilter(t *testing.T) {
 				t.Parallel()
 				auditLogs, err := client.AuditLogs(ctx, codersdk.AuditLogsRequest{
 					SearchQuery: testCase.SearchQuery,
-					Pagination: codersdk.Pagination{
-						Limit: 25,
-					},
 				})
 				if testCase.ExpectedError {
 					require.Error(t, err, "expected error")
diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -263,7 +263,7 @@ func (s *MethodTestSuite) TestAuditLogs() {
 		_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})
 		_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})
 		check.Args(database.GetAuditLogsOffsetParams{
-			Limit: 10,
+			LimitOpt: 10,
 		}).Asserts(rbac.ResourceAuditLog, policy.ActionRead)
 	}))
 }
diff --git a/coderd/database/dbgen/dbgen_test.go b/coderd/database/dbgen/dbgen_test.go
@@ -19,7 +19,7 @@ func TestGenerator(t *testing.T) {
 		t.Parallel()
 		db := dbmem.New()
 		_ = dbgen.AuditLog(t, db, database.AuditLog{})
-		logs := must(db.GetAuditLogsOffset(context.Background(), database.GetAuditLogsOffsetParams{Limit: 1}))
+		logs := must(db.GetAuditLogsOffset(context.Background(), database.GetAuditLogsOffsetParams{LimitOpt: 1}))
 		require.Len(t, logs, 1)
 	})
 
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1965,12 +1965,17 @@ func (q *FakeQuerier) GetAuditLogsOffset(_ context.Context, arg database.GetAudi
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
-	logs := make([]database.GetAuditLogsOffsetRow, 0, arg.Limit)
+	if arg.LimitOpt == 0 {
+		// Default to 100 is set in the SQL query.
+		arg.LimitOpt = 100
+	}
+
+	logs := make([]database.GetAuditLogsOffsetRow, 0, arg.LimitOpt)
 
 	// q.auditLogs are already sorted by time DESC, so no need to sort after the fact.
 	for _, alog := range q.auditLogs {
-		if arg.Offset > 0 {
-			arg.Offset--
+		if arg.OffsetOpt > 0 {
+			arg.OffsetOpt--
 			continue
 		}
 		if arg.OrganizationID != uuid.Nil && arg.OrganizationID != alog.OrganizationID {
@@ -2047,7 +2052,7 @@ func (q *FakeQuerier) GetAuditLogsOffset(_ context.Context, arg database.GetAudi
 			Count:                  0,
 		})
 
-		if len(logs) >= int(arg.Limit) {
+		if len(logs) >= int(arg.LimitOpt) {
 			break
 		}
 	}
diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go
@@ -516,6 +516,29 @@ func TestDefaultOrg(t *testing.T) {
 	require.True(t, all[0].IsDefault, "first org should always be default")
 }
 
+func TestAuditLogDefaultLimit(t *testing.T) {
+	t.Parallel()
+	if testing.Short() {
+		t.SkipNow()
+	}
+
+	sqlDB := testSQLDB(t)
+	err := migrations.Up(sqlDB)
+	require.NoError(t, err)
+	db := database.New(sqlDB)
+
+	for i := 0; i < 110; i++ {
+		dbgen.AuditLog(t, db, database.AuditLog{})
+	}
+
+	ctx := testutil.Context(t, testutil.WaitShort)
+	rows, err := db.GetAuditLogsOffset(ctx, database.GetAuditLogsOffsetParams{})
+	require.NoError(t, err)
+	// The length should match the default limit of the SQL query.
+	// Updating the sql query requires changing the number below to match.
+	require.Len(t, rows, 100)
+}
+
 // TestReadCustomRoles tests the input params returns the correct set of roles.
 func TestReadCustomRoles(t *testing.T) {
 	t.Parallel()
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/auditlogs.sql b/coderd/database/queries/auditlogs.sql
@@ -116,9 +116,12 @@ WHERE
 ORDER BY
     "time" DESC
 LIMIT
-    $1
+	-- a limit of 0 means "no limit". The audit log table is unbounded
+	-- in size, and is expected to be quite large. Implement a default
+	-- limit of 100 to prevent accidental excessively large queries.
+	COALESCE(NULLIF(@limit_opt :: int, 0), 100)
 OFFSET
-    $2;
+    @offset_opt;
 
 -- name: InsertAuditLog :one
 INSERT INTO
diff --git a/coderd/pagination.go b/coderd/pagination.go
@@ -17,8 +17,10 @@ func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Paginat
 	parser := httpapi.NewQueryParamParser()
 	params := codersdk.Pagination{
 		AfterID: parser.UUID(queryParams, uuid.Nil, "after_id"),
-		Limit:   int(parser.PositiveInt32(queryParams, 0, "limit")),
-		Offset:  int(parser.PositiveInt32(queryParams, 0, "offset")),
+		// A limit of 0 should be interpreted by the SQL query as "null" or
+		// "no limit". Do not make this value anything besides 0.
+		Limit:  int(parser.PositiveInt32(queryParams, 0, "limit")),
+		Offset: int(parser.PositiveInt32(queryParams, 0, "offset")),
 	}
 	if len(parser.Errors) > 0 {
 		httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
diff --git a/enterprise/audit/backends/postgres_test.go b/enterprise/audit/backends/postgres_test.go
@@ -30,8 +30,8 @@ func TestPostgresBackend(t *testing.T) {
 		require.NoError(t, err)
 
 		got, err := db.GetAuditLogsOffset(ctx, database.GetAuditLogsOffsetParams{
-			Offset: 0,
-			Limit:  1,
+			OffsetOpt: 0,
+			LimitOpt:  1,
 		})
 		require.NoError(t, err)
 		require.Len(t, got, 1)

Original file line number	Diff line number	Diff line change
`@@ -263,7 +263,7 @@ func (s *MethodTestSuite) TestAuditLogs() {`
`263`	`263`	`_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})`
`264`	`264`	`_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})`
`265`	`265`	`check.Args(database.GetAuditLogsOffsetParams{`
`266`		`- Limit: 10,`
	`266`	`+ LimitOpt: 10,`
`267`	`267`	`}).Asserts(rbac.ResourceAuditLog, policy.ActionRead)`
`268`	`268`	`}))`
`269`	`269`	`}`