AUT-3916: Adding integration tests for MFA Reset #5693
Conversation
This file contains 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
andrew-moores
force-pushed
the
aut-3916
branch
3 times, most recently
from
6d11ab4 to
d63b171
Compare
andrew-moores
changed the title
andrew-moores
force-pushed
the
aut-3916
branch
4 times, most recently
from
8ad58b3 to
4d3961b
Compare
andrew-moores
changed the title
andrew-moores
force-pushed
the
aut-3916
branch
from
4168ef1 to
e3f45c4
Compare
andrew-moores
force-pushed
the
aut-3916
branch
from
e3f45c4 to
1c80b21
Compare
BeckaL
reviewed
Jan 7, 2025
|
|
||
| @SystemStub static EnvironmentVariables environment = new EnvironmentVariables(); | ||
|
|
||
| private static String expectedKid; |
There was a problem hiding this comment.
V minor but I have a pref for spelling this out in full e.g. "expectedKeyId"
There was a problem hiding this comment.
kid is the correct name for this as its a hash of the AWS key id and not the AWS key id itself, see line 117. I agree thats not very clear from the test but it is per the spec. Would something like expectedKidAsCalculatedFromAWSKeyID be more meaningful?
There was a problem hiding this comment.
we discussed - expectedHashKeyArn
| @@ -56,14 +56,15 @@ test { | |||
|
|
|||
| doLast { | |||
| tasks.getByName("jacocoTestReport").sourceDirectories.from( | |||
| project(":account-management-api").sourceSets.main.java, | |||
| project(":delivery-receipts-api").sourceSets.main.java, | |||
There was a problem hiding this comment.
In the commit message here can we add the detail of what the issue was here and why we think this fixes? Just to have the context there for future (you explained it to me and I've already forgotten :-D)
| @@ -33,6 +35,12 @@ public class ConfigurationService implements BaseLambdaConfiguration, AuditPubli | |||
| public static final String FEATURE_SWITCH_ON = "true"; | |||
| private static ConfigurationService configurationService; | |||
|
|
|||
| public static KmsAccessInterceptor getKmsAccessInterceptor() { | |||
There was a problem hiding this comment.
I'm not keen on changing production code here to add something that's used for test. Is there an alternative?
| .orElseGet(MetricsLogger::new); | ||
| } | ||
|
|
||
| public void setMetricsLoggerAdapter(MetricsLogger metricsLoggerAdapter) { |
There was a problem hiding this comment.
Again here, I'm not sure about adding this to production code when it's only used for test. I wonder if an alternative to some of these production code changes is to make use / adapt the IntegrationTestConfigurationService
|
|
||
| var response = | ||
| makeRequest( | ||
| Optional.of(new ReverificationResultRequest("code", "eamil")), |
BeckaL
reviewed
Jan 7, 2025
There was a problem hiding this comment.
Tests generally look good to me. Some comments / questions here. If it helps get this moving, I'd be happy to approve most of the commits that tweak existing stuff before adding the new tests if you want to separate them out into a separate PR, and then look at the more substantive comments in this PR
| .atMost(Duration.ofSeconds(60)) | ||
| .pollInterval(Duration.ofSeconds(1)) | ||
| .until(() -> txmaAuditQueue.getApproximateMessageCount() > 0); | ||
| assertEquals(1, txmaAuditQueue.getRawMessages().size()); |
There was a problem hiding this comment.
Here if it's cheap I wouldn't mind adding to the commit the context you have on why this failed at the moment, just in case useful in future
andrew-moores
force-pushed
the
aut-3916
branch
from
1c80b21 to
d7726f4
Compare
The NotifyCallbackHandlerIntegrationTest was not completing correctly as the wrong class was defined as finalizing the test run. This was causing the test to fail.
…tion-tests test task.
… as it achieved nothing due to all the tests being named correctly.
…thentication-api. This sets showStandardStreams to false which is the default and has no effect but is easy to change if logs from the integration tests are needed.
…e other integration test submodules.
…sed to sign requests sent to IPV. Use the Junit 5 System Stubs extension to change the environment for the lambda. This allows the id of a key created in localstack to be used when testing the lambda and the JwksService. Added a log4j2-test.xml config to allow easier reading of the logs when running tests.
…e public key used to sign the storage tokens.
…fication to make the key id available.
…sfully served. Also modified the error response body to indicate when a key cannot be served.
The integration test checks how the lambda integrates with KMS, redis, SQS and CloudWatch. + Integration with KMS is checked to confirm the expected keys are being utilised. To facilitate this testing the KmsKeyExtension was amended to record the keyId of the created key. Because localstack KMS doesn't allow key usage to be checked an interceptor was added to the KmsClient created in the KmsConnectionService for ue with localstack. This interceptor simply records the latest key use for a signing operation. This can be extended when checks are required for multiple keys and/or operations. + Integration with redis is checked to confirm the expected state is stored. The existing RedisExtension is used. + Integration with SQS is checked to confirm events are published for TxMa. The SQSQueueExtension is used. Localstack is very slow at publishing events so the check for the TxMA event is retried, using awaitility, until the event can be read. This is not a scalable approach and its value will be evaluated on overall test suite run times. An alternative approach could be to add an interceptor to the SQS client. + Integration with CloudWatch is checked to confirm the expected metrics will be created. Localstack does not automatically created CloudWatch metrics from CloudWatch logs so the logs are checked instead of looking up the metric. This is acceptable as we do not need to test that AWS correctly processed log messages we just need to check that we have started the process correctly. The MetricsLogger class is very difficult to configure such that a lambda class being run outside of the localstack container will log to localstack. To work round this issue a test double of the MetricsLogger is used. The test double uses the CloudWatch Logs API rather than the MetricsLogger but exposes the same interface as the MetricsLogger. We can then check the required logs are produced. The test double is very simple and can be extended as we find more complex test scenarios. The logs service is enabled in the docker compose used locally and in the pre-merge-checks github workflow.
…date integration test for MfaResetAuthorizeHandler. Updated the KmsAccessInterceptor to maintain a set of all keys used to sign. Updated the MfaResetAuthorizeHandlerIntegrationTest to use the new version of the KmsAccessInterceptor and added missing check for second key used to sign during its request processing. The ReverificationResultHandlerIntegrationTest follows the same pattern as the MfaResetAuthorizeHandlerIntegrationTest but also uses wiremock to simulate IPV.
…e test IPV encryption key using JOSE rather KMS as this provides access to the private key whcih is required to check the JAR has been encrypted with the correct key.
…ssue that caused the 30 second delay to the Txma message being available to read from the SQS queue.
Changes brought in from main that renamed some of the environment variables are now reflected in the integration tests. Use new IDReverificationStateExtension Junit extension to support the use ot the new IDReverificationStateService in the integration tests.
AWS calls the id of a key a keyId but in the JWKS the key id is abbreviated to kid. This is confusing so the name of the expected key id in the test was changed to make it clear that its not the AWS keyId and what is in the kid.
andrew-moores
force-pushed
the
aut-3916
branch
from
d7726f4 to
dd04adf
Compare
|
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.
What
Added tests for the MFA Reset re-verification.
How to review
Each commit is relatively small and self contained so review commit by commit. The issue where I introduced awaitility to get the TxMA message in the MfaResetAuthorizeHandlerIntegrationTest is fixed in the final commit.
Please check that none of the changes impact orchestration indirectly.
The test suite should not take noticeably longer to run with the new tests included.
KMS has been used to create the signing keys but not to create the IPV encryption key as we need access to the private key to confirm the correct key is being used.
The tests assume that the IPV public key is available in an environment variable which is how we will deploy initially but this will change to the strategic approach of getting the public key from the IPV JWKS.
Checklist
Related PRs