From cada0df76abaf6284b4aa2580cb202ec681fb6bd Mon Sep 17 00:00:00 2001 From: stephen waite Date: Wed, 24 Apr 2024 13:45:40 -0400 Subject: [PATCH 1/2] fix: check for empty array before looking for offsite document (#7391) * fix: check for empty array before looking for offsite document * silence php warning * Changed up event logic,fixed bugs Instead of having the url splitting and parsing in the document class, I pass the file path into the PatientRetrieveOffsiteDocument event and let the module writer be responsible for parsing the url. This ensures that we handle any kind of url variety that is needed. Fixed up the event dispatcher throwing an error because it wasn't even set in the class... Fixed the setter in the event and exposed the URL property. No idea how this was even working in the module. * Bug fix prevent firing events if empty file Made it so that if the filename is empty that the event will not be fired. Changed up the URL so you can get the entire URL schema (which is what I thought was originally happening but apparently it wasn't). This means schemas such as s3:// ftp:// webdav:// etc can be handled by the class properly. Also changed up the event so you can get the entire saved document as an object if additional meta information is needed by the offsite document event handler. --------- Co-authored-by: Stephen Nielson (cherry picked from commit a7a58d3ab3fd44970b6e59909ef0ac9a7b8e8f99) --- controllers/C_Document.class.php | 80 ++++++++++--------- library/htmlspecialchars.inc.php | 2 +- .../PatientRetrieveOffsiteDocument.php | 27 +++++-- 3 files changed, 66 insertions(+), 43 deletions(-) diff --git a/controllers/C_Document.class.php b/controllers/C_Document.class.php index 0729bab2ba5..4f674e4ba0b 100644 --- a/controllers/C_Document.class.php +++ b/controllers/C_Document.class.php @@ -25,7 +25,6 @@ use OpenEMR\Services\PatientService; use OpenEMR\Events\PatientDocuments\PatientDocumentTreeViewFilterEvent; use OpenEMR\Events\PatientDocuments\PatientRetrieveOffsiteDocument; -use Symfony\Component\EventDispatcher\EventDispatcherInterface; class C_Document extends Controller { @@ -41,10 +40,6 @@ class C_Document extends Controller private $cryptoGen; private bool $skip_acl_check = false; private DocumentTemplateService $templateService; - /** - * @var EventDispatcherInterface $eventDispatcher - */ - private $eventDispatcher; public function __construct($template_mod = "general") { @@ -774,42 +769,53 @@ public function retrieve_action(string $patient_id = null, $document_id, $as_fil //strip url of protocol handler $url = preg_replace("|^(.*)://|", "", $url); - //change full path to current webroot. this is for documents that may have - //been moved from a different filesystem and the full path in the database - //is not current. this is also for documents that may of been moved to - //different patients. Note that the path_depth is used to see how far down - //the path to go. For example, originally the path_depth was always 1, which - //only allowed things like documents/1/, but now can have more structured - //directories. For example a path_depth of 2 can give documents/encounters/1/ - // etc. + // change full path to current webroot. this is for documents that may have + // been moved from a different filesystem and the full path in the database + // is not current. this is also for documents that may of been moved to + // different patients. Note that the path_depth is used to see how far down + // the path to go. For example, originally the path_depth was always 1, which + // only allowed things like documents/1/, but now can have more structured + // directories. For example a path_depth of 2 can give documents/encounters/1/ + // etc. // NOTE that $from_filename and basename($url) are the same thing $from_all = explode("/", $url); $from_filename = array_pop($from_all); - $from_pathname_array = array(); - for ($i = 0; $i < $d->get_path_depth(); $i++) { - $from_pathname_array[] = array_pop($from_all); - } - $from_pathname_array = array_reverse($from_pathname_array); - $from_pathname = implode("/", $from_pathname_array); - if ($couch_docid && $couch_revid) { - //for couchDB no URL is available in the table, hence using the foreign_id which is patientID - $temp_url = $GLOBALS['OE_SITE_DIR'] . '/documents/temp/' . $d->get_foreign_id() . '_' . $from_filename; - } else { - $temp_url = $GLOBALS['OE_SITE_DIR'] . '/documents/' . $from_pathname . '/' . $from_filename; - } + // no point in doing any of these checks if $from_filename is empty which can lead to false positives on file_exists + if (!empty($from_filename)) { + $from_pathname_array = array(); + for ($i = 0; $i < $d->get_path_depth(); $i++) { + $from_pathname_array[] = array_pop($from_all); + } + $from_pathname_array = array_reverse($from_pathname_array); + $from_pathname = implode("/", $from_pathname_array); + if ($couch_docid && $couch_revid) { + //for couchDB no URL is available in the table, hence using the foreign_id which is patientID + $temp_url = $GLOBALS['OE_SITE_DIR'] . '/documents/temp/' . $d->get_foreign_id() . '_' . $from_filename; + } else { + $temp_url = $GLOBALS['OE_SITE_DIR'] . '/documents/' . $from_pathname . '/' . $from_filename; + } - if (file_exists($temp_url)) { - $url = $temp_url; - } - //fire a remote call to see if the file is stored somewhere else - $s3Key = explode("//", $temp_url); //split the url to get the s3 key - $retrieveOffsiteDocument = new PatientRetrieveOffsiteDocument("/" . $s3Key[1]); - $this->eventDispatcher->dispatch($retrieveOffsiteDocument, PatientRetrieveOffsiteDocument::REMOTE_DOCUMENT_LOCATION); - //this is for the s3 bucket module. If the file is not found locally, it will be found remotely - if ($retrieveOffsiteDocument->getOffsiteUrl() != null) { - header('Content-Description: File Transfer'); - header("Location: " . $retrieveOffsiteDocument->getOffsiteUrl()); - exit; + if (file_exists($temp_url)) { + $url = $temp_url; + } + + $retrieveOffsiteDocument = new PatientRetrieveOffsiteDocument($d->get_url(), $d); + $updatedOffsiteDocumentEvent = $GLOBALS['kernel']->getEventDispatcher()->dispatch( + $retrieveOffsiteDocument, + PatientRetrieveOffsiteDocument::REMOTE_DOCUMENT_LOCATION + ); + // if a module writer has an independent offsite storage mechanism used this accomdoates that. + // If the file is not found locally, it will be found remotely. Systems like s3, blob stores, etc, can + // be tied and and use those urls. Note NO security is handled here so any kind of security mechanism must + // be handled on the receiving end's URL (s3/azure for example use signed urls with signature verification) + if ( + $updatedOffsiteDocumentEvent instanceof PatientRetrieveOffsiteDocument + && $updatedOffsiteDocumentEvent->getOffsiteUrl() != null + ) { + header('Content-Description: File Transfer'); + header("Location: " . $updatedOffsiteDocumentEvent->getOffsiteUrl()); + exit; + } } if (!file_exists($url)) { diff --git a/library/htmlspecialchars.inc.php b/library/htmlspecialchars.inc.php index 9258c0e99fd..8c38680562a 100644 --- a/library/htmlspecialchars.inc.php +++ b/library/htmlspecialchars.inc.php @@ -41,7 +41,7 @@ function attr_url($text) */ function js_url($text) { - return js_escape(urlencode($text)); + return js_escape(urlencode($text ?? '')); } /** diff --git a/src/Events/PatientDocuments/PatientRetrieveOffsiteDocument.php b/src/Events/PatientDocuments/PatientRetrieveOffsiteDocument.php index ec9b48610b2..0ca99bfd023 100644 --- a/src/Events/PatientDocuments/PatientRetrieveOffsiteDocument.php +++ b/src/Events/PatientDocuments/PatientRetrieveOffsiteDocument.php @@ -13,24 +13,41 @@ namespace OpenEMR\Events\PatientDocuments; use Symfony\Contracts\EventDispatcher\Event; +use Document; class PatientRetrieveOffsiteDocument extends Event { const REMOTE_DOCUMENT_LOCATION = 'remote.document.retrieve.location'; private string $url; - private $offsiteurl; - public function __construct($url) + private $offsiteUrl; + private Document $document; + public function __construct(string $url, ?Document $document = null) { $this->url = $url; + $this->document = $document; } - public function setOffsiteUrl(string $offsitedUrl): void + /** + * Returns the OpenEMR document class that represents this document in the file system + * @return Document|null + */ + public function getOpenEMRDocument(): ?Document { - $this->offsiteurl = $offsiteUrl; + return $this->document; + } + + public function getOpenEMRDocumentUrl(): string + { + return $this->url; + } + + public function setOffsiteUrl(string $offsiteUrl): void + { + $this->offsiteUrl = $offsiteUrl; } public function getOffsiteUrl() { - return $this->offsiteurl; + return $this->offsiteUrl; } } From 4c5543f93b7189acbfb2f4851d710242908e3535 Mon Sep 17 00:00:00 2001 From: Jerry Padgett Date: Thu, 25 Apr 2024 18:17:58 -0400 Subject: [PATCH 2/2] Weno better error handling from fetches (#7408) - this needs a rewrite but maybe later (cherry picked from commit 548db011c1e72316447992130d96ef4306c77070) --- .../oe-module-weno/scripts/weno_log_sync.php | 2 +- .../src/Services/LogProperties.php | 17 ++++++++++++++--- .../oe-module-weno/templates/synch.php | 3 ++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/interface/modules/custom_modules/oe-module-weno/scripts/weno_log_sync.php b/interface/modules/custom_modules/oe-module-weno/scripts/weno_log_sync.php index 11523089816..8948fd97b20 100644 --- a/interface/modules/custom_modules/oe-module-weno/scripts/weno_log_sync.php +++ b/interface/modules/custom_modules/oe-module-weno/scripts/weno_log_sync.php @@ -85,7 +85,7 @@ function downloadWenoPrescriptionLog(): void } $logSync = new LogProperties(); - if (!$logSync->logSync()) { + if (!$logSync->logSync('background')) { error_log("Background services failed for prescription log."); } } diff --git a/interface/modules/custom_modules/oe-module-weno/src/Services/LogProperties.php b/interface/modules/custom_modules/oe-module-weno/src/Services/LogProperties.php index 755937823e2..a023e4055ad 100644 --- a/interface/modules/custom_modules/oe-module-weno/src/Services/LogProperties.php +++ b/interface/modules/custom_modules/oe-module-weno/src/Services/LogProperties.php @@ -139,7 +139,7 @@ public function logReview() /** * @throws Exception */ - public function logSync() + public function logSync($tasked = 'background') { $wenoLog = new WenoLogService(); $provider_info['email'] = $this->weno_admin_email; @@ -167,9 +167,20 @@ public function logSync() if ($isError['is_error']) { $error = $isError['messageText']; error_log('Prescription download failed: ' . errorLogEscape($error)); - $wenoLog->insertWenoLog("prescription", "Invalid Prescriber Credentials Hint:see previous errors."); EventAuditLogger::instance()->newEvent("prescriptions_log", $_SESSION['authUser'], $_SESSION['authProvider'], 0, ($error)); - die(js_escape($error)); + // if background task then return false + if ($tasked == 'background') { + $wenoLog->insertWenoLog("prescription", "Invalid Prescriber Credentials Background Service."); + return false; + } + if ($tasked == 'downloadLog') { + $wenoLog->insertWenoLog("prescription", "Invalid Prescriber Credentials User Download."); + echo(js_escape($error)); + exit; + } + $wenoLog->insertWenoLog("prescription", "Invalid Prescriber Credentials Hint:see previous errors."); + echo(js_escape($error)); + return false; } $wenoLog->insertWenoLog("prescription", "Success"); } else { diff --git a/interface/modules/custom_modules/oe-module-weno/templates/synch.php b/interface/modules/custom_modules/oe-module-weno/templates/synch.php index 536868b44cc..0d8e50cb9f2 100644 --- a/interface/modules/custom_modules/oe-module-weno/templates/synch.php +++ b/interface/modules/custom_modules/oe-module-weno/templates/synch.php @@ -26,7 +26,8 @@ $logProperties = new LogProperties(); try { - $result = $logProperties->logSync(); + $task = $_REQUEST['key'] ?? $_POST['key'] ?? ''; + $result = $logProperties->logSync($task); } catch (Exception $e) { $result = false; error_log('Error syncing log: ' . errorLogEscape($e->getMessage()));