Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/insert manager #213

Merged
merged 13 commits into from
Sep 9, 2020
Merged

Feature/insert manager #213

merged 13 commits into from
Sep 9, 2020

Conversation

ubamrein
Copy link
Contributor

@ubamrein ubamrein commented Aug 5, 2020 •
edited by ineiti
Loading

This PR simplifies the controller logic by introducing a new class InsertManager.
InsertManager holds a list of possible Filters which filter keys based on OS, AppVersion or features activated.
The Filters are added at startup in the WSBaseConfig and allow for Filter activation by Profile or Property.

Additionally, the InsertManager can be configured with modifiers.
Modifiers can be used to modify keys before inserting into the datbase.

Replaces the previous g811a8d461394eecd25bd190c01b933cc177b7d05, according to #226

@ineiti wants to:

  • know whether the comments on the filters not working as advertised are correct or not
  • have somebody add the comments @wouterl requested
  • see an updated insertmanager/README.md with the new filter-queues - didn't see it was there :(

@ubamrein ubamrein requested a review from martinalig August 5, 2020 15:03
@ubamrein ubamrein mentioned this pull request Aug 11, 2020
Merged
@ineiti
Copy link
Collaborator

ineiti commented Aug 27, 2020 •
edited
Loading

Is it possible to extract the _Has some rebrandings and adjustments regarding time handling (extends unify-timehandling PR)
_ into a separate PR? This would help during code-review...

Done now.

@ineiti ineiti force-pushed the feature/insert-manager branch from 811a8d4 to 2f12f1f Compare August 28, 2020 06:07
ineiti added a commit that referenced this pull request Aug 28, 2020
@ineiti
[WIP] Feature/get exposed sql filters
085d7c9 
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).

Further, this should also allow for easier testing and debugging with a query ignoring all filters.

To minmise merge conflicts, this PR depends on #213

Linted and rebased - original is at #f881f566b53a335f56661041b22c3872e145a8bd
ineiti added a commit that referenced this pull request Aug 28, 2020
@ineiti
[WIP] Feature/get exposed sql filters
b0ec0c6 
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).

Further, this should also allow for easier testing and debugging with a query ignoring all filters.

To minmise merge conflicts, this PR depends on #213

Linted and rebased - original is at #f881f566b53a335f56661041b22c3872e145a8bd
@ineiti ineiti force-pushed the feature/insert-manager branch 4 times, most recently from 073f77e to 3a7bf65 Compare August 28, 2020 07:06
@ineiti ineiti changed the base branch from develop to linter_done August 28, 2020 07:07
@ineiti ineiti force-pushed the feature/insert-manager branch from 3a7bf65 to 41aec0d Compare August 28, 2020 07:22
@ineiti ineiti changed the base branch from linter_done to pre-linting August 28, 2020 07:22
@ineiti ineiti changed the base branch from pre-linting to develop August 28, 2020 07:26
@ineiti ineiti changed the base branch from develop to pre-linting August 28, 2020 07:26
@ineiti ineiti force-pushed the feature/insert-manager branch from 41aec0d to c921d44 Compare August 28, 2020 08:31
@ineiti ineiti changed the base branch from pre-linting to develop August 28, 2020 08:32
@ineiti ineiti changed the base branch from develop to linter_done August 28, 2020 08:45
@ineiti ineiti changed the base branch from linter_done to develop August 28, 2020 08:45
@ineiti ineiti force-pushed the feature/insert-manager branch from c921d44 to 32df702 Compare August 28, 2020 08:47
@ineiti
Copy link
Collaborator

ineiti commented Aug 28, 2020

Some notes while trying to extract a clean and a semver branch:

  • GaenControllerTestNotThreadSafe.java - it reproduces some of the errors fixed in Refactor GaenControllerTest test key insert method #220
  • GaenControllerTest - I'm not sure what is the correct User-agent to use - but if it's the Android one, that should go into a class variable.

ineiti added a commit that referenced this pull request Aug 28, 2020
@ineiti
Cleanups from feature/insert-manager
f3595e2 
This is the first PR to make the PR #213 somewhat smaller and more manageable.
It takes only the cleanup parts of it.
See #226
@ineiti ineiti mentioned this pull request Aug 28, 2020
Merged
ineiti added a commit that referenced this pull request Aug 28, 2020
@ineiti
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.
This was referenced Aug 28, 2020
Merged
Closed
ineiti added a commit that referenced this pull request Aug 28, 2020
@ineiti
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 ineiti force-pushed the feature/insert-manager branch from 32df702 to 9cabe4d Compare August 28, 2020 13:38
ineiti added a commit that referenced this pull request Aug 28, 2020
@ineiti
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 ineiti force-pushed the feature/insert-manager branch from 9cabe4d to 0ce2d65 Compare August 28, 2020 13:39
@ineiti ineiti force-pushed the feature/insert-manager branch from 5bb23a1 to 3ade912 Compare September 1, 2020 09:57
@ineiti
Feature/insert-manager
0c9f57b 
This PR simplifies the controller logic by introducing a new class InsertManager.
InsertManager holds a list of possible Filters which filter keys based on OS, AppVersion or features activated.
The Filters are added at startup in the WSBaseConfig and allow for Filter activation by Profile or Property.

Additionally, the InsertManager can be configured with modifiers.
Modifiers can be used to modify keys before inserting into the datbase.

Replaces the previous g811a8d461394eecd25bd190c01b933cc177b7d05, according to #226
@ineiti ineiti force-pushed the feature/insert-manager branch from 3ade912 to 0c9f57b Compare September 1, 2020 10:15
@ineiti ineiti force-pushed the feature/insert-manager branch from 39616da to 7c0d208 Compare September 1, 2020 10:50
wouterl
wouterl reviewed Sep 1, 2020
View reviewed changes
Copy link

@wouterl wouterl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a pass on all filters. Looking good. The documentation and structure really helps :). See separate notes for tiny inconsistency and some requests for a few more specific lines of documentation.

...ain/java/org/dpppt/backend/sdk/ws/insertmanager/insertionmodifier/OldAndroid0RPModifier.java Show resolved Hide resolved
...ain/java/org/dpppt/backend/sdk/ws/insertmanager/insertionmodifier/OldAndroid0RPModifier.java Outdated Show resolved Hide resolved
...rg/dpppt/backend/sdk/ws/insertmanager/insertionmodifier/IOSLegacyProblemRPLT144Modifier.java Outdated Show resolved Hide resolved
...main/java/org/dpppt/backend/sdk/ws/insertmanager/insertionfilters/KeysMatchingJWTFilter.java Outdated Show resolved Hide resolved
...main/java/org/dpppt/backend/sdk/ws/insertmanager/insertionfilters/KeysMatchingJWTFilter.java Outdated Show resolved Hide resolved
...k-ws/src/main/java/org/dpppt/backend/sdk/ws/insertmanager/insertionfilters/Base64Filter.java Outdated Show resolved Hide resolved
...kend-sdk/dpppt-backend-sdk-ws/src/main/java/org/dpppt/backend/sdk/ws/insertmanager/README.md Show resolved Hide resolved
@ineiti
InsertManager cleanup
019cce0 
This PR updates the comments of the filters and modifiers to follow 'plain English':
http://www.plainenglish.co.uk/how-to-write-in-plain-english.html

Also changed ValidationUtils.checkForDelayedKeyDateClaim and its use in the KeysMatchingJWTFilter
which was wrong.
ineiti
ineiti requested changes Sep 3, 2020
View reviewed changes
Copy link
Collaborator

@ineiti ineiti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #239

...rg/dpppt/backend/sdk/ws/insertmanager/insertionmodifier/IOSLegacyProblemRPLT144Modifier.java Outdated Show resolved Hide resolved
...ain/java/org/dpppt/backend/sdk/ws/insertmanager/insertionmodifier/OldAndroid0RPModifier.java Outdated Show resolved Hide resolved
...n/java/org/dpppt/backend/sdk/ws/insertmanager/insertionfilters/ValidRollingPeriodFilter.java Outdated Show resolved Hide resolved
...ain/java/org/dpppt/backend/sdk/ws/insertmanager/insertionmodifier/OldAndroid0RPModifier.java Show resolved Hide resolved
ubhaller pushed a commit that referenced this pull request Sep 3, 2020
@ineiti @ubhaller
Adding semver
2c76310 
Adds the semver class with its tests.
See #226

Once this is merged, #213 and #215 need to be rebased on this.
Base automatically changed from im-semver to develop September 3, 2020 17:06
@ineiti ineiti mentioned this pull request Sep 4, 2020
Closed
@martinalig
Split JWTClaims filter for Exposed and ExposedNextDay
9014336 
Simplify Exception Handling in controller and make InsertException abstract.
@martinalig
Merge pull request #241 from DP-3T/feature/im-seperation
e26dec7 
Split JWTClaims filter for Exposed and ExposedNextDay
@ineiti ineiti marked this pull request as ready for review September 7, 2020 06:27
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 2 Code Smells

83.5% 83.5% Coverage
0.0% 0.0% Duplication

@martinalig martinalig merged commit 2efe45c into develop Sep 9, 2020
@martinalig martinalig deleted the feature/insert-manager branch September 9, 2020 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterl wouterl wouterl left review comments

@martinalig martinalig martinalig approved these changes

@ineiti ineiti ineiti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ubamrein @ineiti @martinalig @wouterl