From 0a0adfd2acb96fef12d65cc87e1a93d1eb54d7ab Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Mon, 30 Jan 2023 20:24:03 +0000 Subject: [PATCH 01/12] added migration for api key resource --- coderd/apidoc/docs.go | 8 ++++++-- coderd/apidoc/swagger.json | 6 ++++-- coderd/apikey.go | 14 +++++++------- coderd/audit.go | 10 ++++++++++ coderd/audit/request.go | 8 ++++++++ coderd/database/dump.sql | 4 +++- .../000094_add_resource_type_api_key.down.sql | 2 ++ .../000094_add_resource_type_api_key.up.sql | 9 +++++++++ coderd/database/models.go | 8 +++++++- coderd/userauth.go | 2 +- coderd/users.go | 18 ++++++++++++++++-- coderd/workspaceapps.go | 2 +- codersdk/audit.go | 6 ++++++ docs/admin/audit-logs.md | 1 + docs/api/schemas.md | 2 ++ enterprise/audit/table.go | 16 +++++++++++++++- site/src/api/typesGenerated.ts | 11 ++++++++++- 17 files changed, 108 insertions(+), 19 deletions(-) create mode 100644 coderd/database/migrations/000094_add_resource_type_api_key.down.sql create mode 100644 coderd/database/migrations/000094_add_resource_type_api_key.up.sql diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index e72a68fd6c105..2b12203488fdf 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -5336,14 +5336,18 @@ const docTemplate = `{ "write", "delete", "start", - "stop" + "stop", + "login", + "logout" ], "x-enum-varnames": [ "AuditActionCreate", "AuditActionWrite", "AuditActionDelete", "AuditActionStart", - "AuditActionStop" + "AuditActionStop", + "AuditActionLogin", + "AuditActionLogout" ] }, "codersdk.AuditDiff": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 1237855b4c4c3..30fb348279cf2 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -4718,13 +4718,15 @@ }, "codersdk.AuditAction": { "type": "string", - "enum": ["create", "write", "delete", "start", "stop"], + "enum": ["create", "write", "delete", "start", "stop", "login", "logout"], "x-enum-varnames": [ "AuditActionCreate", "AuditActionWrite", "AuditActionDelete", "AuditActionStart", - "AuditActionStop" + "AuditActionStop", + "AuditActionLogin", + "AuditActionLogout" ] }, "codersdk.AuditDiff": { diff --git a/coderd/apikey.go b/coderd/apikey.go index 5a8d7882eb7ad..1269aa232ed51 100644 --- a/coderd/apikey.go +++ b/coderd/apikey.go @@ -70,7 +70,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) { return } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypeToken, ExpiresAt: database.Now().Add(lifeTime), @@ -108,7 +108,7 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { } lifeTime := time.Hour * 24 * 7 - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypePassword, RemoteAddr: r.RemoteAddr, @@ -281,10 +281,10 @@ func (api *API) validateAPIKeyLifetime(lifetime time.Duration) error { return nil } -func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, error) { +func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, *database.APIKey, error) { keyID, keySecret, err := GenerateAPIKeyIDSecret() if err != nil { - return nil, xerrors.Errorf("generate API key: %w", err) + return nil, nil, xerrors.Errorf("generate API key: %w", err) } hashed := sha256.Sum256([]byte(keySecret)) @@ -310,7 +310,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h switch scope { case database.APIKeyScopeAll, database.APIKeyScopeApplicationConnect: default: - return nil, xerrors.Errorf("invalid API key scope: %q", scope) + return nil, nil, xerrors.Errorf("invalid API key scope: %q", scope) } key, err := api.Database.InsertAPIKey(ctx, database.InsertAPIKeyParams{ @@ -333,7 +333,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h Scope: scope, }) if err != nil { - return nil, xerrors.Errorf("insert API key: %w", err) + return nil, nil, xerrors.Errorf("insert API key: %w", err) } api.Telemetry.Report(&telemetry.Snapshot{ @@ -349,5 +349,5 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: api.SecureAuthCookie, - }, nil + }, &key, nil } diff --git a/coderd/audit.go b/coderd/audit.go index 23a2375729b1e..e40932e9dd32d 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -264,6 +264,12 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow, additionalFields A codersdk.AuditAction(alog.Action).Friendly(), ) + // API Key resources do not have targets and follow the below format: + // "User {logged in | logged out}" + if alog.ResourceType == database.ResourceTypeApiKey { + return str + } + // Strings for starting/stopping workspace builds follow the below format: // "{user | 'Coder automatically'} started build #{build_number} for workspace {target}" // where target is a workspace (name) instead of a workspace build @@ -484,6 +490,10 @@ func actionFromString(actionString string) string { return actionString case codersdk.AuditActionStop: return actionString + case codersdk.AuditActionLogin: + return actionString + case codersdk.AuditActionLogout: + return actionString default: } return "" diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 9127a66e446ea..01e1e30ccb46b 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -64,6 +64,9 @@ func ResourceTarget[T Auditable](tgt T) string { return typed.PublicKey case database.AuditableGroup: return typed.Group.Name + case database.APIKey: + // this isn't used + return "" default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -85,6 +88,9 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { return typed.UserID case database.AuditableGroup: return typed.Group.ID + case database.APIKey: + // this doesn't seem right + return typed.UserID default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -106,6 +112,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { return database.ResourceTypeGitSshKey case database.AuditableGroup: return database.ResourceTypeGroup + case database.APIKey: + return database.ResourceTypeApiKey default: panic(fmt.Sprintf("unknown resource %T", tgt)) } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 664f2648e2123..7a6d29b9c6001 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -16,7 +16,9 @@ CREATE TYPE audit_action AS ENUM ( 'write', 'delete', 'start', - 'stop' + 'stop', + 'login', + 'logout' ); CREATE TYPE build_reason AS ENUM ( diff --git a/coderd/database/migrations/000094_add_resource_type_api_key.down.sql b/coderd/database/migrations/000094_add_resource_type_api_key.down.sql new file mode 100644 index 0000000000000..d1d1637f4fa90 --- /dev/null +++ b/coderd/database/migrations/000094_add_resource_type_api_key.down.sql @@ -0,0 +1,2 @@ +-- It's not possible to drop enum values from enum types, so the UP has "IF NOT +-- EXISTS". diff --git a/coderd/database/migrations/000094_add_resource_type_api_key.up.sql b/coderd/database/migrations/000094_add_resource_type_api_key.up.sql new file mode 100644 index 0000000000000..15aac87d81a64 --- /dev/null +++ b/coderd/database/migrations/000094_add_resource_type_api_key.up.sql @@ -0,0 +1,9 @@ +ALTER TYPE audit_action + ADD VALUE IF NOT EXISTS 'login'; + +ALTER TYPE audit_action + ADD VALUE IF NOT EXISTS 'logout'; + +ALTER TYPE resource_type + ADD VALUE IF NOT EXISTS 'api_key'; + diff --git a/coderd/database/models.go b/coderd/database/models.go index 8496c75df2114..e0520428451a8 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -144,6 +144,8 @@ const ( AuditActionDelete AuditAction = "delete" AuditActionStart AuditAction = "start" AuditActionStop AuditAction = "stop" + AuditActionLogin AuditAction = "login" + AuditActionLogout AuditAction = "logout" ) func (e *AuditAction) Scan(src interface{}) error { @@ -187,7 +189,9 @@ func (e AuditAction) Valid() bool { AuditActionWrite, AuditActionDelete, AuditActionStart, - AuditActionStop: + AuditActionStop, + AuditActionLogin, + AuditActionLogout: return true } return false @@ -200,6 +204,8 @@ func AllAuditActionValues() []AuditAction { AuditActionDelete, AuditActionStart, AuditActionStop, + AuditActionLogin, + AuditActionLogout, } } diff --git a/coderd/userauth.go b/coderd/userauth.go index 3fbc1c8f00bfa..3b85e62d5f260 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -602,7 +602,7 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook return nil, xerrors.Errorf("in tx: %w", err) } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: params.LoginType, RemoteAddr: r.RemoteAddr, diff --git a/coderd/users.go b/coderd/users.go index 6c237f421c2d8..02939a9176c3d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -995,7 +995,19 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques // @Success 201 {object} codersdk.LoginWithPasswordResponse // @Router /users/login [post] func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() + var ( + ctx = r.Context() + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) + ) + + defer commitAudit() + var loginWithPassword codersdk.LoginWithPasswordRequest if !httpapi.Read(ctx, rw, r, &loginWithPassword) { return @@ -1043,7 +1055,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, key, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypePassword, RemoteAddr: r.RemoteAddr, @@ -1056,6 +1068,8 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } + aReq.New = *key + http.SetCookie(rw, cookie) httpapi.Write(ctx, rw, http.StatusCreated, codersdk.LoginWithPasswordResponse{ diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 168345bc2d27e..8c11277a3d192 100644 --- a/coderd/workspaceapps.go +++ b/coderd/workspaceapps.go @@ -745,7 +745,7 @@ func (api *API) workspaceApplicationAuth(rw http.ResponseWriter, r *http.Request if lifetime > int64((time.Hour * 24 * 7).Seconds()) { lifetime = int64((time.Hour * 24 * 7).Seconds()) } - cookie, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: apiKey.UserID, LoginType: database.LoginTypePassword, ExpiresAt: exp, diff --git a/codersdk/audit.go b/codersdk/audit.go index 38def7f709de3..73f1da91f1b8b 100644 --- a/codersdk/audit.go +++ b/codersdk/audit.go @@ -57,6 +57,8 @@ const ( AuditActionDelete AuditAction = "delete" AuditActionStart AuditAction = "start" AuditActionStop AuditAction = "stop" + AuditActionLogin AuditAction = "login" + AuditActionLogout AuditAction = "logout" ) func (a AuditAction) Friendly() string { @@ -71,6 +73,10 @@ func (a AuditAction) Friendly() string { return "started" case AuditActionStop: return "stopped" + case AuditActionLogin: + return "logged in" + case AuditActionLogout: + return "logged out" default: return "unknown" } diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index 192dacc2c8a75..cd5e388d8ed97 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -11,6 +11,7 @@ We track the following resources: | Resource | | | ----------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_userfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
| | Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| | GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 5166ff9c0b555..fc391abe3ba02 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -484,6 +484,8 @@ | `delete` | | `start` | | `stop` | +| `login` | +| `logout` | ## codersdk.AuditDiff diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 6e4bcfebba935..089d3fa73f33e 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -22,6 +22,7 @@ var AuditActionMap = map[string][]codersdk.AuditAction{ "Workspace": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "WorkspaceBuild": {codersdk.AuditActionStart, codersdk.AuditActionStop}, "Group": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, + "APIKey": {codersdk.AuditActionWrite}, } type Action string @@ -109,7 +110,6 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "ttl": ActionTrack, "last_used_at": ActionIgnore, }, - // We don't show any diff for the WorkspaceBuild resource &database.WorkspaceBuild{}: { "id": ActionIgnore, "created_at": ActionIgnore, @@ -133,6 +133,20 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "quota_allowance": ActionTrack, "members": ActionTrack, }, + // We don't show any diff for the APIKey resource + &database.APIKey{}: { + "id": ActionIgnore, + "hashed_secret": ActionIgnore, + "user_id": ActionIgnore, + "last_user": ActionIgnore, + "expires_at": ActionIgnore, + "created_at": ActionIgnore, + "updated_at": ActionIgnore, + "login_type": ActionIgnore, + "lifetime_seconds": ActionIgnore, + "ip_address": ActionIgnore, + "scope": ActionIgnore, + }, }) // auditMap converts a map of struct pointers to a map of struct names as diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index dd4fcbf8f7c99..409322bfc219f 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1030,10 +1030,19 @@ export type APIKeyScope = "all" | "application_connect" export const APIKeyScopes: APIKeyScope[] = ["all", "application_connect"] // From codersdk/audit.go -export type AuditAction = "create" | "delete" | "start" | "stop" | "write" +export type AuditAction = + | "create" + | "delete" + | "login" + | "logout" + | "start" + | "stop" + | "write" export const AuditActions: AuditAction[] = [ "create", "delete", + "login", + "logout", "start", "stop", "write", From 1acc73c016a0e68d61231ba5bfcdb2e5fd218b4a Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Mon, 30 Jan 2023 21:33:12 +0000 Subject: [PATCH 02/12] sort of working --- coderd/audit/request.go | 2 +- coderd/users.go | 1 + docs/admin/audit-logs.md | 2 +- enterprise/audit/table.go | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 01e1e30ccb46b..3f934e6c3aefe 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -162,7 +162,7 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request auditLog := database.AuditLog{ ID: uuid.New(), Time: database.Now(), - UserID: httpmw.APIKey(p.Request).UserID, + UserID: uuid.Nil, Ip: ip, UserAgent: sql.NullString{String: p.Request.UserAgent(), Valid: true}, ResourceType: either(req.Old, req.New, ResourceType[T]), diff --git a/coderd/users.go b/coderd/users.go index 02939a9176c3d..1f5043cd36c52 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1068,6 +1068,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } + // key := httpmw.APIKey(r) aReq.New = *key http.SetCookie(rw, cookie) diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index cd5e388d8ed97..a705b9e371d56 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -11,7 +11,7 @@ We track the following resources: | Resource | | | ----------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_userfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
| +| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
| | Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| | GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 089d3fa73f33e..ffc0f303bd25a 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -138,7 +138,7 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "id": ActionIgnore, "hashed_secret": ActionIgnore, "user_id": ActionIgnore, - "last_user": ActionIgnore, + "last_used": ActionIgnore, "expires_at": ActionIgnore, "created_at": ActionIgnore, "updated_at": ActionIgnore, From a75392ef29915d46d589d6759f743ff481dae3e9 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 1 Feb 2023 20:13:56 +0000 Subject: [PATCH 03/12] auditing login --- .../AuditLogRow/AuditLogDescription.test.tsx | 22 +++++++++++++++++++ .../AuditLogRow/AuditLogDescription.tsx | 6 +++-- .../components/AuditLogRow/AuditLogRow.tsx | 2 +- site/src/i18n/en/auditLog.json | 3 ++- site/src/testHelpers/entities.ts | 20 +++++++++++++++++ 5 files changed, 49 insertions(+), 4 deletions(-) diff --git a/site/src/components/AuditLogRow/AuditLogDescription.test.tsx b/site/src/components/AuditLogRow/AuditLogDescription.test.tsx index 04bef1a114c77..455642ebfe53a 100644 --- a/site/src/components/AuditLogRow/AuditLogDescription.test.tsx +++ b/site/src/components/AuditLogRow/AuditLogDescription.test.tsx @@ -2,8 +2,12 @@ import { MockAuditLog, MockAuditLogWithWorkspaceBuild, MockWorkspaceCreateAuditLogForDifferentOwner, + MockAuditLogSuccessfulLogin, + MockAuditLogUnsuccessfulLoginKnownUser, + MockAuditLogUnsuccessfulLoginUnknownUser, } from "testHelpers/entities" import { AuditLogDescription } from "./AuditLogDescription" +import { AuditLogRow } from "./AuditLogRow" import { render } from "../../testHelpers/renderHelpers" import { screen } from "@testing-library/react" @@ -59,4 +63,22 @@ describe("AuditLogDescription", () => { ), ).toBeDefined() }) + it("renders the correct string for successful login", async () => { + render() + expect(getByTextContent(`TestUser logged in`)).toBeDefined() + const statusPill = screen.getByRole("status") + expect(statusPill).toHaveTextContent("201") + }) + it("renders the correct string for unsuccessful login for a known user", async () => { + render() + expect(getByTextContent(`TestUser logged in`)).toBeDefined() + const statusPill = screen.getByRole("status") + expect(statusPill).toHaveTextContent("401") + }) + it("renders the correct string for unsuccessful login for an unknown user", async () => { + render() + expect(getByTextContent(`An unknown user logged in`)).toBeDefined() + const statusPill = screen.getByRole("status") + expect(statusPill).toHaveTextContent("401") + }) }) diff --git a/site/src/components/AuditLogRow/AuditLogDescription.tsx b/site/src/components/AuditLogRow/AuditLogDescription.tsx index 11167cbfe50bc..a17f88073f95a 100644 --- a/site/src/components/AuditLogRow/AuditLogDescription.tsx +++ b/site/src/components/AuditLogRow/AuditLogDescription.tsx @@ -12,7 +12,9 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({ const { t } = i18next let target = auditLog.resource_target.trim() - let user = auditLog.user?.username.trim() + let user = auditLog.user + ? auditLog.user.username.trim() + : t("auditLog:table.logRow.unknownUser") if (auditLog.resource_type === "workspace_build") { // audit logs with a resource_type of workspace build use workspace name as a target @@ -22,7 +24,7 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({ auditLog.additional_fields?.build_reason && auditLog.additional_fields?.build_reason !== "initiator" ? t("auditLog:table.logRow.buildReason") - : auditLog.user?.username.trim() + : user } // SSH key entries have no links diff --git a/site/src/components/AuditLogRow/AuditLogRow.tsx b/site/src/components/AuditLogRow/AuditLogRow.tsx index 10719bbe34b4f..ba84c299b79af 100644 --- a/site/src/components/AuditLogRow/AuditLogRow.tsx +++ b/site/src/components/AuditLogRow/AuditLogRow.tsx @@ -93,7 +93,7 @@ export const AuditLogRow: React.FC = ({ className={styles.fullWidth} > diff --git a/site/src/i18n/en/auditLog.json b/site/src/i18n/en/auditLog.json index c0b9c5864d97a..454a1f9853d70 100644 --- a/site/src/i18n/en/auditLog.json +++ b/site/src/i18n/en/auditLog.json @@ -14,7 +14,8 @@ "browser": "Browser: ", "notAvailable": "Not available", "onBehalfOf": " on behalf of {{owner}}", - "buildReason": "Coder automatically" + "buildReason": "Coder automatically", + "unknownUser": "An unknown user" } }, "paywall": { diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 6456fb3abf442..41c3cb176fc1d 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1117,6 +1117,26 @@ export const MockAuditLogGitSSH: TypesGen.AuditLog = { }, } +export const MockAuditLogSuccessfulLogin: TypesGen.AuditLog = { + ...MockAuditLog, + resource_type: "api_key", + resource_target: "", + action: "login", + status_code: 201, + description: "{user} logged in", +} + +export const MockAuditLogUnsuccessfulLoginKnownUser: TypesGen.AuditLog = { + ...MockAuditLogSuccessfulLogin, + status_code: 401, +} + +export const MockAuditLogUnsuccessfulLoginUnknownUser: TypesGen.AuditLog = { + ...MockAuditLogSuccessfulLogin, + status_code: 401, + user: undefined, +} + export const MockWorkspaceQuota: TypesGen.WorkspaceQuota = { credits_consumed: 0, budget: 100, From 4c754d3aeea3e7bca1a0dc07837fca8aca26fd22 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 1 Feb 2023 20:14:24 +0000 Subject: [PATCH 04/12] passing the correct user id --- coderd/audit/request.go | 16 ++++++++++++---- coderd/users.go | 25 ++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 3f934e6c3aefe..0ac85ea0d9d67 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -29,8 +29,9 @@ type RequestParams struct { type Request[T Auditable] struct { params *RequestParams - Old T - New T + Old T + New T + UserId uuid.UUID } type BuildAuditParams[T Auditable] struct { @@ -89,7 +90,6 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { case database.AuditableGroup: return typed.Group.ID case database.APIKey: - // this doesn't seem right return typed.UserID default: panic(fmt.Sprintf("unknown resource %T", tgt)) @@ -158,11 +158,19 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request p.AdditionalFields = json.RawMessage("{}") } + var userID uuid.UUID + key, ok := httpmw.APIKeyOptional(p.Request) + if ok { + userID = key.UserID + } else { + userID = req.UserId + } + ip := parseIP(p.Request.RemoteAddr) auditLog := database.AuditLog{ ID: uuid.New(), Time: database.Now(), - UserID: uuid.Nil, + UserID: userID, Ip: ip, UserAgent: sql.NullString{String: p.Request.UserAgent(), Valid: true}, ResourceType: either(req.Old, req.New, ResourceType[T]), diff --git a/coderd/users.go b/coderd/users.go index 1f5043cd36c52..19c88abf14db9 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1005,11 +1005,15 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { Action: database.AuditActionLogin, }) ) + aReq.Old = database.APIKey{} defer commitAudit() var loginWithPassword codersdk.LoginWithPasswordRequest if !httpapi.Read(ctx, rw, r, &loginWithPassword) { + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -1020,15 +1024,23 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } + aReq.UserId = user.ID + // If the user doesn't exist, it will be a default struct. equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } if !equal { @@ -1037,6 +1049,9 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Incorrect email or password.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -1044,6 +1059,9 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -1052,6 +1070,9 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Your account is suspended. Contact an admin to reactivate your account.", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -1065,10 +1086,12 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { Message: "Failed to create API key.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } - // key := httpmw.APIKey(r) aReq.New = *key http.SetCookie(rw, cookie) From 622733ca1654f616632845f3b69e93d7f205d85d Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 1 Feb 2023 22:03:19 +0000 Subject: [PATCH 05/12] added and fixed tests --- coderd/gitsshkey_test.go | 4 +- coderd/templates_test.go | 17 +++--- coderd/templateversions_test.go | 8 +-- coderd/users_test.go | 92 +++++++++++++++++++++++++++------ coderd/workspacebuilds_test.go | 1 + coderd/workspaces_test.go | 12 ++--- 6 files changed, 98 insertions(+), 36 deletions(-) diff --git a/coderd/gitsshkey_test.go b/coderd/gitsshkey_test.go index 7a0932c5ec62b..6b3e860d5c0e2 100644 --- a/coderd/gitsshkey_test.go +++ b/coderd/gitsshkey_test.go @@ -95,8 +95,8 @@ func TestGitSSHKey(t *testing.T) { require.NotEmpty(t, key2.PublicKey) require.NotEqual(t, key2.PublicKey, key1.PublicKey) - require.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action) + require.Len(t, auditor.AuditLogs, 2) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) }) } diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 73a846fe35991..8336ff608d778 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -96,10 +96,11 @@ func TestPostTemplateByOrganization(t *testing.T) { assert.Equal(t, expected.Name, got.Name) assert.Equal(t, expected.Description, got.Description) - require.Len(t, auditor.AuditLogs, 3) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[2].Action) + require.Len(t, auditor.AuditLogs, 4) + assert.Equal(t, database.AuditActionLogin, auditor.AuditLogs[0].Action) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[1].Action) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[2].Action) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[3].Action) }) t.Run("AlreadyExists", func(t *testing.T) { @@ -320,8 +321,8 @@ func TestPatchTemplateMeta(t *testing.T) { assert.Equal(t, req.DefaultTTLMillis, updated.DefaultTTLMillis) assert.False(t, req.AllowUserCancelWorkspaceJobs) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action) }) t.Run("NoMaxTTL", func(t *testing.T) { @@ -483,8 +484,8 @@ func TestDeleteTemplate(t *testing.T) { err := client.DeleteTemplate(ctx, template.ID) require.NoError(t, err) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionDelete, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionDelete, auditor.AuditLogs[4].Action) }) t.Run("Workspaces", func(t *testing.T) { diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index adf35aeb1c21b..a13a13f939d99 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -126,8 +126,8 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { require.Equal(t, "bananas", version.Name) require.Equal(t, provisionerdserver.ScopeOrganization, version.Job.Tags[provisionerdserver.TagScope]) - require.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action) + require.Len(t, auditor.AuditLogs, 2) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[1].Action) }) t.Run("Example", func(t *testing.T) { t.Parallel() @@ -645,8 +645,8 @@ func TestPatchActiveTemplateVersion(t *testing.T) { }) require.NoError(t, err) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action) }) } diff --git a/coderd/users_test.go b/coderd/users_test.go index 7e6932073b5bd..9e32ffc326741 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -10,7 +10,6 @@ import ( "time" "github.com/google/uuid" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" @@ -121,7 +120,9 @@ func TestPostLogin(t *testing.T) { t.Parallel() t.Run("InvalidUser", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -130,14 +131,20 @@ func TestPostLogin(t *testing.T) { Email: "my@email.org", Password: "password", }) + numLogs++ // add an audit log for login var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("BadPassword", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -153,17 +160,26 @@ func TestPostLogin(t *testing.T) { Email: req.Email, Password: "badpass", }) + numLogs++ // add an audit log for login var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Suspended", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) first := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for create user + numLogs++ // add an audit log for login member := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + numLogs++ // add an audit log for create user ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -173,6 +189,7 @@ func TestPostLogin(t *testing.T) { _, err = client.UpdateUserStatus(ctx, memberUser.Username, codersdk.UserStatusSuspended) require.NoError(t, err, "suspend member") + numLogs++ // add an audit log for update user // Test an existing session _, err = member.User(ctx, codersdk.Me) @@ -186,14 +203,20 @@ func TestPostLogin(t *testing.T) { Email: memberUser.Email, Password: "testpass", }) + numLogs++ // add an audit log for login require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) require.Contains(t, apiErr.Message, "suspended") + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Success", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -204,7 +227,8 @@ func TestPostLogin(t *testing.T) { Password: "testpass", } _, err := client.CreateFirstUser(ctx, req) - require.NoError(t, err) + numLogs++ // add an audit log for create user + numLogs++ // add an audit log for login _, err = client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ Email: req.Email, @@ -218,6 +242,9 @@ func TestPostLogin(t *testing.T) { Password: req.Password, }) require.NoError(t, err) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Lifetime&Expire", func(t *testing.T) { @@ -421,7 +448,11 @@ func TestPostUsers(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + user := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -434,8 +465,9 @@ func TestPostUsers(t *testing.T) { }) require.NoError(t, err) - require.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionCreate, auditor.AuditLogs[numLogs-1].Action) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-2].Action) }) } @@ -486,7 +518,10 @@ func TestUpdateUserProfile(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -497,8 +532,10 @@ func TestUpdateUserProfile(t *testing.T) { }) require.NoError(t, err) require.Equal(t, userProfile.Username, "newusername") - assert.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action) + numLogs++ // add an audit log for user update + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) } @@ -550,8 +587,14 @@ func TestUpdateUserPassword(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + admin := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login + member := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + numLogs++ // add an audit log for user create ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -560,9 +603,12 @@ func TestUpdateUserPassword(t *testing.T) { OldPassword: "testpass", Password: "newpassword", }) + numLogs++ // add an audit log for user update + require.NoError(t, err, "member should be able to update own password") - assert.Len(t, auditor.AuditLogs, 2) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) t.Run("MemberCantUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { t.Parallel() @@ -582,7 +628,10 @@ func TestUpdateUserPassword(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + _ = coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -590,9 +639,12 @@ func TestUpdateUserPassword(t *testing.T) { err := client.UpdateUserPassword(ctx, "me", codersdk.UpdateUserPasswordRequest{ Password: "newpassword", }) + numLogs++ // add an audit log for user update + require.NoError(t, err, "admin should be able to update own password without providing old password") - assert.Len(t, auditor.AuditLogs, 1) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) t.Run("ChangingPasswordDeletesKeys", func(t *testing.T) { @@ -864,8 +916,14 @@ func TestPutUserSuspend(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) + me := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + numLogs++ // add an audit log for login + _, user := coderdtest.CreateAnotherUserWithUser(t, client, me.OrganizationID) + numLogs++ // add an audit log for user create ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -873,8 +931,10 @@ func TestPutUserSuspend(t *testing.T) { user, err := client.UpdateUserStatus(ctx, user.Username, codersdk.UserStatusSuspended) require.NoError(t, err) require.Equal(t, user.Status, codersdk.UserStatusSuspended) - assert.Len(t, auditor.AuditLogs, 2) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action) + numLogs++ // add an audit log for user update + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SuspendItSelf", func(t *testing.T) { diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 7bb8e0a761262..f1c103be6ff0d 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -578,6 +578,7 @@ func TestWorkspaceBuildStatus(t *testing.T) { 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 login version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) numLogs++ // add an audit log for template version creation numLogs++ // add an audit log for template version update diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 52784feaa16c4..54161c5fcce08 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -270,8 +270,8 @@ func TestPostWorkspacesByOrganization(t *testing.T) { coderdtest.AwaitTemplateVersionJob(t, client, version.ID) _ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - require.Len(t, auditor.AuditLogs, 4) - assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[3].Action) + require.Len(t, auditor.AuditLogs, 5) + assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[4].Action) }) t.Run("CreateWithDeletedTemplate", func(t *testing.T) { @@ -1283,8 +1283,8 @@ func TestWorkspaceUpdateAutostart(t *testing.T) { interval := next.Sub(testCase.at) require.Equal(t, testCase.expectedInterval, interval, "unexpected interval") - require.Len(t, auditor.AuditLogs, 6) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[5].Action) + require.Len(t, auditor.AuditLogs, 7) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[6].Action) }) } @@ -1398,8 +1398,8 @@ func TestWorkspaceUpdateTTL(t *testing.T) { require.Equal(t, testCase.ttlMillis, updated.TTLMillis, "expected autostop ttl to equal requested") - require.Len(t, auditor.AuditLogs, 6) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[5].Action) + require.Len(t, auditor.AuditLogs, 7) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[6].Action) }) } From d9480b6a1617e08c2de4f2a11a270cd1aa5f230c Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 1 Feb 2023 22:22:28 +0000 Subject: [PATCH 06/12] gen documentation --- docs/admin/audit-logs.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index a705b9e371d56..966299c6df6ee 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -9,16 +9,16 @@ We track the following resources: -| Resource | | -| ----------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
| -| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| -| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| -| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| -| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| -| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| -| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| -| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| +|Resource|| +|--|-----------------| +|APIKey
write|
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
+|Group
create, write, delete|
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
+|GitSSHKey
create|
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
+|Template
write, delete|
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
+|TemplateVersion
create, write|
FieldTracked
created_atfalse
created_bytrue
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
+|User
create, write, delete|
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
+|Workspace
create, write, delete|
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
+|WorkspaceBuild
start, stop|
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
From 73089434d4973020e5629e3802890f56866411a1 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 1 Feb 2023 22:38:45 +0000 Subject: [PATCH 07/12] formatting and lint --- coderd/users_test.go | 1 + docs/admin/audit-logs.md | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 9e32ffc326741..d4b8d8b5dca40 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -227,6 +227,7 @@ func TestPostLogin(t *testing.T) { Password: "testpass", } _, err := client.CreateFirstUser(ctx, req) + require.NoError(t, err) numLogs++ // add an audit log for create user numLogs++ // add an audit log for login diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index 966299c6df6ee..a705b9e371d56 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -9,16 +9,16 @@ We track the following resources: -|Resource|| -|--|-----------------| -|APIKey
write|
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
-|Group
create, write, delete|
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
-|GitSSHKey
create|
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
-|Template
write, delete|
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
-|TemplateVersion
create, write|
FieldTracked
created_atfalse
created_bytrue
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
-|User
create, write, delete|
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
-|Workspace
create, write, delete|
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
-|WorkspaceBuild
start, stop|
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
+| Resource | | +| ----------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
updated_atfalse
user_idfalse
| +| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| +| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| +| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
is_privatetrue
min_autostart_intervaltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| +| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| +| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| +| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| +| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| From e5dbe177c2ab2ddc9a1aa170329d29617803af9f Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 1 Feb 2023 22:49:22 +0000 Subject: [PATCH 08/12] lint --- coderd/audit/request.go | 4 ++-- coderd/users.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 0ac85ea0d9d67..4599c8376745f 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -31,7 +31,7 @@ type Request[T Auditable] struct { Old T New T - UserId uuid.UUID + UserID uuid.UUID } type BuildAuditParams[T Auditable] struct { @@ -163,7 +163,7 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request if ok { userID = key.UserID } else { - userID = req.UserId + userID = req.UserID } ip := parseIP(p.Request.RemoteAddr) diff --git a/coderd/users.go b/coderd/users.go index 19c88abf14db9..3f300369ddd02 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1030,7 +1030,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } - aReq.UserId = user.ID + aReq.UserID = user.ID // If the user doesn't exist, it will be a default struct. equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password) From e3a6587cbca25eb331fed0ac115e627a39916c7c Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Thu, 2 Feb 2023 18:55:08 +0000 Subject: [PATCH 09/12] audit Github oauth and write tests --- coderd/userauth.go | 82 ++++++++++++++++++++++++++++++------ coderd/userauth_test.go | 92 +++++++++++++++++++++++++++++++++++++++++ coderd/users.go | 1 - 3 files changed, 161 insertions(+), 14 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 3b85e62d5f260..587bcef81be78 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -17,6 +17,7 @@ import ( "golang.org/x/oauth2" "golang.org/x/xerrors" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" @@ -66,9 +67,18 @@ func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { // @Router /users/oauth2/github/callback [get] func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { var ( - ctx = r.Context() - state = httpmw.OAuth2(r) + ctx = r.Context() + state = httpmw.OAuth2(r) + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) ) + aReq.Old = database.APIKey{} + defer commitAudit() oauthClient := oauth2.NewClient(ctx, oauth2.StaticTokenSource(state.Token)) @@ -81,6 +91,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Internal error fetching authenticated Github user organizations.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -101,6 +114,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "You aren't a member of the authorized Github organizations!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } } @@ -111,6 +127,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Internal error fetching authenticated Github user.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -139,6 +158,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: fmt.Sprintf("You aren't a member of an authorized team in the %v Github organization(s)!", organizationNames), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } } @@ -149,6 +171,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Internal error fetching personal Github user.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -164,10 +189,28 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Your primary email must be verified on GitHub!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } - cookie, err := api.oauthLogin(r, oauthLoginParams{ + user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmail.GetEmail()) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to find linked user.", + Detail: err.Error(), + }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} + return + } + aReq.UserID = user.ID + + cookie, key, err := api.oauthLogin(r, oauthLoginParams{ + User: user, + Link: link, State: state, LinkedID: githubLinkedID(ghUser), LoginType: database.LoginTypeGithub, @@ -182,6 +225,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: httpErr.msg, Detail: httpErr.detail, }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } if err != nil { @@ -189,9 +235,14 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Message: "Failed to process OAuth login.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } + aReq.New = key + http.SetCookie(rw, cookie) redirect := state.Redirect @@ -362,7 +413,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { picture, _ = pictureRaw.(string) } - cookie, err := api.oauthLogin(r, oauthLoginParams{ + cookie, _, err := api.oauthLogin(r, oauthLoginParams{ State: state, LinkedID: oidcLinkedID(idToken), LoginType: database.LoginTypeOIDC, @@ -397,6 +448,8 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } type oauthLoginParams struct { + User database.User + Link database.UserLink State httpmw.OAuth2State LinkedID string LoginType database.LoginType @@ -423,7 +476,7 @@ func (e httpError) Error() string { return e.msg } -func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cookie, error) { +func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cookie, database.APIKey, error) { var ( ctx = r.Context() user database.User @@ -435,10 +488,13 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook err error ) - user, link, err = findLinkedUser(ctx, tx, params.LinkedID, params.Email) - if err != nil { - return xerrors.Errorf("find linked user: %w", err) - } + user = params.User + link = params.Link + // + // user, link, err = findLinkedUser(ctx, tx, params.LinkedID, params.Email) + // if err != nil { + // return xerrors.Errorf("find linked user: %w", err) + // } if user.ID == uuid.Nil && !params.AllowSignups { return httpError{ @@ -599,19 +655,19 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook return nil }, nil) if err != nil { - return nil, xerrors.Errorf("in tx: %w", err) + return nil, database.APIKey{}, xerrors.Errorf("in tx: %w", err) } - cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, key, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: params.LoginType, RemoteAddr: r.RemoteAddr, }) if err != nil { - return nil, xerrors.Errorf("create API key: %w", err) + return nil, database.APIKey{}, xerrors.Errorf("create API key: %w", err) } - return cookie, nil + return cookie, *key, nil } // githubLinkedID returns the unique ID for a GitHub user. diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index ea707289639ec..c8a99b0d9e875 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -20,6 +20,7 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/coderd" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" "github.com/coder/coder/codersdk" @@ -105,7 +106,9 @@ func TestUserOAuth2Github(t *testing.T) { t.Run("NotInAllowedOrganization", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) { @@ -118,13 +121,19 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("NotInAllowedTeam", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowOrganizations: []string{"coder"}, AllowTeams: []coderd.GithubOAuth2Team{{"another", "something"}, {"coder", "frontend"}}, @@ -147,12 +156,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("UnverifiedEmail", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -175,13 +192,23 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + _ = coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("BlockSignups", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -205,12 +232,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusForbidden, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("MultiLoginNotAllowed", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -234,16 +269,26 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + // Creates the first user with login_type 'password'. _ = coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user create + // Attempting to login should give us a 403 since the user // already has a login_type of 'password'. resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusForbidden, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Signup", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &oauth2Config{}, AllowOrganizations: []string{"coder"}, @@ -272,7 +317,11 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) client.SetSessionToken(authCookieValue(resp.Cookies())) @@ -281,10 +330,15 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, "kyle@coder.com", user.Email) require.Equal(t, "kyle", user.Username) require.Equal(t, "/hello-world", user.AvatarURL) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowedTeam", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder"}, @@ -315,12 +369,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowedTeamInFirstOrganization", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder", "nil"}, @@ -359,12 +421,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowedTeamInSecondOrganization", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder", "nil"}, @@ -403,12 +473,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupAllowEveryone", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowEveryone: true, @@ -433,12 +511,20 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("SignupFailedInactiveInOrg", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, AllowOrganizations: []string{"coder"}, @@ -469,8 +555,14 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + numLogs := len(auditor.AuditLogs) + resp := oauth2Callback(t, client) + numLogs++ // add an audit log for login + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) } diff --git a/coderd/users.go b/coderd/users.go index 3f300369ddd02..c086850b29b55 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1006,7 +1006,6 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { }) ) aReq.Old = database.APIKey{} - defer commitAudit() var loginWithPassword codersdk.LoginWithPasswordRequest From 809dc7bfda771a883eae7fffc393f337c935b509 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Thu, 2 Feb 2023 19:20:35 +0000 Subject: [PATCH 10/12] audit oauth and write tests --- coderd/userauth.go | 70 +++++++++++++++++++++++++++++++++++------ coderd/userauth_test.go | 38 +++++++++++++++++++++- 2 files changed, 98 insertions(+), 10 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 587bcef81be78..fc9d1b5d4d6bb 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -240,7 +240,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { aReq.New = database.APIKey{UserID: uuid.New()} return } - aReq.New = key http.SetCookie(rw, cookie) @@ -276,9 +275,18 @@ type OIDCConfig struct { // @Router /users/oidc/callback [get] func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { var ( - ctx = r.Context() - state = httpmw.OAuth2(r) + ctx = r.Context() + state = httpmw.OAuth2(r) + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) ) + aReq.Old = database.APIKey{} + defer commitAudit() // See the example here: https://github.com/coreos/go-oidc rawIDToken, ok := state.Token.Extra("id_token").(string) @@ -286,6 +294,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "id_token not found in response payload. Ensure your OIDC callback is configured correctly!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -295,6 +306,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to verify OIDC token.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -308,6 +322,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to extract OIDC claims.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -326,6 +343,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to unmarshal user info claims.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } for k, v := range userInfoClaims { @@ -336,6 +356,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to obtain user information claims.", Detail: "The OIDC provider returned no claims as part of the `id_token`. The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } @@ -355,6 +378,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "No email found in OIDC payload!", }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } emailRaw = username @@ -364,6 +390,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Email in OIDC payload isn't a string. Got: %t", emailRaw), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } verifiedRaw, ok := claims["email_verified"] @@ -374,6 +403,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Verify the %q email address on your OIDC provider to authenticate!", email), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } api.Logger.Warn(ctx, "allowing unverified oidc email %q") @@ -404,6 +436,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Your email %q is not in domains %q !", email, api.OIDCConfig.EmailDomain), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } } @@ -413,7 +448,22 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { picture, _ = pictureRaw.(string) } - cookie, _, err := api.oauthLogin(r, oauthLoginParams{ + user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to find linked user.", + Detail: err.Error(), + }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} + return + } + aReq.UserID = user.ID + + cookie, key, err := api.oauthLogin(r, oauthLoginParams{ + User: user, + Link: link, State: state, LinkedID: oidcLinkedID(idToken), LoginType: database.LoginTypeOIDC, @@ -428,6 +478,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: httpErr.msg, Detail: httpErr.detail, }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } if err != nil { @@ -435,8 +488,12 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Message: "Failed to process OAuth login.", Detail: err.Error(), }) + // We pass a disposable user ID just to force an audit diff + // and generate a log for a failed login + aReq.New = database.APIKey{UserID: uuid.New()} return } + aReq.New = key http.SetCookie(rw, cookie) @@ -490,11 +547,6 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook user = params.User link = params.Link - // - // user, link, err = findLinkedUser(ctx, tx, params.LinkedID, params.Email) - // if err != nil { - // return xerrors.Errorf("find linked user: %w", err) - // } if user.ID == uuid.Nil && !params.AllowSignups { return httpError{ diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index c8a99b0d9e875..ea994bd1dbc37 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -718,6 +718,7 @@ func TestUserOIDC(t *testing.T) { tc := tc t.Run(tc.Name, func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() conf := coderdtest.NewOIDCConfig(t, "") config := conf.OIDCConfig(t, tc.UserInfoClaims) @@ -726,9 +727,13 @@ func TestUserOIDC(t *testing.T) { config.IgnoreEmailVerified = tc.IgnoreEmailVerified client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: config, }) + numLogs := len(auditor.AuditLogs) + resp := oidcCallback(t, client, conf.EncodeClaims(t, tc.IDTokenClaims)) + numLogs++ // add an audit log for login assert.Equal(t, tc.StatusCode, resp.StatusCode) ctx, _ := testutil.Context(t) @@ -738,6 +743,9 @@ func TestUserOIDC(t *testing.T) { user, err := client.User(ctx, "me") require.NoError(t, err) require.Equal(t, tc.Username, user.Username) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) } if tc.AvatarURL != "" { @@ -745,26 +753,33 @@ func TestUserOIDC(t *testing.T) { user, err := client.User(ctx, "me") require.NoError(t, err) require.Equal(t, tc.AvatarURL, user.AvatarURL) + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) } }) } t.Run("AlternateUsername", func(t *testing.T) { t.Parallel() - + auditor := audit.NewMock() conf := coderdtest.NewOIDCConfig(t, "") config := conf.OIDCConfig(t, nil) config.AllowSignups = true client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: config, }) + numLogs := len(auditor.AuditLogs) code := conf.EncodeClaims(t, jwt.MapClaims{ "email": "jon@coder.com", }) resp := oidcCallback(t, client, code) + numLogs++ // add an audit log for login + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) ctx, _ := testutil.Context(t) @@ -781,12 +796,17 @@ func TestUserOIDC(t *testing.T) { "sub": "diff", }) resp = oidcCallback(t, client, code) + numLogs++ // add an audit log for login + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) client.SetSessionToken(authCookieValue(resp.Cookies())) user, err = client.User(ctx, "me") require.NoError(t, err) require.True(t, strings.HasPrefix(user.Username, "jon-"), "username %q should have prefix %q", user.Username, "jon-") + + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("Disabled", func(t *testing.T) { @@ -798,23 +818,33 @@ func TestUserOIDC(t *testing.T) { t.Run("NoIDToken", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: &coderd.OIDCConfig{ OAuth2Config: &oauth2Config{}, }, }) + numLogs := len(auditor.AuditLogs) + resp := oidcCallback(t, client, "asdf") + numLogs++ // add an audit log for login + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) t.Run("BadVerify", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() verifier := oidc.NewVerifier("", &oidc.StaticKeySet{ PublicKeys: []crypto.PublicKey{}, }, &oidc.Config{}) provider := &oidc.Provider{} client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, OIDCConfig: &coderd.OIDCConfig{ OAuth2Config: &oauth2Config{ token: (&oauth2.Token{ @@ -827,8 +857,14 @@ func TestUserOIDC(t *testing.T) { Verifier: verifier, }, }) + numLogs := len(auditor.AuditLogs) + resp := oidcCallback(t, client, "asdf") + numLogs++ // add an audit log for login + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action) }) } From d1644677455f3548e6b93c583a5a16dafd6ca9cd Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Thu, 2 Feb 2023 20:21:13 +0000 Subject: [PATCH 11/12] feat: audit logout --- coderd/users.go | 17 ++++++++++++++++- coderd/users_test.go | 10 +++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index c086850b29b55..2508049a53d10 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1110,7 +1110,18 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { // @Success 200 {object} codersdk.Response // @Router /users/logout [post] func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() + var ( + ctx = r.Context() + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogout, + }) + ) + defer commitAudit() + // Get a blank token cookie. cookie := &http.Cookie{ // MaxAge < 0 means to delete the cookie now. @@ -1122,6 +1133,8 @@ func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { // Delete the session token from database. apiKey := httpmw.APIKey(r) + aReq.Old = apiKey + err := api.Database.DeleteAPIKeyByID(ctx, apiKey.ID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -1175,6 +1188,8 @@ func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) { } } + aReq.New = database.APIKey{} + httpapi.Write(ctx, rw, http.StatusOK, codersdk.Response{ Message: "Logged out!", }) diff --git a/coderd/users_test.go b/coderd/users_test.go index d4b8d8b5dca40..7bebba908e0ce 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -327,9 +327,12 @@ func TestPostLogout(t *testing.T) { // Checks that the cookie is cleared and the API Key is deleted from the database. t.Run("Logout", func(t *testing.T) { t.Parallel() + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + numLogs := len(auditor.AuditLogs) - client := coderdtest.New(t, nil) admin := coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for login ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -343,10 +346,15 @@ func TestPostLogout(t *testing.T) { require.NoError(t, err, "Server URL should parse successfully") res, err := client.Request(ctx, http.MethodPost, fullURL.String(), nil) + numLogs++ // add an audit log for logout + require.NoError(t, err, "/logout request should succeed") res.Body.Close() require.Equal(t, http.StatusOK, res.StatusCode) + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionLogout, auditor.AuditLogs[numLogs-1].Action) + cookies := res.Cookies() var found bool From 7a2114a0bf4809205eb24d1b96480f9f6d99a67e Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Thu, 2 Feb 2023 21:20:43 +0000 Subject: [PATCH 12/12] feat: add preset filter for audit logins --- site/src/pages/AuditPage/AuditPageView.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/site/src/pages/AuditPage/AuditPageView.tsx b/site/src/pages/AuditPage/AuditPageView.tsx index 1347a362c035b..ea2202ee7baa8 100644 --- a/site/src/pages/AuditPage/AuditPageView.tsx +++ b/site/src/pages/AuditPage/AuditPageView.tsx @@ -36,14 +36,14 @@ const presetFilters = [ }, { query: "resource_type:template action:create", name: "Added templates" }, { query: "resource_type:user action:delete", name: "Deleted users" }, - { - query: "resource_type:workspace_build action:start", - name: "Started builds", - }, { query: "resource_type:workspace_build action:start build_reason:initiator", name: "Builds started by a user", }, + { + query: "resource_type:api_key action:login", + name: "User logins", + }, ] export interface AuditPageViewProps {