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.

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

Configure and verify max-http-request-header-size: 32KB #529

Closed
wants to merge 6 commits into from

Conversation

MrSebastian
Copy link
Member

@MrSebastian MrSebastian commented Nov 8, 2024

Beschreibung:

  • Beim AuthService mussten die Settings angepasst werden (analog zu Anfrage failed bei aktiver Security #318 )
  • bei allen Services ein Test ergänzt der sicherstellen soll dass die Property gesetzt ist. Wenn die Property nicht gesetzt kommt es zu einem BadRequest und nicht zu unauthorized

Definition of Done (DoD):

Referenzen1:

Verwandt mit Issue #318

Closes #

Footnotes

  1. Nicht zutreffende Referenzen vor dem Speichern entfernen

vjohnslhm
vjohnslhm previously approved these changes Nov 11, 2024
@vjohnslhm
Copy link
Contributor

muss das für den admin-service und vorfaelleundvorkommnisse-service auch umgesetzt werden?

@MrSebastian
Copy link
Member Author

muss das für den admin-service und vorfaelleundvorkommnisse-service auch umgesetzt werden?

ja, soll ich das gleich hier mit machen? Ich finde das würde zum PR passen. Dann stimmt der Branchname zwar nicht mehr aber den kann ich anpassen und den PR neustellen.

@MrSebastian
Copy link
Member Author

ich habe keinen neuen Branch aufgemacht aber in der Merge-Commit-Nachricht den Titel auf meinen lokalen Branchnamen angepasst: fix-and-verify-request-header-size-settings

@MrSebastian MrSebastian disabled auto-merge November 11, 2024 15:29
@MrSebastian MrSebastian changed the title Configure max-http-request-header-size: 32KB Configure and verify max-http-request-header-size: 32KB Nov 11, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

warum gibt es die application-test.yml in manchen services zwei mal? ich denke es sollte sie nur ein mal geben, weil wenn man zum beispiel eine konfiguration nachsehen möchte muss man immer an mehreren stellen suchen. außer in beiden steht das gleiche, das wäre aber noch ein grund mehr nur eine zu haben.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ist mir auch aufgefallen dass es manchmal ein application-test.yml in den main-Resourcen gibt und manchmal in den test-Ressourcen. Meine Intension war aber kein zweites File aufzumachen wenn es bereits eins gibt. Ich schaue nochmal durchen. Für die Vereinheitlichung mache ich ein Ticket auf. In dem Zuge können wir auch die anderen yml-Files die ursprünglich für Umgebungen gedacht waren aufräumen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ich habe es korrigiert: In den Fällen wo es bereits eine application-yml gab und ich frische neue eingefügt hatte, habe ich meine Daten in den existierende Datei übertragen.

Das Issue zur Bereinigung der yml-Files ist #539

Copy link
Contributor

Choose a reason for hiding this comment

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

kann man das certsStubbing.json und userinfoStubbing.json in wls.common auslagern, um die duplicated files zu minimieren?

Copy link
Member Author

Choose a reason for hiding this comment

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

habe ich nicht probiert. Würde aber ein neues Release von Wls-common erfordern. Ich mache dazu ein Issue auf.


val result = restTemplate.exchange(request, Void.class);

Assertions.assertThat(result.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED); //unauthorized cause token is expired
Copy link
Contributor

Choose a reason for hiding this comment

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

wo in dem test wird sichergestellt, dass die max-header-size gesetzt ist?

Wenn die Property nicht gesetzt kommt es zu einem BadRequest und nicht zu unauthorized

müsste man nicht einen test hinzufügen der die Assertion ...isEqualTo(HttpStatus.BAD_REQUEST); hat?

Copy link
Member Author

Choose a reason for hiding this comment

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

wenn der Header nicht gesetzt kommt es zu dem Bad-Request. Ich hatte überlegt ob ich den negativ Test schreiben. Allerdings müsste dann der Header in einer seperaten yml-Datei gesenkt werden. Dadurch dass die Settings in application.yml vorliegen greift der max-http-request-header-size immer, solange es nicht überschrieben wird. Ich habe mich daher dafür entschieden zu prüfen ob unsere Headgröße ausreichend ist. Daher keinen negativen Test.

@MrSebastian
Copy link
Member Author

nachdem die erstellen Tests keine Aussagekraft haben und es auf Änderung nur an den Settings zum Auth-Service hinausläuft wird der PR eingestellt.

auto-merge was automatically disabled November 13, 2024 13:11

Pull request was closed

@MrSebastian MrSebastian deleted the authservice-fix-request-header-size-settings branch November 13, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants