889 controllerintegrationtests keine dummybezirkid briefwahlservice#2391
889 controllerintegrationtests keine dummybezirkid briefwahlservice#2391NoobieBubie wants to merge 12 commits intodevfrom
Conversation
…ung um zwei tests negativtests
…erweiterung um zwei tests negativtests
github-actions
bot
added
the
service-briefwahl
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request migrates JWT-based authentication testing across two integration test files in the briefwahl-service module. The changes replace Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/beanstandetewahlbriefe/BeanstandeteWahlbriefeControllerIntegrationTest.java (1)
76-82: 🛠️ Refactor suggestion | 🟠 MajorLeftover
@WithMockUserannotations alongsidejwt()— same incomplete migration as the other file.Multiple tests in this class still have
@WithMockUserwhile also usingjwt()on the request. These should be removed for consistency and clarity. Affected tests:
should_returnData_when_dataIsPresentInRepo(line 77)should_returnDataWithEmptyZurueckweisegruende_when_dataIsPresentInRepo(line 145)should_returnFachlicheWlsException_when_requestIsInvalid(GET, line 201; POST, line 257)should_returnForbidden_when_wahlBezirkIdIsWrong(GET, line 230)should_setNewData_when_callingPost(line 290)Also applies to: 144-150, 200-205, 229-234
🤖 Fix all issues with AI agents
In
`@wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/beanstandetewahlbriefe/BeanstandeteWahlbriefeControllerIntegrationTest.java`:
- Around line 63-69: In should_returnNoContent_when_noDataFound
(BeanstandeteWahlbriefeControllerIntegrationTest) the test grants the
Wahlbriefdaten authorities (Authorities.SERVICE_GET_WAHLBRIEFDATEN,
Authorities.REPOSITORY_READ_WAHLBRIEFDATEN); replace these with the
BeanstandeteWahlbriefe authorities
(Authorities.SERVICE_GET_BEANSTANDETE_WAHLBRIEFE and
Authorities.REPOSITORY_READ_BEANSTANDETE_WAHLBRIEFE) so the JWT in the
get("/businessActions/beanstandeteWahlbriefe/...") request matches the endpoint
under test.
In
`@wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/wahlbriefdaten/WahlbriefdatenControllerIntegrationTest.java`:
- Around line 194-214: The test method should simulate a POST user with a
mismatched wahlbezirk claim, not a missing claim or wrong authority: in
should_returnForbidden_when_wahlBezirkIdIsWrong replace the jwt authority from
Authorities.SERVICE_GET_WAHLBRIEFDATEN to
Authorities.SERVICE_POST_WAHLBRIEFDATEN and change the jwt claim key from
"wahlbezirkID_target" to "wahlbezirkID" (set to a different value than the URL
userWahlbezirkID) so the request has the correct POST permission but a differing
wahlbezirkID claim, causing a forbidden response for a mismatch.
- Around line 73-104: Remove the leftover `@WithMockUser` annotations from the
JWT-migrated tests so the request post-processor jwt() is the sole auth source:
delete the `@WithMockUser` block above should_returnData_when_dataIsPresent and
also remove the same annotation blocks above should_setNewData_when_callingPost
and should_replaceData_when_dataIsPresent so the tests rely only on the jwt()
setup (keep the jwt().authorities(...) and jwt.claim(...) code in each test).
🧹 Nitpick comments (4)
wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/wahlbriefdaten/WahlbriefdatenControllerIntegrationTest.java (1)
37-37: Remove commented-out code.The commented-out profile reference
// , Profiles.NO_BEZIRKS_ID_CHECK})should be removed entirely rather than left as a comment, since the PR's purpose is to stop using the dummy bezirkID check.Proposed fix
-@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})//, Profiles.NO_BEZIRKS_ID_CHECK}) +@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/beanstandetewahlbriefe/BeanstandeteWahlbriefeControllerIntegrationTest.java (3)
57-62: Remove commented-out@WithMockUserblock.This commented-out annotation is dead code. Since the migration to JWT is intentional, remove it entirely.
Proposed fix
`@Test` -/* `@WithMockUser`( - authorities = { - Authorities.SERVICE_GET_BEANSTANDETE_WAHLBRIEFE, - Authorities.REPOSITORY_READ_BEANSTANDETE_WAHLBRIEFE - })*/ void should_returnNoContent_when_noDataFound() throws Exception {
39-39: Remove commented-out profile reference.Same as the other file — clean up the comment instead of leaving dead code.
Proposed fix
-@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})/*, "dummy.nobezirkid.check"})*/ +@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})
229-244: Inconsistent indentation — this test method is indented one extra level compared to sibling methods.The
should_returnForbidden_when_wahlBezirkIdIsWrongmethod (lines 229-244) uses 6-space indentation while all other methods in theGetBeanstandeteWahlbriefeclass use 4-space. Same issue for theAddBeanstandeteWahlbriefeforbidden test at lines 342-359.
...okalsystem/briefwahlservice/rest/wahlbriefdaten/WahlbriefdatenControllerIntegrationTest.java
Outdated
Show resolved
Hide resolved
…ezirkid---briefwahlservice
…ntfernt um sauber nur mit jwt und der dazugehörigen security zu arbeiten
…-keine-dummybezirkid---briefwahlservice' into 889-controllerintegrationtests---keine-dummybezirkid---briefwahlservice
…um sauber nur mit jwt und der dazugehörigen security zu arbeiten
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/wahlbriefdaten/WahlbriefdatenControllerIntegrationTest.java`:
- Around line 181-197: In the test method
should_returnForbidden_when_wahlBezirkIdIsWrong update the JWT setup to mirror
the GET forbidden test: remove the unnecessary SecurityUtils.runWith(...) call,
set the granted authority in the jwt() post-processor to
Authorities.SERVICE_POST_WAHLBRIEFDATEN (not SERVICE_GET...), and use the claim
name "wahlbezirkID" with a different value than the path variable (e.g.,
jwt.claim("wahlbezirkID", someOtherId)) so the request truly fails due to a
mismatched Bezirk ID rather than a missing claim.
🧹 Nitpick comments (4)
wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/wahlbriefdaten/WahlbriefdatenControllerIntegrationTest.java (1)
36-36: Remove commented-out profile reference.The trailing comment
//, Profiles.NO_BEZIRKS_ID_CHECK})is leftover from the migration and should be removed for code cleanliness.🧹 Proposed cleanup
-@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})//, Profiles.NO_BEZIRKS_ID_CHECK}) +@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})wls-briefwahl-service/src/test/java/de/muenchen/oss/wahllokalsystem/briefwahlservice/rest/beanstandetewahlbriefe/BeanstandeteWahlbriefeControllerIntegrationTest.java (3)
38-38: Remove commented-out profile reference.The trailing comment
/*, "dummy.nobezirkid.check"})*/is leftover from the migration and should be removed for code cleanliness.🧹 Proposed cleanup
-@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})/*, "dummy.nobezirkid.check"})*/ +@ActiveProfiles(profiles = {SPRING_TEST_PROFILE})
206-216: Minor formatting issues in the forbidden test.The test logic correctly verifies forbidden access when
wahlbezirkIDclaim doesn't match the path parameter. However:
- Indentation is inconsistent with other test methods in this class (extra leading spaces).
- Line 215:
.andReturn()is unnecessary since the result isn't used.🧹 Proposed cleanup
- `@Test` - void should_returnForbidden_when_wahlBezirkIdIsWrong() throws Exception { - val request = get("/businessActions/beanstandeteWahlbriefe/wahlbezirkID/0") - .with(jwt() - .authorities(new SimpleGrantedAuthority(Authorities.SERVICE_GET_BEANSTANDETE_WAHLBRIEFE), - new SimpleGrantedAuthority(Authorities.REPOSITORY_READ_BEANSTANDETE_WAHLBRIEFE)) - .jwt(jwt -> jwt.claim("wahlbezirkID", "wahlbezirkID1")) - ); - - api.perform(request).andExpect(status().isForbidden()).andReturn(); - } + `@Test` + void should_returnForbidden_when_wahlBezirkIdIsWrong() throws Exception { + val request = get("/businessActions/beanstandeteWahlbriefe/wahlbezirkID/0") + .with(jwt() + .authorities(new SimpleGrantedAuthority(Authorities.SERVICE_GET_BEANSTANDETE_WAHLBRIEFE), + new SimpleGrantedAuthority(Authorities.REPOSITORY_READ_BEANSTANDETE_WAHLBRIEFE)) + .jwt(jwt -> jwt.claim("wahlbezirkID", "wahlbezirkID1")) + ); + + api.perform(request).andExpect(status().isForbidden()); + }
302-320: Minor formatting issues in the forbidden test.The test logic is correct. Same formatting issues as the GET forbidden test:
- Inconsistent indentation (extra leading spaces).
- Extra blank lines at 316-317.
🧹 Proposed cleanup
- `@Test` - void should_returnForbidden_when_wahlBezirkIdIsWrong() throws Exception { - val requestBody = BeanstandeteWahlbriefeCreateDTO.builder().build(); - val request = - post("/businessActions/beanstandeteWahlbriefe/wahlbezirkID/0") - .with(jwt() - .authorities(new SimpleGrantedAuthority(Authorities.SERVICE_ADD_BEANSTANDETE_WAHLBRIEFE), - new SimpleGrantedAuthority(Authorities.REPOSITORY_WRITE_BEANSTANDETE_WAHLBRIEFE)) - .jwt(jwt -> jwt.claim("wahlbezirkID", "wahlbezirkID1")) - ) - .with(csrf()) - .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(requestBody)); - - - api.perform(request).andExpect(status().isForbidden()); - - } + `@Test` + void should_returnForbidden_when_wahlBezirkIdIsWrong() throws Exception { + val requestBody = BeanstandeteWahlbriefeCreateDTO.builder().build(); + val request = + post("/businessActions/beanstandeteWahlbriefe/wahlbezirkID/0") + .with(jwt() + .authorities(new SimpleGrantedAuthority(Authorities.SERVICE_ADD_BEANSTANDETE_WAHLBRIEFE), + new SimpleGrantedAuthority(Authorities.REPOSITORY_WRITE_BEANSTANDETE_WAHLBRIEFE)) + .jwt(jwt -> jwt.claim("wahlbezirkID", "wahlbezirkID1")) + ) + .with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(requestBody)); + + api.perform(request).andExpect(status().isForbidden()); + }
...okalsystem/briefwahlservice/rest/wahlbriefdaten/WahlbriefdatenControllerIntegrationTest.java
Outdated
Show resolved
Hide resolved
| webEnvironment = SpringBootTest.WebEnvironment.MOCK) | ||
| @AutoConfigureMockMvc | ||
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE, "dummy.nobezirkid.check"}) | ||
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE}) /* , "dummy.nobezirkid.check"}) */ |
There was a problem hiding this comment.
bitte auskommentierten code vermeiden
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE}) /* , "dummy.nobezirkid.check"}) */ | |
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE}) |
| webEnvironment = SpringBootTest.WebEnvironment.MOCK) | ||
| @AutoConfigureMockMvc | ||
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE, Profiles.NO_BEZIRKS_ID_CHECK}) | ||
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE}) // , Profiles.NO_BEZIRKS_ID_CHECK}) |
There was a problem hiding this comment.
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE}) // , Profiles.NO_BEZIRKS_ID_CHECK}) | |
| @ActiveProfiles(profiles = {SPRING_TEST_PROFILE}) |
| .with( | ||
| jwt() | ||
| .authorities( | ||
| new SimpleGrantedAuthority(Authorities.SERVICE_GET_WAHLBRIEFDATEN)) |
There was a problem hiding this comment.
ich glaube das ist hier die falsche authority
| new SimpleGrantedAuthority(Authorities.SERVICE_GET_WAHLBRIEFDATEN)) | |
| new SimpleGrantedAuthority(Authorities.SERVICE_POST_WAHLBRIEFDATEN)) |
| Authorities.REPOSITORY_READ_BEANSTANDETE_WAHLBRIEFE)) | ||
| .jwt(jwt -> jwt.claim("wahlbezirkID", "wahlbezirkID1"))); | ||
|
|
||
| api.perform(request).andExpect(status().isForbidden()).andReturn(); |
There was a problem hiding this comment.
| api.perform(request).andExpect(status().isForbidden()).andReturn(); | |
| api.perform(request).andExpect(status().isForbidden()); |
2 participants
Beschreibung:
überarbeiten des BeanstandeteWahlbriefeControllerIntegrationTest und erweiterung um zwei tests negativtests
überarbeiten des WahlbriefdatenControllerIntegrationTest und erweiterung um zwei tests negativtests
Beim überarbeiten wurde darauf geachtet das nun tatsächlich die BezirkID geprüft wird und nicht nur gemockt. Somit wurde die Anforderung des Tickets erfüllt, da nun sowohl als positiv und negativ fall getestet wird.
Definition of Done (DoD):
ControllerIntegrationTests verwenden die BezirkIDPermissionEvaluatorImpl-Klasse und keinen Dummy
Es gibt Tests die sowohl den Positiv- als auch den Negativ-Fall einer Prüfung durch BezirkIDPermissionEvaluatorImpl belegen
Backend
Referenzen1:
Verwandt mit Issue ControllerIntegrationTests - keine DummyBezirkID - BriefwahlService #889
Closes #889
Summary by CodeRabbit
Footnotes
Nicht zutreffende Referenzen vor dem Speichern entfernen ↩