From 3a4a9190828293604b47ca36bc7d3e35d52e1b41 Mon Sep 17 00:00:00 2001 From: Stephen von Takach Date: Mon, 19 Oct 2026 15:20:56 +1100 Subject: [PATCH 1/2] feat(history): unified change history Adds History::Tracked, recording creates, updates ({field: [old, new]}), deletes (with snapshot), approvals and restores in the history table with request metadata, retention and undelete. Replaces GroupHistory. Co-Authored-By: Claude Opus 5.5 --- .../20261019100500000_unified_history.sql | 119 ++++++ shard.yml | 4 +- spec/event_metadata_spec.cr | 22 +- spec/generator.cr | 33 +- spec/group_ad_groups_spec.cr | 2 +- spec/group_features_spec.cr | 6 +- spec/group_history_spec.cr | 70 ---- spec/group_invitation_spec.cr | 2 +- spec/group_permissions_spec.cr | 2 +- spec/group_playlist_item_spec.cr | 2 +- spec/group_playlist_spec.cr | 2 +- spec/group_signage_template_spec.cr | 2 +- spec/group_spec.cr | 12 +- spec/group_user_spec.cr | 11 +- spec/group_zone_spec.cr | 2 +- spec/helper.cr | 2 +- spec/history_spec.cr | 357 +++++++++++++++--- spec/signage_template_spec.cr | 53 ++- spec/user_spec.cr | 2 +- src/placeos-models/alert.cr | 3 + src/placeos-models/alert_dashboard.cr | 3 + src/placeos-models/asset.cr | 3 + src/placeos-models/asset_category.cr | 3 + src/placeos-models/asset_purchase_order.cr | 3 + src/placeos-models/asset_type.cr | 3 + src/placeos-models/authority.cr | 4 + src/placeos-models/booking.cr | 9 + src/placeos-models/booking_instance.cr | 13 + src/placeos-models/control_system.cr | 4 + src/placeos-models/doorkeeper_application.cr | 4 + src/placeos-models/driver.cr | 4 + src/placeos-models/event_metadata.cr | 59 ++- src/placeos-models/group.cr | 19 +- src/placeos-models/group/history.cr | 135 ------- src/placeos-models/group/invitation.cr | 26 +- src/placeos-models/group/playlist.cr | 4 +- src/placeos-models/group/playlist_item.cr | 4 +- src/placeos-models/group/signage_template.cr | 2 +- src/placeos-models/group/user.cr | 12 +- src/placeos-models/group/zone.cr | 12 +- src/placeos-models/history.cr | 227 ++++++++++- src/placeos-models/history/tracked.cr | 175 +++++++++ src/placeos-models/ldap_authentication.cr | 4 + src/placeos-models/module.cr | 4 + src/placeos-models/oauth_authentication.cr | 4 + src/placeos-models/playlist.cr | 4 + src/placeos-models/playlist/item.cr | 4 + src/placeos-models/playlist/revision.cr | 16 + src/placeos-models/repository.cr | 5 + src/placeos-models/saml_authentication.cr | 3 + src/placeos-models/signage_template.cr | 36 +- src/placeos-models/trigger.cr | 3 + src/placeos-models/trigger_instance.cr | 9 + src/placeos-models/user.cr | 5 + src/placeos-models/zone.cr | 3 + 55 files changed, 1076 insertions(+), 460 deletions(-) create mode 100644 migration/db/migrations/20261019100500000_unified_history.sql delete mode 100644 spec/group_history_spec.cr delete mode 100644 src/placeos-models/group/history.cr create mode 100644 src/placeos-models/history/tracked.cr diff --git a/migration/db/migrations/20261019100500000_unified_history.sql b/migration/db/migrations/20261019100500000_unified_history.sql new file mode 100644 index 00000000..a8bf6a14 --- /dev/null +++ b/migration/db/migrations/20261019100500000_unified_history.sql @@ -0,0 +1,119 @@ +-- +micrate Up +-- SQL in section 'Up' is executed when this migration is applied + +-- Unified change history for tracked models (see `History::Tracked`). +-- Replaces the previous `history` table (changed fields only, never read) +-- and `group_history`, whose rows are migrated with the group id as the +-- correlation id. +DROP TABLE IF EXISTS "history"; + +CREATE TABLE IF NOT EXISTS "history" ( + id UUID PRIMARY KEY DEFAULT uuidv7(), + resource_type TEXT NOT NULL, + resource_id TEXT NOT NULL, + correlation_id TEXT, + action TEXT NOT NULL, + changed_fields TEXT[] NOT NULL DEFAULT '{}', + -- {"field": [previous, current]} + diff JSONB NOT NULL DEFAULT '{}'::jsonb, + -- the full record, for deletes and approvals + snapshot JSONB, + -- the acting user, kept without a foreign key so the trail survives user deletion + user_id TEXT, + user_email TEXT, + -- request details: app id, name, build, user agent, ip etc + context JSONB NOT NULL DEFAULT '{}'::jsonb, + created_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +CREATE INDEX IF NOT EXISTS history_resource_index + ON "history" USING BTREE (resource_type, resource_id, created_at DESC); +CREATE INDEX IF NOT EXISTS history_correlation_id_index + ON "history" USING BTREE (correlation_id, created_at DESC) + WHERE correlation_id IS NOT NULL; +CREATE INDEX IF NOT EXISTS history_user_id_index + ON "history" USING BTREE (user_id) + WHERE user_id IS NOT NULL; +CREATE INDEX IF NOT EXISTS history_created_at_index + ON "history" USING BTREE (created_at); + +INSERT INTO "history" ( + resource_type, resource_id, correlation_id, action, changed_fields, user_id, user_email, created_at +) +SELECT + CASE resource_type + WHEN 'group' THEN 'groups' + WHEN 'group_user' THEN 'group_users' + WHEN 'group_zone' THEN 'group_zones' + WHEN 'group_invitation' THEN 'group_invitations' + ELSE resource_type + END, + resource_id, + group_id::TEXT, + action, + changed_fields, + user_id, + email, + created_at +FROM "group_history"; + +DROP TABLE IF EXISTS "group_history"; + +-- +micrate Down +-- SQL section 'Down' is executed when this migration is rolled back + +CREATE TABLE IF NOT EXISTS "group_history"( + id UUID PRIMARY KEY DEFAULT uuidv7(), + group_id UUID, + user_id TEXT REFERENCES "user"(id) ON DELETE SET NULL, + email TEXT NOT NULL, + action TEXT NOT NULL, + resource_type TEXT NOT NULL, + resource_id TEXT NOT NULL, + changed_fields TEXT[] NOT NULL DEFAULT '{}', + created_at TIMESTAMPTZ NOT NULL +); + +CREATE INDEX IF NOT EXISTS group_history_group_id_index + ON "group_history" USING BTREE (group_id); +CREATE INDEX IF NOT EXISTS group_history_user_id_index + ON "group_history" USING BTREE (user_id); +CREATE INDEX IF NOT EXISTS group_history_created_at_index + ON "group_history" USING BTREE (created_at); + +INSERT INTO "group_history" ( + group_id, user_id, email, action, resource_type, resource_id, changed_fields, created_at +) +SELECT + h.correlation_id::UUID, + u.id, + h.user_email, + h.action, + CASE h.resource_type + WHEN 'groups' THEN 'group' + WHEN 'group_users' THEN 'group_user' + WHEN 'group_zones' THEN 'group_zone' + WHEN 'group_invitations' THEN 'group_invitation' + END, + h.resource_id, + h.changed_fields, + h.created_at +FROM "history" h +LEFT JOIN "user" u ON u.id = h.user_id +WHERE h.resource_type IN ('groups', 'group_users', 'group_zones', 'group_invitations') + AND h.user_email IS NOT NULL; + +DROP TABLE IF EXISTS "history"; + +CREATE TABLE IF NOT EXISTS "history" ( + id TEXT NOT NULL PRIMARY KEY, + type TEXT NOT NULL, + resource_id TEXT NOT NULL, + action TEXT NOT NULL, + changed_fields TEXT[] NOT NULL, + created_at TIMESTAMPTZ NOT NULL, + updated_at TIMESTAMPTZ NOT NULL +); + +CREATE INDEX IF NOT EXISTS history_type_index ON "history" USING BTREE (type); +CREATE INDEX IF NOT EXISTS history_resource_id_index ON "history" USING BTREE (resource_id); diff --git a/shard.yml b/shard.yml index 6053afc1..32c22963 100644 --- a/shard.yml +++ b/shard.yml @@ -6,7 +6,7 @@ dependencies: # Data validation library active-model: github: spider-gazelle/active-model - version: ~> 4.4 + version: ~> 4.7 # Data validation library CrystalEmail: @@ -41,7 +41,7 @@ dependencies: # ORM for Postgres built on active-model pg-orm: github: spider-gazelle/pg-orm - version: ">= 2.4.1" + version: ">= 2.5.1" # PlaceOS calendar for Tenant model place_calendar: diff --git a/spec/event_metadata_spec.cr b/spec/event_metadata_spec.cr index c820d014..5acd3f16 100644 --- a/spec/event_metadata_spec.cr +++ b/spec/event_metadata_spec.cr @@ -63,10 +63,11 @@ module PlaceOS::Model histories = History.all.to_a histories.size.should eq 1 - histories.first.type.should eq "event" + histories.first.resource_type.should eq "event_metadatas" histories.first.resource_id.should eq event.event_id - histories.first.action.should eq "updated" - histories.first.changed_fields.should contain "setup_time" + histories.first.action.should eq "update" + histories.first.changed_fields.should eq ["setup_time"] + histories.first.diff["setup_time"].should eq JSON.parse("[0, 300]") end it "records multiple changed fields" do @@ -106,6 +107,8 @@ module PlaceOS::Model histories.size.should eq 1 histories.first.changed_fields.should contain "ext_data.catering" histories.first.changed_fields.should contain "ext_data.notes" + histories.first.changed_fields.should_not contain "ext_data" + histories.first.diff["ext_data"][1].should eq JSON.parse(%({"catering": true, "notes": "test"})) end it "does not record history when no fields changed" do @@ -121,6 +124,19 @@ module PlaceOS::Model History.all.to_a.size.should eq 0 end + + it "records deletes with a snapshot" do + tenant = get_tenant + event = Generator.event_metadata(tenant.id, 5.minutes.from_now, 10.minutes.from_now) + event.save! + event.destroy + + histories = History.all.to_a + histories.size.should eq 1 + histories.first.action.should eq "delete" + histories.first.resource_id.should eq event.event_id + histories.first.snapshot.not_nil!["event_id"].should eq event.event_id + end end end end diff --git a/spec/generator.cr b/spec/generator.cr index 1671c559..892308d8 100644 --- a/spec/generator.cr +++ b/spec/generator.cr @@ -956,16 +956,20 @@ module PlaceOS::Model end def self.history( - type : String = "zone", + resource_type : String = "zone", resource_id : String = "zone-#{RANDOM.hex(4)}", action : String = "update", - changed_fields : Array(String) = ["name", "description"], + correlation_id : String? = nil, + created_at : Time = Time.utc, ) History.new( - type: type, + resource_type: resource_type, resource_id: resource_id, action: action, - changed_fields: changed_fields + correlation_id: correlation_id, + changed_fields: ["name"], + diff: JSON.parse(%({"name": ["old", "new"]})), + created_at: created_at, ) end @@ -1059,27 +1063,6 @@ module PlaceOS::Model ) end - def self.group_history( - group_id : UUID? = nil, - user : User? = nil, - action : String = "update", - resource_type : String = "group", - resource_id : String? = nil, - changed_fields : Array(String) = ["name"], - ) - actor_email = user.try(&.email.to_s) || Faker::Internet.email - actor_id = user.try(&.id) - GroupHistory.new( - group_id: group_id, - user_id: actor_id, - email: actor_email, - action: action, - resource_type: resource_type, - resource_id: resource_id || "res-#{RANDOM.hex(4)}", - changed_fields: changed_fields, - ) - end - def self.group_playlist( group : Group? = nil, playlist : Playlist? = nil, diff --git a/spec/group_ad_groups_spec.cr b/spec/group_ad_groups_spec.cr index 3e8c9b55..116280eb 100644 --- a/spec/group_ad_groups_spec.cr +++ b/spec/group_ad_groups_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe "Group AD group mappings" do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/spec/group_features_spec.cr b/spec/group_features_spec.cr index fd7b6bf7..9b306687 100644 --- a/spec/group_features_spec.cr +++ b/spec/group_features_spec.cr @@ -7,7 +7,7 @@ end module PlaceOS::Model describe "Group features" do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear @@ -97,11 +97,11 @@ module PlaceOS::Model actor = Generator.user(authority: authority).save! group = Generator.group(authority: authority, subsystems: ["signage"]).save! - group.acting_user = actor + group.history_metadata = History::Metadata.new.with_user(actor) group.features = feature_json(%({"signage": {"ai": true}})) group.save! - entry = GroupHistory.where(resource_type: "group", resource_id: group.id.to_s, action: "update").to_a.first + entry = History.where(resource_type: "groups", resource_id: group.id.to_s, action: "update").to_a.first entry.changed_fields.should contain("features") end diff --git a/spec/group_history_spec.cr b/spec/group_history_spec.cr deleted file mode 100644 index 5c34abf9..00000000 --- a/spec/group_history_spec.cr +++ /dev/null @@ -1,70 +0,0 @@ -require "./helper" - -module PlaceOS::Model - describe GroupHistory do - Spec.before_each do - GroupHistory.clear - GroupInvitation.clear - GroupZone.clear - GroupUser.clear - Group.clear - User.clear - Authority.clear - end - - it "saves a history entry" do - group = Generator.group.save! - user = Generator.user.save! - - entry = Generator.group_history( - group_id: group.id, - user: user, - action: "update", - resource_type: "group", - resource_id: group.id.to_s, - changed_fields: ["name", "description"], - ).save! - - entry.persisted?.should be_true - entry.id.should be_a(UUID) - entry.changed_fields.should eq ["name", "description"] - entry.created_at.should_not be_nil - end - - it "requires email, action, resource_type, resource_id" do - entry = GroupHistory.new( - email: "", - action: "", - resource_type: "", - resource_id: "", - ) - entry.valid?.should be_false - fields = entry.errors.map(&.field) - fields.should contain(:email) - fields.should contain(:action) - fields.should contain(:resource_type) - fields.should contain(:resource_id) - end - - it "nullifies user_id but preserves email when acting user is deleted" do - user = Generator.user.save! - group = Generator.group.save! - - entry = Generator.group_history( - group_id: group.id, - user: user, - ).save! - - entry.user_id.should eq user.id - entry.email.should eq user.email.to_s - entry.user_deleted?.should be_false - - user.destroy - - reloaded = GroupHistory.find!(entry.id.not_nil!) - reloaded.user_id.should be_nil - reloaded.email.should eq user.email.to_s - reloaded.user_deleted?.should be_true - end - end -end diff --git a/spec/group_invitation_spec.cr b/spec/group_invitation_spec.cr index 91aad9d0..fd475125 100644 --- a/spec/group_invitation_spec.cr +++ b/spec/group_invitation_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe GroupInvitation do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/spec/group_permissions_spec.cr b/spec/group_permissions_spec.cr index 9ed8a64f..dfb151c2 100644 --- a/spec/group_permissions_spec.cr +++ b/spec/group_permissions_spec.cr @@ -119,7 +119,7 @@ module PlaceOS::Model # - effective_permissions / zone_accessible? / accessible_zone_ids describe Group do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/spec/group_playlist_item_spec.cr b/spec/group_playlist_item_spec.cr index 3da7b074..5eb525b3 100644 --- a/spec/group_playlist_item_spec.cr +++ b/spec/group_playlist_item_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe GroupPlaylistItem do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/spec/group_playlist_spec.cr b/spec/group_playlist_spec.cr index 94b82bd4..860f4aec 100644 --- a/spec/group_playlist_spec.cr +++ b/spec/group_playlist_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe GroupPlaylist do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/spec/group_signage_template_spec.cr b/spec/group_signage_template_spec.cr index 7b4912c6..6d937f1c 100644 --- a/spec/group_signage_template_spec.cr +++ b/spec/group_signage_template_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe GroupSignageTemplate do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/spec/group_spec.cr b/spec/group_spec.cr index d15a59a5..e6c46cad 100644 --- a/spec/group_spec.cr +++ b/spec/group_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe Group do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear @@ -116,22 +116,22 @@ module PlaceOS::Model Group.find?(group.id.not_nil!).should be_nil end - it "records history on create/update/delete when acting_user is set" do + it "records history on create/update/delete with the acting user" do authority = Generator.authority.save! actor = Generator.user(authority: authority).save! group = Generator.group(authority: authority) - group.acting_user = actor + group.history_metadata = History::Metadata.new.with_user(actor) group.save! - group.acting_user = actor + group.history_metadata = History::Metadata.new.with_user(actor) group.name = "renamed" group.save! - group.acting_user = actor + group.history_metadata = History::Metadata.new.with_user(actor) group.destroy - entries = GroupHistory.where(resource_type: "group", resource_id: group.id.to_s).to_a + entries = History.where(resource_type: "groups", resource_id: group.id.to_s).to_a actions = entries.map(&.action).sort! actions.should eq ["create", "delete", "update"] end diff --git a/spec/group_user_spec.cr b/spec/group_user_spec.cr index b9577ede..256544ca 100644 --- a/spec/group_user_spec.cr +++ b/spec/group_user_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe GroupUser do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear @@ -57,17 +57,18 @@ module PlaceOS::Model GroupUser.where(user_id: user.id).to_a.should be_empty end - it "records history on create when acting_user is set" do + it "records history on create with the acting user" do group = Generator.group.save! user = Generator.user.save! gu = Generator.group_user(user: user, group: group) - gu.acting_user = user + gu.history_metadata = History::Metadata.new.with_user(user) gu.save! - histories = GroupHistory.where(resource_type: "group_user").to_a + histories = History.where(resource_type: "group_users").to_a histories.size.should eq 1 - histories.first.group_id.should eq group.id + histories.first.correlation_id.should eq group.id.to_s + histories.first.user_id.should eq user.id end it "rejects a user and group from different authorities" do diff --git a/spec/group_zone_spec.cr b/spec/group_zone_spec.cr index 99645c65..f66f90ff 100644 --- a/spec/group_zone_spec.cr +++ b/spec/group_zone_spec.cr @@ -3,7 +3,7 @@ require "./helper" module PlaceOS::Model describe GroupZone do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/spec/helper.cr b/spec/helper.cr index def4d5e6..4f7a6394 100644 --- a/spec/helper.cr +++ b/spec/helper.cr @@ -32,7 +32,7 @@ Spec.after_suite do PlaceOS::Model::SignageTemplate::SystemTemplate, PlaceOS::Model::SignageTemplate, PlaceOS::Model::PendingMail, - PlaceOS::Model::GroupHistory, + PlaceOS::Model::History, PlaceOS::Model::GroupInvitation, PlaceOS::Model::GroupZone, PlaceOS::Model::GroupUser, diff --git a/spec/history_spec.cr b/spec/history_spec.cr index 0a243414..a2543864 100644 --- a/spec/history_spec.cr +++ b/spec/history_spec.cr @@ -4,86 +4,329 @@ module PlaceOS::Model describe History do Spec.before_each do History.clear + History.cleanup_interval = nil end - test_round_trip(History) + Spec.after_each do + History.retention = 30.days + History.max_entries = 250 + History.min_entries = 5 + History.cleanup_interval = 1.hour + end - it "saves a History" do + it "saves a History entry" do history = Generator.history.save! - history.should_not be_nil history.persisted?.should be_true - history.id.as(String).starts_with?("history-").should be_true + history.id.should be_a(UUID) - found = History.find!(history.id.as(String)) - found.id.should eq history.id - found.type.should eq history.type + found = History.find!(history.id.as(UUID)) + found.resource_type.should eq history.resource_type found.resource_id.should eq history.resource_id found.action.should eq history.action - found.changed_fields.should eq history.changed_fields + found.changed_fields.should eq ["name"] + found.diff["name"].should eq JSON.parse(%(["old", "new"])) end - it "requires type" do - history = History.new( - type: "", - resource_id: "zone-123", - action: "update" - ) + it "requires resource_type, resource_id and action" do + history = History.new(resource_type: "", resource_id: "", action: "") history.valid?.should be_false - history.errors.first.field.should eq :type + history.errors.map(&.field).sort!.should eq [:action, :resource_id, :resource_type] end - it "requires resource_id" do - history = History.new( - type: "zone", - resource_id: "", - action: "update" - ) - history.valid?.should be_false - history.errors.first.field.should eq :resource_id + it "queries entries of a resource and correlated entries, newest first" do + older = Generator.history(resource_id: "zone-1", correlation_id: "group-1", created_at: 2.minutes.ago).save! + newer = Generator.history(resource_id: "zone-1", created_at: 1.minute.ago).save! + other = Generator.history(resource_id: "zone-2", correlation_id: "group-1").save! + + History.for_resource("zone", "zone-1").to_a.map(&.id).should eq [newer.id, older.id] + History.correlated("group-1").to_a.map(&.id).should eq [other.id, older.id] end - it "defaults changed_fields to empty array" do - history = History.new( - type: "zone", - resource_id: "zone-123", - action: "create" - ) - history.save! + describe History::Tracked do + it "records creates with every field set" do + zone = Generator.zone + zone.description = "a zone" + zone.save! - history.changed_fields.should eq [] of String - end + entries = zone.change_history.to_a + entries.size.should eq 1 + entry = entries.first + entry.action.should eq "create" + entry.resource_type.should eq "zone" + entry.resource_id.should eq zone.id + entry.diff["name"].should eq JSON::Any.new([JSON::Any.new(nil), JSON::Any.new(zone.name)]) + entry.diff["description"][1].should eq "a zone" + entry.changed_fields.should_not contain "created_at" + entry.snapshot.try(&.raw).should be_nil + end - it "saves action field" do - history = History.new( - type: "zone", - resource_id: "zone-123", - action: "update", - changed_fields: ["name"] - ) - history.save! - - found = History.find!(history.id.as(String)) - found.action.should eq "update" - end + it "records previous and current values of updates" do + zone = Generator.zone.save! + previous_name = zone.name + + zone.name = "renamed" + zone.save! + + entries = zone.change_history.to_a + entries.map(&.action).should eq ["update", "create"] + entries.first.changed_fields.should eq ["name"] + entries.first.diff.should eq JSON.parse({"name" => [previous_name, "renamed"]}.to_json) + Zone.change_history(zone.id.as(String)).count.should eq 2 + end - it "supports different action types" do - ["create", "update", "delete"].each do |action| - history = History.new( - type: "zone", - resource_id: "zone-123", - action: action - ) - history.save! - history.action.should eq action + it "does not record saves without changes or with only ignored changes" do + system = Generator.control_system.save! + system.save! + system.signage_last_seen = Time.utc + system.save! + + system.change_history.to_a.map(&.action).should eq ["create"] + end + + it "records the values written, after other callbacks have run" do + invitation = Generator.group_invitation(email: " Someone@Example.COM ").save! + + entry = invitation.change_history.to_a.first + entry.diff["email"][1].should eq "someone@example.com" + end + + it "redacts sensitive fields" do + user = Generator.user + user.password = "secret password" + user.save! + + entry = user.change_history.to_a.first + entry.diff["password_digest"].should eq JSON.parse(%(["[REDACTED]", "[REDACTED]"])) + entry.diff.to_json.should_not contain(user.password_digest.not_nil!) + + user.destroy + snapshot = user.change_history.to_a.first.snapshot.not_nil! + snapshot.as_h.has_key?("password_digest").should be_false + snapshot["email"].should eq user.email.to_s + end + + it "records deletes with a snapshot" do + zone = Generator.zone.save! + zone.destroy + + entry = zone.change_history.to_a.first + entry.action.should eq "delete" + entry.diff.as_h.should be_empty + entry.snapshot.not_nil!["name"].should eq zone.name + end + + it "records composite keys and correlation ids" do + group_zone = Generator.group_zone.save! + group_zone.permissions = Permissions::All.to_i + group_zone.save! + + entries = History.correlated(group_zone.group_id.to_s).where(resource_type: "group_zones").to_a + entries.size.should eq 2 + entries.first.resource_id.should eq "#{group_zone.group_id}:#{group_zone.zone_id}" + entries.first.changed_fields.should eq ["permissions"] + end + + it "is rolled back with the change" do + zone = Generator.zone + expect_raises(Exception, "rollback") do + ::PgORM::Database.transaction do + zone.save! + raise "rollback" + end + end + + History.count.should eq 0 + end + + it "records approvals only for playlist revisions" do + revision = Generator.revision + revision.save! + revision.items = ["playlist-item-1"] + revision.save! + revision.change_history.count.should eq 0 + + approver = Generator.user.save! + revision.approver = approver + revision.save! + + entries = revision.change_history.to_a + entries.size.should eq 1 + entries.first.action.should eq "approve" + entries.first.correlation_id.should eq revision.playlist_id + entries.first.snapshot.not_nil!["approved_by_id"].should eq approver.id + + History.correlated(revision.playlist_id.not_nil!).count.should eq 1 + end + + it "records booking changes" do + tenant = get_tenant + booking = Generator.booking(tenant.id, "desk-1", 1.hour.from_now, 2.hours.from_now) + booking.save! + booking.checked_in = true + booking.save! + + entries = History.for_resource("bookings", booking.id.to_s).to_a + entries.map(&.action).should eq ["update", "create"] + entries.first.diff["checked_in"].should eq JSON.parse("[false, true]") + entries.first.changed_fields.should_not contain "history" + end + + describe "metadata" do + it "records the fiber's metadata, including cascading changes" do + user = Generator.user.save! + metadata = History::Metadata.new(app_name: "backoffice", ip: "10.0.0.1").with_user(user) + + zone = Generator.zone + other = Generator.zone + History.with_metadata(metadata) do + zone.save! + other.save! + end + History.current_metadata.should be_nil + + [zone, other].each do |record| + entry = record.change_history.to_a.first + entry.user_id.should eq user.id + entry.user_email.should eq user.email.to_s + entry.metadata.app_name.should eq "backoffice" + entry.metadata.ip.should eq "10.0.0.1" + entry.metadata.user_name.should eq user.name + end + end + + it "prefers the record's metadata and restores nested metadata" do + outer = History::Metadata.new(app_name: "outer") + inner = History::Metadata.new(app_name: "inner") + own = History::Metadata.new(app_name: "own") + + zone = Generator.zone + own_zone = Generator.zone + own_zone.history_metadata = own + + History.with_metadata(outer) do + History.with_metadata(inner) { zone.save! } + History.current_metadata.should eq outer + own_zone.save! + end + + zone.change_history.to_a.first.metadata.app_name.should eq "inner" + own_zone.change_history.to_a.first.metadata.app_name.should eq "own" + end + + it "records changes without metadata" do + zone = Generator.zone.save! + entry = zone.change_history.to_a.first + entry.user_id.should be_nil + entry.context.as_h.should be_empty + end + end + + describe ".undelete" do + it "restores a deleted record with its id" do + zone = Generator.zone + zone.description = "restore me" + zone.save! + zone.destroy + Zone.find?(zone.id.as(String)).should be_nil + + restored = Zone.undelete(zone.id.as(String)) + restored.id.should eq zone.id + + found = Zone.find!(zone.id.as(String)) + found.name.should eq zone.name + found.description.should eq "restore me" + + zone.change_history.to_a.map(&.action).should eq ["restore", "delete", "create"] + end + + it "raises when there is nothing to restore" do + expect_raises(::PgORM::Error::RecordNotFound) do + Zone.undelete("zone-missing") + end + end + + it "raises when the record already exists" do + zone = Generator.zone.save! + zone.destroy + Zone.undelete(zone.id.as(String)) + + expect_raises(::PgORM::Error) do + Zone.undelete(zone.id.as(String)) + end + end end end - it "sets timestamps on create" do - history = Generator.history.save! + describe "retention" do + it "keeps at most max_entries per resource" do + History.max_entries = 3 + zone = Generator.zone.save! + 4.times do |index| + zone.name = "name #{index}" + zone.save! + end - history.created_at.should_not be_nil - history.updated_at.should_not be_nil + entries = zone.change_history.to_a + entries.size.should eq 3 + entries.first.diff["name"][1].should eq "name 3" + end + + it "removes entries older than retention beyond min_entries" do + History.min_entries = 2 + zone = Generator.zone.save! + 3.times do |index| + Generator.history(resource_id: zone.id.as(String), created_at: (40 + index).days.ago).save! + end + + zone.name = "renamed" + zone.save! + + entries = zone.change_history.to_a + entries.size.should eq 2 + entries.first.action.should eq "update" + entries.last.action.should eq "create" + end + + it "keeps min_entries of old resources" do + History.min_entries = 2 + 4.times do |index| + Generator.history(resource_id: "zone-old", created_at: (40 + index).days.ago).save! + end + Generator.history(resource_id: "zone-new").save! + + History.cleanup_expired.should eq 2 + History.for_resource("zone", "zone-old").count.should eq 2 + History.for_resource("zone", "zone-new").count.should eq 1 + end + + it "removes every entry once a deletion has expired" do + Generator.history(resource_id: "zone-gone", created_at: 50.days.ago).save! + Generator.history(resource_id: "zone-gone", action: "delete", created_at: 40.days.ago).save! + Generator.history(resource_id: "zone-recent", created_at: 50.days.ago).save! + Generator.history(resource_id: "zone-recent", action: "delete", created_at: 1.day.ago).save! + + History.cleanup_expired.should eq 2 + History.for_resource("zone", "zone-gone").count.should eq 0 + History.for_resource("zone", "zone-recent").count.should eq 2 + end + + it "cleans up in batches" do + History.min_entries = 0 + 3.times { Generator.history(created_at: 40.days.ago).save! } + + History.cleanup_expired(2).should eq 2 + History.cleanup_expired(2).should eq 1 + History.count.should eq 0 + end + + it "cleans up periodically as changes are recorded" do + Generator.history(resource_id: "zone-gone", action: "delete", created_at: 40.days.ago).save! + History.cleanup_interval = Time::Span.zero + + Generator.zone.save! + History.for_resource("zone", "zone-gone").count.should eq 0 + end end end end diff --git a/spec/signage_template_spec.cr b/spec/signage_template_spec.cr index 4e5b7d89..979c4831 100644 --- a/spec/signage_template_spec.cr +++ b/spec/signage_template_spec.cr @@ -178,9 +178,13 @@ module PlaceOS::Model end describe "history recording" do - it "does not record history on create" do - Generator.signage_template.save! - History.count.should eq 0 + it "records history on create" do + template = Generator.signage_template.save! + History.where(resource_type: "signage_template").count.should eq 1 + history = template.change_history.to_a.first + history.action.should eq "create" + history.resource_type.should eq "signage_template" + history.diff["name"].should eq JSON::Any.new([JSON::Any.new(nil), JSON::Any.new(template.name)]) end it "records changed fields on update" do @@ -190,12 +194,13 @@ module PlaceOS::Model template.tags = ["new-tag"] template.save! - History.count.should eq 1 - history = History.all.to_a.first - history.type.should eq "template" + History.where(resource_type: "signage_template").count.should eq 2 + history = template.change_history.to_a.first + history.resource_type.should eq "signage_template" history.resource_id.should eq template.id.to_s - history.action.should eq "updated" + history.action.should eq "update" history.changed_fields.should eq ["name", "tags"] + history.diff["tags"].should eq JSON.parse(%([[], ["new-tag"]])) end it "persists and records merge changes" do @@ -206,8 +211,8 @@ module PlaceOS::Model template.save! SignageTemplate.find!(template.id.as(UUID)).merge.should eq true - History.count.should eq 1 - History.all.to_a.first.changed_fields.should eq ["merge"] + History.where(resource_type: "signage_template").count.should eq 2 + template.change_history.to_a.first.changed_fields.should eq ["merge"] end it "records layout changes" do @@ -216,14 +221,14 @@ module PlaceOS::Model template.layouts = [Generator.layout] template.save! - History.count.should eq 1 - History.all.to_a.first.changed_fields.should eq ["layouts"] + History.where(resource_type: "signage_template").count.should eq 2 + template.change_history.to_a.first.changed_fields.should eq ["layouts"] end it "does not record history when nothing changed" do template = Generator.signage_template.save! template.save! - History.count.should eq 0 + History.where(resource_type: "signage_template").count.should eq 1 end end @@ -250,8 +255,11 @@ module PlaceOS::Model found.approved_by_name.should eq user.name found.approved_by_email.should eq user.email - History.count.should eq 1 - History.all.to_a.first.changed_fields.should eq ["approved"] + History.where(resource_type: "signage_template").count.should eq 2 + history = template.change_history.to_a.first + history.action.should eq "approve" + history.changed_fields.should contain "approved" + history.snapshot.not_nil!["approved_by_id"].should eq user.id end it "removes draft rows when the approved parent is deleted" do @@ -287,8 +295,9 @@ module PlaceOS::Model draft.name = "draft edit" draft.layouts = [Generator.layout] draft.save! + draft.destroy - History.count.should eq 0 + History.where(resource_id: draft.id.to_s).count.should eq 0 end it "approves a draft onto the live template" do @@ -318,9 +327,13 @@ module PlaceOS::Model # the draft is removed once applied SignageTemplate.find?(draft.id.as(UUID)).should be_nil - # the promotion is recorded against the live template - History.count.should eq 1 - History.all.to_a.first.changed_fields.should contain "approved" + # the promotion is recorded against the live template, the draft isn't recorded + History.where(resource_id: draft.id.to_s).count.should eq 0 + history = live.change_history.to_a.first + history.action.should eq "approve" + history.changed_fields.should contain "layouts" + history.user_id.should be_nil + history.snapshot.not_nil!["merge"].should eq true end it "raises when approving a template that is not a draft" do @@ -339,8 +352,8 @@ module PlaceOS::Model template.requested_by_id = Generator.user.save!.id template.save! - History.count.should eq 1 - History.all.to_a.first.changed_fields.should eq ["approval_requested"] + History.where(resource_type: "signage_template").count.should eq 2 + template.change_history.to_a.first.changed_fields.should eq ["approval_requested", "requested_by_id"] end end diff --git a/spec/user_spec.cr b/spec/user_spec.cr index 3e19fd9b..929bf85a 100644 --- a/spec/user_spec.cr +++ b/spec/user_spec.cr @@ -369,7 +369,7 @@ module PlaceOS::Model describe "#subsystem_access" do Spec.before_each do - GroupHistory.clear + History.clear GroupInvitation.clear GroupZone.clear GroupUser.clear diff --git a/src/placeos-models/alert.cr b/src/placeos-models/alert.cr index 3520c59a..bcf60f3f 100644 --- a/src/placeos-models/alert.cr +++ b/src/placeos-models/alert.cr @@ -1,6 +1,7 @@ require "json" require "./base/model" require "./trigger/conditions" +require "./history" module PlaceOS::Model class Alert < ModelBase @@ -8,6 +9,8 @@ module PlaceOS::Model table :alert + include History::Tracked + enum Severity LOW MEDIUM diff --git a/src/placeos-models/alert_dashboard.cr b/src/placeos-models/alert_dashboard.cr index c1d13903..5a3d91d6 100644 --- a/src/placeos-models/alert_dashboard.cr +++ b/src/placeos-models/alert_dashboard.cr @@ -1,5 +1,6 @@ require "json" require "./base/model" +require "./history" module PlaceOS::Model class AlertDashboard < ModelBase @@ -7,6 +8,8 @@ module PlaceOS::Model table :alert_dashboard + include History::Tracked + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute description : String = "", sanitize: :common attribute enabled : Bool = true diff --git a/src/placeos-models/asset.cr b/src/placeos-models/asset.cr index ef406d02..a070ab6c 100644 --- a/src/placeos-models/asset.cr +++ b/src/placeos-models/asset.cr @@ -1,6 +1,7 @@ require "./base/model" require "./asset_type" require "./asset_purchase_order" +require "./history" module PlaceOS::Model class Asset < ModelBase @@ -8,6 +9,8 @@ module PlaceOS::Model table :asset + include History::Tracked + attribute identifier : String?, es_type: "keyword" attribute serial_number : String? attribute other_data : JSON::Any?, sanitize: :common diff --git a/src/placeos-models/asset_category.cr b/src/placeos-models/asset_category.cr index f6e9a2ed..6953c2d9 100644 --- a/src/placeos-models/asset_category.cr +++ b/src/placeos-models/asset_category.cr @@ -1,5 +1,6 @@ require "./base/model" require "./asset_type" +require "./history" module PlaceOS::Model class AssetCategory < ModelBase @@ -7,6 +8,8 @@ module PlaceOS::Model table :asset_category + include History::Tracked + # i.e. a tablet attribute name : String, sanitize: :text, es_subfield: "keyword" # NOTE: intentionally not sanitized — this field is used to hold raw JSON diff --git a/src/placeos-models/asset_purchase_order.cr b/src/placeos-models/asset_purchase_order.cr index a418c179..8cc4dabb 100644 --- a/src/placeos-models/asset_purchase_order.cr +++ b/src/placeos-models/asset_purchase_order.cr @@ -1,5 +1,6 @@ require "./base/model" require "./asset" +require "./history" module PlaceOS::Model class AssetPurchaseOrder < ModelBase @@ -7,6 +8,8 @@ module PlaceOS::Model table :asset_purchase_order + include History::Tracked + attribute purchase_order_number : String, sanitize: :text, es_type: "keyword" attribute invoice_number : String?, sanitize: :text attribute supplier_details : JSON::Any?, sanitize: :common diff --git a/src/placeos-models/asset_type.cr b/src/placeos-models/asset_type.cr index 2800bf0a..925c33fb 100644 --- a/src/placeos-models/asset_type.cr +++ b/src/placeos-models/asset_type.cr @@ -1,6 +1,7 @@ require "./base/model" require "./asset_category" require "./asset" +require "./history" module PlaceOS::Model class AssetType < ModelBase @@ -8,6 +9,8 @@ module PlaceOS::Model table :asset_type + include History::Tracked + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute brand : String, sanitize: :text attribute description : String?, sanitize: :common diff --git a/src/placeos-models/authority.cr b/src/placeos-models/authority.cr index 7db1e6fb..a9fb92d9 100644 --- a/src/placeos-models/authority.cr +++ b/src/placeos-models/authority.cr @@ -6,6 +6,7 @@ require "./ldap_authentication" require "./oauth_authentication" require "./saml_authentication" require "./user" +require "./history" module PlaceOS::Model class Authority < ModelBase @@ -13,6 +14,9 @@ module PlaceOS::Model table :authority + include History::Tracked + history_redact :internals + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute description : String = "", sanitize: :common attribute domain : String diff --git a/src/placeos-models/booking.cr b/src/placeos-models/booking.cr index d8aae01e..283d555b 100644 --- a/src/placeos-models/booking.cr +++ b/src/placeos-models/booking.cr @@ -5,11 +5,20 @@ require "./attendee" require "./tenant" require "./utilities/jsonb_query_helper" require "./email" +require "./history" module PlaceOS::Model class Booking < ModelWithAutoKey table :bookings + include History::Tracked + history_ignore :history, :last_changed, :email_digest, :booked_by_email_digest + + # linked bookings are recorded against their parent + def history_correlation_id : String? + parent_id.try(&.to_s) + end + record History, state : State, time : Int64, source : String? = nil do include JSON::Serializable end diff --git a/src/placeos-models/booking_instance.cr b/src/placeos-models/booking_instance.cr index 6fcaed8d..b47bfa51 100644 --- a/src/placeos-models/booking_instance.cr +++ b/src/placeos-models/booking_instance.cr @@ -1,11 +1,24 @@ require "json" require "./base/model" require "./booking" +require "./history" module PlaceOS::Model class BookingInstance < ModelWithAutoKey table :booking_instances + include History::Tracked + history_ignore :history + + # an occurrence of the booking, recorded against it + def history_resource_id : String + "#{id}:#{instance_start}" + end + + def history_correlation_id : String? + id.to_s + end + alias History = Booking::History # the original starting time of the instance diff --git a/src/placeos-models/control_system.cr b/src/placeos-models/control_system.cr index 41db663d..f54ebb01 100644 --- a/src/placeos-models/control_system.cr +++ b/src/placeos-models/control_system.cr @@ -10,6 +10,7 @@ require "./email" require "./utilities/settings_helper" require "./utilities/metadata_helper" require "./playlist" +require "./history" module PlaceOS::Model class ControlSystem < ModelBase @@ -20,6 +21,9 @@ module PlaceOS::Model table :sys + include History::Tracked + history_ignore :signage_last_seen, :playlist_item_id, :version, :installed_ui_devices + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute description : String = "", sanitize: :common diff --git a/src/placeos-models/doorkeeper_application.cr b/src/placeos-models/doorkeeper_application.cr index 9f721359..451c852d 100644 --- a/src/placeos-models/doorkeeper_application.cr +++ b/src/placeos-models/doorkeeper_application.cr @@ -4,11 +4,15 @@ require "time" require "random/secure" require "./base/model" require "digest/md5" +require "./history" module PlaceOS::Model class DoorkeeperApplication < ModelWithAutoKey table :oauth_applications + include History::Tracked + history_redact :secret + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute secret : String attribute scopes : String = "public" diff --git a/src/placeos-models/driver.cr b/src/placeos-models/driver.cr index e4c274e3..0ba4bfb0 100644 --- a/src/placeos-models/driver.cr +++ b/src/placeos-models/driver.cr @@ -4,6 +4,7 @@ require "./utilities/settings_helper" require "./repository" require "./settings" +require "./history" module PlaceOS::Model class Driver < ModelBase @@ -12,6 +13,9 @@ module PlaceOS::Model table :driver + include History::Tracked + history_ignore :compilation_output, :update_available, :update_info + # Driver metadata and build progress do not change running module configuration. changefeed_ignore_updates :name, :description, :update_available, :update_info, :compilation_output, :updated_at, database_columns: [:search_vector] diff --git a/src/placeos-models/event_metadata.cr b/src/placeos-models/event_metadata.cr index a442be0c..e8f5090b 100644 --- a/src/placeos-models/event_metadata.cr +++ b/src/placeos-models/event_metadata.cr @@ -1,5 +1,6 @@ require "json" require "./base/model" +require "./history" require "./utilities/jsonb_query_helper" module PlaceOS::Model @@ -171,49 +172,33 @@ module PlaceOS::Model end end - before_save :record_metadata_history + # History + ############################################################################################### - protected def record_metadata_history - # Skip if this is a new record being created - event creation is tracked via notify_change - return unless persisted? + include History::Tracked - changed_fields = [] of String - - # Check each trackable field - changed_fields << "system_id" if system_id_changed? - changed_fields << "event_start" if event_start_changed? - changed_fields << "event_end" if event_end_changed? - changed_fields << "cancelled" if cancelled_changed? - changed_fields << "setup_time" if setup_time_changed? - changed_fields << "breakdown_time" if breakdown_time_changed? - changed_fields << "setup_event_id" if setup_event_id_changed? - changed_fields << "breakdown_event_id" if breakdown_event_id_changed? - changed_fields << "permission" if permission_changed? + # event creation is tracked via notify_change + protected def history_record_as(action : String) : String? + super unless action == "create" && history_action.nil? + end - # For ext_data, peek one level to see which keys changed - if ext_data_changed? - current = ext_data.try(&.as_h?) || {} of String => JSON::Any - previous = ext_data_was.try(&.as_h?) || {} of String => JSON::Any + # ext_data changes are listed per key, i.e. `ext_data.catering` + protected def history_changed_fields(diff : Hash(String, JSON::Any)) : Array(String) + fields = diff.keys + return fields unless change = diff["ext_data"]? - # Find keys that were added, removed, or modified - all_keys = (current.keys + previous.keys).uniq - all_keys.each do |key| - if current[key]? != previous[key]? - changed_fields << "ext_data.#{key}" - end - end + previous = change[0].as_h? || {} of String => JSON::Any + current = change[1].as_h? || {} of String => JSON::Any + fields.delete("ext_data") + (previous.keys | current.keys).each do |key| + fields << "ext_data.#{key}" if previous[key]? != current[key]? end + fields + end - return if changed_fields.empty? - - History.create!( - type: "event", - resource_id: event_id, - action: "updated", - changed_fields: changed_fields - ) - rescue ex - Log.error(exception: ex) { "failed to record event metadata history for #{event_id}" } + # recorded against the calendar event + def history_resource_id : String + event_id end def self.migrate_recurring_metadata(system_id : String, recurrance : PlaceCalendar::Event, parent_metadata : EventMetadata) diff --git a/src/placeos-models/group.cr b/src/placeos-models/group.cr index 9fd56355..00a44344 100644 --- a/src/placeos-models/group.cr +++ b/src/placeos-models/group.cr @@ -4,10 +4,7 @@ require "uuid/json" require "./base/model" require "./authority" require "./permissions" -# `history` defines `GroupHistory::Mixin`, which every group-subsystem -# model pulls in via `include`. It must be required before any class that -# does the include, so it goes first. -require "./group/history" +require "./history" module PlaceOS::Model # A node in the group tree for an authority. The tree represents the @@ -112,7 +109,7 @@ module PlaceOS::Model end end - include GroupHistory::Mixin + include History::Tracked # This group's own features for `subsystem`, without inheritance. def features_for(subsystem : String) : Hash(String, JSON::Any) @@ -582,20 +579,12 @@ module PlaceOS::Model end # -------------------------------------------------------------------- - # GroupHistory hooks + # History # -------------------------------------------------------------------- - protected def group_history_resource_type : String - "group" - end - - protected def group_history_resource_id : String + def history_correlation_id : String? self.id.to_s end - - protected def group_history_group_id : UUID? - self.id - end end end diff --git a/src/placeos-models/group/history.cr b/src/placeos-models/group/history.cr deleted file mode 100644 index 1262313b..00000000 --- a/src/placeos-models/group/history.cr +++ /dev/null @@ -1,135 +0,0 @@ -require "uuid" -require "uuid/json" - -require "../base/model" -require "../user" - -module PlaceOS::Model - # Audit trail for changes to Group-system models. Written inside the - # same transaction as the triggering save/destroy via each model's - # after-callbacks. - # - # `user_id` is nullable: it's SET NULL (via the FK) when the acting - # user is later deleted. `email` is preserved so the audit trail stays - # readable even after user deletion. - class GroupHistory < ::PgORM::Base - table :group_history - - default_primary_key id : UUID, autogenerated: true - - attribute group_id : UUID? - attribute user_id : String? - attribute email : String - attribute action : String - attribute resource_type : String - attribute resource_id : String - attribute changed_fields : Array(String) = [] of String - attribute created_at : Time = -> { Time.utc } - - validates :email, presence: true - validates :action, presence: true - validates :resource_type, presence: true - validates :resource_id, presence: true - - before_create do - self.created_at = Time.utc - end - - # True once the acting user has been deleted and their reference - # nulled out. `email` remains set so the audit trail is still useful. - def user_deleted? : Bool - user_id.nil? - end - end - - # Mixin that injects the GroupHistory recording machinery into a model. - # - # Including class must provide three small hook methods that tell the - # mixin *what* to write: - # - # protected def group_history_resource_type : String # e.g. "group" - # protected def group_history_resource_id : String # e.g. self.id.to_s - # protected def group_history_group_id : UUID? # or nil - # - # Optionally override `group_history_changed_fields` to strip sensitive - # attributes (see `GroupInvitation`). - # - # The mixin installs: - # - three virtual (non-persisted, non-serialised) properties: - # `acting_user`, `was_new_record?`, `captured_changed_fields`. - # - `before_save :capture_history_state` — snapshots `new_record?` and - # the changed attribute list before they're cleared by the save. - # - `after_save :record_history_save` — writes a `"create"` or - # `"update"` row iff `acting_user` is set and (for updates) at least - # one attribute changed. - # - `after_destroy :record_history_destroy` — writes a `"delete"` row - # iff `acting_user` is set. - module GroupHistory::Mixin - macro included - @[JSON::Field(ignore: true)] - @[YAML::Field(ignore: true)] - property acting_user : ::PlaceOS::Model::User? - - @[JSON::Field(ignore: true)] - @[YAML::Field(ignore: true)] - property? was_new_record : Bool = false - - @[JSON::Field(ignore: true)] - @[YAML::Field(ignore: true)] - property captured_changed_fields : Array(String) = [] of String - - before_save :capture_history_state - after_save :record_history_save - after_destroy :record_history_destroy - - protected def capture_history_state - self.was_new_record = new_record? - self.captured_changed_fields = was_new_record? ? attributes.keys.map(&.to_s) : changed_attributes.keys.map(&.to_s) - end - - protected def record_history_save - user = acting_user - return unless user - action = was_new_record? ? "create" : "update" - fields = group_history_changed_fields - return if action == "update" && fields.empty? - ::PlaceOS::Model::GroupHistory.create!( - group_id: group_history_group_id, - user_id: user.id, - email: user.email.to_s, - action: action, - resource_type: group_history_resource_type, - resource_id: group_history_resource_id, - changed_fields: fields, - ) - end - - protected def record_history_destroy - user = acting_user - return unless user - ::PlaceOS::Model::GroupHistory.create!( - group_id: group_history_group_id, - user_id: user.id, - email: user.email.to_s, - action: "delete", - resource_type: group_history_resource_type, - resource_id: group_history_resource_id, - changed_fields: [] of String, - ) - end - - # Default: everything captured in `before_save`. Override to strip - # sensitive attributes out of the audit trail. - protected def group_history_changed_fields : Array(String) - captured_changed_fields - end - - def group_history_resource_type : String - self.class.to_s.split("::").last.underscore - end - end - - abstract def group_history_resource_id : String - abstract def group_history_group_id : UUID? - end -end diff --git a/src/placeos-models/group/invitation.cr b/src/placeos-models/group/invitation.cr index b09f98c7..fc74860e 100644 --- a/src/placeos-models/group/invitation.cr +++ b/src/placeos-models/group/invitation.cr @@ -6,7 +6,7 @@ require "uuid/json" require "../base/model" require "../group" require "../permissions" -require "./history" +require "../history" module PlaceOS::Model # Grants a user who is not yet in the system (or external) access to a @@ -43,7 +43,10 @@ module PlaceOS::Model before_save :normalise_email - include GroupHistory::Mixin + include History::Tracked + + # the digest is sensitive even post-hashing + history_redact :secret_digest # Build a fresh invitation, generating a cryptographically strong # secret. The returned instance exposes `.plaintext_secret` *only until @@ -103,9 +106,10 @@ module PlaceOS::Model existing = GroupUser.where(user_id: user.id, group_id: self.group_id).first? gu = existing || GroupUser.new(user_id: user.id.not_nil!, group_id: self.group_id) gu.permissions = self.permissions - gu.acting_user = user + metadata = (History.current_metadata || History::Metadata.new).with_user(user) + gu.history_metadata = metadata gu.save! - self.acting_user = user + self.history_metadata = metadata destroy gu end @@ -114,18 +118,8 @@ module PlaceOS::Model self.email = self.email.strip.downcase end - protected def group_history_resource_id : String - self.id.to_s - end - - protected def group_history_group_id : UUID? - self.group_id - end - - # Treat the SHA256 digest as sensitive even post-hashing — keep it - # out of the audit trail's changed_fields list. - protected def group_history_changed_fields : Array(String) - captured_changed_fields.reject("secret_digest") + def history_correlation_id : String? + self.group_id.to_s end end end diff --git a/src/placeos-models/group/playlist.cr b/src/placeos-models/group/playlist.cr index e3f48c0d..eece6cc8 100644 --- a/src/placeos-models/group/playlist.cr +++ b/src/placeos-models/group/playlist.cr @@ -10,8 +10,8 @@ module PlaceOS::Model # (authority-scoped, legacy TEXT PK). Lets groups co-own / share # playlists with each other within the same authority. # - # Presence-only — no per-row permission bitmask, no GroupHistory - # audit. A user's capability on a linked playlist comes from their + # Presence-only — no per-row permission bitmask, no change + # history. A user's capability on a linked playlist comes from their # `GroupUser.permissions` within the group. # # Both sides must share an authority — enforced at the model layer diff --git a/src/placeos-models/group/playlist_item.cr b/src/placeos-models/group/playlist_item.cr index 151006ee..2c14ab5f 100644 --- a/src/placeos-models/group/playlist_item.cr +++ b/src/placeos-models/group/playlist_item.cr @@ -11,8 +11,8 @@ module PlaceOS::Model # as `GroupPlaylist` but against individual media / plugin / webpage # items. # - # Presence-only — no per-row permission bitmask, no GroupHistory - # audit. A user's capability on a linked item comes from their + # Presence-only — no per-row permission bitmask, no change + # history. A user's capability on a linked item comes from their # `GroupUser.permissions` within the group. # # Both sides must share an authority — enforced here at the model diff --git a/src/placeos-models/group/signage_template.cr b/src/placeos-models/group/signage_template.cr index 2f41ff69..b32644c4 100644 --- a/src/placeos-models/group/signage_template.cr +++ b/src/placeos-models/group/signage_template.cr @@ -10,7 +10,7 @@ module PlaceOS::Model # UUID). Same M:N shape as `GroupPlaylistItem` — allows templates to be # shared with groups so they can manage their own templates. # - # Presence-only — no per-row permission bitmask, no GroupHistory audit. + # Presence-only — no per-row permission bitmask, no change history. # A user's capability on a linked template comes from their # `GroupUser.permissions` within the group. # diff --git a/src/placeos-models/group/user.cr b/src/placeos-models/group/user.cr index a0346238..a4e60d2b 100644 --- a/src/placeos-models/group/user.cr +++ b/src/placeos-models/group/user.cr @@ -5,7 +5,7 @@ require "../base/model" require "../group" require "../permissions" require "../user" -require "./history" +require "../history" module PlaceOS::Model # Junction table: a user's membership in a group, with a permission @@ -45,7 +45,7 @@ module PlaceOS::Model this.validation_error(:user_id, "must belong to the same authority as the group") } - include GroupHistory::Mixin + include History::Tracked def permission_flags : Permissions Permissions.new(self.permissions || 0) @@ -55,12 +55,8 @@ module PlaceOS::Model self.permissions = flags.to_i end - protected def group_history_resource_id : String - "#{self.user_id}:#{self.group_id}" - end - - protected def group_history_group_id : UUID? - self.group_id + def history_correlation_id : String? + self.group_id.to_s end end end diff --git a/src/placeos-models/group/zone.cr b/src/placeos-models/group/zone.cr index fe9cc651..3f2fe470 100644 --- a/src/placeos-models/group/zone.cr +++ b/src/placeos-models/group/zone.cr @@ -5,7 +5,7 @@ require "../base/model" require "../group" require "../permissions" require "../zone" -require "./history" +require "../history" module PlaceOS::Model # Junction table: a group's access to a zone, with a permission bitmask @@ -31,7 +31,7 @@ module PlaceOS::Model validates :group_id, presence: true validates :zone_id, presence: true - include GroupHistory::Mixin + include History::Tracked def permission_flags : Permissions Permissions.new(self.permissions) @@ -41,12 +41,8 @@ module PlaceOS::Model self.permissions = flags.to_i end - protected def group_history_resource_id : String - "#{self.group_id}:#{self.zone_id}" - end - - protected def group_history_group_id : UUID? - self.group_id + def history_correlation_id : String? + self.group_id.to_s end end end diff --git a/src/placeos-models/history.cr b/src/placeos-models/history.cr index 1d012306..e9ac73eb 100644 --- a/src/placeos-models/history.cr +++ b/src/placeos-models/history.cr @@ -1,21 +1,236 @@ +require "json" +require "uuid" +require "uuid/json" + require "./base/model" module PlaceOS::Model - class History < ModelBase - include PlaceOS::Model::Timestamps + # Unified change history for models that include `History::Tracked`. + # + # Entries are written in the same transaction as the change they describe. + # Filter by `resource_type` (the table name) and `resource_id`, or by + # `correlation_id` to see changes recorded against a parent, e.g. every + # permission change of a group. + class History < ::PgORM::Base + include Neuroplastic + + Log = ::Log.for(self) table :history - attribute type : String, es_subfield: "keyword" + default_primary_key id : UUID, autogenerated: true + + attribute resource_type : String, es_subfield: "keyword" attribute resource_id : String, es_subfield: "keyword" + attribute correlation_id : String?, es_subfield: "keyword" + + # create, update, delete, approve or restore attribute action : String, es_subfield: "keyword" attribute changed_fields : Array(String) = [] of String, es_type: "keyword" - # Validation - ############################################################################################### + # `{"field": [previous, current]}` of the persisted changes + attribute diff : JSON::Any = JSON::Any.new({} of String => JSON::Any), es_ignore: true + + # the full record, for deletes (see `History::Tracked.undelete`) and approvals. + # NOTE:: pg-orm reads a NULL column as `JSON::Any(nil)` + attribute snapshot : JSON::Any? = nil, es_ignore: true + + attribute user_id : String?, es_subfield: "keyword" + attribute user_email : String?, es_subfield: "keyword" + + # request details, see `History::Metadata` + attribute context : JSON::Any = JSON::Any.new({} of String => JSON::Any), es_ignore: true - validates :type, presence: true + attribute created_at : Time = -> { Time.utc }, converter: PlaceOS::Model::Timestamps::EpochConverter, type: "integer", format: "Int64" + + validates :resource_type, presence: true validates :resource_id, presence: true validates :action, presence: true + + # Entries of a resource, newest first + def self.for_resource(resource_type : String, resource_id : String) + where(resource_type: resource_type, resource_id: resource_id).order(created_at: :desc, id: :desc) + end + + # Entries recorded against a parent, newest first + def self.correlated(correlation_id : String) + where(correlation_id: correlation_id).order(created_at: :desc, id: :desc) + end + + def metadata : Metadata + Metadata.from_json(context.to_json) + end + + # Request context + ############################################################################################### + + # Details of who made a change and from where, recorded with each entry. + # + # Set for the current fiber with `History.with_metadata` (rest-api does + # this for each request) or per record with `Tracked#history_metadata=`. + record Metadata, + user_id : String? = nil, + user_email : String? = nil, + user_name : String? = nil, + api_key_id : String? = nil, + api_key_name : String? = nil, + app_id : String? = nil, + app_name : String? = nil, + app_date : String? = nil, + app_build : String? = nil, + user_agent : String? = nil, + ip : String? = nil, + request_id : String? = nil do + include JSON::Serializable + + # Copy of this metadata attributed to `user` + def with_user(user) : Metadata + copy_with(user_id: user.id, user_email: user.email.to_s, user_name: user.name) + end + end + + @@fiber_metadata = {} of UInt64 => Metadata + + # Metadata of the current fiber, see `with_metadata` + def self.current_metadata : Metadata? + @@fiber_metadata[Fiber.current.object_id]? + end + + # Attributes changes made by the block, including those of cascading + # callbacks, to `metadata` + def self.with_metadata(metadata : Metadata?, &) + fiber_id = Fiber.current.object_id + previous = @@fiber_metadata[fiber_id]? + set_fiber_metadata(fiber_id, metadata) + begin + yield + ensure + set_fiber_metadata(fiber_id, previous) + end + end + + private def self.set_fiber_metadata(fiber_id : UInt64, metadata : Metadata?) : Nil + if metadata + @@fiber_metadata[fiber_id] = metadata + else + @@fiber_metadata.delete(fiber_id) + end + end + + # Retention + ############################################################################################### + + # Per resource, entries within `retention` are kept up to `max_entries`. + # Older entries are removed, except for the newest `min_entries`, which + # are kept until the resource has been deleted for longer than `retention`. + class_property retention : Time::Span = (ENV["PLACE_HISTORY_RETENTION_DAYS"]?.try(&.to_i?) || 30).days + class_property max_entries : Int32 = ENV["PLACE_HISTORY_MAX_ENTRIES"]?.try(&.to_i?) || 250 + class_property min_entries : Int32 = ENV["PLACE_HISTORY_MIN_ENTRIES"]?.try(&.to_i?) || 5 + + # How often `cleanup_expired` runs as entries are recorded, nil disables it + class_property cleanup_interval : Time::Span? = 1.hour + class_property cleanup_batch : Int32 = 1000 + + @@last_cleanup : Time? = nil + + # Records a change, then applies the retention policy + def self.record_change( + resource_type : String, + resource_id : String, + action : String, + correlation_id : String? = nil, + diff : Hash(String, JSON::Any) = {} of String => JSON::Any, + changed_fields : Array(String) = diff.keys, + snapshot : JSON::Any? = nil, + metadata : Metadata? = nil, + ) : History + metadata ||= current_metadata + entry = create!( + resource_type: resource_type, + resource_id: resource_id, + correlation_id: correlation_id, + action: action, + changed_fields: changed_fields, + diff: JSON::Any.new(diff), + snapshot: snapshot, + user_id: metadata.try(&.user_id), + user_email: metadata.try(&.user_email), + context: metadata ? JSON.parse(metadata.to_json) : JSON::Any.new({} of String => JSON::Any), + ) + trim(resource_type, resource_id) + cleanup_periodically + entry + end + + # Applies the retention policy to a single resource + def self.trim(resource_type : String, resource_id : String) : Nil + ::PgORM::Database.connection &.exec(<<-SQL, resource_type, resource_id, max_entries, min_entries, Time.utc - retention) + DELETE FROM history WHERE id IN ( + SELECT id FROM ( + SELECT id, created_at, row_number() OVER (ORDER BY created_at DESC, id DESC) AS position + FROM history + WHERE resource_type = $1 AND resource_id = $2 + ) ranked + WHERE position > $3 OR (position > $4 AND created_at < $5) + ) + SQL + end + + # Removes expired entries across all resources, in batches of `limit`. + # + # Entries older than `retention` are removed beyond the newest + # `min_entries` of each resource, or entirely once the resource was + # deleted longer than `retention` ago. Returns the number removed. + def self.cleanup_expired(limit : Int32 = cleanup_batch) : Int32 + cutoff = Time.utc - retention + result = ::PgORM::Database.connection &.exec(<<-SQL, cutoff, min_entries, limit) + DELETE FROM history WHERE id IN ( + SELECT h.id FROM history h + LEFT JOIN LATERAL ( + SELECT l.action, l.created_at FROM history l + WHERE l.resource_type = h.resource_type AND l.resource_id = h.resource_id + ORDER BY l.created_at DESC, l.id DESC + LIMIT 1 + ) latest ON TRUE + WHERE h.created_at < $1 + AND ( + (latest.action = 'delete' AND latest.created_at < $1) + OR $2 <= 0 + OR EXISTS ( + SELECT 1 FROM history n + WHERE n.resource_type = h.resource_type AND n.resource_id = h.resource_id + AND (n.created_at, n.id) > (h.created_at, h.id) + OFFSET GREATEST($2 - 1, 0) LIMIT 1 + ) + ) + ORDER BY h.created_at + LIMIT $3 + ) + SQL + result.rows_affected.to_i + end + + # Housekeeping is never worth failing a change over, so this runs in a + # savepoint and failures are logged + private def self.cleanup_periodically : Nil + interval = cleanup_interval + return unless interval + now = Time.utc + if last = @@last_cleanup + return if (now - last) < interval + end + @@last_cleanup = now + + begin + ::PgORM::Database.transaction do + removed = cleanup_expired + Log.info { {message: "history cleaned up", removed: removed} } if removed > 0 + end + rescue ex + Log.warn(exception: ex) { "history cleanup failed" } + end + end end end + +require "./history/tracked" diff --git a/src/placeos-models/history/tracked.cr b/src/placeos-models/history/tracked.cr new file mode 100644 index 00000000..a77fff68 --- /dev/null +++ b/src/placeos-models/history/tracked.cr @@ -0,0 +1,175 @@ +module PlaceOS::Model + # Records changes to a model in the `History` table. + # + # Creates and updates are captured by pg-orm's `on_persist` hook, after every + # `before_*` callback and validation have run, so the entry reflects what was + # written. Deletes are captured by `after_destroy`. Entries are written in the + # same transaction as the change. + # + # ``` + # class Zone < ModelBase + # include History::Tracked + # + # history_ignore :last_seen + # history_redact :secret + # end + # ``` + # + # Override `history_resource_id`, `history_correlation_id` or + # `history_record_as` to customise what is recorded. + module History::Tracked + REDACTED = JSON::Any.new("[REDACTED]") + + macro included + # Request details recorded with changes to this record, defaults to + # `History.current_metadata` + @[JSON::Field(ignore: true)] + @[YAML::Field(ignore: true)] + property history_metadata : ::PlaceOS::Model::History::Metadata? = nil + + # Overrides the action recorded by the next change, e.g. `approve` + @[JSON::Field(ignore: true)] + @[YAML::Field(ignore: true)] + property history_action : String? = nil + + after_destroy :__record_history_destroy__ + + # Fields not recorded, changes to only these fields are not recorded at all + def self.history_ignored : Array(String) + ["created_at", "updated_at"] + end + + # Fields whose values are never recorded + def self.history_redacted : Array(String) + [] of String + end + + # Entries recorded for the resource, newest first + def self.change_history(resource_id : String) + ::PlaceOS::Model::History.for_resource(table_name, resource_id) + end + + # Recreates a deleted record from its latest `delete` entry, keeping its id. + # + # Records removed alongside it (cascading deletes) are not restored and + # redacted fields are left at their defaults. Raises if there is no + # delete entry or the record can't be saved, e.g. if it already exists. + def self.undelete(resource_id : String) : self + entry = change_history(resource_id).where(action: "delete").first? + snapshot = entry.try(&.snapshot).try { |json| json unless json.raw.nil? } + raise ::PgORM::Error::RecordNotFound.new("no deleted #{table_name} #{resource_id} in history") unless snapshot + + record = from_trusted_json(snapshot.to_json) + record.history_action = "restore" + record.save! + record + end + end + + # Fields not recorded, changes to only these fields are not recorded at all + macro history_ignore(*fields) + def self.history_ignored : Array(String) + previous_def + [{{ fields.map(&.id.stringify).splat }}] + end + end + + # Fields whose values are never recorded + macro history_redact(*fields) + def self.history_redacted : Array(String) + previous_def + [{{ fields.map(&.id.stringify).splat }}] + end + end + + # Entries recorded for this record, newest first + def change_history + ::PlaceOS::Model::History.for_resource(self.class.table_name, history_resource_id) + end + + # Identifies the record in history, composite keys are joined with `:` + def history_resource_id : String + case key = id + when Tuple + key.join(':') + else + key.to_s + end + end + + # A parent the change should also be found under + def history_correlation_id : String? + nil + end + + # The action to record for a `create`, `update` or `delete`, or nil to skip it + protected def history_record_as(action : String) : String? + history_action || action + end + + # Fields listed as changed, from the `{"field": [previous, current]}` diff + protected def history_changed_fields(diff : Hash(String, JSON::Any)) : Array(String) + diff.keys + end + + # Actions that store the full record + protected def history_snapshot?(action : String) : Bool + action.in?("delete", "approve") + end + + protected def on_persist(created : Bool) : Nil + super + action = history_record_as(created ? "create" : "update") + self.history_action = nil + return unless action + + diff = history_diff(created) + return if diff.empty? && !created + __record_history__(action, diff) + end + + protected def __record_history_destroy__ + action = history_record_as("delete") + self.history_action = nil + __record_history__(action) if action + end + + # Persisted changes as `{"field": [previous, current]}`, without ignored + # fields and with redacted values replaced. Creates include every field set. + protected def history_diff(created : Bool = false) : Hash(String, JSON::Any) + changes = if created + JSON.parse(attributes_json).as_h.compact_map do |field, value| + {field, JSON::Any.new([JSON::Any.new(nil), value])} unless value.raw.nil? + end.to_h + else + JSON.parse(changes_json).as_h + end + + ignored = self.class.history_ignored + redacted = self.class.history_redacted + diff = {} of String => JSON::Any + changes.each do |field, change| + next if ignored.includes?(field) + diff[field] = redacted.includes?(field) ? JSON::Any.new([REDACTED, REDACTED]) : change + end + diff + end + + # The record's attributes, without redacted fields + protected def history_snapshot : JSON::Any + redacted = self.class.history_redacted + JSON::Any.new(JSON.parse(attributes_json).as_h.reject! { |field, _| redacted.includes?(field) }) + end + + private def __record_history__(action : String, diff = {} of String => JSON::Any) : Nil + ::PlaceOS::Model::History.record_change( + resource_type: self.class.table_name, + resource_id: history_resource_id, + correlation_id: history_correlation_id, + action: action, + changed_fields: history_changed_fields(diff), + diff: diff, + snapshot: history_snapshot?(action) ? history_snapshot : nil, + metadata: history_metadata, + ) + end + end +end diff --git a/src/placeos-models/ldap_authentication.cr b/src/placeos-models/ldap_authentication.cr index 46ddd497..ec4c7c71 100644 --- a/src/placeos-models/ldap_authentication.cr +++ b/src/placeos-models/ldap_authentication.cr @@ -1,6 +1,7 @@ require "uri" require "json" require "./base/model" +require "./history" module PlaceOS::Model # see: https://github.com/omniauth/omniauth-ldap @@ -9,6 +10,9 @@ module PlaceOS::Model table :ldap_strat + include History::Tracked + history_redact :password + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute port : Int32 = 636 diff --git a/src/placeos-models/module.cr b/src/placeos-models/module.cr index e94f7a80..754d1d47 100644 --- a/src/placeos-models/module.cr +++ b/src/placeos-models/module.cr @@ -6,6 +6,7 @@ require "./driver" require "./edge" require "./settings" require "./utilities/settings_helper" +require "./history" module PlaceOS::Model class Module < ModelBase @@ -14,6 +15,9 @@ module PlaceOS::Model table :mod + include History::Tracked + history_ignore :connected, :has_runtime_error, :error_timestamp + # Runtime error reporting and save timestamps do not require a driver reload. changefeed_ignore_updates :updated_at, :has_runtime_error, :error_timestamp diff --git a/src/placeos-models/oauth_authentication.cr b/src/placeos-models/oauth_authentication.cr index efd6a9f8..9d0584f7 100644 --- a/src/placeos-models/oauth_authentication.cr +++ b/src/placeos-models/oauth_authentication.cr @@ -2,6 +2,7 @@ require "uri" require "json" require "./base/model" +require "./history" module PlaceOS::Model # See: https://github.com/omniauth/omniauth-oauth2 @@ -10,6 +11,9 @@ module PlaceOS::Model table :oauth_strat + include History::Tracked + history_redact :client_secret + attribute name : String, sanitize: :text, es_subfield: "keyword" # The client ID configured for this application diff --git a/src/placeos-models/playlist.cr b/src/placeos-models/playlist.cr index 725cee2b..d4c43541 100644 --- a/src/placeos-models/playlist.cr +++ b/src/placeos-models/playlist.cr @@ -8,6 +8,7 @@ require "./zone" # submodels (item, revision, checker) are loaded via `require "./playlist/*"` # at the bottom of this file. require "./playlist/schedule" +require "./history" module PlaceOS::Model class Playlist < ModelBase @@ -32,6 +33,9 @@ module PlaceOS::Model table :playlists + include History::Tracked + history_ignore :play_count, :play_through_count + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute description : String = "", sanitize: :common diff --git a/src/placeos-models/playlist/item.cr b/src/placeos-models/playlist/item.cr index 6452c61d..ff97370b 100644 --- a/src/placeos-models/playlist/item.cr +++ b/src/placeos-models/playlist/item.cr @@ -2,6 +2,7 @@ require "../base/model" require "../playlist" require "../upload" require "uri" +require "../history" module PlaceOS::Model class Playlist::Item < ModelBase @@ -9,6 +10,9 @@ module PlaceOS::Model table :playlist_items + include History::Tracked + history_ignore :play_count + enum MediaType Image Video diff --git a/src/placeos-models/playlist/revision.cr b/src/placeos-models/playlist/revision.cr index 0fb4ff23..299c466e 100644 --- a/src/placeos-models/playlist/revision.cr +++ b/src/placeos-models/playlist/revision.cr @@ -1,4 +1,5 @@ require "../base/model" +require "../history" require "./item" require "./item_schedule" @@ -51,6 +52,21 @@ module PlaceOS::Model self.approved_by_name = user.name self.approved_by_email = user.email self.approved = true + self.history_action = "approve" + end + + # History + ############################################################################################### + + include History::Tracked + + # only approvals are recorded, the approved revision is what's displayed + protected def history_record_as(action : String) : String? + history_action + end + + def history_correlation_id : String? + playlist_id end # finds the latest approved playlist revision for each of the playlist ids provided diff --git a/src/placeos-models/repository.cr b/src/placeos-models/repository.cr index 768c06a9..254fb7d1 100644 --- a/src/placeos-models/repository.cr +++ b/src/placeos-models/repository.cr @@ -2,6 +2,7 @@ require "time" require "./base/model" require "./utilities/encryption" +require "./history" module PlaceOS::Model # Pins engine's driver sources to a specific repository state. @@ -11,6 +12,10 @@ module PlaceOS::Model table :repo + include History::Tracked + history_ignore :deployed_commit_hash, :has_runtime_error, :error_message + history_redact :password + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute description : String = "", sanitize: :common diff --git a/src/placeos-models/saml_authentication.cr b/src/placeos-models/saml_authentication.cr index 476e6411..fab616ae 100644 --- a/src/placeos-models/saml_authentication.cr +++ b/src/placeos-models/saml_authentication.cr @@ -1,6 +1,7 @@ require "uri" require "json" require "./base/model" +require "./history" module PlaceOS::Model # See: https://github.com/omniauth/omniauth-saml @@ -9,6 +10,8 @@ module PlaceOS::Model table :adfs_strat + include History::Tracked + attribute name : String, sanitize: :text, es_subfield: "keyword" # The name of your application diff --git a/src/placeos-models/signage_template.cr b/src/placeos-models/signage_template.cr index 8d1b7944..5cb229f6 100644 --- a/src/placeos-models/signage_template.cr +++ b/src/placeos-models/signage_template.cr @@ -57,6 +57,7 @@ module PlaceOS::Model self.approved_by_name = user.name self.approved_by_email = user.email self.approved = true + self.history_action = "approve" end # approved templates should not be edited in place, so drafts are @@ -174,36 +175,11 @@ module PlaceOS::Model # History ############################################################################################### - before_save :record_template_history - - # tracks which fields changed on updates (creates and draft edits are - # not recorded, history is captured when a draft is approved) - protected def record_template_history - return unless persisted? - return if draft? - - changed_fields = [] of String - changed_fields << "name" if name_changed? - changed_fields << "description" if description_changed? - changed_fields << "tags" if tags_changed? - changed_fields << "background_item_id" if background_item_id_changed? - changed_fields << "layouts" if layouts_changed? - changed_fields << "full_screen_takeover" if full_screen_takeover_changed? - changed_fields << "merge" if merge_changed? - changed_fields << "approval_requested" if approval_requested_changed? - changed_fields << "approved" if approved_changed? - changed_fields << "live_template_id" if live_template_id_changed? - - return if changed_fields.empty? - - History.create!( - type: "template", - resource_id: self.id.to_s, - action: "updated", - changed_fields: changed_fields, - ) - rescue ex - Log.error(exception: ex) { "failed to record signage template history for #{self.id}" } + include History::Tracked + + # drafts are not recorded, history is captured when a draft is approved + protected def history_record_as(action : String) : String? + super unless draft? end end end diff --git a/src/placeos-models/trigger.cr b/src/placeos-models/trigger.cr index e5dab057..509f830c 100644 --- a/src/placeos-models/trigger.cr +++ b/src/placeos-models/trigger.cr @@ -3,6 +3,7 @@ require "time" require "./base/model" require "./trigger/*" +require "./history" module PlaceOS::Model class Trigger < ModelBase @@ -10,6 +11,8 @@ module PlaceOS::Model table :trigger + include History::Tracked + attribute name : String, sanitize: :text, es_subfield: "keyword" attribute description : String = "", sanitize: :common diff --git a/src/placeos-models/trigger_instance.cr b/src/placeos-models/trigger_instance.cr index 2b48c5a1..ccabd968 100644 --- a/src/placeos-models/trigger_instance.cr +++ b/src/placeos-models/trigger_instance.cr @@ -2,6 +2,7 @@ require "random" require "time" require "./base/model" +require "./history" module PlaceOS::Model class TriggerInstance < ModelBase @@ -10,6 +11,14 @@ module PlaceOS::Model table :trig + include History::Tracked + history_ignore :trigger_count, :triggered + history_redact :webhook_secret + + def history_correlation_id : String? + control_system_id + end + attribute enabled : Bool = true attribute triggered : Bool = false attribute important : Bool = false diff --git a/src/placeos-models/user.cr b/src/placeos-models/user.cr index db2c7df8..cbcc2652 100644 --- a/src/placeos-models/user.cr +++ b/src/placeos-models/user.cr @@ -7,6 +7,7 @@ require "./api_key" require "./metadata" require "./email" require "./utilities/metadata_helper" +require "./history" module PlaceOS::Model class User < ModelBase @@ -15,6 +16,10 @@ module PlaceOS::Model table :user + include History::Tracked + history_ignore :email_digest, :login_count, :last_login, :logged_out_at, :expires_at, :expires + history_redact :password_digest, :password, :access_token, :refresh_token + # start_time: Start time of work hours. e.g. `7.5` being 7:30AM # end_time: End time of work hours. e.g. `18.5` being 6:30PM # location: Name of the location the work is being performed at diff --git a/src/placeos-models/zone.cr b/src/placeos-models/zone.cr index 5c186f92..b8adc1bc 100644 --- a/src/placeos-models/zone.cr +++ b/src/placeos-models/zone.cr @@ -6,6 +6,7 @@ require "./utilities/settings_helper" require "./utilities/metadata_helper" require "./converter/time_location" require "./playlist" +require "./history" module PlaceOS::Model class Zone < ModelBase @@ -16,6 +17,8 @@ module PlaceOS::Model table :zone + include History::Tracked + # Descriptive and signage fields do not change running driver configuration. changefeed_ignore_updates :name, :description, :display_name, :playlists, :images, :updated_at, database_columns: [:search_vector] From b5d1cd6a1314e1c74fc6a18e26e28575cebef3ed Mon Sep 17 00:00:00 2001 From: Stephen von Takach Date: Mon, 19 Oct 2026 21:39:06 +1100 Subject: [PATCH 2/2] fix(user): hold admin delete lock for the destroy transaction Co-Authored-By: Claude Opus 5.5 --- src/placeos-models/user.cr | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/placeos-models/user.cr b/src/placeos-models/user.cr index cbcc2652..fd12af64 100644 --- a/src/placeos-models/user.cr +++ b/src/placeos-models/user.cr @@ -141,21 +141,12 @@ module PlaceOS::Model before_save :build_name before_save :write_email_fields - private getter admin_destroy_lock : ::PgORM::PgAdvisoryLock do - ::PgORM::PgAdvisoryLock.new("admin_destroy_lock") - end - - # :inherit: - def destroy - return super unless self.sys_admin - # Locking to protect against concurrent deletes - admin_destroy_lock.synchronize { super } - end - # Prevent the system from entering a state with no admin protected def ensure_admin_remains return unless self.sys_admin + # Serialise admin deletes, the lock is held until the destroy transaction completes + ::PgORM::Database.with_connection &.exec("SELECT pg_advisory_xact_lock(hashtext('admin_destroy_lock'))") if User.where({sys_admin: true}).count == 1 raise Model::Error.new("At least one admin must remain") end