Feature #2375

Add a row to the Logs table whenever a row in any other [interesting] table is created or changed

Added by Tom Clegg over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
-
Start date:
04/08/2014
Due date:
% Done:

100%

Estimated time:
(Total: 9.00 h)
Story points:
1.0

Description

  • Log object is added even if "update" does not actually change the database content (e.g., "node is still alive")
  • Log object's "properties" attribute (rename it, it's currently called "info") includes:
    • uuid of user making the change (usually same as modified_by, except delete and some updates)
    • etag of the new version (nil for delete)
    • etag of the old version (nil for create)
    • all attributes of the new version (nil if deleting)
    • all attributes of the old version (nil if creating)
  • modified_by is current_user (the user whose transaction is being logged)
  • owner_uuid is system_user (this prevents users from deleting logs)

Subtasks

Task #2469: alter existing logs table to support logging of changesResolvedBrett Smith

Task #2468: write one little code section (rake middleware? strategically placed active record call? Cf. acts-as-versioned gem) to log changesResolvedBrett Smith

Task #2559: Review 2375-log-tableResolvedBrett Smith

History

#1 Updated by Tom Clegg over 6 years ago

  • Description updated (diff)

#2 Updated by Brett Smith over 6 years ago

  • Assigned To set to Brett Smith

#3 Updated by Brett Smith over 6 years ago

  • Description updated (diff)

Updated the story to remove redundant information from the properties hash, after discussing it with Tom in IRC.

#4 Updated by Tom Clegg over 6 years ago

Notes
  • In ArvadosModel#make_log_around, log.save should probably be log.save! so failure to log causes transaction rollback/fail, rather than silently missing logs.
  • I don't see how we avoid (or test that we avoid) logging the creation of a (user-initiated) Log object
  • We might want to skip logging on ApiClientAuthorization updates that only change last_used_at, because every single API call does that to the authorizing token. (Is there a reasonably easy way to skip that particular case?)
  • New Log semantics (can't update or delete your own voluntarily-added logs) are a surprise, but arguably a good new feature. (Existing permission system would be enough to protect the automatically generated logs.)
    • is_admin==true should still be able to update and delete logs -- at least user-generated ones, if not all. Generally our approach is "if it doesn't break database consistency, is_admin lets you do it".
    • if permission_to_update/delete is false, setting owner_uuid=system_user_uuid has no effect other than making the log unreadable by the user who created it (which we don't want, do we?)
    • Special "can't update/delete" semantics should be mentioned at http://doc.arvados.org/api/methods/logs.html and possibly http://doc.arvados.org/api/permission-model.html

#5 Updated by Brett Smith over 6 years ago

  • In ArvadosModel#make_log_around, log.save should probably be log.save! so failure to log causes transaction rollback/fail, rather than silently missing logs.

I'm not sure which is stylistically better, but according to the docs I believe the current implementation will cause a rollback. Rails' callback documentation says that any callback can force a rollback by returning false, which log.save should do if it fails.

  • I don't see how we avoid (or test that we avoid) logging the creation of a (user-initiated) Log object

I'll add a test, but if I'm following you right, I think the self.is_a? Log conditional in make_log_around covers this.

  • New Log semantics (can't update or delete your own voluntarily-added logs) are a surprise, but arguably a good new feature. (Existing permission system would be enough to protect the automatically generated logs.)

Sorry for the surprise. I went there because I had to overwrite permission_to_create, and then figured overriding the others made sense for consistency's sake, and I didn't fully understand how that hooked into permission to view. If you're okay with this general approach, I'm happy to make the other changes you suggested.

#6 Updated by Tom Clegg over 6 years ago

Brett Smith wrote:

  • In ArvadosModel#make_log_around, log.save should probably be log.save! so failure to log causes transaction rollback/fail, rather than silently missing logs.

I'm not sure which is stylistically better, but according to the docs I believe the current implementation will cause a rollback. Rails' callback documentation says that any callback can force a rollback by returning false, which log.save should do if it fails.

I see this:

The whole callback chain is wrapped in a transaction. If any before callback method returns exactly false or raises an exception, the execution chain gets halted and a ROLLBACK is issued; after callbacks can only accomplish that by raising an exception.

But this seems to say "before callbacks" do what you want, "after callbacks" don't do what you want, and "around callbacks" ... uh, might do what you want? (One of us is missing something...)

OTOH, raising an exception is almost certain (ha) to do what you want.

Perhaps the best answer involves a test case, although I can't think offhand of a convenient way for a test to make log.save fail.

  • I don't see how we avoid (or test that we avoid) logging the creation of a (user-initiated) Log object

I'll add a test, but if I'm following you right, I think the self.is_a? Log conditional in make_log_around covers this.

Ah, indeed, I missed that. I was too busy looking for a skip_around_filter in the Log class. That works.

  • New Log semantics (can't update or delete your own voluntarily-added logs) are a surprise, but arguably a good new feature. (Existing permission system would be enough to protect the automatically generated logs.)

Sorry for the surprise. I went there because I had to overwrite permission_to_create, and then figured overriding the others made sense for consistency's sake, and I didn't fully understand how that hooked into permission to view. If you're okay with this general approach, I'm happy to make the other changes you suggested.

Yes, I think it does make sense. Normally a logging facility is not expected to provide "update log" and "delete log" operations, right?

#7 Updated by Brett Smith over 6 years ago

You're right about callbacks.

Perhaps the best answer involves a test case, although I can't think offhand of a convenient way for a test to make log.save fail.

Oh, that's easy, we can just redefine the right Log method for the test so it always fails. :)

I pushed a new version that I believe incorporates all your feedback. Please take a look and let me know what you think. Thanks again.

#8 Updated by Tom Clegg over 6 years ago

All looks good to me, thanks!

#9 Updated by Tom Clegg over 6 years ago

Oops, I just thought of one more suggestion: Don't log the api_token attribute when logging transactions on ApiClientAuthorizations -- or do something else to prevent "non-trusted" clients from trivially retrieving all of the user's tokens. (Currently if you look at workbench/logs after logging out+in, you can see secret tokens in log properties. Keeping these out of logs entirely is probably a reasonable thing to do, in any case...?)

#10 Updated by Brett Smith over 6 years ago

  • Status changed from New to Resolved
  • % Done changed from 93 to 100

Applied in changeset arvados|commit:94c762ed797f2567c1dcc70d12582c7d640da7bb.

Also available in: Atom PDF