Add unit test to confirm that no keys are inserted when JWT token contains fake=1 #287
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
| exposedKeys.add(gaenKey2); | ||
|
|
||
| manager.insertIntoDatabase(exposedKeys, "test", jwt, UTCInstant.now()); | ||
| } |
There was a problem hiding this comment.
There doesn't seem to be an actual 'test' here - no assertEqual or such... am I missing something?
Also, like with the test above, I usually prefer to do the double-testing in the same test:
- test it actually gets inserted with
fake == 0 - test it gets refused with
fake == 0
Because I think we got the case where a refusal to store a bad key was due to a previous error, but not the check the test was supposed to do...
There was a problem hiding this comment.
There doesn't seem to be an actual 'test' here - no assertEqual or such... am I missing something?
It uses a MockDataSource which throws a RuntimeException if there is a call to UpsertExposees.
The idea of this test is, to ONLY test the InsertManager totally DataProvider agnostic.
If I'm not mistaken, we check that insertion into the database woks correctly in various integration tests. Here we only ensure that nothing is inserted if there is a JWT with fake=1 (or rather fake != 0).
There was a problem hiding this comment.
but yeah to show that it indeed passes the filters (also if more are added) would be preferable. @UBaggeler same here? Issue?
| @@ -97,6 +97,63 @@ public void testNoTokenFails() throws Exception { | |||
| assertTrue(authenticateError.contains("Bearer")); | |||
| } | |||
|
|
|||
| private void testJWTFakeNoKeysInserted(boolean useV2Upload) throws Exception { | |||
There was a problem hiding this comment.
Wouldn't it make more sense to replace the boolean with a int here? We'll probably not be going above v2, but still...
| private void testJWTFakeNoKeysInserted(boolean useV2Upload) throws Exception { | |
| private void testJWTFakeNoKeysInserted(int APIversion) throws Exception { |
| uploadPayload = exposeeRequest; | ||
| } | ||
|
|
||
| String token = createToken(true, now.plusMinutes(5)); |
There was a problem hiding this comment.
As in the test below, I would run the test twice: once with createToken(false, ... and once with createToken(true, just to make sure you're not missing some keys due to another part of the logic.
Even though I know some of the code, I couldn't guarantee that this actually passes with createToken(false...
| protected String createToken(boolean fake, UTCInstant expiresAt) { | ||
| Claims claims = Jwts.claims(); | ||
| claims.put("scope", "exposed"); | ||
| claims.put("onset", "2020-04-20"); |
There was a problem hiding this comment.
Shouldn't this be put to now() - 3 days or so? It's probably checked nowhere for the moment. But there might be a filter added at a later time, rejecting onset dates older than 14 days...
There was a problem hiding this comment.
Yes we should and yes we should add such a filter and probably write an integration test for it. @UBaggeler shall we make a Issue for it?
| .claim("fake", "1") | ||
| .build(); | ||
|
|
||
| var gaenKey1 = new GaenKey(); |
There was a problem hiding this comment.
This is hard to read and to understand why you have gaenKey1 and gaenKey2.
Either rename them as gaenKeyToday and gaenKeyYesterday, or make an array.
There was a problem hiding this comment.
You are right, we probably should hide this from the actual test.
Just to note here, it does not matter at all (other than the key should pass the filters) what is inserted, since we are bypassing any logic which is executed before (as e.g. checking for a valid JSON object and such)
This PR adds unit tests to confirm that no keys are inserted/published when request was made with a JWT token that contains the claim
fake=1.