fix: ensure admin password hash is updated with {bcrypt} prefix#106
fix: ensure admin password hash is updated with {bcrypt} prefix#106Tyreviel wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes implement conditional GitHub OAuth2 authentication support. The system now reads GitHub OAuth credentials from environment variables and conditionally enables the GitHub login option based on their presence. Additionally, environment variable loading via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/example/projektarendehantering/infrastructure/security/LoginAttemptServiceTest.java (1)
40-51:⚠️ Potential issue | 🔴 CriticalAssertion contradicts
recordSuccesscontract — test will fail.With
threshold=2, the tworecordFailure("user@example.com", "127.0.0.1")calls lock the key at both the email level AND the IP level (seeLoginAttemptService.recordFailure, which registers failures independently for each dimension).recordSuccessis explicitly documented to clear only the email-level counter and to intentionally leave IP-level counters intact to avoid resetting brute-force protection for a shared/NATted IP.currentLockDecisionreturnslocked = max(emailRetry, ipRetry) > 0, so the IP-level lock still causeslocked()to returntrue. The new assertion on line 50 will therefore fail against the current implementation.To truly cover the "email-level counter was cleared" behavior while respecting the contract, verify the post-success state from a different IP (IP-level lock for
127.0.0.1still applies), and—if you want to preserve the IP-persistence coverage the previous version had—keep an assertion that127.0.0.1remains locked.🛠️ Proposed fix
service.recordSuccess("user@example.com", "127.0.0.1"); - assertThat(service.currentLockDecision("user@example.com", "127.0.0.1").locked()).isFalse(); + // Email-level counter was cleared: same account from a different IP is no longer locked. + assertThat(service.currentLockDecision("user@example.com", "127.0.0.2").locked()).isFalse(); + // IP-level counter is intentionally preserved to protect against shared/NATted IPs. + assertThat(service.currentLockDecision("user@example.com", "127.0.0.1").locked()).isTrue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/infrastructure/security/LoginAttemptServiceTest.java` around lines 40 - 51, The test assumes recordSuccess clears both email and IP counters but recordSuccess only clears the email-level counter; update the assertions to reflect that: after calling LoginAttemptService.recordSuccess("user@example.com","127.0.0.1") verify the email-level retry is cleared by calling currentLockDecision for the same email but a different IP (e.g., another IP) and asserting locked() is false, and also preserve coverage that IP-level counters remain by asserting currentLockDecision("user@example.com","127.0.0.1").locked() remains true; locate the logic around LoginAttemptService.recordFailure, recordSuccess, and currentLockDecision in the test to change the two post-success assertions accordingly.
🧹 Nitpick comments (2)
src/main/resources/templates/login/login.html (1)
62-64: Prefer Thymeleaf URL rewriting for consistency.The other anchors on this page use
th:href="@{...}"(e.g. line 48). Hardcodinghref="/oauth2/authorization/github"bypasses context-path rewriting and breaks if the app is ever deployed under a non-root context.- <a th:if="${githubOauthEnabled}" href="/oauth2/authorization/github" class="button button-secondary" style="width: 100%;"> + <a th:if="${githubOauthEnabled}" th:href="@{/oauth2/authorization/github}" class="button button-secondary" style="width: 100%;"> Sign in with GitHub </a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/login/login.html` around lines 62 - 64, The GitHub sign-in anchor uses a hardcoded href which bypasses Thymeleaf context-path rewriting; update the anchor element (the <a> with th:if="${githubOauthEnabled}" and class "button button-secondary") to use Thymeleaf URL rewriting (th:href="@{/oauth2/authorization/github}") instead of href="/oauth2/authorization/github" so the link respects the application's context path and deployment location.start-local.ps1 (1)
38-43: Consider stripping surrounding quotes in.envvalues.Many
.envguides/tools write values asGITHUB_CLIENT_SECRET="abc123"or'abc123'. The current parser keeps the quotes verbatim, which Spring will then pass as a literal"abc123"to GitHub's OAuth endpoint, producing a confusing 401 rather than an obvious configuration error. Stripping a matched pair of surrounding quotes is cheap insurance.♻️ Proposed tweak
$key = $parts[0].Trim() $value = $parts[1].Trim() # Handle accidental trailing semicolons in .env values. if ($value.EndsWith(";")) { $value = $value.Substring(0, $value.Length - 1).Trim() } + # Strip a matched pair of surrounding single or double quotes. + if (($value.Length -ge 2) -and + (($value.StartsWith('"') -and $value.EndsWith('"')) -or + ($value.StartsWith("'") -and $value.EndsWith("'")))) { + $value = $value.Substring(1, $value.Length - 2) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@start-local.ps1` around lines 38 - 43, The parser currently trims trailing semicolons but preserves literal surrounding quotes in values ($value). Update the parsing logic around the $value handling (after trimming and semicolon removal) to detect and remove a matched pair of surrounding single or double quotes: if $value starts and ends with the same quote character (' or ") and length >= 2, strip those outer quotes and re-trim the result so values like "abc" or 'abc' become abc while leaving unmatched or internal quotes intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/resources/application.properties`:
- Around line 18-19: The OAuth2 client registration entries lack the ":none"
sentinel defaults causing Spring to bind literal placeholders into
OAuth2ClientProperties when env vars are absent; revert the change by restoring
the ":none" fallback for both
spring.security.oauth2.client.registration.github.client-id and
spring.security.oauth2.client.registration.github.client-secret so that missing
GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET evaluate to "none" (matching
AuthController#isGithubOauthEnabled() behavior) and prevent an invalid active
client registration.
In `@src/main/resources/templates/login/login.html`:
- Around line 65-68: The login page currently shows raw config names when GitHub
SSO is disabled (the div with th:unless="${githubOauthEnabled}" contains
GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET); replace that user-facing message
with a neutral notice (e.g., "Single sign-on is not available") and remove the
hardcoded environment variable names from the template, and instead log a
detailed setup hint (including GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET)
server-side or expose it on an admin-only diagnostics page (use the same
githubOauthEnabled check to gate admin visibility).
---
Outside diff comments:
In
`@src/test/java/org/example/projektarendehantering/infrastructure/security/LoginAttemptServiceTest.java`:
- Around line 40-51: The test assumes recordSuccess clears both email and IP
counters but recordSuccess only clears the email-level counter; update the
assertions to reflect that: after calling
LoginAttemptService.recordSuccess("user@example.com","127.0.0.1") verify the
email-level retry is cleared by calling currentLockDecision for the same email
but a different IP (e.g., another IP) and asserting locked() is false, and also
preserve coverage that IP-level counters remain by asserting
currentLockDecision("user@example.com","127.0.0.1").locked() remains true;
locate the logic around LoginAttemptService.recordFailure, recordSuccess, and
currentLockDecision in the test to change the two post-success assertions
accordingly.
---
Nitpick comments:
In `@src/main/resources/templates/login/login.html`:
- Around line 62-64: The GitHub sign-in anchor uses a hardcoded href which
bypasses Thymeleaf context-path rewriting; update the anchor element (the <a>
with th:if="${githubOauthEnabled}" and class "button button-secondary") to use
Thymeleaf URL rewriting (th:href="@{/oauth2/authorization/github}") instead of
href="/oauth2/authorization/github" so the link respects the application's
context path and deployment location.
In `@start-local.ps1`:
- Around line 38-43: The parser currently trims trailing semicolons but
preserves literal surrounding quotes in values ($value). Update the parsing
logic around the $value handling (after trimming and semicolon removal) to
detect and remove a matched pair of surrounding single or double quotes: if
$value starts and ends with the same quote character (' or ") and length >= 2,
strip those outer quotes and re-trim the result so values like "abc" or 'abc'
become abc while leaving unmatched or internal quotes intact.
🪄 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: 3c074e8c-308d-4560-abd5-c8c6715b1e85
📒 Files selected for processing (6)
src/main/java/org/example/projektarendehantering/presentation/web/AuthController.javasrc/main/resources/application.propertiessrc/main/resources/data-local.sqlsrc/main/resources/templates/login/login.htmlsrc/test/java/org/example/projektarendehantering/infrastructure/security/LoginAttemptServiceTest.javastart-local.ps1
| spring.security.oauth2.client.registration.github.client-id=${GITHUB_CLIENT_ID} | ||
| spring.security.oauth2.client.registration.github.client-secret=${GITHUB_CLIENT_SECRET} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Spring Boot application.properties unresolved placeholder default behavior strict or literal
💡 Result:
In Spring Boot, the default behavior for unresolved placeholders (${...}) in application.properties depends on the context of usage. For properties loaded into @ConfigurationProperties beans: Unresolved placeholders result in the literal placeholder string being used as the value (not strict failure). This is intentional for flexibility, as confirmed in Spring Boot GitHub issues. Application startup succeeds, but the value remains as-is (e.g., ${missing}). For @Value annotations or SpEL expressions: Unresolved placeholders cause a strict failure with IllegalArgumentException: Could not resolve placeholder..., preventing startup unless configured otherwise. Spring Boot auto-configures PropertySourcesPlaceholderConfigurer with ignoreUnresolvablePlaceholders=false by default (strict for its scope, like bean definitions and @Value). However, for @ConfigurationProperties binding (via Binder/PropertySourcesPlaceholdersResolver), placeholders are resolved leniently during binding, catching exceptions and preserving literals. This distinction has been consistent across versions, including Spring Boot 3.x.
Citations:
- 1: https://stackoverflow.com/questions/43493346/unresolved-placeholder-validation-for-spring-boot-configuration-properties
- 2: Spring Boot configuration doesn't fail on unresolved placeholders spring-projects/spring-boot#10463
- 3: https://docs.spring.io/spring-boot/3.4/reference/features/external-config.html
- 4: https://docs.spring.io/spring-framework/reference/7.1-SNAPSHOT/core/beans/annotation-config/value-annotations.html
- 5: https://github.com/spring-projects/spring-framework/blob/master/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java
- 6: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/beans/factory/config/PlaceholderConfigurerSupport.html
- 7: https://docs.spring.io/spring-boot/3.4.9/reference/features/external-config.html
🏁 Script executed:
fd -type f "AuthController.java" -o "pom.xml" -o "build.gradle" | head -20Repository: ithsjava25/project-backend-code-trauma-team
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Search for AuthController, pom.xml, and build.gradle
echo "=== Searching for AuthController.java ==="
find . -name "AuthController.java" -type f
echo -e "\n=== Searching for pom.xml ==="
find . -name "pom.xml" -type f
echo -e "\n=== Searching for build.gradle ==="
find . -name "build.gradle*" -type fRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Get Spring Boot version from pom.xml
echo "=== Spring Boot Version ==="
grep -A 5 "<spring-boot.version>" pom.xml || grep -A 5 "spring-boot-starter-parent" pom.xml | head -10
echo -e "\n=== AuthController.java (around isGithubOauthEnabled method) ==="
wc -l ./src/main/java/org/example/projektarendehantering/presentation/web/AuthController.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 530
🏁 Script executed:
cat -n ./src/main/java/org/example/projektarendehantering/presentation/web/AuthController.javaRepository: ithsjava25/project-backend-code-trauma-team
Length of output: 3104
Restore the :none fallback — missing defaults misconfigure the OAuth2 client registration.
Removing the default means that when GITHUB_CLIENT_ID / GITHUB_CLIENT_SECRET environment variables aren't set, Spring's placeholder resolver will preserve the literal strings ${GITHUB_CLIENT_ID} and ${GITHUB_CLIENT_SECRET} in the OAuth2ClientProperties binding (Spring's @ConfigurationProperties resolution is lenient, not strict). This creates an active OAuth2 client registration with invalid placeholder strings as credentials, breaking any code path that attempts to use them.
Although AuthController#isGithubOauthEnabled() (lines 58–63) is protected by its own @Value("...:none") default and will correctly return false, the underlying OAuth2ClientProperties bean that Spring's OAuth2 auto-configuration binds will hold the broken literal values. Restore the sentinel default to ensure everything stays consistent:
🐛 Proposed fix
-spring.security.oauth2.client.registration.github.client-id=${GITHUB_CLIENT_ID}
-spring.security.oauth2.client.registration.github.client-secret=${GITHUB_CLIENT_SECRET}
+spring.security.oauth2.client.registration.github.client-id=${GITHUB_CLIENT_ID:none}
+spring.security.oauth2.client.registration.github.client-secret=${GITHUB_CLIENT_SECRET:none}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spring.security.oauth2.client.registration.github.client-id=${GITHUB_CLIENT_ID} | |
| spring.security.oauth2.client.registration.github.client-secret=${GITHUB_CLIENT_SECRET} | |
| spring.security.oauth2.client.registration.github.client-id=${GITHUB_CLIENT_ID:none} | |
| spring.security.oauth2.client.registration.github.client-secret=${GITHUB_CLIENT_SECRET:none} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/application.properties` around lines 18 - 19, The OAuth2
client registration entries lack the ":none" sentinel defaults causing Spring to
bind literal placeholders into OAuth2ClientProperties when env vars are absent;
revert the change by restoring the ":none" fallback for both
spring.security.oauth2.client.registration.github.client-id and
spring.security.oauth2.client.registration.github.client-secret so that missing
GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET evaluate to "none" (matching
AuthController#isGithubOauthEnabled() behavior) and prevent an invalid active
client registration.
| <div th:unless="${githubOauthEnabled}" class="panel panel-error" style="margin-top: 12px;"> | ||
| GitHub SSO is not configured in this environment. Set <code>GITHUB_CLIENT_ID</code> and | ||
| <code>GITHUB_CLIENT_SECRET</code> to enable employee login. | ||
| </div> |
There was a problem hiding this comment.
Avoid leaking internal configuration names to end users.
<div th:unless="${githubOauthEnabled}"> renders on the public login page for any visitor whenever GitHub is unconfigured (including misconfigured production). Hardcoding the env var names into the end-user error message is both confusing for real users and unnecessary internal disclosure. Recommend showing a neutral message to users and surfacing the setup hint via logs or an admin-only page.
🛡️ Suggested softer copy
- <div th:unless="${githubOauthEnabled}" class="panel panel-error" style="margin-top: 12px;">
- GitHub SSO is not configured in this environment. Set <code>GITHUB_CLIENT_ID</code> and
- <code>GITHUB_CLIENT_SECRET</code> to enable employee login.
- </div>
+ <div th:unless="${githubOauthEnabled}" class="panel panel-error" style="margin-top: 12px;">
+ Employee sign-in is temporarily unavailable. Please contact your administrator.
+ </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/login/login.html` around lines 65 - 68, The
login page currently shows raw config names when GitHub SSO is disabled (the div
with th:unless="${githubOauthEnabled}" contains GITHUB_CLIENT_ID and
GITHUB_CLIENT_SECRET); replace that user-facing message with a neutral notice
(e.g., "Single sign-on is not available") and remove the hardcoded environment
variable names from the template, and instead log a detailed setup hint
(including GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET) server-side or expose it on an
admin-only diagnostics page (use the same githubOauthEnabled check to gate admin
visibility).
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores