Update JDBC basicAuth example #2547
Conversation
|
Warning Rate limit exceeded@christiangoerdes has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 57 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCentralized password-hashing logic into SecurityUtils, tightened hash parsing/validation, removed duplicate hashing code, updated tests for static imports, and replaced the H2 example with a PostgreSQL container-based example plus configs and seed SQL. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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. 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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![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
🤖 Fix all issues with AI Agents
In
@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java:
- Around line 130-132: The authentication in JdbcUserDataProvider currently
accepts pre-hashed password inputs by comparing them directly (see
isHashedPassword, createPasswdCompatibleHash, extractMagicString, extractSalt
and the username/password equality check); add an explicit rejection for
hashed-form inputs at the start of the verification path (e.g., if
isHashedPassword(password) then throw/return a NoSuchElementException or
otherwise deny authentication) so only plain-text passwords are accepted and
then rehashed via createPasswdCompatibleHash before comparison against the
stored passwordFromDB.
In @distribution/examples/security/basic-auth/database/insert_users.sql:
- Around line 6-8: The INSERT in insert_users.sql stores plaintext passwords and
mismatches the README credentials; replace those plaintext values with
crypt(3)-formatted hashes (matching SecurityUtils.isHashedPassword()
expectation) and ensure the 'membrane' row uses the password that README expects
(or update README to match): generate SHA-512 crypt hashes (e.g., via Python
crypt.crypt or mkpasswd) for the chosen cleartext passwords (pass123 and gateway
or whichever you pick) and substitute the VALUES tuples in the INSERT with the
resulting $id$salt$hash strings so authentication succeeds.
In @distribution/examples/security/basic-auth/database/README.md:
- Around line 35-44: README.md currently tells users to login as user 'membrane'
with password 'gateway', but insert_users.sql inserts 'membrane' with password
'proxy' and stores plaintext passwords; fix this by updating insert_users.sql to
insert a hashed password that matches the README (e.g., hash of "gateway") and
replace the plaintext 'proxy' value with that hash in the INSERT for user
'membrane' (refer to the INSERT INTO users statement in insert_users.sql), or
alternatively update README.md to match the inserted value; prefer updating
insert_users.sql to use a secure hashing function and the hashed "gateway"
password so the example credentials in README.md remain correct.
🧹 Nitpick comments (4)
distribution/examples/security/basic-auth/database/README.md (2)
5-10: LGTM with optional formatting fix.The prerequisite section is clear and helpful.
Optionally, fix the list indentation to align with markdown best practices:
🔎 Proposed formatting fix
### Prerequisite - **Docker installed:** - - If Docker is already installed, skip to the next step. - - Otherwise, install Docker from [https://docs.docker.com/get-started/get-docker/](https://docs.docker.com/get-started/get-docker/). + - If Docker is already installed, skip to the next step. + - Otherwise, install Docker from [https://docs.docker.com/get-started/get-docker/](https://docs.docker.com/get-started/get-docker/).
40-40: Optional: Format URL as markdown link.The bare URL could be formatted as a proper markdown link for better consistency.
🔎 Proposed fix
-5. Open the URL http://localhost:2000 in your browser. +5. Open the URL [http://localhost:2000](http://localhost:2000) in your browser.core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java (1)
61-67: Good error handling improvement, consider applying to extractSalt too.The try-catch wrapper with a clear error message is a significant improvement over letting array access exceptions bubble up.
Consider applying the same error handling pattern to
extractSalt(line 49-51) for consistency:🔎 Proposed enhancement for extractSalt
public static String extractSalt(String password) { - return password.split(Pattern.quote("$"))[2]; + try { + return password.split(Pattern.quote("$"))[2]; + } catch (Exception e) { + throw new RuntimeException("Password must be in hash notation", e); + } }core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (1)
30-31: Remove redundant import.Line 31 explicitly imports
isHashedPassword, but line 30 already imports all static members fromSecurityUtilsvia the wildcard import. The specific import is unnecessary.🔎 Proposed fix
import static com.predic8.membrane.core.interceptor.registration.SecurityUtils.*; -import static com.predic8.membrane.core.interceptor.registration.SecurityUtils.isHashedPassword;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.javacore/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProviderTest.javadistribution/examples/security/basic-auth/database/README.mddistribution/examples/security/basic-auth/database/apis.yamldistribution/examples/security/basic-auth/database/insert_users.sql
🧰 Additional context used
🧬 Code graph analysis (3)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java (1)
SecurityUtils(23-68)
core/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProviderTest.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java (1)
SecurityUtils(23-68)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java (1)
SecurityUtils(23-68)
🪛 markdownlint-cli2 (0.18.1)
distribution/examples/security/basic-auth/database/README.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
9-9: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
40-40: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (8)
distribution/examples/security/basic-auth/database/insert_users.sql (1)
1-4: LGTM!The table schema is correctly defined and aligns with the configuration in
apis.yaml(tableName: users, userColumnName: nickname, passwordColumnName: password).distribution/examples/security/basic-auth/database/apis.yaml (2)
1-17: LGTM!The PostgreSQL dataSource configuration is correct. The hardcoded credentials (user/password) are acceptable for example/demo purposes. Ensure users are reminded to use proper credential management (environment variables, secrets management) in production.
21-31: LGTM!The API configuration correctly references the dataSource and specifies the appropriate table and column names that match the schema defined in
insert_users.sql.distribution/examples/security/basic-auth/database/README.md (1)
14-33: LGTM!The setup instructions are clear and well-structured. The Docker commands and JDBC driver installation steps are correct.
core/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProviderTest.java (1)
19-29: LGTM!The refactoring to use
SecurityUtils.isHashedPasswordvia static import is clean and aligns with the centralization of password utilities. The test hashes are in the correct crypt(3) format and the test logic remains unchanged.core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java (1)
27-37: LGTM!The enhanced validation in
isHashedPasswordsignificantly strengthens hash format verification:
- Enforces exactly 4 segments when split on "$"
- Validates the leading "$" (first segment must be empty)
- Ensures the hash portion is substantive (>= 20 characters)
- Adds final pattern validation for the complete format
This hardening prevents accepting malformed password hashes without breaking compatibility with valid crypt(3) format passwords.
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (1)
128-128: LGTM!Using
isEmpty()is more idiomatic thansize() > 0and makes the intent clearer.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (1)
23-24: LGTM! Excellent refactoring.The migration from local hashing methods to the centralized
SecurityUtilseliminates code duplication and improves maintainability. The static imports are clean and specific, and the behavior remains consistent with the previous implementation.Also applies to: 66-66
.../java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java
Show resolved
Hide resolved
distribution/examples/security/basic-auth/database/insert_users.sql
Outdated
Show resolved
Hide resolved
|
@coderabbitai configuration |
|
Configuration used: defaults CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
inheritance: false
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_instructions: ''
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: true
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: true
poem: true
enable_prompt_for_ai_agents: true
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
fortitudeLint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
issue_enrichment:
auto_enrich:
enabled: false
planning:
enabled: true
auto_planning:
enabled: true
labels: []
labeling:
labeling_instructions: []
auto_apply_labels: false
|
core/src/main/java/com/predic8/membrane/core/interceptor/registration/SecurityUtils.java
Outdated
Show resolved
Hide resolved
.../java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java
Outdated
Show resolved
Hide resolved
.../java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.