paper_trail: t.integer :item_id does not play well with, nor complains about id: :uuid

Silent data loss occurs for versions generated against models that use id: :uuid if the backing item_id column is not also an :uuid. Versions are created and retrieved by a meaningless portion of the model’s primary key.

This following configuration:

create_table :users, id: :uuid { |t| …
create_table :versions, { |t| … t.integer :item_id, :null => false

Exposes these following issues:

Versions were actually generated
Versions were generated with an incorrect item_id (obviously)
This was not caught by relevant run-time safety checks in has_paper_trail.
If you manually add another version with the correct UUID, then User#versions returns both versions.

This is caused because:

It was possible to grab the UUID-backed object ID and stuff it into an integer column
It was possible to save such an object
The misconfiguration was not caught at run-time
#versions was able to retrieve both version models

I propose hardening the project by:

Auditing has_paper_trail and relevant initializers, so preliminary checks are carried out against the schema of the source table and the target versions table. If there is any inconsistency between the primary key column type on the model and the item_id column type on the versions table, throw an exception and do not proceed.
Auditing the #versions finder, so we know why a version model with an incorrect item_id was returned, and are able to then fix other relevant issues if needed.

I am aware that this is caused by misconfiguration and is not a bug. I am also able to completely mitigate the problem by ensuring type consistency between the model’s identifier column and the item_id column in the versions table. However, I think it is highly beneficial for Paper Trail to identify and expose this kind of misconfiguration intelligently.

I am not aware of such a fix on master and volunteer to fix it, as long as this addition is considered relevant and appropriate. Please close this issue if it is out of scope or inappropriate.

Thanks so much.

Environment:

OS X 10.9.3 13D45a
Postgres 9.3.4 thru Homebrew
Rails 4.1.0
pg 0.17.1
paper_trail 3.0.1

About this issue

Original URL
State: closed
Created 10 years ago
Reactions: 3
Comments: 20 (5 by maintainers)

Most upvoted comments

@batter. Can you confirm that if the following would work reliably? It works for me in Rails 5.

If you are using Postgres, you can change the item_id to :uuid type in the migration file, so that item_id will match with the corresponding record’s id in :uuid.

def change
  create_table :versions, versions_table_options do |t|
    t.string   :item_type, item_type_options
    t.uuid     :item_id,   null: false
    t.string   :event,     null: false
    ...
  end
  add_index :versions, [:item_type, :item_id]
end

...

Also, if you want the each version record to have an id with :uuid type, you can also do the following.

def versions_table_options
  if mysql?
    { options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci", id: :uuid }
  else
    { id: :uuid }
  end
end

h8rry on Sep 23, 2016

Just to be clear, is @h8rry’s solution the “official” (suggested) way of using PT with uuid for now?

I personally think that it works, but I don’t know what would happen if not all of the tables of the models that #has_paper_trail use uuid (haven’t tested, since all of my models use uuid).

danielolivaresd on Jan 30, 2018