feat: #6525 Use EHR authorization for SMART Apps (#6628)

* #6525 Use EHR authorization for SMART Apps First stab at #6525 by allowing the EHR to launch a smart app and reuse the authorization context that the currently logged in user has. I introduce a global that by default is off to allow OpenEMR administrators to enable smart apps to skip the authorization screen when the smart app is launched inside the EHR. I refactored the launch sequence now to be an on-demand generation of the launch token using javascript instead of generating the token and stuffing it into the html. I added a flag on the individual client app to turn on or off the authorization skip flag. This gives admins control over allowing apps to launch immediately inside the EHR without requiring an authorization / authentication step. I expanded the size of the SMART app window so that the app can take up more of the window and offer more functionality. I'm wondering if we should offer different size options based upon the registered intent, or if there is some mechanism the app can communicate of how much real estate it wants to take up. The app will receive any permissions it requests that was originally granted in its registration request. The flow will continue to discard permissions that were not granted in the original registration request or were reduced in a subset of the refresh token. Added to the launch token the currently authenticated user and the type of user. Right now the ehr-launch is only inside the provider side of things, but I put in infrastructure so we can later extend it to launch inside the patient portal. I originally relied on opening up the OpenEMR session cookie, but this doesn't work if the SMART app is hosted on a domain other than the domain OpenEMR is running under. Browsers will drop the cookies on the browser redirect as its not considered a first-party origin so we have to rely on the launch token being the authority for the authentication / authorization. I'm thinking that for more security in order to prevent replay attacks we need to store the generated launch token in the database, time expire it, and make it a one time use token. If a repeat is detected we should invalidate all of the current user's access tokens and do some kind of notification to the administrator. I'm not sure yet if this is warranted, but it seems like we should treat this as serious as what we do with the refresh token. * In-EHR launch use session cookies instead of token By switching to a javascript submitted form on the authorize page to establish a first party context I was able to retrieve the session cookies and verify the user is logged in, even inside of a third party dialog. If for some reason the third party cookie doesn't exist it brings up the standard login form. The downside of this approach is a bit slower processing as an additional round trip to the client has to happen to submit the first party form. Also, switching between the openemr session an oauth2 session has to deal with brief session locking and filesystem access if that is how the php sessions are configured. Overall though this eliminates the security loophole of someone grabbing the launch token via js or some other mechanism and logging in as a user just using the launch token. Now we can tie it to the specific logged in session and close down that security issue. * Style fixes. * Add in missing sql column. * Fix kernel mispelling Fix the kernel mispelling the authorization controller. * Removing user uuid, added main intent tab Somehow the rebase removed the user uuid stuff that was part of the original wip. Added a main intent tab for launching smart apps in one of the OpenEMR tab. Added a few more debug logs.
openemr · Aug 15, 2023 · f3ebdb0 · f3ebdb0
1 parent 6aad5df
commit f3ebdb0
Show file tree

Hide file tree

Showing 16 changed files with 386 additions and 41 deletions.
diff --git a/interface/patient_file/summary/demographics.php b/interface/patient_file/summary/demographics.php
@@ -348,7 +348,7 @@ function image_widget($doc_id, $doc_catg)
 
 <head>
     <?php
-    Header::setupHeader(['common']);
+    Header::setupHeader(['common','utility']);
     require_once("$srcdir/options.js.php");
     ?>
     <script>

diff --git a/interface/smart/ehr-launch-client.php b/interface/smart/ehr-launch-client.php
@@ -12,7 +12,13 @@
 
 require_once("../globals.php");
 
-$controller = new \OpenEMR\FHIR\SMART\SmartLaunchController();
+use OpenEMR\Common\Acl\AccessDeniedException;
+use OpenEMR\Common\Csrf\CsrfInvalidException;
+use OpenEMR\Common\Csrf\CsrfUtils;
+use OpenEMR\Common\Logging\SystemLogger;
+use OpenEMR\FHIR\SMART\SmartLaunchController;
+
+$controller = new SmartLaunchController();
 
 $intentData = [];
 try {

diff --git a/library/globals.inc.php b/library/globals.inc.php
@@ -3233,6 +3233,12 @@ function gblTimeZones()
             '0',
             xl('Approval settings for 3rd party app/api access')
         ),
+        'oauth_ehr_launch_authorization_flow_skip' => array(
+            xl('OAuth2 EHR-Launch Authorization Flow Skip Enable App Setting'),
+            'bool',
+            '0',
+            xl('Enable an OAuth2 Client application to be configured to skip the login screen and the scope authorization screen if the user is already logged into the EHR.')
+        ),
 
         'cc_front_payments' => array(
             xl('Accept Credit Card transactions from Front Payments'),

diff --git a/library/js/utility.js b/library/js/utility.js
@@ -475,3 +475,31 @@ if (typeof top.userDebug !== 'undefined' && (top.userDebug === '1' || top.userDe
     };
 }
 
+(function(window, oeSMART) {
+    oeSMART.initLaunch = function(webroot, csrfToken) {
+        // allows this to be lazy defined
+        let xl = window.top.xl || function(text) { return text; };
+        let smartLaunchers = document.querySelectorAll('.smart-launch-btn');
+        for (let launch of smartLaunchers) {
+                launch.addEventListener('click', function (evt) {
+                    let node = evt.target;
+                    let intent = node.dataset.intent;
+                    let clientId = node.dataset.clientId;
+                    if (!intent || !clientId) {
+                        console.error("mising intent parameter or client-id parameter");
+                        return;
+                    }
+
+                    let url = webroot + '/interface/smart/ehr-launch-client.php?intent='
+                        + encodeURIComponent(intent) + '&client_id=' + encodeURIComponent(clientId)
+                        + "&csrf_token=" + encodeURIComponent(csrfToken);
+                    let title = node.dataset.smartName || JSON.stringify(xl("Smart App"));
+                    // we allow external dialog's  here because that is what a SMART app is
+                    let height = window.top.innerHeight; // do our full height here
+                    dlgopen(url, '_blank', 'modal-full', height, '', title, {allowExternal: true});
+                });
+        }
+    };
+    window.oeSMART = oeSMART;
+})(window, window.top.oeSMART || {});
+
diff --git a/oauth2/smart/ehr-launch-autosubmit.html.twig b/oauth2/smart/ehr-launch-autosubmit.html.twig
@@ -0,0 +1,16 @@
+<html>
+<body>
+<form method="POST" action="{{ endpoint }}">
+    {# Do we want to show any kind of message...? For satellite or slow latency connections... we'd want to show something here. #}
+</form>
+<script>
+    // grab the launch token and autosubmit it to the launch endpoint
+    (function() {
+        document.addEventListener("DOMContentLoaded", function(event) {
+            var form = document.getElementsByTagName('form')[0];
+            form.submit();
+        });
+    })();
+</script>
+</body>
+</html>
diff --git a/sql/7_0_1-to-7_0_2_upgrade.sql b/sql/7_0_1-to-7_0_2_upgrade.sql
@@ -245,6 +245,9 @@ CREATE TABLE recent_patients (
     patients TEXT,
     PRIMARY KEY (user_id)
 ) ENGINE=InnoDB;
+
+#IfMissingColumn oauth_clients skip_ehr_launch_authorization_flow
+ALTER TABLE `oauth_clients` ADD COLUMN `skip_ehr_launch_authorization_flow` tinyint(1) NOT NULL DEFAULT '0';
 #EndIf
 
 #IfMissingColumn document_template_profiles notify_trigger

diff --git a/sql/database.sql b/sql/database.sql
@@ -13077,6 +13077,7 @@ CREATE TABLE `oauth_clients` (
 `policy_uri` text,
 `tos_uri` text,
 `is_enabled` tinyint(1) NOT NULL DEFAULT '0',
+`skip_ehr_launch_authorization_flow` tinyint(1) NOT NULL DEFAULT '0',
 PRIMARY KEY (`client_id`)
 ) ENGINE=InnoDB;
 

diff --git a/src/Common/Auth/OpenIDConnect/Entities/ClientEntity.php b/src/Common/Auth/OpenIDConnect/Entities/ClientEntity.php
@@ -54,9 +54,15 @@ class ClientEntity implements ClientEntityInterface
 
     protected $registrationDate;
 
+    /**
+     * @var bool true if the authorization flow (authentication and scope authorization) should be skipped for logged in ehr users
+     */
+    private $skipEHRLaunchAuthorizationFlow;
+
     public function __construct()
     {
         $this->scopes = [];
+        $this->skipEHRLaunchAuthorizationFlow = false;
     }
 
     public function setName($name): void
@@ -242,4 +248,18 @@ public function setLogoutRedirectUris(?string $logoutRedirectUris): void
     {
         $this->logoutRedirectUris = $logoutRedirectUris;
     }
+
+    /**
+     * Whether the ehr launch should skip the authorization flow for a logged in user.
+     * @return bool
+     */
+    public function shouldSkipEHRLaunchAuthorizationFlow(): bool
+    {
+        return $this->skipEHRLaunchAuthorizationFlow;
+    }
+
+    public function setSkipEHRLaunchAuthorizationFlow(bool $shouldSkip)
+    {
+        $this->skipEHRLaunchAuthorizationFlow = $shouldSkip;
+    }
 }
diff --git a/src/Common/Auth/OpenIDConnect/Repositories/ClientRepository.php b/src/Common/Auth/OpenIDConnect/Repositories/ClientRepository.php
@@ -57,6 +57,7 @@ public function insertNewClient($clientId, $info, $site): bool
     {
         $user = $_SESSION['authUserID'] ?? null; // future use for provider client.
         $is_confidential_client = empty($info['client_secret']) ? 0 : 1;
+        $skip_ehr_launch_authorization_flow = $info['skip_ehr_launch_authorization_flow'] == true ? 1 : 0;
 
         $contacts = $info['contacts'];
         $redirects = $info['redirect_uris'];
@@ -90,7 +91,7 @@ public function insertNewClient($clientId, $info, $site): bool
         }
 
         // TODO: @adunsulag why do we skip over request_uris when we have it in the outer function?
-        $sql = "INSERT INTO `oauth_clients` (`client_id`, `client_role`, `client_name`, `client_secret`, `registration_token`, `registration_uri_path`, `register_date`, `revoke_date`, `contacts`, `redirect_uri`, `grant_types`, `scope`, `user_id`, `site_id`, `is_confidential`, `logout_redirect_uris`, `jwks_uri`, `jwks`, `initiate_login_uri`, `endorsements`, `policy_uri`, `tos_uri`, `is_enabled`) VALUES (?, ?, ?, ?, ?, ?, NOW(), NULL, ?, ?, 'authorization_code', ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+        $sql = "INSERT INTO `oauth_clients` (`client_id`, `client_role`, `client_name`, `client_secret`, `registration_token`, `registration_uri_path`, `register_date`, `revoke_date`, `contacts`, `redirect_uri`, `grant_types`, `scope`, `user_id`, `site_id`, `is_confidential`, `logout_redirect_uris`, `jwks_uri`, `jwks`, `initiate_login_uri`, `endorsements`, `policy_uri`, `tos_uri`, `is_enabled`, `skip_ehr_launch_authorization_flow`) VALUES (?, ?, ?, ?, ?, ?, NOW(), NULL, ?, ?, 'authorization_code', ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
         $i_vals = array(
             $clientId,
             $info['client_role'],
@@ -111,7 +112,8 @@ public function insertNewClient($clientId, $info, $site): bool
             ($info['endorsements'] ?? null),
             ($info['policy_uri'] ?? null),
             ($info['tos_uri'] ?? null),
-            $is_client_enabled
+            $is_client_enabled,
+            $skip_ehr_launch_authorization_flow
         );
 
         return sqlQueryNoLog($sql, $i_vals, true); // throw an exception if it fails
@@ -260,6 +262,7 @@ private function hydrateClientEntityFromArray($client_record): ClientEntity
         $client->setLogoutRedirectUris($client_record['logout_redirect_uris']);
         $client->setContacts($client_record['contacts']);
         $client->setRegistrationDate($client_record['register_date']);
+        $client->setSkipEHRLaunchAuthorizationFlow($client_record['skip_ehr_launch_authorization_flow'] == "1");
         return $client;
     }
 
@@ -272,4 +275,19 @@ public function generateRegistrationClientUriPath()
     {
         return HttpUtils::base64url_encode(RandomGenUtils::produceRandomBytes(16));
     }
+
+    public function saveSkipEHRLaunchFlow(ClientEntity $client, bool $skipFlow)
+    {
+        // TODO: adunsulag do we want to eventually just have a save() method.. it would be very handy but not sure
+        // we want any oauth2 values being overwritten.
+        $skipFlowValue = $skipFlow === true ? 1 : 0;
+        $clientId = $client->getIdentifier();
+        $params = [$skipFlowValue, $clientId];
+        $res = sqlStatement("UPDATE oauth_clients SET skip_ehr_launch_authorization_flow=? WHERE client_id = ?", $params);
+        if ($res === false) {
+            // TODO: adunsulag is there a better exception to throw here in OpenEMR than runtime?
+            throw new \RuntimeException("Failed to save oauth_clients skip_ehr_launch_authorization_flow flag.  Check logs for sql error");
+        }
+        return true;
+    }
 }
diff --git a/src/Common/Auth/OpenIDConnect/SMARTSessionTokenContextBuilder.php b/src/Common/Auth/OpenIDConnect/SMARTSessionTokenContextBuilder.php
@@ -33,6 +33,7 @@ public function __construct($sessionArray = array())
     public function getEHRLaunchContext()
     {
         $launch = $this->sessionArray['launch'] ?? null;
+        $this->logger->debug("SMARTSessionTokenContextBuilder->getEHRLaunchContext() launch context is", ['launch' => $launch]);
         if (empty($launch)) {
             return [];
         }
@@ -101,6 +102,7 @@ public function getContextForScopesWithExistingContext(array $context, array $sc
     public function getContextForScopes($scopes): array
     {
         $context = [];
+        $this->logger->debug("SMARTSessionTokenContextBuilder->getContextForScopes()");
         if ($this->isStandaloneLaunchPatientRequest($scopes)) {
             $context = $this->getStandaloneLaunchContext();
         } else if ($this->isEHRLaunchRequest($scopes)) {

diff --git a/src/Common/Session/SessionUtil.php b/src/Common/Session/SessionUtil.php
@@ -61,6 +61,9 @@
 
 class SessionUtil
 {
+    private const CORE_SESSION_ID = "OpenEMR";
+    private const OAUTH_SESSION_ID = 'authserverOpenEMR';
+
     private static $gc_maxlifetime = 14400;
     private static $sid_bits_per_character = 6;
     private static $sid_length = 48;
@@ -71,6 +74,13 @@ class SessionUtil
     private static $use_cookie_httponly = true;
     private static $use_cookie_secure = false;
 
+    public static function switchToCoreSession($web_root, $read_only = true): void
+    {
+        session_write_close();
+        session_id($_COOKIE[self::CORE_SESSION_ID] ?? '');
+        self::coreSessionStart($web_root, $read_only);
+    }
+
     public static function coreSessionStart($web_root, $read_only = true): void
     {
         // Note there is no system logger here since that class does not
@@ -79,7 +89,7 @@ public static function coreSessionStart($web_root, $read_only = true): void
             'read_and_close' => $read_only,
             'cookie_samesite' => self::$use_cookie_samesite,
             'cookie_secure' => self::$use_cookie_secure,
-            'name' => 'OpenEMR',
+            'name' => self::CORE_SESSION_ID,
             'cookie_httponly' => false,
             'cookie_path' => ((!empty($web_root)) ? $web_root . '/' : '/'),
             'gc_maxlifetime' => self::$gc_maxlifetime,
@@ -187,12 +197,19 @@ public static function apiSessionCookieDestroy(): void
         self::standardSessionCookieDestroy();
     }
 
+    public static function switchToOAuthSession($web_root): void
+    {
+        session_write_close();
+        session_id($_COOKIE[self::OAUTH_SESSION_ID] ?? '');
+        self::oauthSessionStart($web_root);
+    }
+
     public static function oauthSessionStart($web_root): void
     {
         session_start([
             'cookie_samesite' => "None",
             'cookie_secure' => true,
-            'name' => 'authserverOpenEMR',
+            'name' => self::OAUTH_SESSION_ID,
             'cookie_httponly' => self::$use_cookie_httponly,
             'cookie_path' => ((!empty($web_root)) ? $web_root . '/oauth2/' : '/oauth2/'),
             'gc_maxlifetime' => self::$gc_maxlifetime,

diff --git a/src/FHIR/SMART/ClientAdminController.php b/src/FHIR/SMART/ClientAdminController.php
@@ -139,6 +139,10 @@ public function dispatch($action, $request)
                 return $this->revokeAccessToken($clientId, $request);
             } else if ($subAction == self::REVOKE_REFRESH_TOKEN) {
                 return $this->revokeRefreshToken($clientId, $request);
+            } else if ($subAction == 'enable-authorization-flow-skip') {
+                return $this->enableAuthorizationFlowSkipAction($clientId, $request);
+            } else if ($subAction == 'disable-authorization-flow-skip') {
+                return $this->disableAuthorizationFlowSkipAction($clientId, $request);
             } else {
                 return $this->notFoundAction($request);
             }
@@ -369,8 +373,14 @@ private function renderEdit(ClientEntity $client, $request)
         $listAction = $this->getActionUrl(['list']);
         $disableClientLink = $this->getActionUrl(['edit', $client->getIdentifier(), 'disable']);
         $enableClientLink = $this->getActionUrl(['edit', $client->getIdentifier(), 'enable']);
-
+        $disableSkipAuthorizationFlowLink = $this->getActionUrl(['edit', $client->getIdentifier(), 'disable-authorization-flow-skip']);
+        $enableSkipAuthorizationFlowLink = $this->getActionUrl(['edit', $client->getIdentifier(), 'enable-authorization-flow-skip']);
         $isEnabled = $client->isEnabled();
+        $allowSkipAuthSetting = $GLOBALS['oauth_ehr_launch_authorization_flow_skip'] === '1';
+        $skipAuthorizationFlow = $client->shouldSkipEHRLaunchAuthorizationFlow();
+        if (!$allowSkipAuthSetting) {
+            $skipAuthorizationFlow = false; // globals overrides this setting
+        }
         $scopes = $client->getScopes();
 
         $formValues = [
@@ -405,6 +415,12 @@ private function renderEdit(ClientEntity $client, $request)
                 , 'checked' => $isEnabled
                 , 'value' => 1
             ],
+            'skipEHRLaunchAuthorizationFlow' => [
+                'type' => 'checkbox'
+                ,'label' => xl('Skip EHR Launch Authorization Flow')
+                , 'checked' => $skipAuthorizationFlow
+                , 'value' => 1
+            ],
             'role' => [
                 'type' => 'text'
                 ,'label' => xl("Role")
@@ -432,6 +448,10 @@ private function renderEdit(ClientEntity $client, $request)
             ],
         ];
 
+        if (!$allowSkipAuthSetting) {
+            unset($formValues['skipEHRLaunchAuthorizationFlow']);
+        }
+
         ?>
         <a href="<?php echo attr($listAction); ?>" class="btn btn-sm btn-secondary" onclick="top.restoreSession()">&lt; <?php echo xlt("Back to Client List"); ?></a>
 
@@ -440,8 +460,15 @@ private function renderEdit(ClientEntity $client, $request)
                 <h2>
                     <?php echo xlt('Edit'); ?> <em><?php echo text($client->getName()); ?></em>
                     <div class="float-right">
+                        <?php if ($allowSkipAuthSetting) : ?>
+                            <?php if ($skipAuthorizationFlow) : ?>
+                            <a href="<?php echo attr($disableSkipAuthorizationFlowLink); ?>" class="btn btn-sm btn-primary" onclick="top.restoreSession()"><?php echo xlt('Enable EHR Launch Authorization Flow'); ?></a>
+                            <?php else : ?>
+                            <a href="<?php echo attr($enableSkipAuthorizationFlowLink); ?>" class="btn btn-sm btn-danger" onclick="top.restoreSession()"><?php echo xlt('Disable EHR Launch Authorization Flow'); ?></a>
+                            <?php endif; ?>
+                        <?php endif; ?>
                         <?php if ($isEnabled) : ?>
-                        <a href="<?php echo attr($disableClientLink); ?>" class="btn btn-sm btn-primary" onclick="top.restoreSession()"><?php echo xlt('Disable Client'); ?></a>
+                        <a href="<?php echo attr($disableClientLink); ?>" class="btn btn-sm btn-danger" onclick="top.restoreSession()"><?php echo xlt('Disable Client'); ?></a>
                         <?php else : ?>
                         <a href="<?php echo attr($enableClientLink); ?>" class="btn btn-sm btn-primary" onclick="top.restoreSession()"><?php echo xlt('Enable Client'); ?></a>
                         <?php endif; ?>
@@ -1111,4 +1138,47 @@ private function renderTokenToolsFooter()
         <?php
         $this->renderFooter();
     }
+    private function enableAuthorizationFlowSkipAction(string $clientId, array $request)
+    {
+         $client = $this->clientRepo->getClientEntity($clientId);
+        if ($client === false) {
+            $this->notFoundAction($request);
+            return;
+        }
+        $message = xl('Enabled Authorization Flow Skip') . " " . $client->getName();
+        $this->handleAuthorizationFlowSkipAction($client, true, $message);
+        exit;
+    }
+    private function disableAuthorizationFlowSkipAction(string $clientId, array $request)
+    {
+         $client = $this->clientRepo->getClientEntity($clientId);
+        if ($client === false) {
+            $this->notFoundAction($request);
+            return;
+        }
+        $message = xl('Disabled Authorization Flow Skip') . " " . $client->getName();
+        $this->handleAuthorizationFlowSkipAction($client, false, $message);
+        exit;
+    }
+    private function handleAuthorizationFlowSkipAction(ClientEntity $client, bool $skipFlow, string $successMessage)
+    {
+        $client->setSkipEHRLaunchAuthorizationFlow($skipFlow);
+        try {
+            $this->clientRepo->saveSkipEHRLaunchFlow($client, $skipFlow);
+            $url = $this->getActionUrl(['edit', $client->getIdentifier()], ["queryParams" => ['message' => $successMessage]]);
+            header("Location: " . $url);
+        } catch (\Exception $ex) {
+            $this->logger->error(
+                "Failed to save client",
+                [
+                    "exception" => $ex->getMessage(), "trace" => $ex->getTraceAsString()
+                    , 'client' => $client->getIdentifier()
+                ]
+            );
+
+            $message = xl('Client failed to save. Check system logs');
+            $url = $this->getActionUrl(['edit', $client->getIdentifier()], ["queryParams" => ['message' => $message]]);
+            header("Location: " . $url);
+        }
+    }
 }