feat: #6525 Use EHR authorization for SMART Apps #6628
Conversation
|
Couple of screenshots. Added connector setting for allowing confidential apps to skip the authorization flow. Default is to have this OFF. Added a button to disable the EHR authorization flow for the app: Current setting is shown on the left in the checkboxes area: |
adunsulag
changed the title
adunsulag
force-pushed
the
feat-openemr-fix-6225-smart-app-ehr-launch
branch
from
116071b
to
fc9e4d7
Compare
First stab at openemr#6225 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.
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.
adunsulag
force-pushed
the
feat-openemr-fix-6225-smart-app-ehr-launch
branch
from
fc9e4d7
to
054dcaa
Compare
adunsulag
changed the title
|
So I took inspiration from our one time token implementation and was able to figure out a more secure approach than having the user id passed in the launch token. Now if an in-ehr launch is requested, it will do a client side javascript post back to the server which allows the session cookies to be transmitted in a first-party domain handshake instead of treated as a third-party domain. 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. Reviewers should pay special attention to the AuthorizationController::processAuthorizeFlowForLaunch() method where I am switching between the openemr session an oauth2 session. This will have a slight performance hit as the switching of the session has to deal with brief session locking and filesystem access if that is how the php sessions are configured. Redis session configurations will have less of a performance hit. 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 issues that the prior approach was introducing. |
|
Looks like I need to figure out what is going on with the API tests. My local environment is passing just fine on the api tests. I'll test this out either tomorrow or Saturday on an external server or on a vanilla install. Odd. |
| @@ -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'; | |||
There was a problem hiding this comment.
needs to also be in database.sql
There was a problem hiding this comment.
This is what was causing the api test failures. Wasn't revealed by doing tests against the demo population (openemr-cmd drid) but it is revealed with an openemr-cmd dri which does not populate the database and uses the database.sql file.
| private function getSmartAuthController(): SMARTAuthorizationController | ||
| { | ||
| if (!isset($this->smartAuthController)) { | ||
| $twigContainer = new TwigContainer(__DIR__ . "/../../oauth2/", $GLOBALS['kenerl']); |
There was a problem hiding this comment.
looks like kernel is mispelled
There was a problem hiding this comment.
Good catch lol, guess it works when its a null value lol.
|
neat stuff!
|
Fix the kernel mispelling the authorization controller.
adunsulag
changed the title
|
@bradymiller all the issues should be fixed, let me know if this looks good and I'll bring it in. |
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.
|
@adunsulag , looks great! |
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. Keeping it as draft for now in order to discuss the security implications and for a followup commit to add the database tracking to prevent replay attacks.
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.