Add location addition, removal, and change of main location to the audit log #37

thiagocamposviana · 2017-01-25T17:18:57Z

Content move is covered by the audit log, but these actions don't appear to be:
Add a location
Remove a location (but not remove the object)
Change the main location

Knowing who performed these actions could help troubleshoot issues

peterkeung · 2017-01-25T18:59:51Z

kernel/classes/ezcontentobjecttreenode.php

@@ -2906,6 +2906,11 @@ static function updateMainNodeID( $mainNodeID, $objectID, $version = false, $par
        }

        $db->commit();
+        eZAudit::writeAudit( 'main-node-update', array( 'Parent Node ID' => $parentMainNodeID,
+                                              'Main Node ID' => $mainNodeID,


Should we be providing information about the old node ID?

I am going to update the code and log the old main node id and also the old parent node id

peterkeung · 2017-01-25T19:00:27Z

kernel/content/ezcontentoperationcollection.php

@@ -809,7 +809,11 @@ static public function addAssignment( $nodeID, $objectID, $selectedNodeIDArray )
        }
        if ( $locationAdded )
        {
-
+            eZAudit::writeAudit( 'location-assign', array( 'Selected Node ID Array' => implode( ', ' , $selectedNodeIDArray ),
+                                              'Node ID' => $nodeID,


What node ID is this? Can we maybe provide info on the main node ID and then info about what locations were added?

peterkeung · 2017-01-25T19:00:48Z

kernel/content/ezcontentoperationcollection.php

+                                              'Node ID' => $nodeID,
+                                              'Content object ID' => $object->attribute( 'id' ),
+                                              'Content object name' => $object->attribute( 'name' ),
+                                              'Comment' => 'Assigned new locations to the current node: eZContentOperationCollection::addAssignment()' ) );


What about locations that were removed?

I think this operation is only for new locations

peterkeung · 2017-01-25T19:01:25Z

kernel/content/ezcontentoperationcollection.php

@@ -866,6 +870,11 @@ static public function removeNodes( array $removeNodeIdList )

            if ( !isset( $objectIdList[$objectId] ) )
                $objectIdList[$objectId] = eZContentObject::fetch( $objectId );
+            eZAudit::writeAudit( 'location-remove', array( 'Parent Node ID' => $node->attribute( 'parent_node_id' ),
+                                              'Node ID' => $nodeId,


Can we be clear about which node ID was removed, and be clear on what is the (new) main node ID?

Just a note here, this function is a mess and I decided to log each node individually, but yes, we can provide the information about the main node, currently the Node ID refers to the removed node.

I am working on patch to show the new main node id in this log but it also good to not that such info is always added to the main_node_update.log regardless.

peterkeung · 2017-01-25T19:01:45Z

settings/audit.ini

+# Who removed a location of a node
+AuditFileNames[location-remove]=location_remove.log
+
+# Who update the main node


"update" should be "updated"

thiagocamposviana · 2017-01-26T16:25:32Z

I have updated the code according to Peter's feedback.

pkamps · 2017-01-31T11:11:34Z

Hi Thiago,

I now had some time to review the pull request:
The function 'updateMainNodeID' now has a extra SQL statement
$oldMainNodeArray = $db->arrayQuery(. That's required in order to get all info we want to have in the audit log.
That extra SQL statement gets executed even if you don't have audit log enabled. That slows down the updateMainNodeID operation - it is also making that function a little more complex that it needs to be.
I suggest that we move the $oldMainNodeArray = $db->arrayQuery( code into the ezaudit class, function 'writeAudit':

if ( !isset( $auditNameSettings[$auditName] ) )
            return false;

// new code
        switch( $auditName )
        {
            case 'main-node-update':
            {
                // add more stuff like old main node id into $auditAttributes
            }
            break;
            
            default:
        }

That way we end up with a single line in the 'updateMainNodeID' function:

      eZAudit::writeAudit( 'main-node-update', array( 'Content object ID' => $objectID,
                                                         'New Main Node ID' => $mainNodeID,
                                                         'New Parent Node ID' => $parentMainNodeID,
                                                         'Content object name' => $contentObject->attribute( 'name' ),
                                                         'Comment' => 'Updated the main location of the current object: eZContentObjectTreeNode::updateMainNodeID()' ) );

That way, we get now performance penalty if the audit log is not enabled.

thiagocamposviana · 2017-02-03T18:16:59Z

I moved the update node log sql operation to eZAudit::writeAudit but keep in mind I needed to call the function before the main node is updated so we can have access to the old parent/main nodes info.

pkamps · 2017-02-04T11:42:12Z

Hi Thiago,

I hope you don't mind that we do a few iterations on this. I know the process is a bit painful but I'm sure we'll have a great result:

Currently your pull request is calling eZAudit::writeAudit in kernel/content/ezcontentoperationcollection.php (2 times) and in kernel/classes/ezcontentobjecttreenode.php (1 time). I think we should only use kernel/content/ezcontentoperationcollection.php to call eZAudit::writeAudit. So basically I suggest to have this code in

        // audit entry needs to happen before we actually update the main node assignment
        // because we want to log the old main_node_id
        eZAudit::writeAudit( 'main-node-update', array( 'Content object ID' => $ObjectID,
                                                        'New Main Node ID' => $mainAssignmentID,
                                                        'New Parent Node ID' => $mainAssignmentParentID,
                                                        'Comment' => 'Updated the main location of the current object: eZContentOperationCollection::updateMainAssi
        );

directly after

     static public function updateMainAssignment( $mainAssignmentID, $ObjectID, $mainAssignmentParentID )
     {

Line 1262 in kernel/content/ezcontentoperationcollection.php

With that code we don't have to touch kernel/classes/ezcontentobjecttreenode.php at all.

pkamps · 2017-02-04T11:44:27Z

Another thing in eZAudit::writeAudit

I don't think we need a custom SQL statement - writeAudit gets called before we updated the main node id. So we can get the old main node id from the object very easily:

        switch( $auditName )
        {
            case 'main-node-update':
            {
				$contentObject = eZContentObject::fetch( $auditAttributes[ 'Content object ID' ] );

                $auditAttributes[ 'Old Main Node ID' ] = $contentObject->attribute( 'main_node_id' );
                $auditAttributes[ 'Old Main Parent Node ID' ] = $contentObject->attribute( 'main_parent_node_id' );
				$auditAttributes[ 'Content object name' ] = $contentObject->attribute( 'name' );
            }
            break;

            default:
        }

pkamps · 2017-02-04T12:04:02Z

In case of eZContentOperationCollection::removeNodes I think we should fire of 2 different types of audit entries:

A node got removed "location-remove"
The main node id got updated "main-node-update"

So I suggest this code for the foreach loop:

        foreach ( $removeNodeIdList as $nodeId )
        {
            $node     = eZContentObjectTreeNode::fetch($nodeId);
            $objectId = $node->attribute( 'contentobject_id' );
            foreach ( eZNodeAssignment::fetchForObject( $objectId,
                                                        eZContentObject::fetch( $objectId )->attribute( 'current_version' ),
                                                        0, false ) as $nodeAssignmentKey => $nodeAssignment )
            {
                if ( $nodeAssignment['parent_node'] == $node->attribute( 'parent_node_id' ) )
                {
                    $nodeAssignmentIdList[$nodeAssignment['id']] = 1;
                }
            }

            $isMainNode = $nodeId == $node->attribute( 'main_node_id' );

            eZAudit::writeAudit( 'location-remove', array( 'Removed Node ID' => $nodeId,
                                                           'Parent Node ID' => $node->attribute( 'parent_node_id' ),
                                                           'Content object ID' => $objectId,
                                                           'Content object name' => $node->attribute( 'name' ),
                                                           'Was main node' => $isMainNode ? 'Yes' : 'No'
            ) );

            if ( $isMainNode )
                $mainNodeChanged[$objectId] = 1;
            $node->removeThis();

            if ( !isset( $objectIdList[$objectId] ) )
                $objectIdList[$objectId] = eZContentObject::fetch( $objectId );
        }

It is just reporting if a deleted node used to be a main node. I think that's good enough.

Then this code for the foreach which is setting a new (random) main node:

        foreach ( array_keys( $mainNodeChanged ) as $objectId )
        {
            $allNodes = $objectIdList[$objectId]->assignedNodes();
            // Registering node that will be promoted as 'main'
            if ( isset( $allNodes[0] ) )
            {
                $mainNodeChanged[$objectId] = $allNodes[0];

                // audit entry needs to happen before we actually update the main node assignment
                // because we want to log the old main_node_id
                eZAudit::writeAudit( 'main-node-update', array( 'Content object ID' => $objectId,
                                                                'New Main Node ID' => $allNodes[0]->attribute( 'node_id' ),
                                                                'New Parent Node ID' => $allNodes[0]->attribute( 'parent_node_id' ),
                                                                'Comment' => 'Updated the main location of the current object: eZContentOperationCollection::removeNodes()' )
                );

                eZContentObjectTreeNode::updateMainNodeID( $allNodes[0]->attribute( 'node_id' ), $objectId, false, $allNodes[0]->attribute( 'parent_node_id' ) );
            }
        }

I don't think it will report the old main node id because that node already got deleted but we have that information in the 'location-remove' log file.

pkamps · 2017-02-04T12:08:21Z

Side note: the admin UI does not allow you to remove the main node - the checkbox is disabled. But you can edit the page HTML (remove the disabled attribute) and then submit the form.

thiagocamposviana · 2017-02-04T21:41:23Z

"I don't think we need a custom SQL statement - writeAudit gets called before we updated the main node id. So we can get the old main node id from the object very easily:"

If I remeber that correctly when you fetch an object it will be cached, and next time the fetch function is called eZ will get the cached version that is why I wrote the sql query (to avoid any possible cache issues)

pkamps · 2017-02-06T11:31:25Z

I think that's true, there is a chance that you access an ezobject with outdated data. In case of this pull request I don't think it matters: the code is writing the audit log entry before the database gets updated. You don't have the problem of accessing outdated data.

thiagocamposviana · 2017-02-21T16:44:36Z

I think this should be reviewed a bit further, there is a request to remove the main node update log from ezcontentobjecttreenode, because it seems to be better to put it in the eZContentOperationCollection file, but then we need to replicate the code in two places ( updateMainAssignment and removeNodes ), which brings the question what about custom code that removes objects and update main node without using the eZContentOperationCollection? I think we should think about where to place it in order to maximize the likelihood of the code being called.

And thinking about the cache issue again and custom codes, I think fetching an object just about when it is going to be updated without clearing the cache after can possibly break several existing custom codes, and as long the performance here might be one issue I am still favoring the sql query.

pkamps · 2017-02-23T08:49:56Z

I understand that we replicate the code if we don't have it in ezcontentobjecttreenode and I agree it's not very nice.
We can force the code to be called if we move it into ezcontentobjecttreenode. It then gets executed every time even if we develop custom code and not use the eZContentOperationCollection. But it takes away flexibility: maybe we don't want to have audit entries in some use cases: think about a script updating some main node ids, it would spam the audit log.
Also, if your custom code is not using eZContentOperationCollection, it is missing out on some other important steps like clearing the cache. In other words, if your code is directly calling ezcontentobjecttreenode to update main nodes, it is expected that it only updates the DB and not executes a full operation.

For the custom SQL statement: I don't see a problem fetching an object BEFORE it gets updated. I don't see why we cannot rely on the results in that use case.
I do understand that there is a problem if you update an object and then rely on it (afterwards) : there are issues with cached values and also problems in a DB master/slave setup.

thiagocamposviana · 2017-02-23T16:58:28Z

Some notes:

'Old Main Node ID' => '',
'Old Main Parent Node ID' => '',
'Content object name' => '',

Are important to make it easier to specify the correct information position in the output.

I am keeping the SQL query due to the issues that can happen afterwards

thiagocamposviana · 2017-02-23T17:10:08Z

Actually by moving the main node update function from ezcontentobjecttreenode to ezcontentoperationcollection the SQL fails when removing the main node, before it would work just fine, I am now going to check what happens when fetching the object (but in this case it might not be bc as long we will ignore what happens afterwards)

thiagocamposviana · 2017-02-23T17:14:00Z

The change about moving the update node log from ezcontentobjecttreenode to ezcontentoperationcollection also doesn't log the parent node id and main node id even if fetching the object instead of using the sql. I would say the current pull request has the more stable code and is also bc (won't break what happens afterwards).

thiagocamposviana · 2017-02-23T17:15:44Z

The test I made.

Add a secondary location to a node.
Change the main node to that secondary location you just added.
Go to that location you added, which should be now the main location.
Remove this location (clicking the remove button).
Check the logs.

Before the proposed changes it should log just fine the info, after the changes it won't log the old parent node id and old main node id.

thiagocamposviana · 2017-02-23T20:24:37Z

Actually current code also fails in that test case.

thiagocamposviana · 2017-02-23T20:38:20Z

My conclusion here is that we should log the info that we have on hand without needing to execute extra queries or content fetches no matter what.

The issues started happening when we tried to log the old parent node id and old main node, those information is still available in the logs in one way or another, we can back trap main node changes and even object creation or removal.

So my proposal is to not try to log extra info if we need to execute fetches and queries, that means for the main node update we should only log Content object ID. New Main Node ID, New Parent Node ID, and forget even about object name if that requires a fetch or sql query

dougplant · 2017-02-23T20:55:54Z

That makes a lot sense.

…

On Feb 23, 2017 12:38 PM, "Thiago Campos Viana" ***@***.***> wrote: My conclusion here is that we should log the info that we have on hand without needing to execute extra queries or content fetches no matter what. The issues started happening when we tried to log the old parent node id and old main node, those information is still available in the logs in one way or another, we can back trap main node changes and even object creation or removal. So my proposal is to not try to log extra info if we need to execute fetches and queries, that means for the main node update we should only log Content object ID. New Main Node ID, New Parent Node ID, and forget even about object name if that requires a fetch or sql query — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKZv-rzpxNokEIagztFL46rgPUSHmGPks5rfe49gaJpZM4Ltwgq> .

…dit log

thiagocamposviana · 2017-02-23T21:17:11Z

I created a new pull request which doesn't execute the fetch, it is simpler but logs all the information that are really useful, I have also changed so we call the update main node audit on eZContentOperationCollection instead of eZContentObjectTreeNode

pkamps · 2017-03-09T11:07:05Z

I tested it. Looks good to me. +1

ernestob · 2017-04-11T23:11:29Z

I have run some tests on this.
I can see the logs being generated.

On my test I have added a new location to an object --it got logged.
I changed the main location --It got logged.
I can't remove a main location.
I removed a location (not main) --it got logged.
This also logs an entry in the var/ezdemo_site/log/audit/content_delete.log

[ Apr 12 2017 01:04:38 ] [172.28.128.1] [admin:14]
Node ID: 65
Object ID: 60
Content Name: New article testing
Comment: Removed the current node: eZContentObjectTreeNode::removeNode()

Is this the correct behavior?

pkamps · 2017-04-12T11:39:10Z

Test results look correct.

peterkeung reviewed Jan 25, 2017

View reviewed changes

thiagocamposviana force-pushed the 201701_audit_log branch from 53e025b to ea5e9cd Compare January 26, 2017 16:23

thiagocamposviana force-pushed the 201701_audit_log branch from ea5e9cd to cdecc01 Compare January 27, 2017 22:10

thiagocamposviana force-pushed the 201701_audit_log branch from cdecc01 to 4560d33 Compare February 3, 2017 18:14

Add location addition, removal, and change of main location to the au…

1c3c5e1

…dit log

thiagocamposviana force-pushed the 201701_audit_log branch from 4560d33 to 1c3c5e1 Compare February 23, 2017 21:15

pkamps assigned ernestob Apr 11, 2017

ernestob assigned pkamps Apr 11, 2017

pkamps merged commit 9c93119 into master Apr 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add location addition, removal, and change of main location to the audit log #37

Add location addition, removal, and change of main location to the audit log #37

thiagocamposviana commented Jan 25, 2017

peterkeung Jan 25, 2017

thiagocamposviana Jan 26, 2017

peterkeung Jan 25, 2017

peterkeung Jan 25, 2017

thiagocamposviana Jan 25, 2017

peterkeung Jan 25, 2017

thiagocamposviana Jan 25, 2017

thiagocamposviana Jan 26, 2017

peterkeung Jan 25, 2017

thiagocamposviana commented Jan 26, 2017

pkamps commented Jan 31, 2017 •

edited

Loading

thiagocamposviana commented Feb 3, 2017

pkamps commented Feb 4, 2017 •

edited

Loading

pkamps commented Feb 4, 2017

pkamps commented Feb 4, 2017

pkamps commented Feb 4, 2017

thiagocamposviana commented Feb 4, 2017 •

edited

Loading

pkamps commented Feb 6, 2017

thiagocamposviana commented Feb 21, 2017 •

edited

Loading

pkamps commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017 •

edited

Loading

thiagocamposviana commented Feb 23, 2017 •

edited

Loading

thiagocamposviana commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017

dougplant commented Feb 23, 2017 via email

thiagocamposviana commented Feb 23, 2017 •

edited

Loading

pkamps commented Mar 9, 2017

ernestob commented Apr 11, 2017

pkamps commented Apr 12, 2017

Add location addition, removal, and change of main location to the audit log #37

Add location addition, removal, and change of main location to the audit log #37

Conversation

thiagocamposviana commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagocamposviana commented Jan 26, 2017

pkamps commented Jan 31, 2017 • edited Loading

thiagocamposviana commented Feb 3, 2017

pkamps commented Feb 4, 2017 • edited Loading

pkamps commented Feb 4, 2017

pkamps commented Feb 4, 2017

pkamps commented Feb 4, 2017

thiagocamposviana commented Feb 4, 2017 • edited Loading

pkamps commented Feb 6, 2017

thiagocamposviana commented Feb 21, 2017 • edited Loading

pkamps commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017 • edited Loading

thiagocamposviana commented Feb 23, 2017 • edited Loading

thiagocamposviana commented Feb 23, 2017

thiagocamposviana commented Feb 23, 2017

dougplant commented Feb 23, 2017 via email

thiagocamposviana commented Feb 23, 2017 • edited Loading

pkamps commented Mar 9, 2017

ernestob commented Apr 11, 2017

pkamps commented Apr 12, 2017

pkamps commented Jan 31, 2017 •

edited

Loading

pkamps commented Feb 4, 2017 •

edited

Loading

thiagocamposviana commented Feb 4, 2017 •

edited

Loading

thiagocamposviana commented Feb 21, 2017 •

edited

Loading

thiagocamposviana commented Feb 23, 2017 •

edited

Loading

thiagocamposviana commented Feb 23, 2017 •

edited

Loading

thiagocamposviana commented Feb 23, 2017 •

edited

Loading