From 339f1de14b3f9702e412984a9ccf695f96aca307 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Wed, 25 Jun 2025 14:50:09 +0000 Subject: [PATCH 1/8] Extract count from audit logs query to a separate one and optimize audit logs query with conditional joins --- coderd/audit.go | 33 ++- coderd/database/dbauthz/dbauthz.go | 20 ++ coderd/database/dbauthz/dbauthz_test.go | 10 + coderd/database/dbauthz/setup_test.go | 13 +- coderd/database/dbmem/dbmem.go | 85 +++++- coderd/database/dbmetrics/querymetrics.go | 14 + coderd/database/dbmock/dbmock.go | 30 ++ coderd/database/modelqueries.go | 48 +++- coderd/database/querier.go | 1 + coderd/database/querier_test.go | 58 +++- coderd/database/queries.sql.go | 329 +++++++++++++++------- coderd/database/queries/auditlogs.sql | 296 ++++++++++++------- 12 files changed, 699 insertions(+), 238 deletions(-) diff --git a/coderd/audit.go b/coderd/audit.go index 63b6e49ebb05a..882679f521d5a 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -64,7 +64,21 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { filter.Username = "" } - dblogs, err := api.Database.GetAuditLogsOffset(ctx, filter) + // Use the same filters to count the number of audit logs + count, err := api.Database.CountAuditLogs(ctx, database.CountAuditLogsParams{ + ResourceType: filter.ResourceType, + ResourceID: filter.ResourceID, + OrganizationID: filter.OrganizationID, + ResourceTarget: filter.ResourceTarget, + Action: filter.Action, + UserID: filter.UserID, + Username: filter.Username, + Email: filter.Email, + DateFrom: filter.DateFrom, + DateTo: filter.DateTo, + BuildReason: filter.BuildReason, + RequestID: filter.RequestID, + }) if dbauthz.IsNotAuthorizedError(err) { httpapi.Forbidden(rw) return @@ -73,9 +87,8 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { httpapi.InternalServerError(rw, err) return } - // GetAuditLogsOffset does not return ErrNoRows because it uses a window function to get the count. - // So we need to check if the dblogs is empty and return an empty array if so. - if len(dblogs) == 0 { + // If count is 0, then we don't need to query audit logs + if count == 0 { httpapi.Write(ctx, rw, http.StatusOK, codersdk.AuditLogResponse{ AuditLogs: []codersdk.AuditLog{}, Count: 0, @@ -83,9 +96,19 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { return } + dblogs, err := api.Database.GetAuditLogsOffset(ctx, filter) + if dbauthz.IsNotAuthorizedError(err) { + httpapi.Forbidden(rw) + return + } + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + httpapi.Write(ctx, rw, http.StatusOK, codersdk.AuditLogResponse{ AuditLogs: api.convertAuditLogs(ctx, dblogs), - Count: dblogs[0].Count, + Count: count, }) } diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index d63e049abf8ee..a2c3b1d5705da 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1301,6 +1301,22 @@ func (q *querier) CleanTailnetTunnels(ctx context.Context) error { return q.db.CleanTailnetTunnels(ctx) } +func (q *querier) CountAuditLogs(ctx context.Context, arg database.CountAuditLogsParams) (int64, error) { + // Shortcut if the user is an owner. The SQL filter is noticeable, + // and this is an easy win for owners. Which is the common case. + err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceAuditLog) + if err == nil { + return q.db.CountAuditLogs(ctx, arg) + } + + prep, err := prepareSQLFilter(ctx, q.auth, policy.ActionRead, rbac.ResourceAuditLog.Type) + if err != nil { + return 0, xerrors.Errorf("(dev error) prepare sql filter: %w", err) + } + + return q.db.CountAuthorizedAuditLogs(ctx, arg, prep) +} + func (q *querier) CountInProgressPrebuilds(ctx context.Context) ([]database.CountInProgressPrebuildsRow, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspace.All()); err != nil { return nil, err @@ -5256,3 +5272,7 @@ func (q *querier) GetAuthorizedUsers(ctx context.Context, arg database.GetUsersP func (q *querier) GetAuthorizedAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams, _ rbac.PreparedAuthorized) ([]database.GetAuditLogsOffsetRow, error) { return q.GetAuditLogsOffset(ctx, arg) } + +func (q *querier) CountAuthorizedAuditLogs(ctx context.Context, arg database.CountAuditLogsParams, _ rbac.PreparedAuthorized) (int64, error) { + return q.CountAuditLogs(ctx, arg) +} diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 0361bf796cb55..1ab0dfd7c4a36 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -327,6 +327,16 @@ func (s *MethodTestSuite) TestAuditLogs() { LimitOpt: 10, }, emptyPreparedAuthorized{}).Asserts(rbac.ResourceAuditLog, policy.ActionRead) })) + s.Run("CountAuditLogs", s.Subtest(func(db database.Store, check *expects) { + _ = dbgen.AuditLog(s.T(), db, database.AuditLog{}) + _ = dbgen.AuditLog(s.T(), db, database.AuditLog{}) + check.Args(database.CountAuditLogsParams{}).Asserts(rbac.ResourceAuditLog, policy.ActionRead).WithNotAuthorized("nil") + })) + s.Run("CountAuthorizedAuditLogs", s.Subtest(func(db database.Store, check *expects) { + _ = dbgen.AuditLog(s.T(), db, database.AuditLog{}) + _ = dbgen.AuditLog(s.T(), db, database.AuditLog{}) + check.Args(database.CountAuditLogsParams{}, emptyPreparedAuthorized{}).Asserts(rbac.ResourceAuditLog, policy.ActionRead) + })) } func (s *MethodTestSuite) TestFile() { diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go index 29ca421d6f11e..5e12f53963f04 100644 --- a/coderd/database/dbauthz/setup_test.go +++ b/coderd/database/dbauthz/setup_test.go @@ -271,7 +271,7 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd // This is unfortunate, but if we are using `Filter` the error returned will be nil. So filter out // any case where the error is nil and the response is an empty slice. - if err != nil || !hasEmptySliceResponse(resp) { + if err != nil || !hasEmptyResponse(resp) { // Expect the default error if testCase.notAuthorizedExpect == "" { s.ErrorContainsf(err, "unauthorized", "error string should have a good message") @@ -297,7 +297,7 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd // This is unfortunate, but if we are using `Filter` the error returned will be nil. So filter out // any case where the error is nil and the response is an empty slice. - if err != nil || !hasEmptySliceResponse(resp) { + if err != nil || !hasEmptyResponse(resp) { if testCase.cancelledCtxExpect == "" { s.Errorf(err, "method should an error with cancellation") s.ErrorIsf(err, context.Canceled, "error should match context.Canceled") @@ -308,13 +308,20 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd }) } -func hasEmptySliceResponse(values []reflect.Value) bool { +func hasEmptyResponse(values []reflect.Value) bool { for _, r := range values { if r.Kind() == reflect.Slice || r.Kind() == reflect.Array { if r.Len() == 0 { return true } } + + // Special case for int64, as it's the return type for count query. + if r.Kind() == reflect.Int64 { + if r.Int() == 0 { + return true + } + } } return false } diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index e5604d440073b..417425a249dd7 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1779,6 +1779,10 @@ func (*FakeQuerier) CleanTailnetTunnels(context.Context) error { return ErrUnimplemented } +func (q *FakeQuerier) CountAuditLogs(ctx context.Context, arg database.CountAuditLogsParams) (int64, error) { + return q.CountAuthorizedAuditLogs(ctx, arg, nil) +} + func (q *FakeQuerier) CountInProgressPrebuilds(ctx context.Context) ([]database.CountInProgressPrebuildsRow, error) { return nil, ErrUnimplemented } @@ -13930,7 +13934,6 @@ func (q *FakeQuerier) GetAuthorizedAuditLogsOffset(ctx context.Context, arg data UserQuietHoursSchedule: sql.NullString{String: user.QuietHoursSchedule, Valid: userValid}, UserStatus: database.NullUserStatus{UserStatus: user.Status, Valid: userValid}, UserRoles: user.RBACRoles, - Count: 0, }) if len(logs) >= int(arg.LimitOpt) { @@ -13938,10 +13941,82 @@ func (q *FakeQuerier) GetAuthorizedAuditLogsOffset(ctx context.Context, arg data } } - count := int64(len(logs)) - for i := range logs { - logs[i].Count = count + return logs, nil +} + +func (q *FakeQuerier) CountAuthorizedAuditLogs(ctx context.Context, arg database.CountAuditLogsParams, prepared rbac.PreparedAuthorized) (int64, error) { + if err := validateDatabaseType(arg); err != nil { + return 0, err } - return logs, nil + // Call this to match the same function calls as the SQL implementation. + // It functionally does nothing for filtering. + if prepared != nil { + _, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{ + VariableConverter: regosql.AuditLogConverter(), + }) + if err != nil { + return 0, err + } + } + + q.mutex.RLock() + defer q.mutex.RUnlock() + + var count int64 + + // q.auditLogs are already sorted by time DESC, so no need to sort after the fact. + for _, alog := range q.auditLogs { + if arg.RequestID != uuid.Nil && arg.RequestID != alog.RequestID { + continue + } + if arg.OrganizationID != uuid.Nil && arg.OrganizationID != alog.OrganizationID { + continue + } + if arg.Action != "" && string(alog.Action) != arg.Action { + continue + } + if arg.ResourceType != "" && !strings.Contains(string(alog.ResourceType), arg.ResourceType) { + continue + } + if arg.ResourceID != uuid.Nil && alog.ResourceID != arg.ResourceID { + continue + } + if arg.Username != "" { + user, err := q.getUserByIDNoLock(alog.UserID) + if err == nil && !strings.EqualFold(arg.Username, user.Username) { + continue + } + } + if arg.Email != "" { + user, err := q.getUserByIDNoLock(alog.UserID) + if err == nil && !strings.EqualFold(arg.Email, user.Email) { + continue + } + } + if !arg.DateFrom.IsZero() { + if alog.Time.Before(arg.DateFrom) { + continue + } + } + if !arg.DateTo.IsZero() { + if alog.Time.After(arg.DateTo) { + continue + } + } + if arg.BuildReason != "" { + workspaceBuild, err := q.getWorkspaceBuildByIDNoLock(context.Background(), alog.ResourceID) + if err == nil && !strings.EqualFold(arg.BuildReason, string(workspaceBuild.Reason)) { + continue + } + } + // If the filter exists, ensure the object is authorized. + if prepared != nil && prepared.Authorize(ctx, alog.RBACObject()) != nil { + continue + } + + count++ + } + + return count, nil } diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 0d68d0c15e1be..ca2b0c2ce7fa5 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -186,6 +186,13 @@ func (m queryMetricsStore) CleanTailnetTunnels(ctx context.Context) error { return r0 } +func (m queryMetricsStore) CountAuditLogs(ctx context.Context, arg database.CountAuditLogsParams) (int64, error) { + start := time.Now() + r0, r1 := m.s.CountAuditLogs(ctx, arg) + m.queryLatencies.WithLabelValues("CountAuditLogs").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m queryMetricsStore) CountInProgressPrebuilds(ctx context.Context) ([]database.CountInProgressPrebuildsRow, error) { start := time.Now() r0, r1 := m.s.CountInProgressPrebuilds(ctx) @@ -3321,3 +3328,10 @@ func (m queryMetricsStore) GetAuthorizedAuditLogsOffset(ctx context.Context, arg m.queryLatencies.WithLabelValues("GetAuthorizedAuditLogsOffset").Observe(time.Since(start).Seconds()) return r0, r1 } + +func (m queryMetricsStore) CountAuthorizedAuditLogs(ctx context.Context, arg database.CountAuditLogsParams, prepared rbac.PreparedAuthorized) (int64, error) { + start := time.Now() + r0, r1 := m.s.CountAuthorizedAuditLogs(ctx, arg, prepared) + m.queryLatencies.WithLabelValues("CountAuthorizedAuditLogs").Observe(time.Since(start).Seconds()) + return r0, r1 +} diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 03222782a5d68..9d7d6c74cb0ce 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -247,6 +247,36 @@ func (mr *MockStoreMockRecorder) CleanTailnetTunnels(ctx any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CleanTailnetTunnels", reflect.TypeOf((*MockStore)(nil).CleanTailnetTunnels), ctx) } +// CountAuditLogs mocks base method. +func (m *MockStore) CountAuditLogs(ctx context.Context, arg database.CountAuditLogsParams) (int64, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CountAuditLogs", ctx, arg) + ret0, _ := ret[0].(int64) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CountAuditLogs indicates an expected call of CountAuditLogs. +func (mr *MockStoreMockRecorder) CountAuditLogs(ctx, arg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CountAuditLogs", reflect.TypeOf((*MockStore)(nil).CountAuditLogs), ctx, arg) +} + +// CountAuthorizedAuditLogs mocks base method. +func (m *MockStore) CountAuthorizedAuditLogs(ctx context.Context, arg database.CountAuditLogsParams, prepared rbac.PreparedAuthorized) (int64, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CountAuthorizedAuditLogs", ctx, arg, prepared) + ret0, _ := ret[0].(int64) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CountAuthorizedAuditLogs indicates an expected call of CountAuthorizedAuditLogs. +func (mr *MockStoreMockRecorder) CountAuthorizedAuditLogs(ctx, arg, prepared any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CountAuthorizedAuditLogs", reflect.TypeOf((*MockStore)(nil).CountAuthorizedAuditLogs), ctx, arg, prepared) +} + // CountInProgressPrebuilds mocks base method. func (m *MockStore) CountInProgressPrebuilds(ctx context.Context) ([]database.CountInProgressPrebuildsRow, error) { m.ctrl.T.Helper() diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index eaf73af07f6d5..158d66e49784a 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -478,6 +478,7 @@ func (q *sqlQuerier) GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, type auditLogQuerier interface { GetAuthorizedAuditLogsOffset(ctx context.Context, arg GetAuditLogsOffsetParams, prepared rbac.PreparedAuthorized) ([]GetAuditLogsOffsetRow, error) + CountAuthorizedAuditLogs(ctx context.Context, arg CountAuditLogsParams, prepared rbac.PreparedAuthorized) (int64, error) } func (q *sqlQuerier) GetAuthorizedAuditLogsOffset(ctx context.Context, arg GetAuditLogsOffsetParams, prepared rbac.PreparedAuthorized) ([]GetAuditLogsOffsetRow, error) { @@ -548,7 +549,6 @@ func (q *sqlQuerier) GetAuthorizedAuditLogsOffset(ctx context.Context, arg GetAu &i.OrganizationName, &i.OrganizationDisplayName, &i.OrganizationIcon, - &i.Count, ); err != nil { return nil, err } @@ -563,6 +563,52 @@ func (q *sqlQuerier) GetAuthorizedAuditLogsOffset(ctx context.Context, arg GetAu return items, nil } +func (q *sqlQuerier) CountAuthorizedAuditLogs(ctx context.Context, arg CountAuditLogsParams, prepared rbac.PreparedAuthorized) (int64, error) { + authorizedFilter, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{ + VariableConverter: regosql.AuditLogConverter(), + }) + if err != nil { + return 0, xerrors.Errorf("compile authorized filter: %w", err) + } + + filtered, err := insertAuthorizedFilter(countAuditLogs, fmt.Sprintf(" AND %s", authorizedFilter)) + if err != nil { + return 0, xerrors.Errorf("insert authorized filter: %w", err) + } + + query := fmt.Sprintf("-- name: CountAuthorizedAuditLogs :one\n%s", filtered) + + rows, err := q.db.QueryContext(ctx, query, + arg.ResourceType, + arg.ResourceID, + arg.OrganizationID, + arg.ResourceTarget, + arg.Action, + arg.UserID, + arg.Username, + arg.Email, + arg.DateFrom, + arg.DateTo, + arg.BuildReason, + arg.RequestID, + ) + if err != nil { + return 0, err + } + defer rows.Close() + var count int64 + for rows.Next() { + count++ + } + if err := rows.Close(); err != nil { + return 0, err + } + if err := rows.Err(); err != nil { + return 0, err + } + return count, nil +} + func insertAuthorizedFilter(query string, replaceWith string) (string, error) { if !strings.Contains(query, authorizedQueryPlaceholder) { return "", xerrors.Errorf("query does not contain authorized replace string, this is not an authorized query") diff --git a/coderd/database/querier.go b/coderd/database/querier.go index b1c13d31ceb6d..4d5052b42aadc 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -64,6 +64,7 @@ type sqlcQuerier interface { CleanTailnetCoordinators(ctx context.Context) error CleanTailnetLostPeers(ctx context.Context) error CleanTailnetTunnels(ctx context.Context) error + CountAuditLogs(ctx context.Context, arg CountAuditLogsParams) (int64, error) // CountInProgressPrebuilds returns the number of in-progress prebuilds, grouped by preset ID and transition. // Prebuild considered in-progress if it's in the "starting", "stopping", or "deleting" state. CountInProgressPrebuilds(ctx context.Context) ([]CountInProgressPrebuildsRow, error) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 5ae43138bd634..c105ceb34872f 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1567,6 +1567,26 @@ func TestAuditLogDefaultLimit(t *testing.T) { require.Len(t, rows, 100) } +func TestAuditLogCount(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) + + ctx := testutil.Context(t, testutil.WaitLong) + + dbgen.AuditLog(t, db, database.AuditLog{}) + + count, err := db.CountAuditLogs(ctx, database.CountAuditLogsParams{}) + require.NoError(t, err) + require.Equal(t, int64(1), count) +} + func TestWorkspaceQuotas(t *testing.T) { t.Parallel() orgMemberIDs := func(o database.OrganizationMember) uuid.UUID { @@ -1947,9 +1967,13 @@ func TestAuthorizedAuditLogs(t *testing.T) { }) // When: The user queries for audit logs + count, err := db.CountAuditLogs(memberCtx, database.CountAuditLogsParams{}) + require.NoError(t, err) logs, err := db.GetAuditLogsOffset(memberCtx, database.GetAuditLogsOffsetParams{}) require.NoError(t, err) - // Then: No logs returned + + // Then: No logs returned and count is 0 + require.Equal(t, int64(0), count, "count should be 0") require.Len(t, logs, 0, "no logs should be returned") }) @@ -1965,10 +1989,14 @@ func TestAuthorizedAuditLogs(t *testing.T) { }) // When: the auditor queries for audit logs + count, err := db.CountAuditLogs(siteAuditorCtx, database.CountAuditLogsParams{}) + require.NoError(t, err) logs, err := db.GetAuditLogsOffset(siteAuditorCtx, database.GetAuditLogsOffsetParams{}) require.NoError(t, err) - // Then: All logs are returned - require.ElementsMatch(t, auditOnlyIDs(allLogs), auditOnlyIDs(logs)) + + // Then: All logs are returned and count matches + require.Equal(t, int64(len(allLogs)), count, "count should match total number of logs") + require.ElementsMatch(t, auditOnlyIDs(allLogs), auditOnlyIDs(logs), "all logs should be returned") }) t.Run("SingleOrgAuditor", func(t *testing.T) { @@ -1984,10 +2012,14 @@ func TestAuthorizedAuditLogs(t *testing.T) { }) // When: The auditor queries for audit logs + count, err := db.CountAuditLogs(orgAuditCtx, database.CountAuditLogsParams{}) + require.NoError(t, err) logs, err := db.GetAuditLogsOffset(orgAuditCtx, database.GetAuditLogsOffsetParams{}) require.NoError(t, err) - // Then: Only the logs for the organization are returned - require.ElementsMatch(t, orgAuditLogs[orgID], auditOnlyIDs(logs)) + + // Then: Only the logs for the organization are returned and count matches + require.Equal(t, int64(len(orgAuditLogs[orgID])), count, "count should match organization logs") + require.ElementsMatch(t, orgAuditLogs[orgID], auditOnlyIDs(logs), "only organization logs should be returned") }) t.Run("TwoOrgAuditors", func(t *testing.T) { @@ -2004,10 +2036,16 @@ func TestAuthorizedAuditLogs(t *testing.T) { }) // When: The user queries for audit logs + count, err := db.CountAuditLogs(multiOrgAuditCtx, database.CountAuditLogsParams{}) + require.NoError(t, err) logs, err := db.GetAuditLogsOffset(multiOrgAuditCtx, database.GetAuditLogsOffsetParams{}) require.NoError(t, err) - // Then: All logs for both organizations are returned - require.ElementsMatch(t, append(orgAuditLogs[first], orgAuditLogs[second]...), auditOnlyIDs(logs)) + + // Then: All logs for both organizations are returned and count matches + expectedLogs := append(orgAuditLogs[first], orgAuditLogs[second]...) + expectedCount := int64(len(expectedLogs)) + require.Equal(t, expectedCount, count, "count should match sum of both organizations") + require.ElementsMatch(t, expectedLogs, auditOnlyIDs(logs), "logs from both organizations should be returned") }) t.Run("ErroneousOrg", func(t *testing.T) { @@ -2022,9 +2060,13 @@ func TestAuthorizedAuditLogs(t *testing.T) { }) // When: The user queries for audit logs + count, err := db.CountAuditLogs(userCtx, database.CountAuditLogsParams{}) + require.NoError(t, err) logs, err := db.GetAuditLogsOffset(userCtx, database.GetAuditLogsOffsetParams{}) require.NoError(t, err) - // Then: No logs are returned + + // Then: No logs are returned and count is 0 + require.Equal(t, int64(0), count, "count should be 0") require.Len(t, logs, 0, "no logs should be returned") }) } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 733b42db7a461..0edfc90b19659 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -441,128 +441,247 @@ func (q *sqlQuerier) UpdateAPIKeyByID(ctx context.Context, arg UpdateAPIKeyByIDP return err } -const getAuditLogsOffset = `-- name: GetAuditLogsOffset :many -SELECT - audit_logs.id, audit_logs.time, audit_logs.user_id, audit_logs.organization_id, audit_logs.ip, audit_logs.user_agent, audit_logs.resource_type, audit_logs.resource_id, audit_logs.resource_target, audit_logs.action, audit_logs.diff, audit_logs.status_code, audit_logs.additional_fields, audit_logs.request_id, audit_logs.resource_icon, - -- sqlc.embed(users) would be nice but it does not seem to play well with - -- left joins. - users.username AS user_username, - users.name AS user_name, - users.email AS user_email, - users.created_at AS user_created_at, - users.updated_at AS user_updated_at, - users.last_seen_at AS user_last_seen_at, - users.status AS user_status, - users.login_type AS user_login_type, - users.rbac_roles AS user_roles, - users.avatar_url AS user_avatar_url, - users.deleted AS user_deleted, - users.quiet_hours_schedule AS user_quiet_hours_schedule, - COALESCE(organizations.name, '') AS organization_name, - COALESCE(organizations.display_name, '') AS organization_display_name, - COALESCE(organizations.icon, '') AS organization_icon, - COUNT(audit_logs.*) OVER () AS count -FROM - audit_logs - LEFT JOIN users ON audit_logs.user_id = users.id - LEFT JOIN - -- First join on workspaces to get the initial workspace create - -- to workspace build 1 id. This is because the first create is - -- is a different audit log than subsequent starts. - workspaces ON - audit_logs.resource_type = 'workspace' AND - audit_logs.resource_id = workspaces.id - LEFT JOIN - workspace_builds ON - -- Get the reason from the build if the resource type - -- is a workspace_build - ( - audit_logs.resource_type = 'workspace_build' - AND audit_logs.resource_id = workspace_builds.id - ) - OR - -- Get the reason from the build #1 if this is the first - -- workspace create. - ( - audit_logs.resource_type = 'workspace' AND - audit_logs.action = 'create' AND - workspaces.id = workspace_builds.workspace_id AND - workspace_builds.build_number = 1 - ) - LEFT JOIN organizations ON audit_logs.organization_id = organizations.id +const countAuditLogs = `-- name: CountAuditLogs :one +SELECT COUNT(*) +FROM audit_logs + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + -- First join on workspaces to get the initial workspace create + -- to workspace build 1 id. This is because the first create is + -- is a different audit log than subsequent starts. + LEFT JOIN workspaces ON + audit_logs.resource_type = 'workspace' AND + audit_logs.resource_id = workspaces.id + -- Get the reason from the build if the resource type + -- is a workspace_build + LEFT JOIN workspace_builds wb_build ON + audit_logs.resource_type = 'workspace_build' AND + audit_logs.resource_id = wb_build.id + -- Get the reason from the build #1 if this is the first + -- workspace create. + LEFT JOIN workspace_builds wb_workspace ON + audit_logs.resource_type = 'workspace' AND + audit_logs.action = 'create' AND + workspaces.id = wb_workspace.workspace_id AND + wb_workspace.build_number = 1 WHERE - -- Filter resource_type + -- Filter resource_type CASE - WHEN $1 :: text != '' THEN - resource_type = $1 :: resource_type + WHEN $1 :: text != '' THEN resource_type = $1 :: resource_type ELSE true + END + -- Filter resource_id + AND CASE + WHEN $2 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = $2 + ELSE true END - -- Filter resource_id - AND CASE - WHEN $2 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - resource_id = $2 - ELSE true + -- Filter organization_id + AND CASE + WHEN $3 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = $3 + ELSE true END - -- Filter organization_id - AND CASE - WHEN $3 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - audit_logs.organization_id = $3 - ELSE true + -- Filter by resource_target + AND CASE + WHEN $4 :: text != '' THEN resource_target = $4 + ELSE true END - -- Filter by resource_target - AND CASE - WHEN $4 :: text != '' THEN - resource_target = $4 + -- Filter action + AND CASE + WHEN $5 :: text != '' THEN + action = $5 :: audit_action + ELSE true +END + -- Filter by user_id +AND CASE + WHEN $6 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + user_id = $6 ELSE true - END - -- Filter action - AND CASE - WHEN $5 :: text != '' THEN - action = $5 :: audit_action +END + -- Filter by username +AND CASE + WHEN $7 :: text != '' THEN + user_id = (SELECT id FROM users WHERE lower(username) = lower($7) AND deleted = false) ELSE true +END + -- Filter by user_email +AND CASE + WHEN $8 :: text != '' THEN + users.email = $8 + ELSE true +END + -- Filter by date_from +AND CASE + WHEN $9 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + "time" >= $9 + ELSE true +END + -- Filter by date_to +AND CASE + WHEN $10 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + "time" <= $10 + ELSE true +END + -- Filter by build_reason +AND CASE + WHEN $11::text != '' THEN + COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 + ELSE true +END + -- Filter request_id +AND CASE + WHEN $12 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + audit_logs.request_id = $12 + ELSE true +END + + -- Authorize Filter clause will be injected below in CountAuthorizedAuditLogs + -- @authorize_filter +` + +type CountAuditLogsParams struct { + ResourceType string `db:"resource_type" json:"resource_type"` + ResourceID uuid.UUID `db:"resource_id" json:"resource_id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + ResourceTarget string `db:"resource_target" json:"resource_target"` + Action string `db:"action" json:"action"` + UserID uuid.UUID `db:"user_id" json:"user_id"` + Username string `db:"username" json:"username"` + Email string `db:"email" json:"email"` + DateFrom time.Time `db:"date_from" json:"date_from"` + DateTo time.Time `db:"date_to" json:"date_to"` + BuildReason string `db:"build_reason" json:"build_reason"` + RequestID uuid.UUID `db:"request_id" json:"request_id"` +} + +func (q *sqlQuerier) CountAuditLogs(ctx context.Context, arg CountAuditLogsParams) (int64, error) { + row := q.db.QueryRowContext(ctx, countAuditLogs, + arg.ResourceType, + arg.ResourceID, + arg.OrganizationID, + arg.ResourceTarget, + arg.Action, + arg.UserID, + arg.Username, + arg.Email, + arg.DateFrom, + arg.DateTo, + arg.BuildReason, + arg.RequestID, + ) + var count int64 + err := row.Scan(&count) + return count, err +} + +const getAuditLogsOffset = `-- name: GetAuditLogsOffset :many +SELECT audit_logs.id, audit_logs.time, audit_logs.user_id, audit_logs.organization_id, audit_logs.ip, audit_logs.user_agent, audit_logs.resource_type, audit_logs.resource_id, audit_logs.resource_target, audit_logs.action, audit_logs.diff, audit_logs.status_code, audit_logs.additional_fields, audit_logs.request_id, audit_logs.resource_icon, + -- sqlc.embed(users) would be nice but it does not seem to play well with + -- left joins. + users.username AS user_username, + users.name AS user_name, + users.email AS user_email, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.last_seen_at AS user_last_seen_at, + users.status AS user_status, + users.login_type AS user_login_type, + users.rbac_roles AS user_roles, + users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + COALESCE(organizations.name, '') AS organization_name, + COALESCE(organizations.display_name, '') AS organization_display_name, + COALESCE(organizations.icon, '') AS organization_icon +FROM audit_logs + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + -- First join on workspaces to get the initial workspace create + -- to workspace build 1 id. This is because the first create is + -- is a different audit log than subsequent starts. + LEFT JOIN workspaces ON + audit_logs.resource_type = 'workspace' AND + audit_logs.resource_id = workspaces.id + -- Get the reason from the build if the resource type + -- is a workspace_build + LEFT JOIN workspace_builds wb_build ON + audit_logs.resource_type = 'workspace_build' AND + audit_logs.resource_id = wb_build.id + -- Get the reason from the build #1 if this is the first + -- workspace create. + LEFT JOIN workspace_builds wb_workspace ON + audit_logs.resource_type = 'workspace' AND + audit_logs.action = 'create' AND + workspaces.id = wb_workspace.workspace_id AND + wb_workspace.build_number = 1 +WHERE + -- Filter resource_type + CASE + WHEN $1 :: text != '' THEN resource_type = $1 :: resource_type + ELSE true + END + -- Filter resource_id + AND CASE + WHEN $2 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = $2 + ELSE true + END + -- Filter organization_id + AND CASE + WHEN $3 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = $3 + ELSE true END + -- Filter by resource_target + AND CASE + WHEN $4 :: text != '' THEN resource_target = $4 + ELSE true + END + -- Filter action + AND CASE + WHEN $5 :: text != '' THEN + action = $5 :: audit_action + ELSE true +END -- Filter by user_id - AND CASE +AND CASE WHEN $6 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN user_id = $6 ELSE true - END +END -- Filter by username - AND CASE +AND CASE WHEN $7 :: text != '' THEN user_id = (SELECT id FROM users WHERE lower(username) = lower($7) AND deleted = false) ELSE true - END +END -- Filter by user_email - AND CASE +AND CASE WHEN $8 :: text != '' THEN users.email = $8 ELSE true - END +END -- Filter by date_from - AND CASE +AND CASE WHEN $9 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" >= $9 ELSE true - END +END -- Filter by date_to - AND CASE +AND CASE WHEN $10 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" <= $10 ELSE true - END +END -- Filter by build_reason - AND CASE +AND CASE WHEN $11::text != '' THEN - workspace_builds.reason::text = $11 + COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 ELSE true - END +END -- Filter request_id - AND CASE +AND CASE WHEN $12 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.request_id = $12 ELSE true - END +END -- Authorize Filter clause will be injected below in GetAuthorizedAuditLogsOffset -- @authorize_filter @@ -611,7 +730,6 @@ type GetAuditLogsOffsetRow struct { OrganizationName string `db:"organization_name" json:"organization_name"` OrganizationDisplayName string `db:"organization_display_name" json:"organization_display_name"` OrganizationIcon string `db:"organization_icon" json:"organization_icon"` - Count int64 `db:"count" json:"count"` } // GetAuditLogsBefore retrieves `row_limit` number of audit logs before the provided @@ -671,7 +789,6 @@ func (q *sqlQuerier) GetAuditLogsOffset(ctx context.Context, arg GetAuditLogsOff &i.OrganizationName, &i.OrganizationDisplayName, &i.OrganizationIcon, - &i.Count, ); err != nil { return nil, err } @@ -687,26 +804,22 @@ func (q *sqlQuerier) GetAuditLogsOffset(ctx context.Context, arg GetAuditLogsOff } const insertAuditLog = `-- name: InsertAuditLog :one -INSERT INTO - audit_logs ( - id, - "time", - user_id, - organization_id, - ip, - user_agent, - resource_type, - resource_id, - resource_target, - action, - diff, - status_code, - additional_fields, - request_id, - resource_icon - ) -VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING id, time, user_id, organization_id, ip, user_agent, resource_type, resource_id, resource_target, action, diff, status_code, additional_fields, request_id, resource_icon +INSERT INTO audit_logs (id, + "time", + user_id, + organization_id, + ip, + user_agent, + resource_type, + resource_id, + resource_target, + action, + diff, + status_code, + additional_fields, + request_id, + resource_icon) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING id, time, user_id, organization_id, ip, user_agent, resource_type, resource_id, resource_target, action, diff, status_code, additional_fields, request_id, resource_icon ` type InsertAuditLogParams struct { diff --git a/coderd/database/queries/auditlogs.sql b/coderd/database/queries/auditlogs.sql index 9016908a75feb..86fc0f86b9577 100644 --- a/coderd/database/queries/auditlogs.sql +++ b/coderd/database/queries/auditlogs.sql @@ -1,127 +1,114 @@ -- GetAuditLogsBefore retrieves `row_limit` number of audit logs before the provided -- ID. -- name: GetAuditLogsOffset :many -SELECT - sqlc.embed(audit_logs), - -- sqlc.embed(users) would be nice but it does not seem to play well with - -- left joins. - users.username AS user_username, - users.name AS user_name, - users.email AS user_email, - users.created_at AS user_created_at, - users.updated_at AS user_updated_at, - users.last_seen_at AS user_last_seen_at, - users.status AS user_status, - users.login_type AS user_login_type, - users.rbac_roles AS user_roles, - users.avatar_url AS user_avatar_url, - users.deleted AS user_deleted, - users.quiet_hours_schedule AS user_quiet_hours_schedule, - COALESCE(organizations.name, '') AS organization_name, - COALESCE(organizations.display_name, '') AS organization_display_name, - COALESCE(organizations.icon, '') AS organization_icon, - COUNT(audit_logs.*) OVER () AS count -FROM - audit_logs - LEFT JOIN users ON audit_logs.user_id = users.id - LEFT JOIN - -- First join on workspaces to get the initial workspace create - -- to workspace build 1 id. This is because the first create is - -- is a different audit log than subsequent starts. - workspaces ON - audit_logs.resource_type = 'workspace' AND - audit_logs.resource_id = workspaces.id - LEFT JOIN - workspace_builds ON - -- Get the reason from the build if the resource type - -- is a workspace_build - ( - audit_logs.resource_type = 'workspace_build' - AND audit_logs.resource_id = workspace_builds.id - ) - OR - -- Get the reason from the build #1 if this is the first - -- workspace create. - ( - audit_logs.resource_type = 'workspace' AND - audit_logs.action = 'create' AND - workspaces.id = workspace_builds.workspace_id AND - workspace_builds.build_number = 1 - ) - LEFT JOIN organizations ON audit_logs.organization_id = organizations.id +SELECT sqlc.embed(audit_logs), + -- sqlc.embed(users) would be nice but it does not seem to play well with + -- left joins. + users.username AS user_username, + users.name AS user_name, + users.email AS user_email, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.last_seen_at AS user_last_seen_at, + users.status AS user_status, + users.login_type AS user_login_type, + users.rbac_roles AS user_roles, + users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + COALESCE(organizations.name, '') AS organization_name, + COALESCE(organizations.display_name, '') AS organization_display_name, + COALESCE(organizations.icon, '') AS organization_icon +FROM audit_logs + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + -- First join on workspaces to get the initial workspace create + -- to workspace build 1 id. This is because the first create is + -- is a different audit log than subsequent starts. + LEFT JOIN workspaces ON + audit_logs.resource_type = 'workspace' AND + audit_logs.resource_id = workspaces.id + -- Get the reason from the build if the resource type + -- is a workspace_build + LEFT JOIN workspace_builds wb_build ON + audit_logs.resource_type = 'workspace_build' AND + audit_logs.resource_id = wb_build.id + -- Get the reason from the build #1 if this is the first + -- workspace create. + LEFT JOIN workspace_builds wb_workspace ON + audit_logs.resource_type = 'workspace' AND + audit_logs.action = 'create' AND + workspaces.id = wb_workspace.workspace_id AND + wb_workspace.build_number = 1 WHERE - -- Filter resource_type + -- Filter resource_type CASE - WHEN @resource_type :: text != '' THEN - resource_type = @resource_type :: resource_type + WHEN @resource_type :: text != '' THEN resource_type = @resource_type :: resource_type ELSE true + END + -- Filter resource_id + AND CASE + WHEN @resource_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = @resource_id + ELSE true END - -- Filter resource_id - AND CASE - WHEN @resource_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - resource_id = @resource_id - ELSE true - END - -- Filter organization_id - AND CASE - WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - audit_logs.organization_id = @organization_id - ELSE true + -- Filter organization_id + AND CASE + WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = @organization_id + ELSE true END - -- Filter by resource_target - AND CASE - WHEN @resource_target :: text != '' THEN - resource_target = @resource_target - ELSE true - END - -- Filter action - AND CASE - WHEN @action :: text != '' THEN - action = @action :: audit_action - ELSE true + -- Filter by resource_target + AND CASE + WHEN @resource_target :: text != '' THEN resource_target = @resource_target + ELSE true END + -- Filter action + AND CASE + WHEN @action :: text != '' THEN + action = @action :: audit_action + ELSE true +END -- Filter by user_id - AND CASE +AND CASE WHEN @user_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN user_id = @user_id ELSE true - END +END -- Filter by username - AND CASE +AND CASE WHEN @username :: text != '' THEN user_id = (SELECT id FROM users WHERE lower(username) = lower(@username) AND deleted = false) ELSE true - END +END -- Filter by user_email - AND CASE +AND CASE WHEN @email :: text != '' THEN users.email = @email ELSE true - END +END -- Filter by date_from - AND CASE +AND CASE WHEN @date_from :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" >= @date_from ELSE true - END +END -- Filter by date_to - AND CASE +AND CASE WHEN @date_to :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" <= @date_to ELSE true - END +END -- Filter by build_reason - AND CASE +AND CASE WHEN @build_reason::text != '' THEN - workspace_builds.reason::text = @build_reason + COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason ELSE true - END +END -- Filter request_id - AND CASE +AND CASE WHEN @request_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.request_id = @request_id ELSE true - END +END -- Authorize Filter clause will be injected below in GetAuthorizedAuditLogsOffset -- @authorize_filter @@ -136,23 +123,116 @@ OFFSET @offset_opt; -- name: InsertAuditLog :one -INSERT INTO - audit_logs ( - id, - "time", - user_id, - organization_id, - ip, - user_agent, - resource_type, - resource_id, - resource_target, - action, - diff, - status_code, - additional_fields, - request_id, - resource_icon - ) -VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING *; +INSERT INTO audit_logs (id, + "time", + user_id, + organization_id, + ip, + user_agent, + resource_type, + resource_id, + resource_target, + action, + diff, + status_code, + additional_fields, + request_id, + resource_icon) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING *; + +-- name: CountAuditLogs :one +SELECT COUNT(*) +FROM audit_logs + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + -- First join on workspaces to get the initial workspace create + -- to workspace build 1 id. This is because the first create is + -- is a different audit log than subsequent starts. + LEFT JOIN workspaces ON + audit_logs.resource_type = 'workspace' AND + audit_logs.resource_id = workspaces.id + -- Get the reason from the build if the resource type + -- is a workspace_build + LEFT JOIN workspace_builds wb_build ON + audit_logs.resource_type = 'workspace_build' AND + audit_logs.resource_id = wb_build.id + -- Get the reason from the build #1 if this is the first + -- workspace create. + LEFT JOIN workspace_builds wb_workspace ON + audit_logs.resource_type = 'workspace' AND + audit_logs.action = 'create' AND + workspaces.id = wb_workspace.workspace_id AND + wb_workspace.build_number = 1 +WHERE + -- Filter resource_type + CASE + WHEN @resource_type :: text != '' THEN resource_type = @resource_type :: resource_type + ELSE true + END + -- Filter resource_id + AND CASE + WHEN @resource_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = @resource_id + ELSE true + END + -- Filter organization_id + AND CASE + WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = @organization_id + ELSE true + END + -- Filter by resource_target + AND CASE + WHEN @resource_target :: text != '' THEN resource_target = @resource_target + ELSE true + END + -- Filter action + AND CASE + WHEN @action :: text != '' THEN + action = @action :: audit_action + ELSE true +END + -- Filter by user_id +AND CASE + WHEN @user_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + user_id = @user_id + ELSE true +END + -- Filter by username +AND CASE + WHEN @username :: text != '' THEN + user_id = (SELECT id FROM users WHERE lower(username) = lower(@username) AND deleted = false) + ELSE true +END + -- Filter by user_email +AND CASE + WHEN @email :: text != '' THEN + users.email = @email + ELSE true +END + -- Filter by date_from +AND CASE + WHEN @date_from :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + "time" >= @date_from + ELSE true +END + -- Filter by date_to +AND CASE + WHEN @date_to :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN + "time" <= @date_to + ELSE true +END + -- Filter by build_reason +AND CASE + WHEN @build_reason::text != '' THEN + COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason + ELSE true +END + -- Filter request_id +AND CASE + WHEN @request_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + audit_logs.request_id = @request_id + ELSE true +END + + -- Authorize Filter clause will be injected below in CountAuthorizedAuditLogs + -- @authorize_filter +; From dad1a28b447f539e0b2451450a53e86544063f7f Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 26 Jun 2025 08:43:01 +0000 Subject: [PATCH 2/8] Fix lint --- coderd/database/querier_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index c105ceb34872f..f80f68115ad2c 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -2042,9 +2042,9 @@ func TestAuthorizedAuditLogs(t *testing.T) { require.NoError(t, err) // Then: All logs for both organizations are returned and count matches - expectedLogs := append(orgAuditLogs[first], orgAuditLogs[second]...) - expectedCount := int64(len(expectedLogs)) - require.Equal(t, expectedCount, count, "count should match sum of both organizations") + expectedLogs := append([]uuid.UUID{}, orgAuditLogs[first]...) + expectedLogs = append(expectedLogs, orgAuditLogs[second]...) + require.Equal(t, int64(len(expectedLogs)), count, "count should match sum of both organizations") require.ElementsMatch(t, expectedLogs, auditOnlyIDs(logs), "logs from both organizations should be returned") }) From 0587a914fc7d6005f212635a7034fdad8ddaabd2 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 26 Jun 2025 10:11:14 +0000 Subject: [PATCH 3/8] Fix CountAuthorizedAuditLogs query impl --- coderd/database/dbauthz/setup_test.go | 2 +- coderd/database/modelqueries.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go index 5e12f53963f04..555a17fb2070f 100644 --- a/coderd/database/dbauthz/setup_test.go +++ b/coderd/database/dbauthz/setup_test.go @@ -296,7 +296,7 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd resp, err := callMethod(ctx) // This is unfortunate, but if we are using `Filter` the error returned will be nil. So filter out - // any case where the error is nil and the response is an empty slice. + // any case where the error is nil and the response is an empty slice or int64(0). if err != nil || !hasEmptyResponse(resp) { if testCase.cancelledCtxExpect == "" { s.Errorf(err, "method should an error with cancellation") diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index 158d66e49784a..785ccf86afd27 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -598,7 +598,9 @@ func (q *sqlQuerier) CountAuthorizedAuditLogs(ctx context.Context, arg CountAudi defer rows.Close() var count int64 for rows.Next() { - count++ + if err := rows.Scan(&count); err != nil { + return 0, err + } } if err := rows.Close(); err != nil { return 0, err From ee0e49cad78ab006065bdd5adb67b7240dd2e6fd Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 26 Jun 2025 12:07:55 +0000 Subject: [PATCH 4/8] Add consistency check for audit logs SQL queries --- coderd/database/modelqueries_internal_test.go | 41 +++++++++++++++++++ coderd/database/queries.sql.go | 2 +- coderd/database/queries/auditlogs.sql | 2 +- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/coderd/database/modelqueries_internal_test.go b/coderd/database/modelqueries_internal_test.go index 992eb269ddc14..4f675a1b60785 100644 --- a/coderd/database/modelqueries_internal_test.go +++ b/coderd/database/modelqueries_internal_test.go @@ -1,9 +1,12 @@ package database import ( + "regexp" + "strings" "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/testutil" @@ -54,3 +57,41 @@ func TestWorkspaceTableConvert(t *testing.T) { "'workspace.WorkspaceTable()' is not missing at least 1 field when converting to 'WorkspaceTable'. "+ "To resolve this, go to the 'func (w Workspace) WorkspaceTable()' and ensure all fields are converted.") } + +// TestAuditLogsQueryConsistency ensures that GetAuditLogsOffset and CountAuditLogs +// have identical WHERE clauses to prevent filtering inconsistencies. +// This test is a guard rail to prevent developer oversight mistakes. +func TestAuditLogsQueryConsistency(t *testing.T) { + t.Parallel() + + getWhereClause := extractWhereClause(getAuditLogsOffset) + require.NotEmpty(t, getWhereClause, "failed to extract WHERE clause from GetAuditLogsOffset") + + countWhereClause := extractWhereClause(countAuditLogs) + require.NotEmpty(t, countWhereClause, "failed to extract WHERE clause from CountAuditLogs") + + // Compare the WHERE clauses + if diff := cmp.Diff(getWhereClause, countWhereClause); diff != "" { + t.Errorf("GetAuditLogsOffset and CountAuditLogs WHERE clauses must be identical to ensure consistent filtering.\nDiff:\n%s", diff) + } +} + +// extractWhereClause extracts the WHERE clause from a SQL query string +func extractWhereClause(query string) string { + // Find WHERE and get everything after it + wherePattern := regexp.MustCompile(`(?is)WHERE\s+(.*)`) + whereMatches := wherePattern.FindStringSubmatch(query) + if len(whereMatches) < 2 { + return "" + } + + whereClause := whereMatches[1] + + // Remove ORDER BY, LIMIT, OFFSET clauses from the end + whereClause = regexp.MustCompile(`(?is)\s+(ORDER BY|LIMIT|OFFSET).*$`).ReplaceAllString(whereClause, "") + + // Remove SQL comments + whereClause = regexp.MustCompile(`(?m)--.*$`).ReplaceAllString(whereClause, "") + + return strings.TrimSpace(whereClause) +} diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 0edfc90b19659..1cea56a79379c 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -524,7 +524,7 @@ END -- Filter by build_reason AND CASE WHEN $11::text != '' THEN - COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 + COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 ELSE true END -- Filter request_id diff --git a/coderd/database/queries/auditlogs.sql b/coderd/database/queries/auditlogs.sql index 86fc0f86b9577..c615de4ad238e 100644 --- a/coderd/database/queries/auditlogs.sql +++ b/coderd/database/queries/auditlogs.sql @@ -223,7 +223,7 @@ END -- Filter by build_reason AND CASE WHEN @build_reason::text != '' THEN - COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason + COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason ELSE true END -- Filter request_id From b599e4dcaad3246f95e3ec469bee7aa8c5d27b0e Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 30 Jun 2025 08:02:18 +0000 Subject: [PATCH 5/8] Construct count filter in searchquery.AuditLogs method --- coderd/audit.go | 19 ++++--------------- coderd/searchquery/search.go | 22 +++++++++++++++++++--- coderd/searchquery/search_test.go | 7 ++++++- 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/coderd/audit.go b/coderd/audit.go index 882679f521d5a..786707768c05e 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -46,7 +46,7 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { } queryStr := r.URL.Query().Get("q") - filter, errs := searchquery.AuditLogs(ctx, api.Database, queryStr) + filter, countFilter, errs := searchquery.AuditLogs(ctx, api.Database, queryStr) if len(errs) > 0 { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid audit search query.", @@ -62,23 +62,12 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { if filter.Username == "me" { filter.UserID = apiKey.UserID filter.Username = "" + countFilter.UserID = apiKey.UserID + countFilter.Username = "" } // Use the same filters to count the number of audit logs - count, err := api.Database.CountAuditLogs(ctx, database.CountAuditLogsParams{ - ResourceType: filter.ResourceType, - ResourceID: filter.ResourceID, - OrganizationID: filter.OrganizationID, - ResourceTarget: filter.ResourceTarget, - Action: filter.Action, - UserID: filter.UserID, - Username: filter.Username, - Email: filter.Email, - DateFrom: filter.DateFrom, - DateTo: filter.DateTo, - BuildReason: filter.BuildReason, - RequestID: filter.RequestID, - }) + count, err := api.Database.CountAuditLogs(ctx, countFilter) if dbauthz.IsNotAuthorizedError(err) { httpapi.Forbidden(rw) return diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index 721e593d4dd8d..d5986e0ed11ea 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -33,7 +33,8 @@ import ( // - resource_type: string (enum) // - action: string (enum) // - build_reason: string (enum) -func AuditLogs(ctx context.Context, db database.Store, query string) (database.GetAuditLogsOffsetParams, []codersdk.ValidationError) { +func AuditLogs(ctx context.Context, db database.Store, query string) (database.GetAuditLogsOffsetParams, + database.CountAuditLogsParams, []codersdk.ValidationError) { // Always lowercase for all searches. query = strings.ToLower(query) values, errors := searchTerms(query, func(term string, values url.Values) error { @@ -41,7 +42,7 @@ func AuditLogs(ctx context.Context, db database.Store, query string) (database.G return nil }) if len(errors) > 0 { - return database.GetAuditLogsOffsetParams{}, errors + return database.GetAuditLogsOffsetParams{}, database.CountAuditLogsParams{}, errors } const dateLayout = "2006-01-02" @@ -63,8 +64,23 @@ func AuditLogs(ctx context.Context, db database.Store, query string) (database.G filter.DateTo = filter.DateTo.Add(23*time.Hour + 59*time.Minute + 59*time.Second) } + // Prepare the count filter, which uses the same parameters as the GetAuditLogsOffsetParams. + countFilter := database.CountAuditLogsParams{ + RequestID: filter.RequestID, + ResourceID: filter.ResourceID, + ResourceTarget: filter.ResourceTarget, + Username: filter.Username, + Email: filter.Email, + DateFrom: filter.DateFrom, + DateTo: filter.DateTo, + OrganizationID: filter.OrganizationID, + ResourceType: filter.ResourceType, + Action: filter.Action, + BuildReason: filter.BuildReason, + } + parser.ErrorExcessParams(values) - return filter, parser.Errors + return filter, countFilter, parser.Errors } func Users(query string) (database.GetUsersParams, []codersdk.ValidationError) { diff --git a/coderd/searchquery/search_test.go b/coderd/searchquery/search_test.go index dd879839e552f..2b7f4f402e008 100644 --- a/coderd/searchquery/search_test.go +++ b/coderd/searchquery/search_test.go @@ -343,6 +343,7 @@ func TestSearchAudit(t *testing.T) { Name string Query string Expected database.GetAuditLogsOffsetParams + ExpectedCountParams database.CountAuditLogsParams ExpectedErrorContains string }{ { @@ -372,6 +373,9 @@ func TestSearchAudit(t *testing.T) { Expected: database.GetAuditLogsOffsetParams{ ResourceTarget: "foo", }, + ExpectedCountParams: database.CountAuditLogsParams{ + ResourceTarget: "foo", + }, }, { Name: "RequestID", @@ -386,7 +390,7 @@ func TestSearchAudit(t *testing.T) { // Do not use a real database, this is only used for an // organization lookup. db := dbmem.New() - values, errs := searchquery.AuditLogs(context.Background(), db, c.Query) + values, countValues, errs := searchquery.AuditLogs(context.Background(), db, c.Query) if c.ExpectedErrorContains != "" { require.True(t, len(errs) > 0, "expect some errors") var s strings.Builder @@ -397,6 +401,7 @@ func TestSearchAudit(t *testing.T) { } else { require.Len(t, errs, 0, "expected no error") require.Equal(t, c.Expected, values, "expected values") + require.Equal(t, c.ExpectedCountParams, countValues, "expected count values") } }) } From 1d8b0ac6cf84e139ceaae8b23cb1dee2bc1a114f Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 30 Jun 2025 08:13:12 +0000 Subject: [PATCH 6/8] Reformat auditlogs.sql --- coderd/database/queries/auditlogs.sql | 361 +++++++++++++------------- 1 file changed, 181 insertions(+), 180 deletions(-) diff --git a/coderd/database/queries/auditlogs.sql b/coderd/database/queries/auditlogs.sql index c615de4ad238e..6269f21cd27e4 100644 --- a/coderd/database/queries/auditlogs.sql +++ b/coderd/database/queries/auditlogs.sql @@ -2,237 +2,238 @@ -- ID. -- name: GetAuditLogsOffset :many SELECT sqlc.embed(audit_logs), - -- sqlc.embed(users) would be nice but it does not seem to play well with - -- left joins. - users.username AS user_username, - users.name AS user_name, - users.email AS user_email, - users.created_at AS user_created_at, - users.updated_at AS user_updated_at, - users.last_seen_at AS user_last_seen_at, - users.status AS user_status, - users.login_type AS user_login_type, - users.rbac_roles AS user_roles, - users.avatar_url AS user_avatar_url, - users.deleted AS user_deleted, - users.quiet_hours_schedule AS user_quiet_hours_schedule, - COALESCE(organizations.name, '') AS organization_name, - COALESCE(organizations.display_name, '') AS organization_display_name, - COALESCE(organizations.icon, '') AS organization_icon + -- sqlc.embed(users) would be nice but it does not seem to play well with + -- left joins. + users.username AS user_username, + users.name AS user_name, + users.email AS user_email, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.last_seen_at AS user_last_seen_at, + users.status AS user_status, + users.login_type AS user_login_type, + users.rbac_roles AS user_roles, + users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + COALESCE(organizations.name, '') AS organization_name, + COALESCE(organizations.display_name, '') AS organization_display_name, + COALESCE(organizations.icon, '') AS organization_icon FROM audit_logs - LEFT JOIN users ON audit_logs.user_id = users.id - LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id -- First join on workspaces to get the initial workspace create -- to workspace build 1 id. This is because the first create is -- is a different audit log than subsequent starts. - LEFT JOIN workspaces ON - audit_logs.resource_type = 'workspace' AND - audit_logs.resource_id = workspaces.id + LEFT JOIN workspaces ON audit_logs.resource_type = 'workspace' + AND audit_logs.resource_id = workspaces.id -- Get the reason from the build if the resource type -- is a workspace_build - LEFT JOIN workspace_builds wb_build ON - audit_logs.resource_type = 'workspace_build' AND - audit_logs.resource_id = wb_build.id + LEFT JOIN workspace_builds wb_build ON audit_logs.resource_type = 'workspace_build' + AND audit_logs.resource_id = wb_build.id -- Get the reason from the build #1 if this is the first -- workspace create. - LEFT JOIN workspace_builds wb_workspace ON - audit_logs.resource_type = 'workspace' AND - audit_logs.action = 'create' AND - workspaces.id = wb_workspace.workspace_id AND - wb_workspace.build_number = 1 + LEFT JOIN workspace_builds wb_workspace ON audit_logs.resource_type = 'workspace' + AND audit_logs.action = 'create' + AND workspaces.id = wb_workspace.workspace_id + AND wb_workspace.build_number = 1 WHERE - -- Filter resource_type + -- Filter resource_type CASE - WHEN @resource_type :: text != '' THEN resource_type = @resource_type :: resource_type - ELSE true - END - -- Filter resource_id - AND CASE - WHEN @resource_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = @resource_id - ELSE true - END - -- Filter organization_id - AND CASE - WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = @organization_id - ELSE true - END - -- Filter by resource_target - AND CASE - WHEN @resource_target :: text != '' THEN resource_target = @resource_target - ELSE true - END - -- Filter action - AND CASE - WHEN @action :: text != '' THEN - action = @action :: audit_action - ELSE true -END + WHEN @resource_type::text != '' THEN resource_type = @resource_type::resource_type + ELSE true + END + -- Filter resource_id + AND CASE + WHEN @resource_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = @resource_id + ELSE true + END + -- Filter organization_id + AND CASE + WHEN @organization_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = @organization_id + ELSE true + END + -- Filter by resource_target + AND CASE + WHEN @resource_target::text != '' THEN resource_target = @resource_target + ELSE true + END + -- Filter action + AND CASE + WHEN @action::text != '' THEN action = @action::audit_action + ELSE true + END -- Filter by user_id -AND CASE - WHEN @user_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - user_id = @user_id + AND CASE + WHEN @user_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN user_id = @user_id ELSE true -END + END -- Filter by username -AND CASE - WHEN @username :: text != '' THEN - user_id = (SELECT id FROM users WHERE lower(username) = lower(@username) AND deleted = false) + AND CASE + WHEN @username::text != '' THEN user_id = ( + SELECT id + FROM users + WHERE lower(username) = lower(@username) + AND deleted = false + ) ELSE true -END + END -- Filter by user_email -AND CASE - WHEN @email :: text != '' THEN - users.email = @email + AND CASE + WHEN @email::text != '' THEN users.email = @email ELSE true -END + END -- Filter by date_from -AND CASE - WHEN @date_from :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" >= @date_from + AND CASE + WHEN @date_from::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" >= @date_from ELSE true -END + END -- Filter by date_to -AND CASE - WHEN @date_to :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" <= @date_to - ELSE true -END - -- Filter by build_reason -AND CASE - WHEN @build_reason::text != '' THEN - COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason - ELSE true -END + AND CASE + WHEN @date_to::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" <= @date_to + ELSE true + END + -- Filter by build_reason + AND CASE + WHEN @build_reason::text != '' THEN COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason + ELSE true + END -- Filter request_id -AND CASE - WHEN @request_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - audit_logs.request_id = @request_id + AND CASE + WHEN @request_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.request_id = @request_id ELSE true -END - + END -- Authorize Filter clause will be injected below in GetAuthorizedAuditLogsOffset -- @authorize_filter -ORDER BY - "time" DESC -LIMIT - -- a limit of 0 means "no limit". The audit log table is unbounded +ORDER BY "time" DESC +LIMIT -- 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 - @offset_opt; + COALESCE(NULLIF(@limit_opt::int, 0), 100) OFFSET @offset_opt; -- name: InsertAuditLog :one -INSERT INTO audit_logs (id, - "time", - user_id, - organization_id, - ip, - user_agent, - resource_type, - resource_id, - resource_target, - action, - diff, - status_code, - additional_fields, - request_id, - resource_icon) -VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING *; +INSERT INTO audit_logs ( + id, + "time", + user_id, + organization_id, + ip, + user_agent, + resource_type, + resource_id, + resource_target, + action, + diff, + status_code, + additional_fields, + request_id, + resource_icon + ) +VALUES ( + $1, + $2, + $3, + $4, + $5, + $6, + $7, + $8, + $9, + $10, + $11, + $12, + $13, + $14, + $15 + ) +RETURNING *; -- name: CountAuditLogs :one SELECT COUNT(*) FROM audit_logs - LEFT JOIN users ON audit_logs.user_id = users.id - LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id -- First join on workspaces to get the initial workspace create -- to workspace build 1 id. This is because the first create is -- is a different audit log than subsequent starts. - LEFT JOIN workspaces ON - audit_logs.resource_type = 'workspace' AND - audit_logs.resource_id = workspaces.id + LEFT JOIN workspaces ON audit_logs.resource_type = 'workspace' + AND audit_logs.resource_id = workspaces.id -- Get the reason from the build if the resource type -- is a workspace_build - LEFT JOIN workspace_builds wb_build ON - audit_logs.resource_type = 'workspace_build' AND - audit_logs.resource_id = wb_build.id + LEFT JOIN workspace_builds wb_build ON audit_logs.resource_type = 'workspace_build' + AND audit_logs.resource_id = wb_build.id -- Get the reason from the build #1 if this is the first -- workspace create. - LEFT JOIN workspace_builds wb_workspace ON - audit_logs.resource_type = 'workspace' AND - audit_logs.action = 'create' AND - workspaces.id = wb_workspace.workspace_id AND - wb_workspace.build_number = 1 + LEFT JOIN workspace_builds wb_workspace ON audit_logs.resource_type = 'workspace' + AND audit_logs.action = 'create' + AND workspaces.id = wb_workspace.workspace_id + AND wb_workspace.build_number = 1 WHERE - -- Filter resource_type + -- Filter resource_type CASE - WHEN @resource_type :: text != '' THEN resource_type = @resource_type :: resource_type - ELSE true - END - -- Filter resource_id - AND CASE - WHEN @resource_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = @resource_id - ELSE true - END - -- Filter organization_id - AND CASE - WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = @organization_id - ELSE true - END - -- Filter by resource_target - AND CASE - WHEN @resource_target :: text != '' THEN resource_target = @resource_target - ELSE true - END - -- Filter action - AND CASE - WHEN @action :: text != '' THEN - action = @action :: audit_action - ELSE true -END + WHEN @resource_type::text != '' THEN resource_type = @resource_type::resource_type + ELSE true + END + -- Filter resource_id + AND CASE + WHEN @resource_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = @resource_id + ELSE true + END + -- Filter organization_id + AND CASE + WHEN @organization_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = @organization_id + ELSE true + END + -- Filter by resource_target + AND CASE + WHEN @resource_target::text != '' THEN resource_target = @resource_target + ELSE true + END + -- Filter action + AND CASE + WHEN @action::text != '' THEN action = @action::audit_action + ELSE true + END -- Filter by user_id -AND CASE - WHEN @user_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - user_id = @user_id + AND CASE + WHEN @user_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN user_id = @user_id ELSE true -END + END -- Filter by username -AND CASE - WHEN @username :: text != '' THEN - user_id = (SELECT id FROM users WHERE lower(username) = lower(@username) AND deleted = false) + AND CASE + WHEN @username::text != '' THEN user_id = ( + SELECT id + FROM users + WHERE lower(username) = lower(@username) + AND deleted = false + ) ELSE true -END + END -- Filter by user_email -AND CASE - WHEN @email :: text != '' THEN - users.email = @email + AND CASE + WHEN @email::text != '' THEN users.email = @email ELSE true -END + END -- Filter by date_from -AND CASE - WHEN @date_from :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" >= @date_from + AND CASE + WHEN @date_from::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" >= @date_from ELSE true -END + END -- Filter by date_to -AND CASE - WHEN @date_to :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" <= @date_to - ELSE true -END - -- Filter by build_reason -AND CASE - WHEN @build_reason::text != '' THEN - COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason - ELSE true -END + AND CASE + WHEN @date_to::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" <= @date_to + ELSE true + END + -- Filter by build_reason + AND CASE + WHEN @build_reason::text != '' THEN COALESCE(wb_build.reason::text, wb_workspace.reason::text) = @build_reason + ELSE true + END -- Filter request_id -AND CASE - WHEN @request_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - audit_logs.request_id = @request_id + AND CASE + WHEN @request_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.request_id = @request_id ELSE true -END - + END -- Authorize Filter clause will be injected below in CountAuthorizedAuditLogs -- @authorize_filter ; From 4e578a484176918c81f8968732d7c268df6a7920 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 30 Jun 2025 08:31:57 +0000 Subject: [PATCH 7/8] Fix lint --- coderd/searchquery/search.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index d5986e0ed11ea..df536d3598b4f 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -42,6 +42,7 @@ func AuditLogs(ctx context.Context, db database.Store, query string) (database.G return nil }) if len(errors) > 0 { + // nolint:exhaustruct // We don't need to initialize these structs because we return an error. return database.GetAuditLogsOffsetParams{}, database.CountAuditLogsParams{}, errors } @@ -65,6 +66,7 @@ func AuditLogs(ctx context.Context, db database.Store, query string) (database.G } // Prepare the count filter, which uses the same parameters as the GetAuditLogsOffsetParams. + // nolint:exhaustruct // UserID is not obtained from the query parameters. countFilter := database.CountAuditLogsParams{ RequestID: filter.RequestID, ResourceID: filter.ResourceID, From e82acd021b4b31bc2ef253d9f424ba4e48e87efc Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 30 Jun 2025 09:00:48 +0000 Subject: [PATCH 8/8] Regenerate queries, fix fmt --- coderd/database/queries.sql.go | 341 +++++++++++++++++---------------- coderd/searchquery/search.go | 3 +- 2 files changed, 173 insertions(+), 171 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 1cea56a79379c..e3e893df0c633 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -444,96 +444,89 @@ func (q *sqlQuerier) UpdateAPIKeyByID(ctx context.Context, arg UpdateAPIKeyByIDP const countAuditLogs = `-- name: CountAuditLogs :one SELECT COUNT(*) FROM audit_logs - LEFT JOIN users ON audit_logs.user_id = users.id - LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id -- First join on workspaces to get the initial workspace create -- to workspace build 1 id. This is because the first create is -- is a different audit log than subsequent starts. - LEFT JOIN workspaces ON - audit_logs.resource_type = 'workspace' AND - audit_logs.resource_id = workspaces.id + LEFT JOIN workspaces ON audit_logs.resource_type = 'workspace' + AND audit_logs.resource_id = workspaces.id -- Get the reason from the build if the resource type -- is a workspace_build - LEFT JOIN workspace_builds wb_build ON - audit_logs.resource_type = 'workspace_build' AND - audit_logs.resource_id = wb_build.id + LEFT JOIN workspace_builds wb_build ON audit_logs.resource_type = 'workspace_build' + AND audit_logs.resource_id = wb_build.id -- Get the reason from the build #1 if this is the first -- workspace create. - LEFT JOIN workspace_builds wb_workspace ON - audit_logs.resource_type = 'workspace' AND - audit_logs.action = 'create' AND - workspaces.id = wb_workspace.workspace_id AND - wb_workspace.build_number = 1 + LEFT JOIN workspace_builds wb_workspace ON audit_logs.resource_type = 'workspace' + AND audit_logs.action = 'create' + AND workspaces.id = wb_workspace.workspace_id + AND wb_workspace.build_number = 1 WHERE - -- Filter resource_type + -- Filter resource_type CASE - WHEN $1 :: text != '' THEN resource_type = $1 :: resource_type + WHEN $1::text != '' THEN resource_type = $1::resource_type ELSE true - END - -- Filter resource_id - AND CASE - WHEN $2 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = $2 - ELSE true END - -- Filter organization_id - AND CASE - WHEN $3 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = $3 - ELSE true + -- Filter resource_id + AND CASE + WHEN $2::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = $2 + ELSE true END - -- Filter by resource_target - AND CASE - WHEN $4 :: text != '' THEN resource_target = $4 - ELSE true + -- Filter organization_id + AND CASE + WHEN $3::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = $3 + ELSE true + END + -- Filter by resource_target + AND CASE + WHEN $4::text != '' THEN resource_target = $4 + ELSE true + END + -- Filter action + AND CASE + WHEN $5::text != '' THEN action = $5::audit_action + ELSE true END - -- Filter action - AND CASE - WHEN $5 :: text != '' THEN - action = $5 :: audit_action - ELSE true -END -- Filter by user_id -AND CASE - WHEN $6 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - user_id = $6 + AND CASE + WHEN $6::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN user_id = $6 ELSE true -END + END -- Filter by username -AND CASE - WHEN $7 :: text != '' THEN - user_id = (SELECT id FROM users WHERE lower(username) = lower($7) AND deleted = false) + AND CASE + WHEN $7::text != '' THEN user_id = ( + SELECT id + FROM users + WHERE lower(username) = lower($7) + AND deleted = false + ) ELSE true -END + END -- Filter by user_email -AND CASE - WHEN $8 :: text != '' THEN - users.email = $8 + AND CASE + WHEN $8::text != '' THEN users.email = $8 ELSE true -END + END -- Filter by date_from -AND CASE - WHEN $9 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" >= $9 + AND CASE + WHEN $9::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" >= $9 ELSE true -END + END -- Filter by date_to -AND CASE - WHEN $10 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" <= $10 + AND CASE + WHEN $10::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" <= $10 ELSE true -END - -- Filter by build_reason -AND CASE - WHEN $11::text != '' THEN - COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 - ELSE true -END + END + -- Filter by build_reason + AND CASE + WHEN $11::text != '' THEN COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 + ELSE true + END -- Filter request_id -AND CASE - WHEN $12 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - audit_logs.request_id = $12 + AND CASE + WHEN $12::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.request_id = $12 ELSE true -END - + END -- Authorize Filter clause will be injected below in CountAuthorizedAuditLogs -- @authorize_filter ` @@ -575,125 +568,114 @@ func (q *sqlQuerier) CountAuditLogs(ctx context.Context, arg CountAuditLogsParam const getAuditLogsOffset = `-- name: GetAuditLogsOffset :many SELECT audit_logs.id, audit_logs.time, audit_logs.user_id, audit_logs.organization_id, audit_logs.ip, audit_logs.user_agent, audit_logs.resource_type, audit_logs.resource_id, audit_logs.resource_target, audit_logs.action, audit_logs.diff, audit_logs.status_code, audit_logs.additional_fields, audit_logs.request_id, audit_logs.resource_icon, - -- sqlc.embed(users) would be nice but it does not seem to play well with - -- left joins. - users.username AS user_username, - users.name AS user_name, - users.email AS user_email, - users.created_at AS user_created_at, - users.updated_at AS user_updated_at, - users.last_seen_at AS user_last_seen_at, - users.status AS user_status, - users.login_type AS user_login_type, - users.rbac_roles AS user_roles, - users.avatar_url AS user_avatar_url, - users.deleted AS user_deleted, - users.quiet_hours_schedule AS user_quiet_hours_schedule, - COALESCE(organizations.name, '') AS organization_name, - COALESCE(organizations.display_name, '') AS organization_display_name, - COALESCE(organizations.icon, '') AS organization_icon + -- sqlc.embed(users) would be nice but it does not seem to play well with + -- left joins. + users.username AS user_username, + users.name AS user_name, + users.email AS user_email, + users.created_at AS user_created_at, + users.updated_at AS user_updated_at, + users.last_seen_at AS user_last_seen_at, + users.status AS user_status, + users.login_type AS user_login_type, + users.rbac_roles AS user_roles, + users.avatar_url AS user_avatar_url, + users.deleted AS user_deleted, + users.quiet_hours_schedule AS user_quiet_hours_schedule, + COALESCE(organizations.name, '') AS organization_name, + COALESCE(organizations.display_name, '') AS organization_display_name, + COALESCE(organizations.icon, '') AS organization_icon FROM audit_logs - LEFT JOIN users ON audit_logs.user_id = users.id - LEFT JOIN organizations ON audit_logs.organization_id = organizations.id + LEFT JOIN users ON audit_logs.user_id = users.id + LEFT JOIN organizations ON audit_logs.organization_id = organizations.id -- First join on workspaces to get the initial workspace create -- to workspace build 1 id. This is because the first create is -- is a different audit log than subsequent starts. - LEFT JOIN workspaces ON - audit_logs.resource_type = 'workspace' AND - audit_logs.resource_id = workspaces.id + LEFT JOIN workspaces ON audit_logs.resource_type = 'workspace' + AND audit_logs.resource_id = workspaces.id -- Get the reason from the build if the resource type -- is a workspace_build - LEFT JOIN workspace_builds wb_build ON - audit_logs.resource_type = 'workspace_build' AND - audit_logs.resource_id = wb_build.id + LEFT JOIN workspace_builds wb_build ON audit_logs.resource_type = 'workspace_build' + AND audit_logs.resource_id = wb_build.id -- Get the reason from the build #1 if this is the first -- workspace create. - LEFT JOIN workspace_builds wb_workspace ON - audit_logs.resource_type = 'workspace' AND - audit_logs.action = 'create' AND - workspaces.id = wb_workspace.workspace_id AND - wb_workspace.build_number = 1 + LEFT JOIN workspace_builds wb_workspace ON audit_logs.resource_type = 'workspace' + AND audit_logs.action = 'create' + AND workspaces.id = wb_workspace.workspace_id + AND wb_workspace.build_number = 1 WHERE - -- Filter resource_type + -- Filter resource_type CASE - WHEN $1 :: text != '' THEN resource_type = $1 :: resource_type + WHEN $1::text != '' THEN resource_type = $1::resource_type ELSE true - END - -- Filter resource_id - AND CASE - WHEN $2 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = $2 - ELSE true END - -- Filter organization_id - AND CASE - WHEN $3 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = $3 - ELSE true + -- Filter resource_id + AND CASE + WHEN $2::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN resource_id = $2 + ELSE true END - -- Filter by resource_target - AND CASE - WHEN $4 :: text != '' THEN resource_target = $4 - ELSE true + -- Filter organization_id + AND CASE + WHEN $3::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.organization_id = $3 + ELSE true + END + -- Filter by resource_target + AND CASE + WHEN $4::text != '' THEN resource_target = $4 + ELSE true + END + -- Filter action + AND CASE + WHEN $5::text != '' THEN action = $5::audit_action + ELSE true END - -- Filter action - AND CASE - WHEN $5 :: text != '' THEN - action = $5 :: audit_action - ELSE true -END -- Filter by user_id -AND CASE - WHEN $6 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - user_id = $6 + AND CASE + WHEN $6::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN user_id = $6 ELSE true -END + END -- Filter by username -AND CASE - WHEN $7 :: text != '' THEN - user_id = (SELECT id FROM users WHERE lower(username) = lower($7) AND deleted = false) + AND CASE + WHEN $7::text != '' THEN user_id = ( + SELECT id + FROM users + WHERE lower(username) = lower($7) + AND deleted = false + ) ELSE true -END + END -- Filter by user_email -AND CASE - WHEN $8 :: text != '' THEN - users.email = $8 + AND CASE + WHEN $8::text != '' THEN users.email = $8 ELSE true -END + END -- Filter by date_from -AND CASE - WHEN $9 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" >= $9 + AND CASE + WHEN $9::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" >= $9 ELSE true -END + END -- Filter by date_to -AND CASE - WHEN $10 :: timestamp with time zone != '0001-01-01 00:00:00Z' THEN - "time" <= $10 + AND CASE + WHEN $10::timestamp with time zone != '0001-01-01 00:00:00Z' THEN "time" <= $10 ELSE true -END - -- Filter by build_reason -AND CASE - WHEN $11::text != '' THEN - COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 - ELSE true -END + END + -- Filter by build_reason + AND CASE + WHEN $11::text != '' THEN COALESCE(wb_build.reason::text, wb_workspace.reason::text) = $11 + ELSE true + END -- Filter request_id -AND CASE - WHEN $12 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN - audit_logs.request_id = $12 + AND CASE + WHEN $12::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN audit_logs.request_id = $12 ELSE true -END - + END -- Authorize Filter clause will be injected below in GetAuthorizedAuditLogsOffset -- @authorize_filter -ORDER BY - "time" DESC -LIMIT - -- a limit of 0 means "no limit". The audit log table is unbounded +ORDER BY "time" DESC +LIMIT -- 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($14 :: int, 0), 100) -OFFSET - $13 + COALESCE(NULLIF($14::int, 0), 100) OFFSET $13 ` type GetAuditLogsOffsetParams struct { @@ -804,22 +786,41 @@ func (q *sqlQuerier) GetAuditLogsOffset(ctx context.Context, arg GetAuditLogsOff } const insertAuditLog = `-- name: InsertAuditLog :one -INSERT INTO audit_logs (id, - "time", - user_id, - organization_id, - ip, - user_agent, - resource_type, - resource_id, - resource_target, - action, - diff, - status_code, - additional_fields, - request_id, - resource_icon) -VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING id, time, user_id, organization_id, ip, user_agent, resource_type, resource_id, resource_target, action, diff, status_code, additional_fields, request_id, resource_icon +INSERT INTO audit_logs ( + id, + "time", + user_id, + organization_id, + ip, + user_agent, + resource_type, + resource_id, + resource_target, + action, + diff, + status_code, + additional_fields, + request_id, + resource_icon + ) +VALUES ( + $1, + $2, + $3, + $4, + $5, + $6, + $7, + $8, + $9, + $10, + $11, + $12, + $13, + $14, + $15 + ) +RETURNING id, time, user_id, organization_id, ip, user_agent, resource_type, resource_id, resource_target, action, diff, status_code, additional_fields, request_id, resource_icon ` type InsertAuditLogParams struct { diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index df536d3598b4f..634d4b6632ed3 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -34,7 +34,8 @@ import ( // - action: string (enum) // - build_reason: string (enum) func AuditLogs(ctx context.Context, db database.Store, query string) (database.GetAuditLogsOffsetParams, - database.CountAuditLogsParams, []codersdk.ValidationError) { + database.CountAuditLogsParams, []codersdk.ValidationError, +) { // Always lowercase for all searches. query = strings.ToLower(query) values, errors := searchTerms(query, func(term string, values url.Values) error {