Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#1784 closed enhancement (fixed)

Add support for logging old values

Reported by: Nicklas Nordborg Owned by: Nicklas Nordborg
Priority: major Milestone: BASE 3.3
Component: core Version:
Keywords: Cc:

Description

The current logging implementation has support for detecting changes to individual properties, but it can only list which properties that was changed, not the store the old value.

If logging at this level should be enabled or not should be controlled by a flag in base.config. There is already a section with logging settings so this should not be a problem.

It should also be possible to display the old values in the gui.

To make things simple all values should be converted to strings before logging using their JRE .toString() methods.

Change History (12)

comment:1 by Nicklas Nordborg, 11 years ago

Owner: changed from everyone to Nicklas Nordborg
Status: newassigned

comment:2 by Nicklas Nordborg, 11 years ago

There is at least one security-related issue that must be considered before implementing this. Access to annotations is controlled by the permissions on the annotation type. This means that a user that can see information about, for example, a sample may not have access to all annotations on that sample.

If an administrator changes the value of a restricted annotation, the old value is suddenly visible in the change history of that item. In a default BASE installation the change history is only visible for administrators so this may not actually be an issue. But it is easy to give other users access to the change log, and this also give those users access to (old) annotation values that they may not have access to otherwise.

So, we need to think about if the current permission control is enough (a global on/off depending on user role) or if we need more fine-grained access control to the change history log.

comment:3 by Nicklas Nordborg, 11 years ago

Here are a few ideas that can be implemented independently of each other:

  1. A new role-based permission that control access to the "old values log" in the same way that the current CHANGE_HISTORY permission control access to the existing change history. This should be fairly easy to implement but since it is still a global permission the administrator has to be careful when granting it if the log can contain sensitive data.
  1. A flag on AnnotationType that can disable the logging for annotations that contain sensitive date. It will still be visible that a change has been made but the actual values are not logged. This is a secure solution since the sensitive data is not logged in the first place, but not even administrators will know what the old data was.

2b. A variant is to add the flag to other item types as well to be able to control logging for each individual item. Instead of a simple on/off flag a numeric log-level can be used to control the amount of detail to log. However there is an issue with this that a user that want to "hide" changes may first change the log level to "off", make the change and then restore the log level. Even if there is a log entry telling that the log level has been modified it may not satisfy an administrator. If this variant is implemented it must probably be paired with a new access permission (eg. SET_LOG_LEVEL) for modifying the log level.

  1. Implement access control on for log entries that is compatible with the current access control for Shareable items. This requires syncing of permissions with a "sync parent" item so that the access permissions to the log entries is always the same as the access permissions to the "sync parent" item. The "sync parent" item may different from the item the log entry belongs to. For example, a log entry for a modified sample annotation has the annotation type as the "sync parent" item (not the sample).

The third option is the only option that doesn't impose any restrictions on the setup of logging and/or permissions for users. But this is also the most complicated to implement and performance considerations are required to make sure that the system is able to handle it.

Also note that the three options cover slightly different use cases and there is nothing that prevents all of them from being implemented for optimal(?) flexibility.

comment:4 by Nicklas Nordborg, 11 years ago

After a short meeting it was decided that 1 and 2 (not 2b) should be good enough and cover the use cases we need to cover at the moment.

comment:5 by Nicklas Nordborg, 11 years ago

(In [6351]) References #1784: Add support for logging old values

The core functionality has been implemented. Added two new columns in the ChangeHistorDetails table (old_value and new_value). The DefaultEntityLogger is used for testing and it works ok for some values (string and numbers), dates are ok but need better formatting than the default provided by Java. Need to think about how to format changes to entities that simply use the output from BasicData.toString() (eg. old value=ItemSubtypeData[id=64]) which is not very informative.

The update script will also modify existing log entries so that they as much as possible use the same format as the updated loggers. All 'Annotation updated: <name>' enties are replaced with 'Annotation[<name>]'. File changes get similar updates. Entries that list changes to multiple properties are cloned and modified to only display a single property each.

comment:6 by Nicklas Nordborg, 11 years ago

(In [6353]) References #1784: Add support for logging old values

Formatting of logged values has been improved. Dates and some entity classes use specific formatting for their type. Components are inspected closer and each sub-property is logged independently.

comment:7 by Nicklas Nordborg, 11 years ago

(In [6354]) References #1784: Add support for logging old values

The annotation logger and file set logger are now able to log old/new values.

comment:8 by Nicklas Nordborg, 11 years ago

(In [6355]) References #1784: Add support for logging old values

Added permission check for reading old and new values from the change history. The permission is handled by the new Item.CHANGEHISTORY_VALUES and can be assigned to roles. The permissions is only given to administrators by default.

Special care is needed when using the Metadata class to load property values via reflection since that bypasses the regular permission checking. The Metadata class is for example used by the table exporter plug-in and could potentially by exploited by a user to get access to the logged values without having the correct permission.

To solve this problem the PropertyPathProtected annotation was created. If this annotation is present on a getter method, the property will not be loadable by the Metadata class. The table exporter will have to use the specific ChangeHistoryDetailLoader instead which uses the regular (permission-controlled) methods.

comment:9 by Nicklas Nordborg, 11 years ago

Resolution: fixed
Status: assignedclosed

comment:10 by Nicklas Nordborg, 11 years ago

(In [6420]) References #1784: Add support for logging old values

Logging of old values was enabled by an incorrect setting.

Fixed a LazyInitializationException when logging a DELETE event for an annotation. The getValues() collection was never initialized and the old values could not be logged.

comment:11 by Nicklas Nordborg, 11 years ago

(In [6430]) References #1784: Add support for logging old values

Extending the test case with updating some properties of a sample and making sure that log entries are created.

comment:12 by Nicklas Nordborg, 11 years ago

(In [6483]) References #1784: Add support for logging old values

Improved update script so that it uses less memory. Changed to StatelessSession and Iterator instead of Session and List.

Note: See TracTickets for help on using tickets.