FI-2247 backend services migration #59

alisawallace · 2023-12-12T00:33:13Z

Summary

Migrating the Bulk Data Authorization tests into the SMART App Launch test kit. Bulk Data test files were copied from the ONC (g)(10) Certification Test Kits, including this PR from an external contributor, and now live under a Backend Services group for STU2.

Updates From First Round of Feedback

Test Structure
- Backend services test group is now located after EHR launch and before Token Introspection group
- Backend services test group includes SMART on FHIR Discovery tests as required by the IG, which will provide the token endpoint input
- Individual tests now live in their own files
- Eliminated redundant behavior between ClientAssertionBuilder and AuthenticationRequestBuilder
  - Renamed AuthenticationRequestBuilder to BackendServicesAuthorizationRequestBuilder and had it use ClientAssertionBuilder
Inputs
- Backend services tests now reuse an existing input for encryption method from standalone launch tests using confidential asymmetric client authorization, rather than having a separate, redundant input
- Inferno Reference Server STU2 preset updated to include backend_services_client_id
  - Value was taken from ref server preset used on inferno.healthit.gov for bulk data authorization
  - All other inputs are either covered by default values or pre-existing presets
Naming, Descriptions, & Requirements
- Replaced all references to (g)(10) and/or bulk data with backend services in test names, inputs, and descriptions
- Ensured all tests adhere to backend services STU 2.0.0 IG requirements and do not contain requirements specific to (g)(10) or bulk data

Testing Guidance

Run the test kit locally per the README instructions
Navigate to http://localhost:4567 in a web browser
Select SMART App Launch STU2
Select Inferno Reference Server as preset
Select Group 3 Backend Services in the left-side menu, then Run Tests
Run the tests - all tests should pass

Jammjammjamm · 2023-12-14T13:41:44Z

lib/smart_app_launch/authorization_request_builder.rb

+require 'json/jwt'
+
+module SMARTAppLaunch
+  class AuthorizationRequestBuilder


I would hope that this and ClientAssertionBuilder could be combined, as backend services authorization is based on the asymmetric client credentials authorization method. If we need a specific class to handle this for backend services, the class name should reflect that.

ClientAssertionBuilder and AuthorizationRequestBuilder address different scopes and use cases for the tests that use them. AuthorizationRequestBuilder, used for Backend Services, is too broad for use in standalone/EHR tests, and ClientAssertionBuilder is too narrow for Backend Services (tests would have duplicated code across them that AuthRequestBuilder addresses).

So I opted to keep both classes but had AuthRequestBuilder use ClientAssertionBuilder in building the requests, so there isn't redundant behavior between the two classes. I also renamed AuthRequestBuilder to be specific to Backend Services, since those are the only tests that use it.

Jammjammjamm

Why isn't this being merged into main?

The preset needs to be updated. This functionality needs unit tests.

Jammjammjamm · 2023-12-14T13:48:33Z

lib/smart_app_launch/smart_stu2_suite.rb

@@ -222,5 +223,7 @@ class SMARTSTU2Suite < Inferno::TestSuite

    group from: :smart_token_introspection

+    group from: :smart_backend_services


I think this should go before the token introspection tests.

Updated so backend services shows up before token introspection tests.

Jammjammjamm · 2023-12-14T13:51:06Z

lib/smart_app_launch/backend_services_group.rb

+    test from: :tls_version_test do
+      title 'Authorization service token endpoint secured by transport layer security'
+      description <<~DESCRIPTION
+        [§170.315(g)(10) Test


This shouldn't contain anything g10 specific. Is this a backend services requirement or just a g10 requirement?

This is a backend services requirement too, so I left it, but updated all naming so there isn't anything g10 specific.

Jammjammjamm · 2023-12-14T13:52:47Z

lib/smart_app_launch/backend_services_group.rb

+            The OAuth 2.0 Token Endpoint used by the Backend Services specification to provide bearer tokens.
+          DESCRIPTION
+    input :bulk_client_id,
+          title: 'Bulk Data Client ID',


These titles should be updated as well.

All titles/inputs/descriptions have been updated so they no longer refer to bulk data.

Jammjammjamm · 2023-12-14T13:56:50Z

lib/smart_app_launch/backend_services_group.rb

+      )
+    end
+
+    test do


These tests should all be moved into their own files and given good ids.

Each test was moved into its own file and given a descriptive ID.

Jammjammjamm · 2023-12-14T13:57:52Z

lib/smart_app_launch/backend_services_group.rb

+
+    id :smart_backend_services
+
+    input :bulk_token_endpoint,


The input ids should be updated to reflect that these are backend services inputs not bulk data inputs.

All inputs were updated.

alisawallace · 2023-12-15T03:11:28Z

Why isn't this being merged into main?

The preset needs to be updated. This functionality needs unit tests.

@Jammjammjamm I mentioned in the PR summary that I split the original 2247 ticket into two tickets, and updated the definition of done for this one to be that the bulk data/backend services tests properly show up in the SMART test kit and that the tests pass with the reference server. Everything else beyond this (renaming, refactoring, unit tests, etc) will be addressed in the next ticket, 2369, that is still in progress. I set up an interim working branch to hold everything until the effort is completed. Rob and I used this approach for token introspection since we kept having to break down the tasking further, and it was helpful for me get PR feedback before things were ready for main. Maybe backend services will be more straightforward and this won't end up being as useful, but that was my thought process.

Thank you for the feedback though - it'll be very helpful as I get started on the next ticket. I will work through each one and reach out if I have questions.

Jammjammjamm · 2023-12-15T16:41:38Z

I don't think this work justifies a separate non-ephemeral branch. Rather than merging this into a separate branch, this should be a draft PR targeting main, and FI-2369, to the extent that it lines up with my comments, should be incorporated into this branch.

Any work on enhancements needed to expand the functionality and make it more generic that were envisioned as part of FI-2369 would make sense as a separate ticket, but not the sort of organizational changes I'm requesting here. I'd be happy to discuss this further on slack if you like.

Note that since `smart_token_url` is an input value set from the output of another test suite the preset does not properly apply. Need to determine how we want to handle this.

This reverts commit 0525bc0.

…inate redundant behavior

…messaging

This class is currently used exclusively in backend services tests, so the class name now reflects that.

alisawallace · 2024-01-11T22:22:55Z

@Jammjammjamm I have addressed all of your PR feedback at this point except unit tests, so the PR is still a draft but is ready for re-review. I'll get started on unit tests while waiting for feedback.

Source: https://github.com/onc-healthit/onc-certification-g10-test-kit/blob/main/spec/onc_certification_g10_test_kit/bulk_data_authorization_spec.rb

Jammjammjamm · 2024-01-19T14:30:46Z

config/presets/inferno_reference_server_stu2_preset.json

@@ -41,7 +41,7 @@
      "value": null
    },
    {
-      "name": "client_auth_encryption_method",
+      "name": "asymm_conf_client_encryption_method",


I don't think we should change this input name.

Name has been reverted back to client_auth_encryption_method

Jammjammjamm · 2024-01-19T14:36:28Z

lib/smart_app_launch/backend_auth_request_success_test.rb

+    id :smart_backend_services_auth_request_success
+    title 'Authorization request succeeds when supplied correct information'
+    description <<~DESCRIPTION
+      [The SMART Backend Services IG STU 2.0.0](https://hl7.org/fhir/smart-app-launch/STU2/backend-services.html#issue-access-token)


There is no SMART Backend Services IG. There's only the SMART App Launch IG.

I updated the link titles to be "SMART App Launch 2.0.0 specification for Backend Services" instead.

Jammjammjamm · 2024-01-19T14:47:03Z

lib/smart_app_launch/smart_stu2_suite.rb

@@ -220,6 +221,23 @@ class SMARTSTU2Suite < Inferno::TestSuite
            }
    end

+    group do
+      title 'Backend Services'
+      id :smart_full_backend_services


This is awkward. I think the subgroup should be something like "Backend Services Authorization" to avoid this.

Changed main suite group to smart_backend_services and the subgroup to smart_backend_services_authorization

Jammjammjamm · 2024-01-19T14:47:51Z

lib/smart_app_launch/token_exchange_stu2_test.rb

@@ -1,5 +1,6 @@
 require_relative 'client_assertion_builder'
 require_relative 'token_exchange_test'
+require_relative 'backend_services_group'


Why does this test require the backend services group?

It doesn't, it was accidentally leftover from when I was testing some inputs and has been removed now.

Jammjammjamm · 2024-01-19T14:56:50Z

spec/smart_app_launch/backend_services_group_spec.rb

+    Inferno::TestRunner.new(test_session:, test_run:).run(runnable)
+  end
+
+  describe '[Invalid grant_type] test' do


Is there a reason for these specs to be here rather than in a spec for the specific tests?

I copied and pasted the specs from the ones already present in the (g)(10) test kit, found in a single file here, and just made some minor naming updates for them to pass. If they need to be refactored and separated out to separate files I can do so.

Yeah, they should each have their own file.

Spec tests have been refactored so each backend services test/class has its own spec file.

Jammjammjamm · 2024-01-19T14:59:30Z

spec/smart_app_launch/client_assertion_builder_spec.rb

+
+        expect {
+          build_and_decode_jwt(encryption_method, kid)
+        }.to raise_error(RuntimeError)


Raising generic errors is something we try very hard to avoid in tests. What are the circumstances where this error could appear?

I included the underlying exception, found here, to support the new functionality of making the kid of the JWKS a user input (from this PR). I found that when I entered a valid kid but accidentally specified the wrong algorithm I got a somewhat cryptic error "undefined method signing_key for nil:NilClass" in client_assertion_builder.rb. What was happening was that the signing_key method of that class was returning a null object as a result of there being no match for the specified kid and algorithm input when searching the key set. I catch an exception in this case to communicate that to the user.

Maybe this doesn't warrant the custom error message? Or if it does, there's a better way to structure the exception? I'm not very familiar with this in Ruby.

So this error would be the result of bad inputs? If so, it should be an Inferno::Exceptions::AssertionException. If this would be caused by private_key being nil, the intention would be clearer by having a guard against it being nil that raises this exception rather than rescuing NoMethodError.

Thanks! Yes, this is a result of bad inputs. I've updated this to check for a nil private_key and to raise Inferno::Exceptions::AssertionException instead.

Jammjammjamm · 2024-01-19T15:45:29Z

lib/smart_app_launch/backend_auth_request_builder.rb

+    end
+
+    def client_assertion
+      @client_assertion ||= ClientAssertionBuilder.build(


Jammjammjamm · 2024-01-19T15:52:06Z

lib/smart_app_launch/backend_auth_request_builder.rb

+require_relative 'client_assertion_builder'
+
+module SMARTAppLaunch
+  class BackendServicesAuthorizationRequestBuilder


All of these file names should be the lower snake case version of the class name.

I updated all the file names (though some of them are pretty long now, hopefully that's okay).

alisawallace · 2024-01-22T23:05:59Z

@Jammjammjamm I addressed all of your comments and am ready for re-review. The following items in particular could use your input:

If I need to put all the spec tests in separate files or can leave them grouped
Whether my approach of raising an exception to clarify bad kid input is warranted/could be improved.

Thanks!

alisawallace · 2024-01-30T19:43:36Z

@Jammjammjamm I addressed the remaining comments and am ready for re-review:

Changed the except raised for bad kid input to your suggestion
Refactored all spec tests for backend services so each test/class has their own file

Confirmed that all spec tests are passing and tests in the browser pass with reference server preset. Thanks!

alisawallace added 3 commits December 11, 2023 15:05

First pass at backend services refactor, page loads but tests fail

0cc043b

Included missing JSON file, all tests pass with ref server

fcc900a

Changed input name to use existing SMART token endpoint

b35b418

alisawallace requested a review from emichaud998 December 12, 2023 00:33

Jammjammjamm reviewed Dec 14, 2023

View reviewed changes

alisawallace changed the base branch from backend-services to main December 19, 2023 16:51

alisawallace marked this pull request as draft December 19, 2023 16:52

alisawallace removed the request for review from emichaud998 December 19, 2023 16:53

alisawallace added 17 commits December 19, 2023 15:49

Moved Backend Services tests before Token Introspection tests

6293689

WIP refactor

5c002fd

Moved backend services tests to individual files

261b182

Renamed inputs to reflect general backend services instead of bulk data

39429fe

Updated test descriptions and links

e3d251b

Updated presets for backend services.

25d947a

Note that since `smart_token_url` is an input value set from the output of another test suite the preset does not properly apply. Need to determine how we want to handle this.

Added SMART on FHIR Discovery to Backend Services group

927e21e

Removed smart_token_url as a preset

b8c1941

Added SMART to group title

0525bc0

Revert "Added SMART to group title"

24af3f5

This reverts commit 0525bc0.

Made common input for asymmetric confidential client encryption method

1df356e

Added SMART toBackend Services group name

7482711

Added ClientAssertionBuilder into AuthorizationRequestBuilder to elim…

bf35427

…inate redundant behavior

Updated input used in token introspection test with new name

bc931bc

Added kid as parameter for ClientAssertionBuilder and improved error …

4cec6be

…messaging

Fixed exception so error is properly propogated to calling object

1b87257

Renamed AuthorizationRequestBuilder to include backend services

b725194

This class is currently used exclusively in backend services tests, so the class name now reflects that.

alisawallace requested a review from Jammjammjamm January 11, 2024 22:23

alisawallace added 9 commits January 11, 2024 16:39

Fixed bug with kid in ClientAssertionBuilder

00e4459

WIP refactored ClientAssertionBuilder spec tests

ecf8270

Exception test for ClientAssertionBuilder

73f5a4b

Fixing input name references for token exchange stu2 test

d747c71

Stub for backend auth request builder spec

f25c9e0

Copy/paste of bulk data auth tests from g10

afb00c0

Source: https://github.com/onc-healthit/onc-certification-g10-test-kit/blob/main/spec/onc_certification_g10_test_kit/bulk_data_authorization_spec.rb

Updated c/p spec to work for backend services group

a18707b

Updated value of expected error message, all tests pass

670c5c9

Spec test for BackendServicesAuthorizationRequestBuilder

754a8f6

alisawallace marked this pull request as ready for review January 17, 2024 21:34

Jammjammjamm reviewed Jan 19, 2024

View reviewed changes

alisawallace added 5 commits January 22, 2024 09:41

Correcting references to SMART App Launch IG in descriptions

f6d9d1a

Improved suite and group names for backend services

e94a0bc

Fixed rspec test references

5c76829

File names andclass names aligned

871a10e

Reverted authentication method input name back to original

a807acb

alisawallace requested a review from Jammjammjamm January 22, 2024 23:06

alisawallace added 7 commits January 22, 2024 17:10

Fixed bug in which exp and jti values not included in client assertion

f0c762e

WIP spec refactor, individual test won't pass

d6252f2

Improved client assertion exception for bad inputs

5632d71

Individual backend services spec test working

633c774

All backend services specs refactored to individual tests/files

3913d4b

Cleared up confusion from duplicated input

04c7092

Removed pry references

e517050

Jammjammjamm approved these changes Jan 31, 2024

View reviewed changes

alisawallace merged commit c4340a8 into main Jan 31, 2024
2 checks passed

alisawallace deleted the FI-2247-backend-services-migration branch February 1, 2024 23:30

		@@ -222,5 +223,7 @@ class SMARTSTU2Suite < Inferno::TestSuite

		group from: :smart_token_introspection

		group from: :smart_backend_services

+                    )
+                  end
+                  test do

FI-2247 backend services migration #59

FI-2247 backend services migration #59

Conversation

alisawallace commented Dec 12, 2023 • edited Loading

Summary

Updates From First Round of Feedback

Testing Guidance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jammjammjamm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisawallace commented Dec 15, 2023

Jammjammjamm commented Dec 15, 2023

alisawallace commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisawallace commented Jan 22, 2024

alisawallace commented Jan 30, 2024

alisawallace commented Dec 12, 2023 •

edited

Loading