-
Notifications
You must be signed in to change notification settings - Fork 14
Upgrade to Spring Boot 2.7.18, migrate off WebSecurityConfigurerAdapter, add JaCoCo coverage #848
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| name: Build | ||
|
|
||
| on: | ||
| push: | ||
| branches: [master] | ||
| pull_request: | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: '11' | ||
| - uses: gradle/actions/setup-gradle@v3 | ||
| - name: Build, test and verify coverage | ||
| run: ./gradlew build | ||
| - name: Upload JaCoCo report | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: jacoco-report | ||
| path: build/reports/jacoco/test/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| org.gradle.jvmargs=--add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED --add-opens jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,10 @@ | |
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.security.config.annotation.web.builders.HttpSecurity; | ||
| import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; | ||
| import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; | ||
| import org.springframework.security.config.http.SessionCreationPolicy; | ||
| import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; | ||
| import org.springframework.security.crypto.password.PasswordEncoder; | ||
| import org.springframework.security.web.SecurityFilterChain; | ||
| import org.springframework.security.web.authentication.HttpStatusEntryPoint; | ||
| import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; | ||
| import org.springframework.web.cors.CorsConfiguration; | ||
|
|
@@ -20,7 +20,7 @@ | |
|
|
||
| @Configuration | ||
| @EnableWebSecurity | ||
| public class WebSecurityConfig extends WebSecurityConfigurerAdapter { | ||
| public class WebSecurityConfig { | ||
|
|
||
| @Bean | ||
| public JwtTokenFilter jwtTokenFilter() { | ||
|
Comment on lines
25
to
26
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: JwtTokenFilter registered both as security filter and servlet filter (pre-existing)
(Refers to lines 25-28) Was this helpful? React with 👍 or 👎 to provide feedback.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct — this dual registration is pre-existing behavior carried over unchanged from the adapter-based config, and |
||
|
|
@@ -32,8 +32,8 @@ public PasswordEncoder passwordEncoder() { | |
| return new BCryptPasswordEncoder(); | ||
| } | ||
|
|
||
| @Override | ||
| protected void configure(HttpSecurity http) throws Exception { | ||
| @Bean | ||
| public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | ||
|
|
||
| http.csrf() | ||
| .disable() | ||
|
|
@@ -62,6 +62,8 @@ protected void configure(HttpSecurity http) throws Exception { | |
| .authenticated(); | ||
|
|
||
| http.addFilterBefore(jwtTokenFilter(), UsernamePasswordAuthenticationFilter.class); | ||
|
|
||
| return http.build(); | ||
| } | ||
|
|
||
| @Bean | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| package io.spring.api.security; | ||
|
|
||
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; | ||
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.options; | ||
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; | ||
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||
|
|
||
| import io.spring.api.TagsApi; | ||
| import io.spring.application.TagsQueryService; | ||
| import io.spring.core.service.JwtService; | ||
| import io.spring.core.user.User; | ||
| import io.spring.core.user.UserRepository; | ||
| import io.spring.infrastructure.mybatis.readservice.UserReadService; | ||
| import java.util.Optional; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.ArgumentMatchers; | ||
| import org.mockito.Mockito; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; | ||
| import org.springframework.boot.test.mock.mockito.MockBean; | ||
| import org.springframework.context.annotation.Import; | ||
| import org.springframework.test.web.servlet.MockMvc; | ||
|
|
||
| @WebMvcTest(TagsApi.class) | ||
| @Import(WebSecurityConfig.class) | ||
| public class WebSecurityConfigTest { | ||
|
|
||
| @Autowired private MockMvc mvc; | ||
|
|
||
| @MockBean private UserRepository userRepository; | ||
| @MockBean private UserReadService userReadService; | ||
| @MockBean private JwtService jwtService; | ||
| @MockBean private TagsQueryService tagsQueryService; | ||
|
|
||
| private User user; | ||
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| user = new User("john@jacob.com", "johnjacob", "123", "", ""); | ||
| Mockito.when(userRepository.findById(ArgumentMatchers.eq(user.getId()))) | ||
| .thenReturn(Optional.of(user)); | ||
| Mockito.when(jwtService.getSubFromToken(ArgumentMatchers.eq("valid-token"))) | ||
| .thenReturn(Optional.of(user.getId())); | ||
| Mockito.when(jwtService.getSubFromToken(ArgumentMatchers.eq("invalid-token"))) | ||
| .thenReturn(Optional.empty()); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_permit_options_requests_without_token() throws Exception { | ||
| mvc.perform(options("/articles")) | ||
| .andExpect( | ||
| result -> | ||
| org.junit.jupiter.api.Assertions.assertNotEquals( | ||
| 401, result.getResponse().getStatus())); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_permit_graphql_endpoints_without_token() throws Exception { | ||
| mvc.perform(get("/graphiql")) | ||
| .andExpect( | ||
| result -> | ||
| org.junit.jupiter.api.Assertions.assertNotEquals( | ||
| 401, result.getResponse().getStatus())); | ||
| mvc.perform(post("/graphql")) | ||
| .andExpect( | ||
| result -> | ||
| org.junit.jupiter.api.Assertions.assertNotEquals( | ||
| 401, result.getResponse().getStatus())); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_permit_user_registration_and_login_without_token() throws Exception { | ||
| mvc.perform(post("/users")).andExpect(status().isNotFound()); | ||
| mvc.perform(post("/users/login")).andExpect(status().isNotFound()); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_permit_public_read_endpoints_without_token() throws Exception { | ||
| mvc.perform(get("/articles")).andExpect(status().isNotFound()); | ||
| mvc.perform(get("/articles/some-slug")).andExpect(status().isNotFound()); | ||
| mvc.perform(get("/profiles/johnjacob")).andExpect(status().isNotFound()); | ||
| mvc.perform(get("/tags")).andExpect(status().isOk()); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_reject_articles_feed_without_token() throws Exception { | ||
| mvc.perform(get("/articles/feed")).andExpect(status().isUnauthorized()); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_reject_protected_endpoints_without_token() throws Exception { | ||
| mvc.perform(get("/user")).andExpect(status().isUnauthorized()); | ||
| mvc.perform(post("/articles")).andExpect(status().isUnauthorized()); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_reject_protected_endpoints_with_invalid_token() throws Exception { | ||
| mvc.perform(get("/user").header("Authorization", "Token invalid-token")) | ||
| .andExpect(status().isUnauthorized()); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_allow_protected_endpoints_with_valid_token() throws Exception { | ||
| mvc.perform(get("/articles/feed").header("Authorization", "Token valid-token")) | ||
| .andExpect(status().isNotFound()); | ||
| mvc.perform(get("/user").header("Authorization", "Token valid-token")) | ||
| .andExpect(status().isNotFound()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: JaCoCo line-coverage threshold set well below actual baseline
The coverage gate is set to
minimum = 0.50(build.gradle:92) while the PR description states the actual baseline is ~0.53. This leaves ~3 percentage points of slack, meaning a meaningful coverage regression could pass the build unnoticed. Not a correctness bug, but worth confirming the threshold intentionally trails the baseline rather than tracking it.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional — the 0.50 threshold trails the 0.53 baseline slightly so the gate doesn't flake on small legitimate refactors while still catching meaningful regressions. Happy to tighten it (or raise it after adding GraphQL datafetcher tests) if a stricter gate is preferred.