oauth updates (openemr#4040)

- added support so that the client secret is validated - supported encryption of the client secret - updated password grant - removed need for user_roles
DiscoverAndChange · Nov 18, 2020 · 5384f4e · 5384f4e
1 parent 46d9fc3
commit 5384f4e
Show file tree

Hide file tree

Showing 16 changed files with 370 additions and 213 deletions.
diff --git a/_rest_config.php b/_rest_config.php
@@ -354,48 +354,30 @@ public static function emitResponse($response, $build = false): void
  echo $response->getBody();
  }
 
- public function authenticateUserToken($tokenId, $userId, $userRole)
+ public function authenticateUserToken($tokenId, $userId)
  {
  $ip = collectIpAddresses();
 
  // check for token
- $authToken = sqlQueryNoLog("SELECT `expiry` FROM `api_token` WHERE `token` = ? AND `user_id` = ? AND `user_role` = ?", [$tokenId, $userId, $userRole]);
+ $authToken = sqlQueryNoLog("SELECT `expiry` FROM `api_token` WHERE `token` = ? AND `user_id` = ?", [$tokenId, $userId]);
  if (empty($authToken) || empty($authToken['expiry'])) {
- EventAuditLogger::instance()->newEvent('api', '', '', 0, "API failure: " . $ip['ip_string'] . ". Token not found for " . $userId . " with role " . $userRole . ".");
+ EventAuditLogger::instance()->newEvent('api', '', '', 0, "API failure: " . $ip['ip_string'] . ". Token not found for " . $userId . ".");
  return false;
  }
 
  // Ensure token not expired (note an expired token should have already been caught by oauth2, however will also check here)
  $currentDateTime = date("Y-m-d H:i:s");
  $expiryDateTime = date("Y-m-d H:i:s", strtotime($authToken['expiry']));
  if ($expiryDateTime <= $currentDateTime) {
- EventAuditLogger::instance()->newEvent('api', '', '', 0, "API failure: " . $ip['ip_string'] . ". Token expired for " . $userId . " with role " . $userRole . ".");
+ EventAuditLogger::instance()->newEvent('api', '', '', 0, "API failure: " . $ip['ip_string'] . ". Token expired for " . $userId . ".");
  return false;
  }
 
  // Token authentication passed
- EventAuditLogger::instance()->newEvent('api', '', '', 1, "API success: " . $ip['ip_string'] . ". Token successfully used for " . $userId . " with role " . $userRole . ".");
+ EventAuditLogger::instance()->newEvent('api', '', '', 1, "API success: " . $ip['ip_string'] . ". Token successfully used for " . $userId . ".");
  return true;
  }
 
- public function getUserAccount($userId, $userRole)
- {
- $userId = UuidRegistry::uuidToBytes($userId);
-
- switch ($userRole) {
- case 'users':
- $account_sql = "SELECT `id`, `username`, `authorized`, `lname` AS lastname, `fname` AS firstname, `mname` AS middlename, `phone`, `email`, `street`, `city`, `state`, `zip`, CONCAT(fname, ' ', lname) AS fullname FROM `users` WHERE `uuid` = ?";
- break;
- case 'patient':
- $account_sql = "SELECT `pid`, `uuid`, `lname` AS lastname, `fname` AS firstname, `mname` AS middlename, `phone_contact` AS phone, `sex` AS gender, `email`, `DOB` AS birthdate, `street`, `postal_code` AS zip, `city`, `state`, CONCAT(fname, ' ', lname) AS fullname FROM `patient_data` WHERE `uuid` = ?";
- break;
- default:
- return null;
- }
-
- return sqlQueryNoLog($account_sql, array($userId));
- }
-
  /** prevents external cloning */
  private function __clone()
  {

diff --git a/apis/dispatch.php b/apis/dispatch.php
@@ -10,19 +10,19 @@
  * @author Brady Miller <[email protected]>
  * @copyright Copyright (c) 2018 Matthew Vita <[email protected]>
  * @copyright Copyright (c) 2020 Jerry Padgett <[email protected]>
- * @copyright Copyright (c) 2019 Brady Miller <[email protected]>
+ * @copyright Copyright (c) 2019-2020 Brady Miller <[email protected]>
  * @license https://github.com/openemr/openemr/blob/master/LICENSE GNU General Public License 3
  */
 
 require_once("./../_rest_config.php");
 
+use OpenEMR\Common\Auth\UuidUserAccount;
 use OpenEMR\Common\Csrf\CsrfUtils;
 use OpenEMR\Common\Http\HttpRestRouteHandler;
 use OpenEMR\Events\RestApiExtend\RestApiCreateEvent;
 use Psr\Http\Message\ResponseInterface;
 
 $gbl = RestConfig::GetInstance();
-$base_path = $gbl::$ROOT_URL;
 $routes = array();
 
 // Parse needed information from Redirect or REQUEST_URI
@@ -47,25 +47,16 @@
  // collect token attributes
  $attributes = $tokenRaw->getAttributes();
 
- // collect site and user role
+ // collect site
  $site = '';
- $userRole = '';
  $scopes = $attributes['oauth_scopes'];
  foreach ($scopes as $attr) {
  if (stripos($attr, 'site:') !== false) {
  $site = str_replace('site:', '', $attr);
  // while here parse site from endpoint
  $resource = str_replace('/' . $site, '', $resource);
- } else if (stripos($attr, 'user_role:') !== false) {
- $userRole = str_replace('user_role:', '', $attr);
  }
  }
- // ensure user_role in access token
- if (empty($userRole)) {
- error_log("OpenEMR Error - api user role not available, so forced exit");
- http_response_code(400);
- exit();
- }
  // ensure 1) sane site 2) site from gbl and access token are the same and 3) ensure the site exists on filesystem
  if (empty($site) || empty($gbl::$SITE) || preg_match('/[^A-Za-z0-9\\-.]/', $gbl::$SITE) || ($site !== $gbl::$SITE) || !file_exists(__DIR__ . '/../sites/' . $gbl::$SITE)) {
  error_log("OpenEMR Error - api site error, so forced exit");
@@ -120,6 +111,30 @@
  exit();
  }
 } else {
+ // authenticate the token
+ if (!$gbl->authenticateUserToken($tokenId, $userId)) {
+ $gbl::destroySession();
+ http_response_code(401);
+ exit();
+ }
+ // collect user information and user role
+ $uuidToUser = new UuidUserAccount($userId);
+ $user = $uuidToUser->getUserAccount();
+ $userRole = $uuidToUser->getUserRole();
+ if (empty($user)) {
+ // unable to identify the users user role
+ error_log("OpenEMR Error - api user account could not be identified, so forced exit");
+ $gbl::destroySession();
+ http_response_code(400);
+ exit();
+ }
+ if (empty($userRole)) {
+ // unable to identify the users user role
+ error_log("OpenEMR Error - api user role for user could not be identified, so forced exit");
+ $gbl::destroySession();
+ http_response_code(400);
+ exit();
+ }
  // ensure user role has access to the resource
  // for now assuming:
  // users has access to oemr and fhir
@@ -134,14 +149,7 @@
  http_response_code(401);
  exit();
  }
- // authenticate the token
- if (!$gbl->authenticateUserToken($tokenId, $userId, $userRole)) {
- $gbl::destroySession();
- http_response_code(401);
- exit();
- }
- // collect user information and then set pertinent session variables
- $user = $gbl->getUserAccount($userId, $userRole);
+ // set pertinent session variables
  if ($userRole == 'users') {
  $_SESSION['authUser'] = $user["username"] ?? null;
  $_SESSION['authUserID'] = $user["id"] ?? null;

diff --git a/library/globals.inc.php b/library/globals.inc.php
@@ -3114,6 +3114,18 @@ function gblTimeZones()
  xl('Enable OpenEMR Patient Portal FHIR RESTful API.')
  ),
 
+ 'oauth_password_grant' => array(
+ xl('Enable Oauth2 Password Grant (Not considered secure)'),
+ array(
+ 0 => xl('Off (Recommended setting)'),
+ 1 => xl('On for Users Role'),
+ 2 => xl('On for Patient Role'),
+ 3 => xl('On for Both Roles')
+ ),
+ '0',
+ xl('Enable Oauth2 Password Grant. Recommend turning this setting off for production server. Recommend only using for testing.')
+ ),
+
  'fhir_enable' => array(
  xl('Enable FHIR Provider Client Service'),
  array(

diff --git a/oauth2/authorize.php b/oauth2/authorize.php
@@ -61,10 +61,6 @@
  $authServer->oauthAuthorizationFlow();
 }
 
-if (false !== stripos($end_point, '/password')) {
- $authServer->oauthPasswordFlow();
-}
-
 if (false !== stripos($end_point, '/device/code')) {
  $authServer->authorizeUser();
 }

diff --git a/oauth2/provider/.well-known/discovery.php b/oauth2/provider/.well-known/discovery.php
@@ -3,6 +3,11 @@
 global $authServer;
 $base_url = $authServer->authBaseFullUrl;
 
+$passwordGrantString = '';
+if (!empty($GLOBALS['oauth_password_grant'])) {
+ $passwordGrantString = '"password",';
+}
+
 $discovery = <<<TEMPLATE
 {
 "issuer": "$authServer->authIssueFullUrl",
@@ -42,7 +47,7 @@
 ],
 "grant_types_supported": [
  "authorization_code",
- "password",
+ $passwordGrantString
  "refresh_token"
 ],
 "response_modes_supported": [

diff --git a/sql/5_0_2-to-6_0_0_upgrade.sql b/sql/5_0_2-to-6_0_0_upgrade.sql
@@ -2295,7 +2295,7 @@ CREATE TABLE `oauth_clients` (
 `client_id` varchar(80) NOT NULL,
 `client_role` varchar(20) DEFAULT NULL,
 `client_name` varchar(80) NOT NULL,
-`client_secret` varchar(120) DEFAULT NULL,
+`client_secret` text,
 `registration_token` varchar(80) DEFAULT NULL,
 `registration_uri_path` varchar(40) DEFAULT NULL,
 `register_date` datetime DEFAULT NULL,
@@ -2315,15 +2315,13 @@ PRIMARY KEY (`client_id`)
 CREATE TABLE `oauth_trusted_user` (
 `id` bigint(20) NOT NULL AUTO_INCREMENT,
 `user_id` varchar(80) DEFAULT NULL,
-`user_role` varchar(16) DEFAULT NULL,
 `client_id` varchar(80) DEFAULT NULL,
 `scope` text,
 `persist_login` tinyint(1) DEFAULT '0',
 `time` timestamp NULL DEFAULT NULL,
 PRIMARY KEY (`id`),
 KEY `accounts_id` (`user_id`),
-KEY `clients_id` (`client_id`),
-KEY `user_role` (`user_role`)
+KEY `clients_id` (`client_id`)
 ) ENGINE=InnoDB;
 #EndIf
 
@@ -2342,18 +2340,14 @@ UPDATE `icd10_dx_order_code` SET `revision` = @newMax WHERE `dx_code` = 'U072';
 ALTER TABLE `oauth_clients` CHANGE `client_id` `client_id` varchar(80) NOT NULL;
 #EndIf
 
-#IfNotColumnType oauth_clients client_secret varchar(120)
-ALTER TABLE `oauth_clients` CHANGE `client_secret` `client_secret` varchar(120) DEFAULT NULL;
+#IfNotColumnType oauth_clients client_secret text
+ALTER TABLE `oauth_clients` CHANGE `client_secret` `client_secret` text;
 #EndIf
 
 #IfNotColumnType oauth_clients registration_token varchar(80)
 ALTER TABLE `oauth_clients` CHANGE `registration_token` `registration_token` varchar(80) DEFAULT NULL;
 #EndIf
 
-#IfMissingColumn api_token user_role
-ALTER TABLE `api_token` ADD `user_role` varchar(40) DEFAULT NULL;
-#EndIf
-
 #IfColumn api_token token_auth
 ALTER TABLE `api_token` DROP COLUMN `token_auth`;
 #EndIf
@@ -2397,3 +2391,11 @@ ALTER TABLE `icd10_dx_order_code` MODIFY `long_desc` text;
 #IfNotColumnType icd10_pcs_order_code long_desc text
 ALTER TABLE `icd10_pcs_order_code` MODIFY `long_desc` text;
 #EndIf
+
+#IfColumn api_token user_role
+ALTER TABLE `api_token` DROP COLUMN `user_role`;
+#EndIf
+
+#IfColumn oauth_trusted_user user_role
+ALTER TABLE `oauth_trusted_user` DROP COLUMN `user_role`;
+#EndIf
diff --git a/sql/database.sql b/sql/database.sql
@@ -109,7 +109,6 @@ DROP TABLE IF EXISTS `api_token`;
 CREATE TABLE `api_token` (
  `id` bigint(20) NOT NULL AUTO_INCREMENT,
  `user_id` varchar(40) DEFAULT NULL,
- `user_role` varchar(40) DEFAULT NULL,
  `token` varchar(128) DEFAULT NULL,
  `expiry` datetime DEFAULT NULL,
  `client_id` varchar(80) DEFAULT NULL,
@@ -12248,7 +12247,7 @@ CREATE TABLE `oauth_clients` (
 `client_id` varchar(80) NOT NULL,
 `client_role` varchar(20) DEFAULT NULL,
 `client_name` varchar(80) NOT NULL,
-`client_secret` varchar(120) DEFAULT NULL,
+`client_secret` text,
 `registration_token` varchar(80) DEFAULT NULL,
 `registration_uri_path` varchar(40) DEFAULT NULL,
 `register_date` datetime DEFAULT NULL,
@@ -12267,7 +12266,6 @@ DROP TABLE IF EXISTS `oauth_trusted_user`;
 CREATE TABLE `oauth_trusted_user` (
 `id` bigint(20) NOT NULL AUTO_INCREMENT,
 `user_id` varchar(80) DEFAULT NULL,
-`user_role` varchar(16) DEFAULT NULL,
 `client_id` varchar(80) DEFAULT NULL,
 `scope` text,
 `persist_login` tinyint(1) DEFAULT '0',
@@ -12276,6 +12274,5 @@ CREATE TABLE `oauth_trusted_user` (
 `session_cache` text,
 PRIMARY KEY (`id`),
 KEY `accounts_id` (`user_id`),
-KEY `clients_id` (`client_id`),
-KEY `user_role` (`user_role`)
+KEY `clients_id` (`client_id`)
 ) ENGINE=InnoDB;