Skip to content

Better utilize Spring Security — remove reinvented wheel patterns#41

Merged
dmccoystephenson merged 2 commits intomainfrom
copilot/identify-spring-security-opportunities
Mar 15, 2026
Merged

Better utilize Spring Security — remove reinvented wheel patterns#41
dmccoystephenson merged 2 commits intomainfrom
copilot/identify-spring-security-opportunities

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 14, 2026

Three places in the codebase duplicated or misused functionality Spring Security already provides out of the box.

Changes

  • WebSocketAuthInterceptor: Removed SecurityContextHolder.getContext().setAuthentication(authentication). In STOMP context, messages are processed on Spring's messaging thread pool; SecurityContextHolder is thread-local so this call never reached the message handler. accessor.setUser(authentication) — already present — is the correct mechanism.

  • UserService.verifyPassword(): Removed. This was a one-liner wrapper around passwordEncoder.matches(), which DaoAuthenticationProvider already calls internally via authenticationManager.authenticate(). The method was never invoked in the actual auth flow.

  • UserController.login(): The Authentication result from authenticationManager.authenticate() was being discarded; the raw request string was then used to look up the user. Now uses authentication.getName() — the verified principal — for the lookup.

// Before
authenticationManager.authenticate(new UsernamePasswordAuthenticationToken(username.trim(), password));
User user = userService.findByUsername(username.trim())...

// After
Authentication authentication = authenticationManager.authenticate(
    new UsernamePasswordAuthenticationToken(username.trim(), password));
User user = userService.findByUsername(authentication.getName())...
Original prompt

identify opportunities to better utilize spring security in the project. we should not reinvent the wheel


📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

…terns

- WebSocketAuthInterceptor: remove SecurityContextHolder.setAuthentication()
  call; in WebSocket/STOMP context accessor.setUser() is the correct Spring
  Security mechanism — SecurityContextHolder is thread-local and the call
  did not carry over to the messaging thread pool
- UserService: remove verifyPassword() wrapper around PasswordEncoder.matches();
  Spring Security's DaoAuthenticationProvider (via authenticationManager
  .authenticate()) already handles password verification — no need to
  duplicate it
- UserController.login(): use Authentication.getName() returned by
  authenticationManager.authenticate() instead of re-trusting the raw
  request string for the subsequent user lookup
- UserControllerTest: update testLogin_Success mock to return a proper
  Authentication object instead of null
- UserServiceTest: remove tests for the deleted verifyPassword() method

Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Copilot AI changed the title [WIP] Identify opportunities to better utilize Spring Security Better utilize Spring Security — remove reinvented wheel patterns Mar 14, 2026
Copilot AI requested a review from dmccoystephenson March 14, 2026 19:34
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes custom authentication “wrapper” logic and relies more directly on Spring Security’s intended mechanisms for HTTP login and STOMP/WebSocket authentication propagation.

Changes:

  • Remove UserService.verifyPassword() and its unit tests (it duplicated PasswordEncoder.matches() and wasn’t part of the real auth flow).
  • Fix /login to use the verified Authentication result (authentication.getName()) rather than the raw request username for user lookup.
  • In STOMP CONNECT handling, stop setting SecurityContextHolder and rely on accessor.setUser(authentication) for per-message principal propagation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
backend/src/main/java/com/accordion/controller/UserController.java Uses the authenticated principal name from Spring Security for user lookup during login.
backend/src/test/java/com/accordion/controller/UserControllerTest.java Updates login test to mock and return an Authentication with a username.
backend/src/main/java/com/accordion/service/UserService.java Removes the unused verifyPassword() wrapper.
backend/src/test/java/com/accordion/service/UserServiceTest.java Removes tests that only exercised the deleted verifyPassword() wrapper.
backend/src/main/java/com/accordion/security/WebSocketAuthInterceptor.java Avoids thread-local SecurityContextHolder usage; continues to set the STOMP Principal via message headers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dmccoystephenson dmccoystephenson marked this pull request as ready for review March 15, 2026 00:17
@dmccoystephenson dmccoystephenson merged commit bba1160 into main Mar 15, 2026
8 checks passed
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.

3 participants