Smart standalone patientrole migrate #4177
Conversation
Implemented a patient selector for oauth2 standalone launch. Also made it so we could have some primitive unit tests on the Gacl class with resetting the ACL cache. Wrote a smart auth class for any context / ui needs that are required as part of the oauth2 flow. Eventually if ONC requires the context-launch-encounter piece we can easily add to this class to facilitate additional selector flows. Implemented ACL checks and a search class to be used as part of the oauth2 flow for patients.
Changed fhirUser to be a Person resource instead of Practitioner as not all users are clinician staff. Implemented the Person resource and their Read only pieces. Added some logging so we could debug session problems. Fixed prior commit that was removing the Scope authorization.
Made the UI look more consistent with the login page and so it follows other styling of the login pages. Made it so the app registration can choose what scopes they are wanting to have with their app.
Updated the scope permissions so that only the ones we currently support for patient/<resource>.* is supported. Centralized the scope checks in the HttpRestRouteHandler to remove redundant code from the routes and to make sure all possible routes are checked against the access code. This also prevents developers who extend the api from accidently forgetting to check against the AccessToken. For now I've only enabled it on the FHIR api, but if it tests well we can open it up to the rest of the APIs.
|
I'll fix the style issues and stuff, just wanted to push this out there so people can see what I'm doing. |
|
@adunsulag Kidding, looks great but, i'll have to look over the code in a.m. |
|
each scope should get it's own fontawesome icon (another bad joke :) ) |
|
Only had time tonight for a quick code review, but really nice stuff!
|
Fixed some scope permission checks. Refactored the route parsing algorithm into its own class that can be unit tested against. The parsing logic could then be leveraged in the scope auth check which made matching against the REST FHIR resource a lot easier. Added the additional SMART capabilities we now support with patient standalone and launch standalone. Fixed the refresh token issues. We don't send patient context parameters as part of the refresh_grant oauth2 flow so we only send the parameters now in the authorization_grant flow inside our SMARTResponse object. There may be a better way to make this work, but for now this is functioning.
Got the client-public support working by disabling the client_challenge (PKCE support) on public profile oauth2 clients. This is required for ONC inferno testing w/ public clients. V2 of SMART is indicating this will be required back, so hopefully ONC issues an update at some point. Also made it so the offline_access works by restricting the refresh_token issued when offline_access isn't present.
|
@bradymiller @sjpadgett SMART standalone is now working with this update. We just have to have the FHIR resources implemented (I only tested with the following scopes:
I also have the client-public profile working. I had to add the following in AuthorizationController.php for the AuthGrant type (lines 606-609) in order to make the public profile work: It seems like we would want to restrict what scopes we allow (maybe anything other than patient/.) for public-client apps or provide big warning boxes on the Client Apps authorization screen that these types of applications are very insecure. I also made the Offline access work. If you don't provide offline_access in your scope request you will not get a refresh_token back as part of the authorization_code grant flow. One other thing you both might be interested in. It looks like the V2 SMART apps will refine the scopes of Read/Write to use more of a CRUDS model (Create, Read, Update, Delete, Search). I think the way we have things structured right now it won't be too challenging to move to that model: http://build.fhir.org/ig/HL7/smart-app-launch/scopes-v2-wip.html I am running into problems and maybe @bradymiller you can help me out. For some reason my docker image no longer has any of the developer code tools on them. When I attempt to run the code style cleanup tool I get the following: I checked the /root/ folder in the docker and there's nothing inside the composer. I destroyed the dockers with a docker-compose down -v and rebuilt but still getting the same behavior. This is on the easy-development dockers. Any thoughts? |
|
hi @adunsulag , |
|
@bradymiller So figured out the issue again. If you have your development machine do a composer install outside of the dockers to put dependencies in the vendor folder (so IDEs like PHPStorm can find the libraries) it blows up the docker containers which prevents the devtools from installing. I'm wondering how other people keep their IDE working w/ intellisense while also using the docker environments. Oh well. Got it working at least. |
|
agree would be nice to have a guide on how to configure docker and xdebug that enables finding the libraries for developers |
Fixing the refresh token issues broke the patient context missing due to the way the ResponseType object was cloned for the League AuthorizationController.
Implemented a logged in user being able to restrict what scopes they are giving access to for the requesting application. This allows OpenEMR users to prevent an application from giving offline_access (credentials past the 1 hour access token), and other resources to an application that they don't want to provide.
|
@adunsulag , yep, that docker environment will crash down if do any building outside the docker (ie. if the vendor directory is already there and has something in it, it will not build). I still don't "get" how the easy docker even works (ie. hides all that stuff from the repo) :) |
|
I wonder if there is a way to point it to the docker html link (like xdebug does in mapping the openemr scripts) to map the libraries there. |
|
Patients can now restrict an app's scope requests based on user selection. This meets Inferno / ONC test requirements for Limited Patient Standalone Access:
We pass the standalone limited restriction tests except for the fact we currently don't support the FHIR resources of CarePlan, Device, DiagnosticReport, DocumentReference, and Goal |
|
@bradymiller and @sjpadgett this completes all of the SMART/ONC requirements for Standalone & Launch in EHR for patient and clinical uses. I'm moving onto the Flat FHIR (system level scopes) for SMART system apps. That looks pretty involved so I'll probably break that out into its own PR and I may not get anything out on it until next week (I'm off Thursday/Friday). So if you want to review this that'd be great. I haven't provided a sample standalone app on this due to time constraints (trying to get the Bulk FHIR done), but if you need that in order to review this let me know and I can work on that. I'm assuming any of the SMART sample apps will work that are out there as you no longer need to pass in the 'api:fhir' scope to make the SMART apps work. |
|
|
interface/smart/register-app.php
Outdated
|
|
||
| <div class="form-group"> | ||
| Scopes Requested: |
There was a problem hiding this comment.
xlt (place the colon outside the translation)
oauth2/smart/patient-select.php
Outdated
| (function(window) { | ||
| window.opener = null; | ||
| })(window || {}); | ||
| <?php require($GLOBALS['srcdir'] . "/restoreSession.php"); ?> |
There was a problem hiding this comment.
this seems odd. was this needed for something?
There was a problem hiding this comment.
no, originally this was in a dialog. Will remove.
| <div class="row w-100"> | ||
| <div class="col"> | ||
| <?php if (!empty($errorMessage)) : ?> | ||
| <p class="alert alert-warning"><?php echo xlt($errorMessage); ?></p> |
There was a problem hiding this comment.
where are these error messages coming from (just odd to translate them after the fact like this)?
There was a problem hiding this comment.
src/RestControllers/SMART/SMARTAuthorizationController.php line 213.
oauth2/smart/patient-select.php
Outdated
| value="<?php echo attr($mname); ?>" /> | ||
| <input class="w-25" name="search[lname]" type="text" class="form-control form-input" placeholder="<?php echo xlt("Last Name"); ?>" | ||
| value="<?php echo attr($lname); ?>" /> | ||
| <input type="submit" value="Search" /> |
There was a problem hiding this comment.
if this is a button, it needs to be translated
oauth2/smart/patient-select.php
Outdated
| <input type="submit" value="Search" /> | ||
| </form> | ||
| <?php if ($hasMore) : ?> | ||
| <p class="alert alert-info"><?php echo xlt("Too many search results found. Displaying a limited set of patients. Narrow your search results through the filters above."); ?></p> |
There was a problem hiding this comment.
only use single spacing in constants (double spacing can get messed up in the translation pipeline)
oauth2/smart/patient-select.php
Outdated
| <?php echo $patient['sex']; ?> | ||
| </td> | ||
| <td> | ||
| <?php echo $patient['email']; ?> |
There was a problem hiding this comment.
text() here and in all above places (line 91 to here) where are outputting $patient variables
oauth2/smart/patient-select.php
Outdated
| <?php echo $patient['email']; ?> | ||
| </td> | ||
| <td> | ||
| <button data-patient-id="<?php echo $patient['uuid']; ?>" class="btn btn-primary patient-btn">Select patient</button> |
There was a problem hiding this comment.
attr() $patient
xlt() the Select Patient
oauth2/smart/patient-select.php
Outdated
| <!-- <label for="persist_login" class="form-check-label">--><?php //echo xlt("Remember Me"); ?><!--</label>--> | ||
| <!-- </div>--> | ||
| <!-- </div>--> | ||
| <!-- </div>--> |
There was a problem hiding this comment.
is there plan to use this commented out code in future. if not, then would remove it.
oauth2/smart/patient-select.php
Outdated
| var target = evt.target; | ||
| var patientId = target.dataset.patientId || undefined; | ||
| if (!patientId) { | ||
| console.error("<?php echo xlt("Developer error. Patient id is missing from dataset");?>)"); |
oauth2/smart/patient-select.php
Outdated
| } | ||
| var patientInput = document.getElementById('patient_id'); | ||
| if (!patientInput) { | ||
| console.error("<?php echo xlt("Developer error missing hidden form element 'selectedPatient'");?>)"); |
There was a problem hiding this comment.
xls is the javascript escape function?
There was a problem hiding this comment.
actually, that is wrong. that is the "old" function. Use xlj() which will translate/escape in javascript context
(the key here is when using this is to have quotes around it which you have already)
https://github.com/openemr/openemr/blob/master/library/htmlspecialchars.inc.php#L177
(js_escape basically calls js_encode)
oauth2/smart/patient-select.php
Outdated
| // now submit our form. | ||
| let form = document.getElementById('patientForm'); | ||
| if (!form) { | ||
| console.error("<?php echo xlt("Developer error missing form 'patientForm'");?>)"); |
| $routeControllerParameters = $parsedRoute->getRouteParams(); | ||
| $routeControllerParameters[] = $dispatchRestRequest; // add in the request object to everything | ||
| // call the function and use array unpacking to make this faster | ||
| $result = $routeCallback(...$routeControllerParameters); |
There was a problem hiding this comment.
neat! the splat operator:
https://blog.programster.org/php-splat-operator
bradymiller
added
Status: Needs Review
Status: Needs Work
and removed
Status: Needs Work
labels
| $scopes_api['user/' . $scopeRead] = 'user/' . $scopeRead; | ||
| $scopes_api['system/' . $scopeRead] = 'system/' . $scopeRead; | ||
| break; | ||
| case 'insert': | ||
| case 'update': | ||
| $scopes_api['patient/' . $scopeRead] = 'patient/' . $scopeRead; |
There was a problem hiding this comment.
@bradymiller I was just looking at this and this came from your change requests, do we want this to be $scopeRead? Shouldn't it be $scopeWrite?
There was a problem hiding this comment.
yep, i messed up that line
There was a problem hiding this comment.
btw, I will fix this in code currently working on from this PR
| @@ -864,6 +955,7 @@ private function deserializeUserSession(): AuthorizationRequest | |||
| $authRequest->setCodeChallenge($outer['codeChallenge']); | |||
| $authRequest->setCodeChallengeMethod($outer['codeChallengeMethod']); | |||
| } catch (Exception $e) { | |||
| // TODO: @adunsulag check with @sjpadgett was just echoing the exception what you wanted to happen here? | |||
There was a problem hiding this comment.
@sjpadgett question for you here, in AuthorizationController I see we are just echoing out the exception, is this what you wanted as part of the oauth2 flow?
| * @author Yash Bothra <yashrajbothra786gmail.com> | ||
| * @copyright Copyright (c) 2018 Jerry Padgett <sjpadgett@gmail.com> | ||
| * @license https://github.com/openemr/openemr/blob/master/LICENSE GNU General Public License 3 | ||
| */ |
There was a problem hiding this comment.
Going to merge these two comments, as I copied PersonRestController from PractioninerRestController
| $resource = $request->getResource(); | ||
|
|
||
| if (!$request->isFhir()) { | ||
| $resource = strtolower($resource); |
There was a problem hiding this comment.
@adunsulag ,
I'm doing some work off the code in this PR to extend your scope checking mechanism to entire api. Is there anything specific that requires the strtolower? (I couldn't figure it out)
There was a problem hiding this comment.
disregard above question. turns out the real function for scope checking is at checkSecurity, so just ignore my post above.
There was a problem hiding this comment.
My bad, I should probably remove this function, originally I was using it, but I centralized it all into the dispatch. I had the strtolower because I noticed all of the resource names in the standard API were lowercased while the FHIR api started with uppercase.
|
@adunsulag , |
|
@bradymiller A public client only needs to know their client identifier in order to establish an oauth2 authorized session. There's an additional RFC to make public client's more secure called PKCE but the ONC inferno testing tool does not support PKCE authentication so we've turned it off. If I remember when I was testing this manually, A public app will authorize by only sending their client identifier in the username field of the HTTP Basic Auth (skipping the client_secret that is sent as the password) to the token endpoint /default/token |
|
@bradymiller if you can tag me when you pull in this PR, or if I need to make any more changes just let me know. I have another chunk to push out with the Bulk FHIR Client Credentials implementation. I can add it to this PR, or make a new PR for it and rebase when you pull in this one. |
|
@adunsulag , haven't been able to test the SMART standalone, but gonna bring this in to keep things moving (plan to test/document SMART standalone in near future). Otherwise appears to be testing well (and the other PR is also working well). Gonna bring this in now and follow it with the other PR. |
This is a working PR with ongoing implementation of the SMART standalone piece.
Changes include
Here's some pictures.
Patient Selector
App Registration Scope Selection
Fixes to make SMART Standalone Work
(This is with the following scopes
launch/patient openid fhirUser offline_access patient/Patient.readMore work to do on the other items:
