Activitiy Log fails to load when certain values are null due to insufficient error handling in lib/pkp/classes/services/PKPSubmissionFileService.inc.php

[eirikhanssen]

Application: OJS 3.3.0-8

Bug description
These are similar problems as reported in: #7453
When clicking Activity Log in the submission workflow, it doesn't finish loading when certain objects/values are unexpectedly null in lib/pkp/classes/services/PKPSubmissionFileService.inc.php

My suggestion is that all of the cases in this file should be reviewed and missing values should be handled better or skipped.

Here are two different causes to activity log not loading that I have worked around on two different occations.

problem with: case SUBMISSION_FILE_INTERNAL_REVIEW_REVISION

Log:
[17-Nov-2021 14:27:24 Europe/Oslo] PHP Notice: Undefined index: stageId in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/controllers/grid/eventLog/SubmissionEventLogGridHandler.inc.php on line 111
[17-Nov-2021 14:27:24 Europe/Oslo] PHP Notice: Undefined variable: query in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/services/PKPSubmissionFileService.inc.php on line 784
[17-Nov-2021 14:27:24 Europe/Oslo] PHP Fatal error: Uncaught Error: Call to a member function getAssocType() on null in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/services/PKPSubmissionFileService.inc.php:784
Stack trace:
#0 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/controllers/grid/eventLog/EventLogGridRow.inc.php(79): PKP\Services\PKPSubmissionFileService->getWorkflowStageId(Object(SubmissionFile))
#1 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/controllers/grid/GridHandler.inc.php(1066): EventLogGridRow->initialize(Object(Request))
#2 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/controllers/grid/GridHandler.inc.php(983): GridHandler->_getInitializedRowInstance(Object(Request), 0, Object(SubmissionEventLogEntry))
#3 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/controllers/grid/GridHandler.inc.php(1031): GridHandler->renderRowsInternally(Object(Request), Array)
#4 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/contro in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/services/PKPSubmissionFileService.inc.php on line 784:

Solution:
added if clause after the comment: // The file should be associated with a review round. If not, fail.

case SUBMISSION_FILE_INTERNAL_REVIEW_REVISION:
    $reviewRoundDao = DAORegistry::getDAO('ReviewRoundDAO'); /* @var $reviewRoundDao ReviewRoundDAO */
     $reviewRound = $reviewRoundDao->getBySubmissionFileId($submissionFile->getId());

      // The file should be associated with a review round. If not, fail.
      if(is_null($reviewRound)) {
          return null;
       }

    // Get the associated stage.
    return $reviewRound->getStageId();

problem with: case SUBMISSION_FILE_QUERY:

Log:
[21-Dec-2021 11:03:43 Europe/Oslo] PHP Notice: Undefined index: stageId in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/controllers/grid/eventLog/SubmissionEventLogGridHandler.inc.php on line 111
[21-Dec-2021 11:03:43 Europe/Oslo] PHP Fatal error: Uncaught Error: Call to a member function getAssocType() on null in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/services/PKPSubmissionFileService.inc.php:795
Stack trace:
#0 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/controllers/grid/eventLog/EventLogGridRow.inc.php(79): PKP\Services\PKPSubmissionFileService->getWorkflowStageId(Object(SubmissionFile))
#1 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/controllers/grid/GridHandler.inc.php(1066): EventLogGridRow->initialize(Object(Request))
#2 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/controllers/grid/GridHandler.inc.php(983): GridHandler->_getInitializedRowInstance(Object(Request), 28, Object(SubmissionEventLogEntry))
#3 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/controllers/grid/GridHandler.inc.php(1031): GridHandler->renderRowsInternally(Object(Request), Array)
#4 /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/controllers/grid/GridHandler.inc.php(921) in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/classes/services/PKPSubmissionFileService.inc.php on line 795

According to log this means $query was null.

Solution:
Added more return null statements when the following were null: $query, $note, $query->getAssocType()

case SUBMISSION_FILE_QUERY:
                // This file should be associated with a note. If not, fail.
                if ($submissionFile->getData('assocType') != ASSOC_TYPE_NOTE) {
                    return null;
                }

                // Get the associated note.
				$noteDao = DAORegistry::getDAO('NoteDAO'); /* @var $noteDao NoteDAO */
				$note = $noteDao->getById($submissionFile->getData('assocId'));
				
                                // if note is null, fail
				if(!$note) {
					return null;
				}

                // Get the associated query.
				$queryDao = DAORegistry::getDAO('QueryDAO'); /* @var $queryDao QueryDAO */
				$query = $queryDao->getById($note->getAssocId());

				// if query is null, fail
				if ($query == null) {
					return null;
				}
                // The note should be associated with a query. If not, fail.
                if ($query->getAssocType() != ASSOC_TYPE_QUERY) {
                    return null;
                }

                // The query will have an associated file stage.
				return $query ? $query->getStageId() : null;

Now loading the same activity log will load, and only produces the following warning:
[21-Dec-2021 11:11:45 Europe/Oslo] PHP Notice: Undefined index: stageId in /var/www/vhosts/ojs-3.3.0-8/lib/pkp/controllers/grid/eventLog/SubmissionEventLogGridHandler.inc.php on line 11

So stageId is still undefined at some point, but it doesn't prevent the activity log from loading anymore.

Best,
Eirik Hanssen
University Library
Oslo Metropolitan University

[asmecher]

@eirikhanssen, thanks for documenting this so well! Could you open a pull request with your proposed changes? (It'll be easiest to open it against the stable-3_3_0 branch; I can take care of forward-porting it from there.)

[asmecher]

(Assigning to @withanage to pick up the changes proposed above!)

[Vitaliy-1]

The solution posted above leads to the issue with file uploading during discussion. There is a good description on the forum: https://forum.pkp.sfu.ca/t/ojs-journal-is-having-an-issue-with-uploading-documents/73718

In short, it causes PKP\security\authorization\internal\SubmissionFileMatchesWorkflowStageIdPolicy to incorrectly determine workflow stage ID, and authorization in FileUploadWizardHandler fails here:

pkp-lib/controllers/wizard/fileUpload/FileUploadWizardHandler.inc.php

Line 114 in 1f64bda

$this->addPolicy(new SubmissionFileAccessPolicy($request, $args, $roleAssignments, SubmissionFileAccessPolicy::SUBMISSION_FILE_ACCESS_MODIFY, $submissionFileIdToValidate));

This solution is already applied to the main branch, see PKP\submissionFile\Repository::getWorkflowStageId(), thus needs to be addressed there also.

[Vitaliy-1]

This initial issue is due to the Discussion being deleted and the log is unable to retrieve it. Maybe it makes sense not to remove Query from the database but rather archive it?

[withanage]

Hi @NateWr ,

Could you please have a look at this PR ?
ojs

• pkp/pkp-lib#7563 Activitiy Log fails to load when certain values are null due to insufficient error handling in lib/pkp/classes/services/PKPSubmissionFileService.inc.php #8208

pkp-lib

• pkp/pkp-lib#7563 Activitiy Log fails to load when certain values are null due to insufficient error handling in lib/pkp/classes/services/PKPSubmissionFileService.inc.php ojs#3512

[NateWr]

Thanks @withanage. I think that all of these cases arise from scenarios where an object has been deleted (review round, query, etc) but the submission file has not been deleted. We should probably clean this up instead of building work-arounds for it in the code.

@asmecher what do you think? See https://github.com/pkp/pkp-lib/pull/8208/files

[asmecher]

Agreed, @NateWr. This is actually a possible duplicate of #8073; if it's not, that issue should provide a good example of how we're correcting data issues lately.

This is probably an assoc_type / assoc_id issue, so we can't add a foreign key relationship, but in cases where that's possible we're adding them as part of #6093; this will help prevent data problems from occurring in the first place. As we do that, we can start to remove some of the conditional code peppered through the system that's intended to work around bad data. Everthing runs tighter that way 👍

[asmecher]

@withanage, are you still working on this one? Should it be deferred to 3.3.0-13?

[withanage]

@asmecher I could not come to this until now and wanted to look at it this weekend! May be it is better to defer this, if I do not get this finished, cause this area is little new to me.

[asmecher]

Sure, let's defer it.

[withanage]

Hi @asmecher or @defstat .
I have looked into this issue and it seems to me that this is handled in the following file by @defstat s PR by deleting orphaned files.

84ee403#diff-6bd4382888007c21acb6bf540ce1eb2ba123dcc507ab2fb20edc3f97e36181c8

As notes is referencing an assoc_id and assoc_type a cascase delete has to handle all the possible cases.

@defsta can you please have a look or r do I miss something ?

[asmecher]

I agree, I think this is a duplicate of #8073.