DamienHarper / auditor

auditor, the missing audit log library
MIT License
164 stars 53 forks source link

Include schema in auditable table names #122

Closed janklan closed 2 years ago

janklan commented 2 years ago

PostgreSQL (and some others, but not MySQL) has a concept of multiple schemas in a single database. Doctrine supports this.

This PR fixes a problem when running audit:clean tried to remove things from audited tables, but SchemaManager::getAuditableTableNames() didn't account for the schema name.

This change prepends the schema name when found. I'm not convinced this is the only place this behaviour should change, but it's a start - it fixed the problem for me, allowing ClanAuditLogCommand::computeAuditTablename() to compile the correct name.

Return value before:

array:36 [
  "App\Entity\Address" => "address"
  "App\Entity\Feed" => "feed"
  "App\Entity\FeedContent" => "feed_content"
...

Return value after:

array:36 [
  "App\Entity\Address" => "core.address"
  "App\Entity\Feed" => "core.feed"
  "App\Entity\FeedContent" => "core.feed_content"
...
janklan commented 2 years ago

As a side note, I think a broader schema support would be appropriate. Where Auditor relies on Doctrine's SchemaTool, things work fine. For example:

/srv/app $ bin/console audit:schema:update --dump-sql

 The following SQL statements will be executed:

     CREATE TABLE logs.login_history_audit (id SERIAL NOT NULL, type VARCHAR(10) NOT NULL, object_id VARCHAR(255) NOT NULL, discriminator VARCHAR(255) DEFAULT NULL, transaction_hash VARCHAR(40) DEFAULT NULL, diffs TEXT DEFAULT NULL, blame_id VARCHAR(255) DEFAULT NULL, blame_user VARCHAR(255) DEFAULT NULL, blame_user_fqdn VARCHAR(255) DEFAULT NULL, blame_user_firewall VARCHAR(100) DEFAULT NULL, ip VARCHAR(45) DEFAULT NULL, created_at TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, PRIMARY KEY(id));
     CREATE INDEX type_99a32f1d5a22fd7ff59b2d1d24efe57e_idx ON logs.login_history_audit (type);
     CREATE INDEX object_id_99a32f1d5a22fd7ff59b2d1d24efe57e_idx ON logs.login_history_audit (object_id);
     CREATE INDEX discriminator_99a32f1d5a22fd7ff59b2d1d24efe57e_idx ON logs.login_history_audit (discriminator);
     CREATE INDEX transaction_hash_99a32f1d5a22fd7ff59b2d1d24efe57e_idx ON logs.login_history_audit (transaction_hash);
     CREATE INDEX blame_id_99a32f1d5a22fd7ff59b2d1d24efe57e_idx ON logs.login_history_audit (blame_id);
     CREATE INDEX created_at_99a32f1d5a22fd7ff59b2d1d24efe57e_idx ON logs.login_history_audit (created_at);
     COMMENT ON COLUMN logs.login_history_audit.created_at IS '(DC2Type:datetime_immutable)';

@DamienHarper I'm happy to keep chipping away with changes in that regard, but I would appreciate some direction from you as the maintainer?

codecov[bot] commented 2 years ago

Codecov Report

Merging #122 (aa2ed15) into master (2d4cfa1) will decrease coverage by 0.05%. The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
- Coverage   94.87%   94.81%   -0.06%     
==========================================
  Files          40       40              
  Lines        1444     1447       +3     
==========================================
+ Hits         1370     1372       +2     
- Misses         74       75       +1     
Impacted Files Coverage Δ
...ider/Doctrine/Persistence/Schema/SchemaManager.php 97.12% <75.00%> (-0.68%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

DamienHarper commented 2 years ago

@janklan thanks for the PR, I'll review it asap

DamienHarper commented 2 years ago

@janklan Sorry for the delay, as you noted in your latest comment, I had to dig into bringing a broader support of schema attribute/annotation while keeping compatibility with vendors other than PostgreSQL. I created a dedicated PR: https://github.com/DamienHarper/auditor/pull/127 (it should fix several schema related issues including yours)

Would you mind trying it and providing feedback.

DamienHarper commented 2 years ago

@janklan I close this PR in favor of https://github.com/DamienHarper/auditor/pull/127 which brings broader schema support