From ff40644b4ffe166d0415cc84cc157357f68d6b8c Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 20 Jun 2024 19:11:46 +0000 Subject: [PATCH] feat(enterprise): add auditing to SCIM --- coderd/audit/request.go | 15 +++++-- coderd/workspaces.go | 7 +--- enterprise/coderd/scim.go | 39 ++++++++++++++++- enterprise/coderd/scim_test.go | 36 +++++++++++++--- .../AuditLogDescription.stories.tsx | 42 +++++++++++++++++++ .../AuditLogDescription.tsx | 10 ++++- 6 files changed, 131 insertions(+), 18 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 20eb8185af53e..2171366f4c66f 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -31,7 +31,7 @@ type RequestParams struct { OrganizationID uuid.UUID Request *http.Request Action database.AuditAction - AdditionalFields json.RawMessage + AdditionalFields interface{} } type Request[T Auditable] struct { @@ -283,8 +283,15 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request } } - if p.AdditionalFields == nil { - p.AdditionalFields = json.RawMessage("{}") + additionalFieldsRaw := json.RawMessage("{}") + + if p.AdditionalFields != nil { + data, err := json.Marshal(p.AdditionalFields) + if err != nil { + p.Log.Warn(logCtx, "marshal additional fields", slog.Error(err)) + } else { + additionalFieldsRaw = json.RawMessage(data) + } } var userID uuid.UUID @@ -319,7 +326,7 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request Diff: diffRaw, StatusCode: int32(sw.Status), RequestID: httpmw.RequestID(p.Request), - AdditionalFields: p.AdditionalFields, + AdditionalFields: additionalFieldsRaw, OrganizationID: requireOrgID[T](logCtx, p.OrganizationID, p.Log), } err := p.Audit.Export(ctx, auditLog) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 22a269fc5fb7f..7e6698736eeb6 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -361,17 +361,12 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req } ) - wriBytes, err := json.Marshal(workspaceResourceInfo) - if err != nil { - api.Logger.Warn(ctx, "marshal workspace owner name") - } - aReq, commitAudit := audit.InitRequest[database.Workspace](rw, &audit.RequestParams{ Audit: *auditor, Log: api.Logger, Request: r, Action: database.AuditActionCreate, - AdditionalFields: wriBytes, + AdditionalFields: workspaceResourceInfo, OrganizationID: organization.ID, }) diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/scim.go index ca3f19fce2d3a..2e638e667e9a1 100644 --- a/enterprise/coderd/scim.go +++ b/enterprise/coderd/scim.go @@ -15,6 +15,7 @@ import ( "golang.org/x/xerrors" agpl "github.com/coder/coder/v2/coderd" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -118,6 +119,11 @@ type SCIMUser struct { } `json:"meta"` } +var SCIMAuditAdditionalFields = map[string]string{ + "automatic_actor": "coder", + "automatic_subsystem": "scim", +} + // scimPostUser creates a new user, or returns the existing user if it exists. // // @Summary SCIM 2.0: Create new user @@ -135,6 +141,16 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { return } + auditor := *api.AGPL.Auditor.Load() + aReq, commitAudit := audit.InitRequest[database.User](rw, &audit.RequestParams{ + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionCreate, + AdditionalFields: SCIMAuditAdditionalFields, + }) + defer commitAudit() + var sUser SCIMUser err := json.NewDecoder(r.Body).Decode(&sUser) if err != nil { @@ -170,7 +186,7 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { if sUser.Active && dbUser.Status == database.UserStatusSuspended { //nolint:gocritic - _, err = api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ + newUser, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ ID: dbUser.ID, // The user will get transitioned to Active after logging in. Status: database.UserStatusDormant, @@ -180,8 +196,13 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { _ = handlerutil.WriteError(rw, err) return } + aReq.New = newUser + } else { + aReq.New = dbUser } + aReq.Old = dbUser + httpapi.Write(ctx, rw, http.StatusOK, sUser) return } @@ -223,6 +244,8 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) { _ = handlerutil.WriteError(rw, err) return } + aReq.New = dbUser + aReq.UserID = dbUser.ID sUser.ID = dbUser.ID.String() sUser.UserName = dbUser.Username @@ -248,6 +271,15 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { return } + auditor := *api.AGPL.Auditor.Load() + aReq, commitAudit := audit.InitRequest[database.User](rw, &audit.RequestParams{ + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + }) + defer commitAudit() + id := chi.URLParam(r, "id") var sUser SCIMUser @@ -270,6 +302,8 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { _ = handlerutil.WriteError(rw, err) return } + aReq.Old = dbUser + aReq.UserID = dbUser.ID var status database.UserStatus if sUser.Active { @@ -280,7 +314,7 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { } //nolint:gocritic // needed for SCIM - _, err = api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ + userNew, err := api.Database.UpdateUserStatus(dbauthz.AsSystemRestricted(r.Context()), database.UpdateUserStatusParams{ ID: dbUser.ID, Status: status, UpdatedAt: dbtime.Now(), @@ -289,6 +323,7 @@ func (api *API) scimPatchUser(rw http.ResponseWriter, r *http.Request) { _ = handlerutil.WriteError(rw, err) return } + aReq.New = userNew httpapi.Write(ctx, rw, http.StatusOK, sUser) } diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go index 95d297605e1fc..237d53983a1a3 100644 --- a/enterprise/coderd/scim_test.go +++ b/enterprise/coderd/scim_test.go @@ -11,6 +11,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/coderd/audit" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" "github.com/coder/coder/v2/enterprise/coderd" @@ -109,21 +112,34 @@ func TestScim(t *testing.T) { defer cancel() scimAPIKey := []byte("hi") + mockAudit := audit.NewMock() client, _ := coderdenttest.New(t, &coderdenttest.Options{ - SCIMAPIKey: scimAPIKey, + Options: &coderdtest.Options{Auditor: mockAudit}, + SCIMAPIKey: scimAPIKey, + AuditLogging: true, LicenseOptions: &coderdenttest.LicenseOptions{ AccountID: "coolin", Features: license.Features{ - codersdk.FeatureSCIM: 1, + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, }, }, }) + mockAudit.ResetLogs() sUser := makeScimUser(t) res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) require.NoError(t, err) defer res.Body.Close() - assert.Equal(t, http.StatusOK, res.StatusCode) + require.Equal(t, http.StatusOK, res.StatusCode) + + aLogs := mockAudit.AuditLogs() + require.Len(t, aLogs, 1) + af := map[string]string{} + err = json.Unmarshal([]byte(aLogs[0].AdditionalFields), &af) + require.NoError(t, err) + assert.Equal(t, coderd.SCIMAuditAdditionalFields, af) + assert.Equal(t, database.AuditActionCreate, aLogs[0].Action) userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: sUser.Emails[0].Value}) require.NoError(t, err) @@ -306,21 +322,27 @@ func TestScim(t *testing.T) { defer cancel() scimAPIKey := []byte("hi") + mockAudit := audit.NewMock() client, _ := coderdenttest.New(t, &coderdenttest.Options{ - SCIMAPIKey: scimAPIKey, + Options: &coderdtest.Options{Auditor: mockAudit}, + SCIMAPIKey: scimAPIKey, + AuditLogging: true, LicenseOptions: &coderdenttest.LicenseOptions{ AccountID: "coolin", Features: license.Features{ - codersdk.FeatureSCIM: 1, + codersdk.FeatureSCIM: 1, + codersdk.FeatureAuditLog: 1, }, }, }) + mockAudit.ResetLogs() sUser := makeScimUser(t) res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey)) require.NoError(t, err) defer res.Body.Close() assert.Equal(t, http.StatusOK, res.StatusCode) + mockAudit.ResetLogs() err = json.NewDecoder(res.Body).Decode(&sUser) require.NoError(t, err) @@ -333,6 +355,10 @@ func TestScim(t *testing.T) { _ = res.Body.Close() assert.Equal(t, http.StatusOK, res.StatusCode) + aLogs := mockAudit.AuditLogs() + require.Len(t, aLogs, 1) + assert.Equal(t, database.AuditActionWrite, aLogs[0].Action) + userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: sUser.Emails[0].Value}) require.NoError(t, err) require.Len(t, userRes.Users, 1) diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.stories.tsx b/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.stories.tsx index 956ef1df1d903..36eb866890eb1 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.stories.tsx +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.stories.tsx @@ -56,3 +56,45 @@ export const UnsuccessfulLoginForUnknownUser: Story = { auditLog: MockAuditLogUnsuccessfulLoginKnownUser, }, }; + +export const CreateUser: Story = { + args: { + auditLog: { + ...MockAuditLog, + resource_type: "user", + resource_target: "colin", + description: "{user} created user {target}", + }, + }, +}; + +export const SCIMCreateUser: Story = { + args: { + auditLog: { + ...MockAuditLog, + resource_type: "user", + resource_target: "colin", + description: "{user} created user {target}", + additional_fields: { + automatic_actor: "coder", + automatic_subsystem: "scim", + }, + }, + }, +}; + +export const SCIMUpdateUser: Story = { + args: { + auditLog: { + ...MockAuditLog, + action: "write", + resource_type: "user", + resource_target: "colin", + description: "{user} updated user {target}", + additional_fields: { + automatic_actor: "coder", + automatic_subsystem: "scim", + }, + }, + }, +}; diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx b/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx index 3eea4ca12d6a1..e2ad2ab379dca 100644 --- a/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx +++ b/site/src/pages/AuditPage/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx @@ -12,7 +12,7 @@ export const AuditLogDescription: FC = ({ auditLog, }) => { let target = auditLog.resource_target.trim(); - const user = auditLog.user?.username.trim(); + let user = auditLog.user?.username.trim(); if (auditLog.resource_type === "workspace_build") { return ; @@ -23,6 +23,14 @@ export const AuditLogDescription: FC = ({ target = ""; } + // This occurs when SCIM creates a user. + if ( + auditLog.resource_type === "user" && + auditLog.additional_fields?.automatic_actor === "coder" + ) { + user = "Coder automatically"; + } + const truncatedDescription = auditLog.description .replace("{user}", `${user}`) .replace("{target}", "");