Skip to content

chore: add audit log tests #4764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Oct 27, 2022
Merged

chore: add audit log tests #4764

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
acf1ad3
added test for stopping a workspace build
Kira-Pilot Oct 24, 2022
2cd8b3a
Merge remote-tracking branch 'origin/main' into add-audit-coverage/ki…
Kira-Pilot Oct 26, 2022
c50db18
formatted sfriendly string; added tests
Kira-Pilot Oct 26, 2022
fdc94d8
logging unmarshal error in auditLogDescription
Kira-Pilot Oct 26, 2022
93aedaf
prettier
Kira-Pilot Oct 26, 2022
1a9936f
got rid of extra workspace word
Kira-Pilot Oct 26, 2022
3e4cb89
PR feedback
Kira-Pilot Oct 27, 2022
f669898
Merge remote-tracking branch 'origin/main' into add-audit-coverage/ki…
Kira-Pilot Oct 27, 2022
dc7e4a9
fixed mistake; wrote tests in penance
Kira-Pilot Oct 27, 2022
ac68b3c
fix be
Kira-Pilot Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions coderd/audit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package coderd

import (
"context"
"encoding/json"
"fmt"
"net"
Expand All @@ -13,6 +14,7 @@ import (
"github.com/google/uuid"
"github.com/tabbed/pqtype"

"cdr.dev/slog"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/httpapi"
"github.com/coder/coder/coderd/httpmw"
Expand Down Expand Up @@ -57,7 +59,7 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) {
}

httpapi.Write(ctx, rw, http.StatusOK, codersdk.AuditLogResponse{
AuditLogs: convertAuditLogs(dblogs),
AuditLogs: api.convertAuditLogs(ctx, dblogs),
})
}

Expand Down Expand Up @@ -165,17 +167,17 @@ func (api *API) generateFakeAuditLog(rw http.ResponseWriter, r *http.Request) {
rw.WriteHeader(http.StatusNoContent)
}

func convertAuditLogs(dblogs []database.GetAuditLogsOffsetRow) []codersdk.AuditLog {
func (api *API) convertAuditLogs(ctx context.Context, dblogs []database.GetAuditLogsOffsetRow) []codersdk.AuditLog {
alogs := make([]codersdk.AuditLog, 0, len(dblogs))

for _, dblog := range dblogs {
alogs = append(alogs, convertAuditLog(dblog))
alogs = append(alogs, api.convertAuditLog(ctx, dblog))
}

return alogs
}

func convertAuditLog(dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog {
func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog {
ip, _ := netip.AddrFromSlice(dblog.Ip.IPNet.IP)

diff := codersdk.AuditDiff{}
Expand Down Expand Up @@ -214,7 +216,7 @@ func convertAuditLog(dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog {
Diff: diff,
StatusCode: dblog.StatusCode,
AdditionalFields: dblog.AdditionalFields,
Description: auditLogDescription(dblog),
Description: api.auditLogDescription(ctx, dblog),
User: user,
}
}
Expand All @@ -223,25 +225,31 @@ type WorkspaceResourceInfo struct {
WorkspaceName string
}

func auditLogDescription(alog database.GetAuditLogsOffsetRow) string {
func (api *API) auditLogDescription(ctx context.Context, alog database.GetAuditLogsOffsetRow) string {
str := fmt.Sprintf("{user} %s %s",
codersdk.AuditAction(alog.Action).FriendlyString(),
codersdk.ResourceType(alog.ResourceType).FriendlyString(),
)

// Strings for build updates follow the below format:
// "{user} started workspace build for workspace {target}"
// where target is a workspace instead of the workspace build
// "{user} started workspace build for {target}"
// where target is a workspace instead of the workspace build.
// Note the workspace name will be bolded on the FE.
if alog.ResourceType == database.ResourceTypeWorkspaceBuild {
workspaceBytes := []byte(alog.AdditionalFields)
var workspaceResourceInfo WorkspaceResourceInfo
_ = json.Unmarshal(workspaceBytes, &workspaceResourceInfo)
str += " for workspace " + workspaceResourceInfo.WorkspaceName
err := json.Unmarshal(workspaceBytes, &workspaceResourceInfo)
if err != nil {
api.Logger.Error(ctx, "could not unmarshal workspace name for friendly string", slog.Error(err))
}
str += " for " + workspaceResourceInfo.WorkspaceName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about doing:

Suggested change
workspaceBytes := []byte(alog.AdditionalFields)
var workspaceResourceInfo WorkspaceResourceInfo
_ = json.Unmarshal(workspaceBytes, &workspaceResourceInfo)
str += " for workspace " + workspaceResourceInfo.WorkspaceName
err := json.Unmarshal(workspaceBytes, &workspaceResourceInfo)
if err != nil {
api.Logger.Error(ctx, "could not unmarshal workspace name for friendly string", slog.Error(err))
}
str += " for " + workspaceResourceInfo.WorkspaceName
str += " for {target}"

Then the frontend can replace("{target}", auditLog.additional_fields.workspaceName) which lets the backend put the target wherever it wants if we want to change up this string in the future (rather than requiring it be the last item).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much simpler; lets me remove the logger, too.

Copy link
Member

@code-asher code-asher Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets me remove the logger

good point!!

}

// We don't display the name for git ssh keys. It's fairly long and doesn't
// make too much sense to display.
if alog.ResourceType != database.ResourceTypeGitSshKey {

// The UI-visible target for workspace builds is workspace (see above block) so we don't add it to the friendly string
if alog.ResourceType != database.ResourceTypeGitSshKey && alog.ResourceType != database.ResourceTypeWorkspaceBuild {
str += " {target}"
}

Expand Down
2 changes: 1 addition & 1 deletion coderd/audit/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func ResourceTarget[T Auditable](tgt T) string {
return typed.Name
case database.WorkspaceBuild:
// this isn't used
return string(typed.BuildNumber)
return ""
case database.GitSSHKey:
return typed.PublicKey
case database.Group:
Expand Down
23 changes: 21 additions & 2 deletions coderd/workspacebuilds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,13 +536,20 @@ func TestWorkspaceBuildStatus(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
auditor := audit.NewMock()
numLogs := len(auditor.AuditLogs)
client, closeDaemon, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor})
user := coderdtest.CreateFirstUser(t, client)
numLogs++ // add an audit log for user
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
numLogs++ // add an audit log for template version

coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
closeDaemon.Close()
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
numLogs++ // add an audit log for template creation

workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
numLogs++ // add an audit log for workspace creation

// initial returned state is "pending"
require.EqualValues(t, codersdk.WorkspaceStatusPending, workspace.LatestBuild.Status)
Expand All @@ -561,11 +568,22 @@ func TestWorkspaceBuildStatus(t *testing.T) {
require.NoError(t, err)
require.EqualValues(t, codersdk.WorkspaceStatusStopped, workspace.LatestBuild.Status)

// assert an audit log has been created for workspace stopping
numLogs++ // add an audit log for workspace_build stop
require.Len(t, auditor.AuditLogs, numLogs)
require.Equal(t, database.AuditActionStop, auditor.AuditLogs[numLogs-1].Action)

_ = closeDaemon.Close()
// after successful cancel is "canceled"
build = coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStart)
err = client.CancelWorkspaceBuild(ctx, build.ID)
require.NoError(t, err)

numLogs++ // add an audit log for workspace build start
// assert an audit log has been created workspace starting
require.Len(t, auditor.AuditLogs, numLogs)
require.Equal(t, database.AuditActionStart, auditor.AuditLogs[numLogs-1].Action)

workspace, err = client.Workspace(ctx, workspace.ID)
require.NoError(t, err)
require.EqualValues(t, codersdk.WorkspaceStatusCanceled, workspace.LatestBuild.Status)
Expand All @@ -577,8 +595,9 @@ func TestWorkspaceBuildStatus(t *testing.T) {
workspace, err = client.DeletedWorkspace(ctx, workspace.ID)
require.NoError(t, err)
require.EqualValues(t, codersdk.WorkspaceStatusDeleted, workspace.LatestBuild.Status)
numLogs++ // add an audit log for workspace build deletion

// assert an audit log has been created for deletion
require.Len(t, auditor.AuditLogs, 7)
assert.Equal(t, database.AuditActionDelete, auditor.AuditLogs[6].Action)
require.Len(t, auditor.AuditLogs, numLogs)
require.Equal(t, database.AuditActionDelete, auditor.AuditLogs[numLogs-1].Action)
}
1 change: 1 addition & 0 deletions docs/admin/audit-logs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ We track **create, update and delete** events for the following resources:
- Template
- TemplateVersion
- Workspace
- Workspace start/stop
- User
- Group

Expand Down
11 changes: 10 additions & 1 deletion site/src/components/AuditLogRow/AuditLogRow.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ import TableContainer from "@material-ui/core/TableContainer"
import TableHead from "@material-ui/core/TableHead"
import TableRow from "@material-ui/core/TableRow"
import { ComponentMeta, Story } from "@storybook/react"
import { MockAuditLog, MockAuditLog2 } from "testHelpers/entities"
import {
MockAuditLog,
MockAuditLog2,
MockAuditLogWithWorkspaceBuild,
} from "testHelpers/entities"
import { AuditLogRow, AuditLogRowProps } from "./AuditLogRow"

export default {
Expand Down Expand Up @@ -38,3 +42,8 @@ WithDiff.args = {
auditLog: MockAuditLog2,
defaultIsDiffOpen: true,
}

export const WithWorkspaceBuild = Template.bind({})
WithWorkspaceBuild.args = {
auditLog: MockAuditLogWithWorkspaceBuild,
}
27 changes: 25 additions & 2 deletions site/src/components/AuditLogRow/AuditLogRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,28 @@ import userAgentParser from "ua-parser-js"
import { combineClasses } from "util/combineClasses"
import { AuditLogDiff } from "./AuditLogDiff"

const readableActionMessage = (auditLog: AuditLog) => {
// the BE returns additional_field as a string, since it's stored as JSON but
// technically, it's a map, so we adjust the type here.
type ExtendedAuditLog = Omit<AuditLog, "additional_fields"> & {
additional_fields: Record<string, string>
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not proud of this lol
I am not sure how to amend the BE type - the DB needs it stored as JSON which seems to correspond to string when make gen is run. Open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not totally sure but gonna check it out! This seems like a reasonable workaround to me for the time being though.

Copy link
Member

@code-asher code-asher Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: ignore, see next comment

So we could get closer by adding:

		case "encoding/json.RawMessage":
			return TypescriptType{ValueType: "object"}, nil

to scripts/apitypings/main.go:691. But that gets us object which we would still need to typecast (better than string though).

To fully resolve I think we might have to type the thing fully with every possible additional field of using json.RawMessage and make every property optional.

Or we have to add separate audit log types for every type of audit log that has additional fields and on the frontend we check which it is with some kind of function (a: AuditLog) => a is WorkspaceBuildAuditLog type of deal.

I feel like making it an object and casting is pretty decent for now though.

@Emyrk just in case you have some insight

Copy link
Member

@code-asher code-asher Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er wait looks like Record<string, string> is working without explicitly needing to add workspaceName to the types so we can just add this to scripts/apitypings/main.go:691:

		case "encoding/json.RawMessage":
			return TypescriptType{ValueType: "Record<string, string>"}, nil


const readableActionMessage = (auditLog: ExtendedAuditLog) => {
// workspace builds audit logs don't have targets; therefore format them differently
if (auditLog.resource_type === "workspace_build") {
// remove the "{target}" identifier in the string description as we don't use it
const amendedDescription = auditLog.description.substring(
0,
auditLog.description.lastIndexOf(" "),
)
return amendedDescription
.replace("{user}", `<strong>${auditLog.user?.username}</strong>`)
.replace(
auditLog.additional_fields.workspaceName,
`<strong>${auditLog.additional_fields.workspaceName}</strong>`,
)
Copy link
Member

@code-asher code-asher Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the replace on the name here will not match the name since it was stripped out in amendedDescription

If workspace name is hello:
"admin stopped workspace build for hello" > "admin stopped workspace build for".replace("hello", "<strong>hello</strong>") == admin stopped workspace build for
If workspace name is workspace:
"admin stopped workspace build for workspace" > "admin stopped workspace build for".replace("workspace", "<strong>workspace</strong>") == admin stopped workspace build for

If we make the {target} change in my other comment we could do the replacement with something like:

let target = auditLog.resource_target.trim()
if (auditLog.resource_type === "workspace_build") {
  target = auditLog.additional_fields.workspaceName
}
return auditLog.description
    .replace("{user}", `<strong>${auditLog.user?.username.trim()}</strong>`)
    .replace("{target}", `<strong>${target}</strong>`)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof, just realized this had the exact same issue you already called out previously - sorry to make you type it out again! I guess I didn't have my coffee this morning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrote tests in penance

😂 😂 😂

}

return auditLog.description
.replace("{user}", `<strong>${auditLog.user?.username.trim()}</strong>`)
.replace("{target}", `<strong>${auditLog.resource_target.trim()}</strong>`)
Expand Down Expand Up @@ -111,7 +132,9 @@ export const AuditLogRow: React.FC<AuditLogRowProps> = ({
>
<span
dangerouslySetInnerHTML={{
__html: readableActionMessage(auditLog),
__html: readableActionMessage(
auditLog as unknown as ExtendedAuditLog,
),
}}
/>
<span className={styles.auditLogTime}>
Expand Down
9 changes: 9 additions & 0 deletions site/src/testHelpers/entities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,15 @@ export const MockAuditLog2: TypesGen.AuditLog = {
},
}

export const MockAuditLogWithWorkspaceBuild: TypesGen.AuditLog = {
...MockAuditLog,
id: "f90995bf-4a2b-4089-b597-e66e025e523e",
request_id: "61555889-2875-475c-8494-f7693dd5d75b",
action: "stop",
resource_type: "workspace_build",
description: "{user} stopped workspace build for workspace test2",
}

export const MockWorkspaceQuota: TypesGen.WorkspaceQuota = {
user_workspace_count: 0,
user_workspace_limit: 100,
Expand Down