Open
Conversation
…details setup, update authentication UI in header fragment. Add mock user to test environment.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds GitHub OAuth2 login: new env template and gitignore entries, Maven OAuth2 client dependency, Spring Security configuration with OAuth2/form login and an in-memory admin user, application/test properties for GitHub client, and header template switched to server-driven auth controls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser
participant App as App (Spring Boot)
participant GitHub as GitHub OAuth
Browser->>App: GET /protected-resource
App->>Browser: 302 Redirect to /oauth2/authorization/github
Browser->>GitHub: GET /authorize (client_id, redirect_uri, scope)
GitHub->>Browser: 302 Redirect back with code
Browser->>App: GET /login/oauth2/code/github?code=...
App->>GitHub: POST /token (code, client_secret)
GitHub->>App: 200 OK (access_token)
App->>GitHub: GET /user (Authorization: Bearer ...)
GitHub->>App: 200 OK (user info)
App->>App: Authenticate user, create session
App->>Browser: 302 Redirect to /
Browser->>App: GET / (authenticated)
App->>Browser: 200 OK (protected content)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🧹 Nitpick comments (3)
src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java (2)
20-22: Consider adding "/" to permitAll for unauthenticated landing page access.The
defaultSuccessUrl("/", true)redirects authenticated users to/, but/is not in thepermitAll()list. If the index page should be publicly accessible (e.g., as a landing page), add it to the allowed paths. Otherwise, unauthenticated users will be redirected to login when visiting the root URL.Proposed fix if index should be public
.authorizeHttpRequests(authorize -> authorize - .requestMatchers("/login**", "/error**", "/static/**", "/app.css", "/app.js").permitAll() + .requestMatchers("/", "/login**", "/error**", "/static/**", "/app.css", "/app.js").permitAll() .anyRequest().authenticated() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java` around lines 20 - 22, In SecurityConfig inside the authorizeHttpRequests configuration update the requestMatchers call to include the root path "/" so the unauthenticated landing page is allowed; specifically, modify the list used in requestMatchers("/login**", "/error**", "/static/**", "/app.css", "/app.js") to also contain "/" (this aligns with the defaultSuccessUrl("/", true) behavior) so that anyRequest().authenticated() won't block the index page.
17-34: CSRF protection will block POST requests from non-browser API clients.CSRF protection is enabled by default in Spring Security. The
POST /api/casesendpoint will reject requests from non-browser clients (e.g., external API consumers, curl, Postman) unless they provide a CSRF token, which is impractical for API usage.Add CSRF exception for API paths:
http .authorizeHttpRequests(authorize -> authorize .requestMatchers("/login**", "/error**", "/static/**", "/app.css", "/app.js").permitAll() .anyRequest().authenticated() ) + .csrf(csrf -> csrf + .ignoringRequestMatchers("/api/**") + ) .oauth2Login(oauth2 -> oauth2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java` around lines 17 - 34, The security configuration currently enforces CSRF for all requests in the SecurityFilterChain (method securityFilterChain), which will block non-browser POSTs to /api/cases; update the HttpSecurity setup to exclude API paths from CSRF by adding a csrf configuration that ignores the API request matcher (e.g., configure csrf to ignore "/api/**" using AntPathRequestMatcher or ignoringRequestMatchers) so that requests to /api/** are not subject to CSRF while leaving CSRF enabled for browser endpoints.src/main/resources/application.properties (1)
15-15: Consider reducing log level for production.DEBUG level logging for Spring Security is verbose and may expose sensitive authentication details in logs. Consider using INFO or WARN for production, or making this configurable via profiles.
Consider profile-based configuration
-logging.level.org.springframework.security=DEBUG +# Uncomment for debugging authentication issues (not recommended for production) +# logging.level.org.springframework.security=DEBUGOr use a separate
application-dev.propertiesfor development-only settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application.properties` at line 15, The current property logging.level.org.springframework.security=DEBUG is too verbose for production; change it to a less verbose level (e.g., INFO or WARN) or make it profile-configurable so DEBUG is only enabled for development. Update the property value from DEBUG to INFO/WARN, and/or move this setting into a development-specific profile (e.g., application-dev.properties or a Spring profile property) so production uses a safer default; reference the logging property key logging.level.org.springframework.security when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 103-106: The pom dependency uses an incorrect artifactId
"spring-boot-starter-security-oauth2-client"; update the Maven dependency in the
pom.xml so the <artifactId> value for the org.springframework.boot OAuth2 client
dependency is "spring-boot-starter-oauth2-client" (replace the incorrect
artifactId string in that dependency block).
In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`:
- Around line 36-44: The userDetailsService bean in SecurityConfig exposes an
in-memory user with a plaintext {noop}password which is unsafe for production;
update userDetailsService to use a secure approach: replace the hardcoded noop
password by wiring a PasswordEncoder (e.g., BCryptPasswordEncoder) and encode
the password, externalize the actual credentials into environment properties
(use `@Value` or a properties-backed UserDetails creation) or disable/register
this bean only for development by annotating the SecurityConfig or the
userDetailsService method with `@Profile`("dev") and importing
org.springframework.context.annotation.Profile, ensuring production profiles use
a real user store or secret-managed credentials.
In `@src/main/resources/application.properties`:
- Around line 12-13: The properties
spring.security.oauth2.client.registration.github.client-id and
spring.security.oauth2.client.registration.github.client-secret currently rely
solely on environment variables and can cause startup failures if missing;
update application.properties to supply safe fallbacks (e.g., use the Spring
placeholder syntax with defaults like ${GITHUB_CLIENT_ID:} and
${GITHUB_CLIENT_SECRET:} or provide clearly invalid sentinel values) or add a
startup validation that throws a clear error if those values are unset, and also
add a README note describing the required GITHUB_CLIENT_ID and
GITHUB_CLIENT_SECRET environment variables so operators know to set them.
In `@src/main/resources/templates/fragments/header.html`:
- Around line 16-27: Remove the stray trailing "│" characters from the Thymeleaf
template so they don't render in output: edit the block containing
th:if="${`#authorization.expression`('isAuthenticated()')}" (including the span
with class="auth-label" and the form with th:action="@{/logout}") and the
subsequent th:unless="${`#authorization.expression`('isAuthenticated()')}" link,
deleting the trailing artifact characters at the ends of those lines so only
valid HTML/Thymeleaf markup remains.
In `@src/test/resources/application.properties`:
- Around line 19-21: The OAuth2 client property values contain stray trailing
'?' characters which will be read into the configuration; remove the '?'
characters from the values for the keys
spring.security.oauth2.client.registration.github.client-id and
spring.security.oauth2.client.registration.github.client-secret so the
properties become plain "mock-id" and "mock-secret" (leave the scope line
as-is).
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`:
- Around line 20-22: In SecurityConfig inside the authorizeHttpRequests
configuration update the requestMatchers call to include the root path "/" so
the unauthenticated landing page is allowed; specifically, modify the list used
in requestMatchers("/login**", "/error**", "/static/**", "/app.css", "/app.js")
to also contain "/" (this aligns with the defaultSuccessUrl("/", true) behavior)
so that anyRequest().authenticated() won't block the index page.
- Around line 17-34: The security configuration currently enforces CSRF for all
requests in the SecurityFilterChain (method securityFilterChain), which will
block non-browser POSTs to /api/cases; update the HttpSecurity setup to exclude
API paths from CSRF by adding a csrf configuration that ignores the API request
matcher (e.g., configure csrf to ignore "/api/**" using AntPathRequestMatcher or
ignoringRequestMatchers) so that requests to /api/** are not subject to CSRF
while leaving CSRF enabled for browser endpoints.
In `@src/main/resources/application.properties`:
- Line 15: The current property logging.level.org.springframework.security=DEBUG
is too verbose for production; change it to a less verbose level (e.g., INFO or
WARN) or make it profile-configurable so DEBUG is only enabled for development.
Update the property value from DEBUG to INFO/WARN, and/or move this setting into
a development-specific profile (e.g., application-dev.properties or a Spring
profile property) so production uses a safer default; reference the logging
property key logging.level.org.springframework.security when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2942ad1-bc06-42aa-89b5-59041d4dfec1
📒 Files selected for processing (7)
.env.template.gitignorepom.xmlsrc/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.javasrc/main/resources/application.propertiessrc/main/resources/templates/fragments/header.htmlsrc/test/resources/application.properties
src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java
Show resolved
Hide resolved
…nd rename security dependency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4
Summary by CodeRabbit
New Features
Chores