[WIP] Feature/get exposed sql filters #215

ubamrein · 2020-08-11T15:02:08Z

With the new pipeline we try to sanitise input on insert. Since due to various possible issues (and for easier debugging) we could insert keys with validity exceeding the current bucket (e.g. a key submitted today with todays keydate and rolling period 144), we additionally filter for such keys on release. Adding the rules for filtering into the SQL query should decrease the risk of accidentally releasing keys too early (e.g. bugs during insert, manipulating JWTs or other yet unknown problems).

To minmise merge conflicts, this PR depends on #213

Linted and rebased - original is at f881f56
Moved non-related changes to #234 - original is at baa1f96

Adds the semver class with its tests. See #226 Once this is merged, #213 and #215 need to be rebased on this.

ineiti · 2020-09-01T05:56:56Z

This PR contains also a lot of clean-up. I will remove the clean-up stuff, so that it's better visible what is done here. See #232

Update: Done...

Adds the semver class with its tests. See #226 Once this is merged, #213 and #215 need to be rebased on this.

(kkeys are released at 4am since timeskew brings it exactly in the 4am bucket)

ineiti

Starts looking good. I'm really not happy about the use of UTCInstant.SetClock:

tests cannot be run in parallel
I'm not sure if it will not give problems in other parts of the tests

Also, UTCInstant.today() should not be used more than once per test - even if the chance of failure is quite small.

ineiti · 2020-09-15T13:13:05Z

...pppt-backend-sdk-data/src/test/java/org/dpppt/backend/sdk/data/gaen/GaenDataServiceTest.java

+  @Test
+  public void testNoEarlyRelease() throws Exception {
+    Clock twoOClock = Clock.fixed(UTCInstant.today().plusHours(2).getInstant(), ZoneOffset.UTC);
+    Clock elevenOClock = Clock.fixed(UTCInstant.today().plusHours(11).getInstant(), ZoneOffset.UTC);


I'm not very happy with the use of setClock in the tests - I would even remove it from UTCInstant. Even though I'm not sure if you could still run all the tests...

Solving this and solving a very improbable race-error (UTCInstant.today() might change during the test), could give:

start with

var today = UTCInstant.today()

use that throughout the code, and then only update now and pass that to the methods.

Would that work? Or are there other UTCInstant.now() and UTCInstant.today() in the code-path?

I've discussed it with @martinalig but since it is very improbable we did not really care. It would only be a problem, iff the test is executed exactly at 00:00... but I can change it :)

ineiti · 2020-09-15T13:29:55Z

...ppt-backend-sdk-ws/src/test/java/org/dpppt/backend/sdk/ws/controller/GaenControllerTest.java

+    List<GaenKey> keys = new ArrayList<>();
+    for (int i = 0; i < 30; i++) {
+      var tmpKey = new GaenKey();
+      tmpKey.setRollingStartNumber((int) UTCInstant.today().minusDays(i).get10MinutesSince1970());


Again a very improbable race-condition when using UTCInstant.today(). You should put it at the beginning and then use today in the code.

same as above

ineiti · 2020-09-15T13:30:56Z

...ppt-backend-sdk-ws/src/test/java/org/dpppt/backend/sdk/ws/controller/GaenControllerTest.java

+    Clock fourAMTomorrow =
+        Clock.fixed(UTCInstant.today().plusDays(1).plusHours(4).getInstant(), ZoneOffset.UTC);
+
+    UTCInstant.setClock(fourAMTomorrow);


To avoid this, could you pass the now to mockMvc?

After a bit of research I think our only option would be to do something like https://github.com/raphw/byte-buddy. I tried the Mockito Framework, but that one only allows to replace a class with all stubs and redefine functions. I think this is not really feasible, since it would blow up the test code, since we would have to redefine all functions of the mocked class.

Also the PowerMockito plugin seems to be rather verbose, when our only intention is to manipulate actual system time.

Another option would be to replace the static UTCInstant approach with a singleton approach using Beans and dependency injection. I'm not sure how easy it is to change everything and inject everywhere the Bean (maybe with @Autowired but then we introduce more magic...)

A way to prevent forgetting the resetClock could be to implement the Disposable interface and use it with a try-with statement.

Some remarks on test parallelisation:

Since every test class itself initialises a new VM instance (and hence a new static object is instantiated) as far as I understand it, and tests aren't parallelised within test classes (this would anyways be catastrophic, since for the integration tests we relay on database inserts and then we would have to mock them as well), this should not be problematic. The only "dangerous" thing is that one could use the setClock function outside test-packages, which is currently not prevented (though we could check for org.junit being on the classPath).

Maybe I'm also completely on the wrong path here, but in my opinion it is easier that way.

@martinalig any input? :)

ineiti · 2020-09-15T13:31:42Z

...kend-sdk-data/src/test/java/org/dpppt/backend/sdk/data/gaen/PostgresGaenDataServiceTest.java

@@ -72,27 +72,31 @@ public void tearDown() throws SQLException {

  @Test
  public void testFakeKeyContainsKeysForLast21Days() {
+    Clock threeOClock = Clock.fixed(UTCInstant.today().plusHours(4).getInstant(), ZoneOffset.UTC);
+    UTCInstant.setClock(threeOClock);


Again, can this setClock be removed?

ineiti · 2020-09-16T09:33:20Z

Please make sure that the two functions use same checks.

The other method with the incorrect sql query has been removed.

All comments have been addressed.

sonarcloud · 2020-09-16T09:35:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

100.0% Coverage
0.0% Duplication

ubamrein marked this pull request as draft August 11, 2020 15:02

ubamrein changed the title ~~Feature/get exposed sql filters~~ [WIP] Feature/get exposed sql filters Aug 11, 2020

ineiti mentioned this pull request Aug 28, 2020

Lint PR #215 #227

Closed

ineiti force-pushed the feature/get-exposed-sql-filters branch 4 times, most recently from 9d3e9d3 to 857a7e7 Compare August 28, 2020 07:25

ineiti changed the base branch from develop to pre-linting August 28, 2020 07:25

ineiti force-pushed the feature/get-exposed-sql-filters branch from 857a7e7 to f653838 Compare August 28, 2020 08:43

ineiti changed the base branch from pre-linting to develop August 28, 2020 08:43

ineiti force-pushed the feature/get-exposed-sql-filters branch from f653838 to 4e3844c Compare August 28, 2020 08:48

ineiti mentioned this pull request Aug 28, 2020

Cleanups from feature/insert-manager #229

Merged

ineiti added a commit that referenced this pull request Aug 28, 2020

Adding semver

984e49b

Adds the semver class with its tests. See #226 Once this is merged, #213 and #215 need to be rebased on this.

ineiti mentioned this pull request Aug 28, 2020

Adding semver #230

Merged

2 tasks

ineiti added a commit that referenced this pull request Aug 28, 2020

Adding semver

1074949

Adds the semver class with its tests. See #226 Once this is merged, #213 and #215 need to be rebased on this.

ineiti added a commit that referenced this pull request Aug 28, 2020

Adding semver

c9f7bd7

Adds the semver class with its tests. See #226 Once this is merged, #213 and #215 need to be rebased on this.

ineiti changed the base branch from develop to feature/insert-manager August 31, 2020 09:00

ineiti changed the base branch from feature/insert-manager to develop August 31, 2020 09:01

ineiti force-pushed the feature/get-exposed-sql-filters branch from 4e3844c to 792995e Compare August 31, 2020 12:26

ineiti changed the base branch from develop to feature/insert-manager August 31, 2020 12:26

ineiti force-pushed the feature/get-exposed-sql-filters branch from 792995e to baa1f96 Compare August 31, 2020 12:27

ineiti mentioned this pull request Sep 1, 2020

Separate clean-up and new functionalities in #215 #232

Closed

ineiti changed the base branch from feature/insert-manager to utcinstant_roundtobucket September 1, 2020 06:54

ineiti force-pushed the feature/get-exposed-sql-filters branch 2 times, most recently from 324b5f5 to 3b13d75 Compare September 1, 2020 07:02

ineiti force-pushed the utcinstant_roundtobucket branch from c02c522 to a8559d1 Compare September 1, 2020 07:07

ineiti force-pushed the feature/get-exposed-sql-filters branch from 3b13d75 to 3d02f2b Compare September 1, 2020 07:07

ineiti added a commit that referenced this pull request Sep 1, 2020

Adding semver

1b6b300

Adds the semver class with its tests. See #226 Once this is merged, #213 and #215 need to be rebased on this.

ineiti added a commit that referenced this pull request Sep 1, 2020

Adding semver

8387381

Adds the semver class with its tests. See #226 Once this is merged, #213 and #215 need to be rebased on this.

ubamrein added 4 commits September 14, 2020 19:59

add some comments and configurable timeskew

b6a6f99

change the plus to minus

3dbeb9d

add test for no early release

ec128c9

reset clck after tests

d019ab7

martinalig force-pushed the feature/get-exposed-sql-filters branch from 42ef973 to d019ab7 Compare September 14, 2020 18:00

ubamrein added 9 commits September 15, 2020 08:14

add integration test

1c2b146

fix some tests

b38017b

fix one test

994277b

make keys random foor class

6d6c7d5

make tests a bit clearer

7d78d40

fix test

99d80d5

(kkeys are released at 4am since timeskew brings it exactly in the 4am bucket)

remove deprecated function for etag calculation

d568f8d

add some comments

d7b4710

add comments to dataservice

47b1254

ubamrein requested review from ineiti, wouterl and martinalig September 15, 2020 12:44

ineiti requested changes Sep 15, 2020

View reviewed changes

ineiti marked this pull request as ready for review September 15, 2020 13:34

ubamrein added 2 commits September 16, 2020 09:19

use try with statement

b26cbd3

use less invocations of now

076730a

ubamrein mentioned this pull request Sep 16, 2020

Change how UTCInstant gets Clock #246

Open

ubamrein requested a review from ineiti September 16, 2020 07:54

add comment to function

482d0e4

ineiti approved these changes Sep 16, 2020

View reviewed changes

ubamrein merged commit 8348737 into develop Sep 16, 2020

ubamrein deleted the feature/get-exposed-sql-filters branch September 16, 2020 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feature/get exposed sql filters #215

[WIP] Feature/get exposed sql filters #215

ubamrein commented Aug 11, 2020 •

edited by ineiti

Loading

ineiti commented Sep 1, 2020 •

edited

Loading

ineiti left a comment

ineiti Sep 15, 2020

ubamrein Sep 15, 2020

ineiti Sep 15, 2020

ubamrein Sep 15, 2020

ineiti Sep 15, 2020

ubamrein Sep 15, 2020

ineiti Sep 15, 2020

ineiti commented Sep 16, 2020

sonarcloud bot commented Sep 16, 2020

[WIP] Feature/get exposed sql filters #215

[WIP] Feature/get exposed sql filters #215

Conversation

ubamrein commented Aug 11, 2020 • edited by ineiti Loading

ineiti commented Sep 1, 2020 • edited Loading

ineiti left a comment

Choose a reason for hiding this comment

ineiti Sep 15, 2020

Choose a reason for hiding this comment

ubamrein Sep 15, 2020

Choose a reason for hiding this comment

ineiti Sep 15, 2020

Choose a reason for hiding this comment

ubamrein Sep 15, 2020

Choose a reason for hiding this comment

ineiti Sep 15, 2020

Choose a reason for hiding this comment

ubamrein Sep 15, 2020

Choose a reason for hiding this comment

ineiti Sep 15, 2020

Choose a reason for hiding this comment

ineiti commented Sep 16, 2020

sonarcloud bot commented Sep 16, 2020

ubamrein commented Aug 11, 2020 •

edited by ineiti

Loading

ineiti commented Sep 1, 2020 •

edited

Loading