From 8af625dba2a4848e7a55fa67ac14caf6f77b301c Mon Sep 17 00:00:00 2001 From: Kian Parvin <46668016+kian99@users.noreply.github.com> Date: Tue, 15 Aug 2023 11:53:36 +0200 Subject: [PATCH] CSS-5179 Audit log enhacement (#1026) * Improve audit logging - Fixed inability to filter by method - Added ability to sort by time * Added test for API params conversion * Fixed godoc * Tweak json for sortTime --- api/params/params.go | 4 ++ cmd/jimmctl/cmd/listauditevents.go | 2 + internal/db/audit.go | 7 +++ internal/jimm/jimm_test.go | 8 ++++ internal/jujuapi/export_test.go | 3 ++ internal/jujuapi/jimm.go | 25 ++++++---- internal/jujuapi/jimm_test.go | 74 ++++++++++++++++++++++++++++++ 7 files changed, 115 insertions(+), 8 deletions(-) diff --git a/api/params/params.go b/api/params/params.go index 2064b26c6..9cebfd7e2 100644 --- a/api/params/params.go +++ b/api/params/params.go @@ -204,6 +204,10 @@ type FindAuditEventsRequest struct { // Limit is the maximum number of audit events to return. Limit int `json:"limit,omitempty"` + + // SortTime will sort by most recent (time descending) when true. + // When false no explicit ordering will be applied. + SortTime bool `json:"sortTime,omitempty"` } // A ListControllersResponse is the response that is sent in a diff --git a/cmd/jimmctl/cmd/listauditevents.go b/cmd/jimmctl/cmd/listauditevents.go index c21f2902a..e25446d3d 100644 --- a/cmd/jimmctl/cmd/listauditevents.go +++ b/cmd/jimmctl/cmd/listauditevents.go @@ -73,6 +73,8 @@ func (c *listAuditEventsCommand) SetFlags(f *gnuflag.FlagSet) { f.StringVar(&c.args.Model, "model", "", "display events for a specific model (model name is controller/model)") f.IntVar(&c.args.Limit, "offset", 0, "offset the set of returned audit events") f.IntVar(&c.args.Limit, "limit", 0, "limit the maximum number of returned audit events") + f.BoolVar(&c.args.SortTime, "reverse", false, "reverse the order of logs, showing the most recent first") + } // Init implements the cmd.Command interface. diff --git a/internal/db/audit.go b/internal/db/audit.go index 83c754f06..27a0b3413 100644 --- a/internal/db/audit.go +++ b/internal/db/audit.go @@ -55,6 +55,10 @@ type AuditLogFilter struct { // Limit is the maximum number of audit events to return. // A value of zero will ignore the limit. Limit int `json:"limit,omitempty"` + + // SortTime will sort by most recent first (time descending) when true. + // When false no explicit ordering will be applied. + SortTime bool `json:"sortTime,omitempty"` } // ForEachAuditLogEntry iterates through all audit log entries that match @@ -82,6 +86,9 @@ func (d *Database) ForEachAuditLogEntry(ctx context.Context, filter AuditLogFilt if filter.Method != "" { db = db.Where("facade_method = ?", filter.Method) } + if filter.SortTime { + db = db.Order("time DESC") + } db = db.Limit(filter.Limit) db = db.Offset(filter.Offset) diff --git a/internal/jimm/jimm_test.go b/internal/jimm/jimm_test.go index d78eab52f..c2cd0b189 100644 --- a/internal/jimm/jimm_test.go +++ b/internal/jimm/jimm_test.go @@ -114,6 +114,14 @@ func TestFindAuditEvents(t *testing.T) { Model: "TestModel", }, expectedEvents: []dbmodel.AuditLogEntry{events[2], events[3]}, + }, { + about: "admin/privileged user is allowed to find audit events by model and sort by time", + users: []*openfga.User{admin, privileged}, + filter: db.AuditLogFilter{ + Model: "TestModel", + SortTime: true, + }, + expectedEvents: []dbmodel.AuditLogEntry{events[3], events[2]}, }, { about: "admin/privileged user is allowed to find audit events with limit/offset", users: []*openfga.User{admin, privileged}, diff --git a/internal/jujuapi/export_test.go b/internal/jujuapi/export_test.go index 213b849c0..d993712cb 100644 --- a/internal/jujuapi/export_test.go +++ b/internal/jujuapi/export_test.go @@ -16,6 +16,9 @@ var ( ParseTag = parseTag ResolveTag = resolveTag ModelInfoFromPath = modelInfoFromPath + AuditParamsToFilter = auditParamsToFilter + AuditLogDefaultLimit = limitDefault + AuditLogUpperLimit = maxLimit ) func NewModelSummaryWatcher() *modelSummaryWatcher { diff --git a/internal/jujuapi/jimm.go b/internal/jujuapi/jimm.go index 6276648e1..061682304 100644 --- a/internal/jujuapi/jimm.go +++ b/internal/jujuapi/jimm.go @@ -324,32 +324,32 @@ func (r *controllerRoot) SetControllerDeprecated(ctx context.Context, req apipar const maxLimit = 1000 const limitDefault = 50 -// FindAuditEvents finds the audit-log entries that match the given filter. -func (r *controllerRoot) FindAuditEvents(ctx context.Context, req apiparams.FindAuditEventsRequest) (apiparams.AuditEvents, error) { - const op = errors.Op("jujuapi.FindAuditEvents") - +func auditParamsToFilter(req apiparams.FindAuditEventsRequest) (db.AuditLogFilter, error) { var filter db.AuditLogFilter var err error + filter.Method = req.Method + filter.Model = req.Model + filter.SortTime = req.SortTime + if req.After != "" { filter.Start, err = time.Parse(time.RFC3339, req.After) if err != nil { - return apiparams.AuditEvents{}, errors.E(op, err, errors.CodeBadRequest, `invalid "after" filter`) + return filter, errors.E(err, errors.CodeBadRequest, `invalid "after" filter`) } } if req.Before != "" { filter.End, err = time.Parse(time.RFC3339, req.Before) if err != nil { - return apiparams.AuditEvents{}, errors.E(op, err, errors.CodeBadRequest, `invalid "before" filter`) + return filter, errors.E(err, errors.CodeBadRequest, `invalid "before" filter`) } } if req.UserTag != "" { tag, err := names.ParseUserTag(req.UserTag) if err != nil { - return apiparams.AuditEvents{}, errors.E(op, err, errors.CodeBadRequest, `invalid "user-tag" filter`) + return filter, errors.E(err, errors.CodeBadRequest, `invalid "user-tag" filter`) } filter.UserTag = tag.String() } - filter.Model = req.Model limit := int(req.Limit) if limit < 1 { @@ -364,7 +364,16 @@ func (r *controllerRoot) FindAuditEvents(ctx context.Context, req apiparams.Find offset = 0 } filter.Offset = offset + return filter, nil +} +// FindAuditEvents finds the audit-log entries that match the given filter. +func (r *controllerRoot) FindAuditEvents(ctx context.Context, req apiparams.FindAuditEventsRequest) (apiparams.AuditEvents, error) { + const op = errors.Op("jujuapi.FindAuditEvents") + filter, err := auditParamsToFilter(req) + if err != nil { + return apiparams.AuditEvents{}, errors.E(op, err) + } entries, err := r.jimm.FindAuditEvents(ctx, r.user, filter) if err != nil { return apiparams.AuditEvents{}, errors.E(op, err) diff --git a/internal/jujuapi/jimm_test.go b/internal/jujuapi/jimm_test.go index 080a3506f..00867dd11 100644 --- a/internal/jujuapi/jimm_test.go +++ b/internal/jujuapi/jimm_test.go @@ -4,8 +4,10 @@ package jujuapi_test import ( "context" + "testing" "time" + qt "github.com/frankban/quicktest" "github.com/google/go-cmp/cmp/cmpopts" "github.com/juju/juju/api/client/modelmanager" "github.com/juju/juju/apiserver/common" @@ -18,6 +20,7 @@ import ( "github.com/canonical/jimm/api" apiparams "github.com/canonical/jimm/api/params" + "github.com/canonical/jimm/internal/db" "github.com/canonical/jimm/internal/dbmodel" "github.com/canonical/jimm/internal/jimmtest" "github.com/canonical/jimm/internal/jujuapi" @@ -400,6 +403,77 @@ func (s *jimmSuite) TestAuditLog(c *gc.C) { c.Check(evs, jc.DeepEquals, expectedEvents) } +func (s *jimmSuite) TestAuditLogFilterByMethod(c *gc.C) { + conn := s.open(c, nil, "alice") + defer conn.Close() + client := api.NewClient(conn) + evs, err := client.FindAuditEvents(&apiparams.FindAuditEventsRequest{Method: "Deploy"}) + c.Assert(err, gc.Equals, nil) + c.Assert(len(evs.Events), gc.Equals, 0) +} + +// TestAuditLogAPIParamsConversion tests the conversion of API params to a AuditLogFilter struct. +// Note that this test doesn't require a running Juju/JIMM controller so it doesn't use gc + the jimmSuite. +func TestAuditLogAPIParamsConversion(t *testing.T) { + c := qt.New(t) + testCases := []struct { + about string + request apiparams.FindAuditEventsRequest + result db.AuditLogFilter + err error + }{ + { + about: "Test basic conversion", + request: apiparams.FindAuditEventsRequest{ + After: "2023-08-14T00:00:00Z", + Before: "2023-08-14T00:00:00Z", + UserTag: "user-alice", + Model: "123", + Method: "Deploy", + Offset: 10, + Limit: 10, + SortTime: false, + }, + result: db.AuditLogFilter{ + Start: time.Date(2023, 8, 14, 0, 0, 0, 0, time.UTC), + End: time.Date(2023, 8, 14, 0, 0, 0, 0, time.UTC), + UserTag: "user-alice", + Model: "123", + Method: "Deploy", + Offset: 10, + Limit: 10, + SortTime: false, + }, + }, { + about: "Test limit lower bound", + request: apiparams.FindAuditEventsRequest{ + Limit: 0, + }, + result: db.AuditLogFilter{ + Limit: jujuapi.AuditLogDefaultLimit, + }, + }, { + about: "Test limit upper bound", + request: apiparams.FindAuditEventsRequest{ + Limit: jujuapi.AuditLogUpperLimit + 1, + }, + result: db.AuditLogFilter{ + Limit: jujuapi.AuditLogUpperLimit, + }, + }, + } + for _, test := range testCases { + c.Log(test.about) + res, err := jujuapi.AuditParamsToFilter(test.request) + if test.err == nil { + c.Assert(err, qt.IsNil) + c.Assert(res, qt.DeepEquals, test.result) + } else { + c.Assert(err, qt.ErrorMatches, test.err) + } + } +} + func (s *jimmSuite) TestFullModelStatus(c *gc.C) { s.AddController(c, "controller-2", s.APIInfo(c)) mt := s.AddModel(c, names.NewUserTag("charlie@external"), "model-1", names.NewCloudTag(jimmtest.TestCloudName), jimmtest.TestCloudRegionName, s.Model2.CloudCredential.ResourceTag())