fix: show audit logs for forgot password flow

coder · DanielleMaywood · Oct 22, 2024 · Oct 22, 2024 · Oct 22, 2024 · Oct 22, 2024
commit ecd43f12bab12a500b2bf167668df085f33aa198
diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/audit.go b/coderd/audit.go
@@ -274,8 +274,15 @@ func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogs
 
 func auditLogDescription(alog database.GetAuditLogsOffsetRow) string {
 	b := strings.Builder{}
+
 	// NOTE: WriteString always returns a nil error, so we never check it
-	_, _ = b.WriteString("{user} ")
+
+	// Requesting a one-time passcode can be performed by anyone that knows the email
+	// of a user so saying the user performed this action might be slightly misleading.
+	if alog.AuditLog.Action != database.AuditActionRequestOneTimePasscode {
+		_, _ = b.WriteString("{user} ")
+	}
+
 	if alog.AuditLog.StatusCode >= 400 {
 		_, _ = b.WriteString("unsuccessfully attempted to ")
 		_, _ = b.WriteString(string(alog.AuditLog.Action))
@@ -298,13 +305,16 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string {
 		return b.String()
 	}
 
-	_, _ = b.WriteString(" ")
-	_, _ = b.WriteString(codersdk.ResourceType(alog.AuditLog.ResourceType).FriendlyString())
+	if alog.AuditLog.Action == database.AuditActionRequestOneTimePasscode {
+		_, _ = b.WriteString(" for")
+	} else {
+		_, _ = b.WriteString(" ")
+		_, _ = b.WriteString(codersdk.ResourceType(alog.AuditLog.ResourceType).FriendlyString())
+	}
 
 	if alog.AuditLog.ResourceType == database.ResourceTypeConvertLogin {
 		_, _ = b.WriteString(" to")
 	}
-
 	_, _ = b.WriteString(" {target}")
 
 	return b.String()

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000268_add_audit_action_forgot_password.down.sql b/coderd/database/migrations/000268_add_audit_action_forgot_password.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/000268_add_audit_action_forgot_password.up.sql b/coderd/database/migrations/000268_add_audit_action_forgot_password.up.sql
@@ -0,0 +1,2 @@
+ALTER TYPE audit_action
+  ADD VALUE IF NOT EXISTS 'request_one_time_passcode';
diff --git a/coderd/database/models.go b/coderd/database/models.go
diff --git a/coderd/database/pubsub/psmock/psmock.go b/coderd/database/pubsub/psmock/psmock.go
diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -220,7 +220,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque
 			Audit:   *auditor,
 			Log:     api.Logger,
 			Request: r,
-			Action:  database.AuditActionWrite,
+			Action:  database.AuditActionRequestOneTimePasscode,
 		})
 	)
 	defer commitAudit()
@@ -253,6 +253,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque
 	}
 	// We continue if err == sql.ErrNoRows to help prevent a timing-based attack.
 	aReq.Old = user
+	aReq.UserID = user.ID
 
 	passcode := uuid.New()
 	passcodeExpiresAt := dbtime.Now().Add(api.OneTimePasscodeValidityPeriod)
@@ -365,6 +366,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r
 		}
 		// We continue if err == sql.ErrNoRows to help prevent a timing-based attack.
 		aReq.Old = user
+		aReq.UserID = user.ID
 
 		equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode)
 		if err != nil {

diff --git a/codersdk/audit.go b/codersdk/audit.go
@@ -86,14 +86,15 @@ func (r ResourceType) FriendlyString() string {
 type AuditAction string
 
 const (
-	AuditActionCreate   AuditAction = "create"
-	AuditActionWrite    AuditAction = "write"
-	AuditActionDelete   AuditAction = "delete"
-	AuditActionStart    AuditAction = "start"
-	AuditActionStop     AuditAction = "stop"
-	AuditActionLogin    AuditAction = "login"
-	AuditActionLogout   AuditAction = "logout"
-	AuditActionRegister AuditAction = "register"
+	AuditActionCreate                 AuditAction = "create"
+	AuditActionWrite                  AuditAction = "write"
+	AuditActionDelete                 AuditAction = "delete"
+	AuditActionStart                  AuditAction = "start"
+	AuditActionStop                   AuditAction = "stop"
+	AuditActionLogin                  AuditAction = "login"
+	AuditActionLogout                 AuditAction = "logout"
+	AuditActionRegister               AuditAction = "register"
+	AuditActionRequestOneTimePasscode AuditAction = "request_one_time_passcode"
 )
 
 func (a AuditAction) Friendly() string {
@@ -114,6 +115,8 @@ func (a AuditAction) Friendly() string {
 		return "logged out"
 	case AuditActionRegister:
 		return "registered"
+	case AuditActionRequestOneTimePasscode:
+		return "one time passcode requested"
 	default:
 		return "unknown"
 	}

diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md
diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go
@@ -145,7 +145,7 @@ var auditableResourcesTypes = map[any]map[string]Action{
 		"theme_preference":             ActionIgnore,
 		"name":                         ActionTrack,
 		"github_com_user_id":           ActionIgnore,
-		"hashed_one_time_passcode":     ActionSecret, // Do not expose a user's one time passcode.
+		"hashed_one_time_passcode":     ActionIgnore,
 		"one_time_passcode_expires_at": ActionTrack,
 		"must_reset_password":          ActionTrack,
 	},

diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts
diff --git a/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/AuditLogDiff.tsx b/site/src/pages/AuditPage/AuditLogRow/AuditLogDiff/AuditLogDiff.tsx
@@ -9,6 +9,14 @@ const getDiffValue = (value: unknown): string => {
 		return `"${value}"`;
 	}
 
+	if (isTimeObject(value)) {
+		if (!value.Valid) {
+			return "null";
+		}
+
+		return new Date(value.Time).toLocaleString();
+	}
+
 	if (Array.isArray(value)) {
 		const values = value.map((v) => getDiffValue(v));
 		return `[${values.join(", ")}]`;
@@ -21,6 +29,19 @@ const getDiffValue = (value: unknown): string => {
 	return String(value);
 };
 
+const isTimeObject = (
+	value: unknown,
+): value is { Time: string; Valid: boolean } => {
+	return (
+		value !== null &&
+		typeof value === "object" &&
+		"Time" in value &&
+		typeof value.Time === "string" &&
+		"Valid" in value &&
+		typeof value.Valid === "boolean"
+	);
+};
+
 interface AuditLogDiffProps {
 	diff: AuditDiff;
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		-- It's not possible to drop enum values from enum types, so the UP has "IF NOT
		-- EXISTS".
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		ALTER TYPE audit_action
		ADD VALUE IF NOT EXISTS 'request_one_time_passcode';