Openemr api route refactor #7
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adunsulag
force-pushed
the
openemr-api-route-refactor
branch
from
9c85ea9 to
45dcc9b
Compare
allow duration to be 0 for all day event scheduling.
* chore: eye form escaping * database fixes * remove database changes from eye mag functions * pst * eye form parent category * make room for new eye form folders in db.sql * eye form is parent category * re-number new folders * function to insert new categories * simplify sql statement for affected categories * root node and eye form fixes * rght
This reverts commit 51e04f4.
* fix(Installer): type annotations * test(Installer) * doc(Installer): docblocks * style(InstallerTest) * test(InstallerTest) * test(Installer): install_additional_users * chore(Installer): mysqli_fetch_array wrapper * test(Installer): on_care_coordination * test(Installer): get_initial_user_mfa_totp * chore(Installer): wrappers for unlink, glob * test(Installer): create_site_directory * chore(Installer): wrapper for `touch` * chore(Installer): wrapper for `fwrite` * test(Installer): write_configuration_file * chore(Installer): wrap GACL for mocking * test(Installer): install_gacl * chore(Installer): no coverage for escapeSql * chore(Installer): reorder mock wrappers at end * test(Installer): no auto-mock connect_to_database * test(Installer): mock die * test(Installer): test some of quick_install * test(Installer): execute_sql * test(Installer): connect_to_database * test(Installer): theme methods * refactor(InstallerTest): rector * style(InstallerTest): whitespace
* feat(Installer): provide for arbitrary globals * style(README-Isolated-Testing.md): whitespace * test(Installer): upsertCustomGlobals
This is my initial stab at refactoring the api system and using the symfony kernel framework
Wrote up a ApiApplication class that does all of the api bootstrapping and setup enabling us to test it with a smoke screen test. This identified a number of errors and so I was able to fix it. had to modify the .htaccess to work like the symfony .htaccess instead of passing the path as a URL argument, we treat it like a path which makes the URI parsing from symfony work properly. Unit tests have to be currently executed like this: ``` ./vendor/bin/phpunit -c phpunit.api.xml --testsuite api --filter ApiApplicationTest ``` Since the application bootstraps the entire globals.php, we can't use the standard phpunit config which starts the globals.php as part of bootstrap otherwise we end up with double session problems.
Fixed the site path so skipped locations are still skipped. Would still prefer to have just an overall single firewall setup but for now we will leave this as is and hopefully we can replace it in a few months. Telemetry was force exiting so wasn't seeing my phpunit results. Fixed that also
Changed the fhir metadata endpoint to no longer api log since handled by listener. Moved apiType url check methods (isFhirRequest, isApi, etc..) to HttpRestRequest method so can be used throughout the application. Added helper method HttpRestRequest::getRequestPathWithoutSite so we can grab the suffix path after the site. Added an ApiResponseLoggerListener to handle all of the route logging that is scattered throughout the routes. Made the SkipAuthorizationStrategy populate the userId attributes and other request instance attributes so we track the user in the request even if they are requesting a non-authenticated request. Values are set to null if user is not authenticated. Fixed up the FhirMetadataRestController to not rely on RestConfig since we can't easily pull it in via PSR. Fixed up the other listener classes so that we finally have a working implementation from the dispatch.php refactoring.
Got the bearer token to work for standard api request on facility but discovered that the RestConfig is making it an absolute chore to refactor the apis due to how it isn't PSR compliant includes a bunch of files and changes some of the global values. I'm going to commit this chunk and see if I can refactor the authorize.php endpoint to use the same AppApplication and handle the authentication pieces via the AuthorizationController without needing to depend on the RestConfig. I also realized that we're pretty much duplicating a ton of work in the ScopeRepository by checking the scopes against the _rest_routes. It keeps things in sync but its a ton of extra processing pile that is done on every single request. Instead if we just force people to put in scopes in the ScopeRepository at the same time they add an endpoint we can eliminate a ton of processing on every request which should speed things up quite a bit when getting access tokens, registering, etc.
Put in the ApiApplication using the OEHttpKernel for the authorize.php system. Introduced a OAuth2AuthorizationListener that intercepts the kernel request and provides all of the oauth2 handling that was being done in AuthorizationController. Currently we have to figure out how to handle the session & cookies in AuthorizationController because they use SessionUtil a singleton, we can't handle the testability components of the request/response that the rest of the system relies on. This is also creating issues with the oauth2 logout piece so we have one test failing in the ApiTestClientTest class. PSR style fixes in this as well. Fixed a number of bugs in the ScopeRepository. Also got all of the api test suites working for Facility, Patient, and Practitioner.
ADded in an integration testing that does not run the regular bootstrap.php which in turns includes the globals. Since we are running globals through the site setup listener for our api application we don't want to use the regular phpunit process. However, ran into an issue where the require_once makes it so I can only have a single test that uses the SiteSetupListenerTest as the require_once will never run again. We need to figure out how to abstract the globals initialization steps better so we can run the setup process multiple times across integration tests without having to do a full e2e setup.
Implemented the oauth2 and api sessions in the session factory and the session cleanup. I think we may still have some outstanding issues in the AuthorizationController class but until we can merge this in with brady's pr on session cleanup I won't worry about it.
Removed AuthorizationController dependency on SmartConfigurationController Refactored globals.php to use a container object that we can use for test dependency injection, classes should now use the OEGlobalsBag object for referencing global parameters / configuration. This improves testability and allows for items to be mocked/stubbed out much easier. Fixes openemr#8646. Changed UserService::getAuthGroupForUser from static to instance method so we can mock the behavior in testing. Refactored OAuth2 grants to use SessionInterface so we can mock the session for testing. Changed up the ScopeRepository to handle both v1 and v2 SMART on FHIR scopes for openemr#8639. We still need to handle the actual scope verification piece which I'll be working on next once I've verified the SMART v1 and us core 3.1.1 are unaffected with these changes. Changed up CSRFUtils to work with the SessionInterface so we can use it in testing. Fixed up the HttpSessionFactory so we could keep it backwards compatible with the existing session management pieces. Added the OEGlobalsBag to the OEHttpKernel so we can grab the configuration variables wherever we need, but still have it as a test hub for mocking. Removed the SystemConfigurationContext as I couldn't see it used anywhere. Added ServerConfig::getJsonWebKeySetUrl as an instance method so we can use it in the SmartConfiguration. Updated the SMART on FHIR Capability well-known configuration to handle all of the v2 requirements. Fixes openemr#8642. Fixed up a lot of the BearerTokenAuthorizationStrategy so we could make it more testable. This was originally refactored from the dispatch.php class and can definitely have a lot more improvements as the class is just doing too much (as evidenced by how much we have to mock dependencies in the test classes). Major refactoring of AuthorizationController so we can use SessionInterface and the OEGlobalsBag to make the class testable and use the data that comes from the HTTPKernel instead of global variable and session state. Changed the class up to always return response objects instead of exiting anywhere in the class, again so we can test and have the class sit inside of the api request/response lifecycle.
The v1 compatability layer on ScopePermissionObject was not working properly once hooked up to the scope check on the specific api access with the v2 scopes (cruds syntax vs read/write). Changed up the ScopePermissionObject to handle multi-operation scopes so the scopeContains operation in ScopeEntity works properly. Fixed the SkipAuthorization and LocalApi authorization pieces as well as requests were failing against metadata / version, product endpoints. Populated the resource Instance Identifier in the HttpRestParsedRoute so we can differentiate on a FHIR 'read' operation vs a FHIR 'search' operation. Kept the same methodology for the standard api. Fixed a number of phpstan issues in the HttpRestRequest class as well as in some consumers of the class. Marked HttpRestRequest::setAccessTokenScopes as deprecated so we can work with ScopeEntity objects and not strings. Set a _route property in the HttpRestRequest->attributes that will have the valid parsed HttpRestParsedRoute with all of its attributes. Set HttpRestRequest to use the scope validator factory method instead of handling the logic manually inside the class.
The dispatch.php was using an invalid function for sending the status code.
Unit tests for the v2 single api test suite which is now passing.
HttpRestRequest::requestSite was not being set, now set to 'default' by default. BearerTokenAuthorizationStrategy's deferred AccessTokenRepository instantiation was failing the test. Changed it up so we can inject the dependency for testing. Made the EventAuditLogger injectable for the BearerTokenAuthorizationStrategy.
Fixed rector function return issues. Need to figure out why the site_addr_oath is being set to an empty string on the ci engine.
Added tests for granular scope permissions that will modify the request query to bind a resource request to a specific query search string. This will override any user specified queries that would go beyond the granular condition.
Added OEGlobalsBag to SMARTSessionTokenContextBuilder in order to remove the superglobals that are used there. Changed up the UserEntity class to have an injected fhirBaseURL so we can populate the fhirUser claim properly and remove the superglobals reference. Changed up CustomRefereshTokenGrant to throw an exception if the context can't be decoded. Changed up IdTokenSMARTResponse to remove superglobals reference, built out a unit test and integration test for the class to verify it is functioning properly. Deprecated IdentityRepository and added the corresponding methods to UserRepository since they both instantiate UserEntity objects for OIDC/OAuth2 purposes. This should fix the failing unit tests due to the fact the site_address_oauth property was not being set.
They didn't show up on the local box... so odd.
Fixed the sessions using a builder class to consolidate the session information. Rebased onto master to deal with conflicts. Added PATCH to CORS list. Fixed twig escaping issues.
adunsulag
force-pushed
the
openemr-api-route-refactor
branch
from
539b422 to
9b03630
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
I never modified these files but rector is complaining about them so went ahead and made the proposed suggestions. I verified calendar is still loading fine so I believe this is safe.
Based on PR review, removed unused static variables and default setting options in the Session classes. Added back in the php code coverage vendor libraries that got removed in the rebase.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a major WIP where I am working on refactoring the routes so I can have individual route files for the various FHIR apis. Also so I can actually write out unit tests against the API for test ability and to make sure I don't break anything going forward.