FI-4186: UDAP Attestation tests#41
Conversation
ChristineDuong
force-pushed
the
fi-4186-attestation-tests
branch
from
147317b to
4f3bcb2
Compare
edeyoung
left a comment
edeyoung
left a comment
There was a problem hiding this comment.
Please address the ruby linter errors and the items in the comments.
I'll do another pass after.
Thanks!
| @@ -0,0 +1,45 @@ | |||
| module UDAPSecurityTestKit | |||
| class AuthorizationCodeUsageAttestationTest < Inferno::Test | |||
There was a problem hiding this comment.
This class name is duplicated, which is causing the code to fail to run.
This class name must be unique.
| - Ensuring the authorization code is not used more than once. | ||
| - Requesting an authorization code as per Section 4.1.1 of RFC 6749. | ||
| ) | ||
| verifies_requirements 'hl7.fhir.us.udap-security@136', |
There was a problem hiding this comment.
hl7.fhir.us.udap-security should be hl7.fhir.us.udap-security_1.0.0
You can see this in the excel file in the metadata tab:
It's also referenced in the suite lib/udap_security_test_kit.rb:
requirement_sets(
{
identifier: 'hl7.fhir.us.udap-security_1.0.0',
title: 'Security for Scalable Registration, Authentication, and Authorization (UDAP)',
actor: 'Server'
}
)
There was a problem hiding this comment.
One way to check that you're referencing things as expected is to look at the coverage document lib/udap_security_test_kit/requirements/generated/udap_security_client_requirements_coverage.csv. You can see in the client coverage document that there are no test ids associated with these requirements ids
edeyoung
left a comment
edeyoung
left a comment
There was a problem hiding this comment.
Added comments based on our discussion yesterday
| @@ -0,0 +1,45 @@ | |||
| module UDAPSecurityTestKit | |||
| class AuthorizationCodeUsageAttestationTest < Inferno::Test | |||
| title 'Authorization code is used correctly' | |||
There was a problem hiding this comment.
Per discussion, make titles verb first: "Uses authorization code correctly"
Change throughout
| verifies_requirements 'hl7.fhir.us.udap-security@184' | ||
|
|
||
| input :access_token_lifetime_correct, | ||
| title: "Access tokens have a lifetime of no longer than 60 minutes", |
There was a problem hiding this comment.
Per discussion, put section title here and then colon:
title: "Authorization Code and Token Requests: Limits access token lifetime",
Repeat throughout
| @@ -0,0 +1,40 @@ | |||
| module UDAPSecurityTestKit | |||
| class AccessTokenLifetimeAttestationTest < Inferno::Test | |||
| title 'Access tokens have a lifetime of no longer than 60 minutes' | |||
There was a problem hiding this comment.
Per discussion, make verb first. An idea here is "Limits access token lifetime"
| title 'Authorization code is used correctly' | ||
| id :udap_security_client_auth_code_usage | ||
| description %( | ||
| Client applications SHALL use the authorization code correctly by: |
There was a problem hiding this comment.
Per discussion, description here should align with the input description, removing "I attest..." text.
Change throughout.
edeyoung
left a comment
edeyoung
left a comment
There was a problem hiding this comment.
I've requested a few formatting / viewing changes.
I've also noticed that several tests are uncovered. We don't need to get to 100% coverage, but it would be good to note which SHALL/SHALL NOT requirements are uncovered and why. This could be recorded for now in the "Notes" column of the excel spreadsheet.
Server-side organization makes sense to me. The only possible change would be the error handling ones as those are cross-cutting, and I could see them making more sense in their parent groupings, but I can see it either way so it can stay as is.
|
Looks good other than lack of coverage around requirements 258-269. Is there a reason these aren't covered? If so, document (excel file is ok for now). Same with all other SHALL/SHALL NOT requirements. |
They were covered in AuthenticationRequestValidationAttestationTest ( |
2 participants
Summary
This PR adds attestation tests for the UDAP Security Test Kit. Tests are based on requirements that can be found here; tests are added based on not having existing coverage in generated coverage csv.
Tests are added to both client and server suite.