Skip to content

Add audit links/kira pilot #5156

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.

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 13 commits into from
Dec 2, 2022
117 changes: 102 additions & 15 deletions coderd/audit.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package coderd

import (
"context"
"database/sql"
"encoding/json"
"fmt"
Expand All @@ -14,6 +15,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 @@ -69,7 +71,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),
Count: dblogs[0].Count,
})
}
Expand Down Expand Up @@ -147,17 +149,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 @@ -197,34 +199,119 @@ func convertAuditLog(dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog {
Diff: diff,
StatusCode: dblog.StatusCode,
AdditionalFields: dblog.AdditionalFields,
Description: auditLogDescription(dblog),
User: user,
Description: auditLogDescription(dblog),
ResourceLink: api.auditLogResourceLink(ctx, dblog),
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this calls api.auditLogIsResourceDeleted as well, you are making the same database calls twice here. One way to speed this up and save the work would be to call api.auditLogIsResourceDeleted before this struct and pass it into both IsDeleted and also as an arg into api.auditLogResourceLink.

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops, you're totally right! Pushing a fix now.

IsDeleted: api.auditLogIsResourceDeleted(ctx, dblog),
}
}

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

// Strings for workspace_builds follow the below format:
// "{user} started workspace build for {target}"
// where target is a workspace instead of the workspace build,
// Strings for starting/stopping workspace builds follow the below format:
// "{user} started build for workspace {target}"
// where target is a workspace instead of a workspace build
// passed in on the FE via AuditLog.AdditionalFields rather than derived in request.go:35
if alog.ResourceType == database.ResourceTypeWorkspaceBuild {
str += " for"
if alog.ResourceType == database.ResourceTypeWorkspaceBuild && alog.Action != database.AuditActionDelete {
str += " build for"
}

// We don't display the name for git ssh keys. It's fairly long and doesn't
// We don't display the name (target) for git ssh keys. It's fairly long and doesn't
// make too much sense to display.
if alog.ResourceType != database.ResourceTypeGitSshKey {
str += " {target}"
if alog.ResourceType == database.ResourceTypeGitSshKey {
str += fmt.Sprintf(" the %s",
codersdk.ResourceType(alog.ResourceType).FriendlyString())
return str
}

str += fmt.Sprintf(" %s",
codersdk.ResourceType(alog.ResourceType).FriendlyString())

str += " {target}"

return str
}

type AdditionalFields struct {
WorkspaceName string
WorkspaceID string
BuildNumber string
}

func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.GetAuditLogsOffsetRow) bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

@coadler I'm concerned the queries in this switch will take a while to resolve. Is there a better way to determine if a resource has been deleted?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's a great way to do so without something hacky like reflection here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the only way to really fix this would be to return if the resource is deleted in the GetAuditLogsOffset query.

Copy link
Member Author

Choose a reason for hiding this comment

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

As per our Slack convo, will try to add that as a fast follow!

switch alog.ResourceType {
case database.ResourceTypeTemplate:
template, err := api.Database.GetTemplateByID(ctx, alog.ResourceID)
if err != nil {
api.Logger.Error(ctx, "could not fetch template", slog.Error(err))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also check if xerrors.Is(err, sql.ErrNoRows) { return true }, in case the assumption that rows are never deleted changes.

return template.Deleted
case database.ResourceTypeUser:
user, err := api.Database.GetUserByID(ctx, alog.ResourceID)
if err != nil {
api.Logger.Error(ctx, "could not fetch user", slog.Error(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit; normally we don't prefix log lines with could not/failed to because the error level implies it

}
return user.Deleted
case database.ResourceTypeWorkspace:
workspace, err := api.Database.GetWorkspaceByID(ctx, alog.ResourceID)
if err != nil {
api.Logger.Error(ctx, "could not fetch workspace", slog.Error(err))
}
return workspace.Deleted
case database.ResourceTypeWorkspaceBuild:
additionalFieldsBytes := []byte(alog.AdditionalFields)
var additionalFields AdditionalFields
err := json.Unmarshal(additionalFieldsBytes, &additionalFields)
if err != nil {
api.Logger.Error(ctx, "could not unmarshal workspace ID", slog.Error(err))
}
// if we don't have a WorkspaceID, we return true so as to hide the link in the UI
if len(additionalFields.WorkspaceID) < 1 {
return true
}
// We use workspace as a proxy for workspace build here
workspace, err := api.Database.GetWorkspaceByID(ctx, uuid.MustParse(additionalFields.WorkspaceID))
if err != nil {
api.Logger.Error(ctx, "could not fetch workspace", slog.Error(err))
}
return workspace.Deleted
default:
return false
}
}

func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAuditLogsOffsetRow) string {
if api.auditLogIsResourceDeleted(ctx, alog) {
return ""
}

switch alog.ResourceType {
case database.ResourceTypeTemplate:
return fmt.Sprintf("/templates/%s",
alog.ResourceTarget)
case database.ResourceTypeUser:
return fmt.Sprintf("/users?filter=%s",
alog.ResourceTarget)
case database.ResourceTypeWorkspace:
return fmt.Sprintf("/@%s/%s",
alog.UserUsername.String, alog.ResourceTarget)
case database.ResourceTypeWorkspaceBuild:
additionalFieldsBytes := []byte(alog.AdditionalFields)
var additionalFields AdditionalFields
err := json.Unmarshal(additionalFieldsBytes, &additionalFields)
if err != nil {
api.Logger.Error(ctx, "could not unmarshal workspace name", slog.Error(err))
}
return fmt.Sprintf("/@%s/%s/builds/%s",
alog.UserUsername.String, additionalFields.WorkspaceName, additionalFields.BuildNumber)
default:
return ""
}
}

// auditSearchQuery takes a query string and returns the auditLog filter.
// It also can return the list of validation errors to return to the api.
func auditSearchQuery(query string) (database.GetAuditLogsOffsetParams, []codersdk.ValidationError) {
Expand Down
15 changes: 11 additions & 4 deletions coderd/provisionerdserver/provisionerdserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"net/http"
"net/url"
"reflect"
"strconv"
"sync"
"sync/atomic"
"time"
Expand Down Expand Up @@ -537,10 +538,13 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p
if getWorkspaceErr != nil {
server.Logger.Error(ctx, "failed to create audit log - get workspace err", slog.Error(err))
} else {
// We pass the workspace name to the Auditor so that it
// can form a friendly string for the user.
fmt.Println("Hello World! 1", workspace.ID.String())
// We pass the below information to the Auditor so that it
// can form a friendly string for the user to view in the UI.
workspaceResourceInfo := map[string]string{
"workspaceName": workspace.Name,
"workspaceId": workspace.ID.String(),
"buildNumber": strconv.FormatInt(int64(build.BuildNumber), 10),
}

wriBytes, err := json.Marshal(workspaceResourceInfo)
Expand Down Expand Up @@ -751,11 +755,14 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete
if getWorkspaceError == nil {
auditor := server.Auditor.Load()
auditAction := auditActionFromTransition(workspaceBuild.Transition)
fmt.Println("Hello World! 2", workspace.ID.String())

// We pass the workspace name to the Auditor so that it
// can form a friendly string for the user.
// We pass the below information to the Auditor so that it
// can form a friendly string for the user to view in the UI.
workspaceResourceInfo := map[string]string{
"workspaceName": workspace.Name,
"workspaceId": workspace.ID.String(),
"buildNumber": strconv.FormatInt(int64(workspaceBuild.BuildNumber), 10),
}

wriBytes, err := json.Marshal(workspaceResourceInfo)
Expand Down
6 changes: 5 additions & 1 deletion codersdk/audit.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ func (r ResourceType) FriendlyString() string {
case ResourceTypeWorkspace:
return "workspace"
case ResourceTypeWorkspaceBuild:
return "workspace build"
// workspace builds have a unique friendly string
// see coderd/audit.go:298 for explanation
return "workspace"
case ResourceTypeGitSSHKey:
return "git ssh key"
case ResourceTypeAPIKey:
Expand Down Expand Up @@ -102,6 +104,8 @@ type AuditLog struct {
StatusCode int32 `json:"status_code"`
AdditionalFields json.RawMessage `json:"additional_fields"`
Description string `json:"description"`
ResourceLink string `json:"resource_link"`
IsDeleted bool `json:"is_deleted"`

User *User `json:"user"`
}
Expand Down
2 changes: 2 additions & 0 deletions site/src/api/typesGenerated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ export interface AuditLog {
readonly status_code: number
readonly additional_fields: Record<string, string>
readonly description: string
readonly resource_link: string
readonly is_deleted: boolean
readonly user?: User
}

Expand Down
63 changes: 63 additions & 0 deletions site/src/components/AuditLogRow/AuditLogDescription.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { FC } from "react"
import { AuditLog } from "api/typesGenerated"
import { Link as RouterLink } from "react-router-dom"
import Link from "@material-ui/core/Link"
import { makeStyles } from "@material-ui/core/styles"
import i18next from "i18next"

export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({
auditLog,
}): JSX.Element => {
const classes = useStyles()
const { t } = i18next

let target = auditLog.resource_target.trim()

// audit logs with a resource_type of workspace build use workspace name as a target
if (
auditLog.resource_type === "workspace_build" &&
auditLog.additional_fields.workspaceName
) {
target = auditLog.additional_fields.workspaceName.trim()
}

// SSH key entries have no links
if (auditLog.resource_type === "git_ssh_key") {
return (
<span>
{auditLog.description
.replace("{user}", `${auditLog.user?.username.trim()}`)
.replace("{target}", `${target}`)}
</span>
)
}

const truncatedDescription = auditLog.description
.replace("{user}", `${auditLog.user?.username.trim()}`)
.replace("{target}", "")

return (
<span>
{truncatedDescription}
{auditLog.resource_link ? (
<Link component={RouterLink} to={auditLog.resource_link}>
<strong>{target}</strong>
</Link>
Copy link
Member Author

Choose a reason for hiding this comment

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

Having a link on workspace name for workspace_build strings is a little awkward - it might be better if this link was built around the word build instead of on the workspace target. However, that add a surprising amount of complexity, so I figured this was okay for now. If you object, let me know!

) : (
<strong>{target}</strong>
)}
{auditLog.is_deleted && (
<span className={classes.deletedLabel}>
<> {t("auditLog:table.logRow.deletedLabel")}</>
</span>
)}
</span>
)
}

const useStyles = makeStyles((theme) => ({
deletedLabel: {
...theme.typography.caption,
color: theme.palette.text.secondary,
},
}))
46 changes: 20 additions & 26 deletions site/src/components/AuditLogRow/AuditLogRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,8 @@ import { PaletteIndex } from "theme/palettes"
import userAgentParser from "ua-parser-js"
import { combineClasses } from "util/combineClasses"
import { AuditLogDiff } from "./AuditLogDiff"

export const readableActionMessage = (auditLog: AuditLog): string => {
let target = auditLog.resource_target.trim()

// audit logs with a resource_type of workspace build use workspace name as a target
if (
auditLog.resource_type === "workspace_build" &&
auditLog.additional_fields.workspaceName
) {
target = auditLog.additional_fields.workspaceName.trim()
}

return auditLog.description
.replace("{user}", `<strong>${auditLog.user?.username.trim()}</strong>`)
.replace("{target}", `<strong>${target}</strong>`)
}
import i18next from "i18next"
import { AuditLogDescription } from "./AuditLogDescription"

const httpStatusColor = (httpStatus: number): PaletteIndex => {
if (httpStatus >= 300 && httpStatus < 500) {
Expand All @@ -55,14 +41,14 @@ export const AuditLogRow: React.FC<AuditLogRowProps> = ({
defaultIsDiffOpen = false,
}) => {
const styles = useStyles()
const { t } = i18next
const [isDiffOpen, setIsDiffOpen] = useState(defaultIsDiffOpen)
const diffs = Object.entries(auditLog.diff)
const shouldDisplayDiff = diffs.length > 0
const { os, browser } = userAgentParser(auditLog.user_agent)
const notAvailableLabel = "Not available"
const displayBrowserInfo = browser.name
? `${browser.name} ${browser.version}`
: notAvailableLabel
: t("auditLog:table.logRow.notAvailable")

const toggle = () => {
if (shouldDisplayDiff) {
Expand Down Expand Up @@ -119,11 +105,7 @@ export const AuditLogRow: React.FC<AuditLogRowProps> = ({
alignItems="baseline"
spacing={1}
>
<span
dangerouslySetInnerHTML={{
__html: readableActionMessage(auditLog),
}}
/>
<AuditLogDescription auditLog={auditLog} />
<span className={styles.auditLogTime}>
{new Date(auditLog.time).toLocaleTimeString()}
</span>
Expand All @@ -132,15 +114,27 @@ export const AuditLogRow: React.FC<AuditLogRowProps> = ({
<Stack direction="row" alignItems="center">
<Stack direction="row" spacing={1} alignItems="baseline">
<span className={styles.auditLogInfo}>
IP: <strong>{auditLog.ip ?? notAvailableLabel}</strong>
<>{t("auditLog:table.logRow.ip")}</>
<strong>
{auditLog.ip
? auditLog.ip
: t("auditLog:table.logRow.notAvailable")}
</strong>
</span>

<span className={styles.auditLogInfo}>
OS: <strong>{os.name ?? notAvailableLabel}</strong>
<>{t("auditLog:table.logRow.os")}</>
<strong>
{os.name
? os.name
: // https://github.com/i18next/next-i18next/issues/1795
t<string>("auditLog:table.logRow.notAvailable")}
</strong>
</span>

<span className={styles.auditLogInfo}>
Browser: <strong>{displayBrowserInfo}</strong>
<>{t("auditLog:table.logRow.browser")}</>
<strong>{displayBrowserInfo}</strong>
</span>
</Stack>

Expand Down
Loading