feat(client): support custom authSource for SCRAM authentication#41
Conversation
SCRAM authentication was hardcoded to run against the admin database, which fails when the MongoDB user was created in the application database itself (db.createUser() run inside the target database). Add an authSource option to Auth and a matching constructor parameter to Client so callers can authenticate against the database that actually holds the user. Defaults to admin, preserving existing behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configurable MongoDB ChangesConfigurable authSource
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant Client
participant Auth
participant MongoDB
App->>Client: __construct(..., authSource)
Client->>Auth: new Auth(authcid, secret, authSource ?? 'admin')
App->>Auth: start()
Auth->>MongoDB: saslStart with authSource
App->>Auth: continue()
Auth->>MongoDB: saslContinue with authSource
Related Issues: None specified Related PRs: None specified Suggested labels: enhancement, tests Suggested reviewers: None specified 🐰 A source once fixed, now set at will, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR makes the SCRAM authentication database configurable, fixing authentication failures for deployments where the MongoDB user is created inside the application database rather than
Confidence Score: 5/5Safe to merge — the change is backward-compatible and touches only the SCRAM auth flow. The change is minimal and well-scoped: two hardcoded strings replaced by a configurable property, with a sensible default, an optional constructor parameter, and matching test coverage. No existing behaviour is altered when the parameter is omitted. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "Apply suggestion from @greptile-apps[bot..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Auth.php (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant default duplicated in
Client.php.
Client::__constructpasses$authSource ?? 'admin'intoAuth, butAuth::__construct(Line 27) already defaults to'admin'when the key is absent/null. Having the same default hardcoded in two places risks drift if the default ever changes.Consider letting
Clientpass$authSourcethrough as-is (includingnull) and rely solely onAuth's default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Auth.php` at line 27, The default auth source is duplicated between Client::__construct and Auth::__construct, which can drift over time. Update Client to pass the authSource value through unchanged, including null, and keep the defaulting behavior only in Auth::__construct where $this->authSource is set. Use the Auth constructor and Client constructor symbols to locate the call site and remove the extra hardcoded 'admin' fallback.tests/AuthTest.php (1)
10-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a Client-level test for authSource pass-through.
These tests validate
Authdirectly, but the PR also changesClient::__constructto forwardauthSourcetoAuth. A small test (e.g., via reflection on the private$authproperty, or exposing it for testing) would verify the cross-file wiring described in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AuthTest.php` around lines 10 - 55, Add a Client-level test for authSource pass-through: the current AuthTest only covers Auth directly, but the changed Client::__construct wiring should also be verified. Create a small test around Client that confirms the private $auth instance receives the provided authSource (for example by inspecting the Client::$auth property via reflection or another test-only access path), and keep the existing Auth::start and Auth::continue coverage as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Auth.php`:
- Line 27: The default auth source is duplicated between Client::__construct and
Auth::__construct, which can drift over time. Update Client to pass the
authSource value through unchanged, including null, and keep the defaulting
behavior only in Auth::__construct where $this->authSource is set. Use the Auth
constructor and Client constructor symbols to locate the call site and remove
the extra hardcoded 'admin' fallback.
In `@tests/AuthTest.php`:
- Around line 10-55: Add a Client-level test for authSource pass-through: the
current AuthTest only covers Auth directly, but the changed Client::__construct
wiring should also be verified. Create a small test around Client that confirms
the private $auth instance receives the provided authSource (for example by
inspecting the Client::$auth property via reflection or another test-only access
path), and keep the existing Auth::start and Auth::continue coverage as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a9b542b-9e71-4bd3-99e5-ce29d8565edc
📒 Files selected for processing (3)
src/Auth.phpsrc/Client.phptests/AuthTest.php
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
What does this PR do?
SCRAM authentication (
saslStart/saslContinue) was hardcoded to run against theadmindatabase. That fails for deployments where the user is created in the application database itself (db.createUser()executed inside the target database) — a common pattern for per-database app users.Authaccepts anauthSourceoption (defaults toadmin) and runs both SCRAM steps against it.Clientgains a trailing, optionalauthSourceconstructor parameter, passed through toAuth.Fully backward compatible: omitted/null
authSourcebehaves exactly as before.Test plan
tests/AuthTest.php: asserts the default isadminand that a providedauthSourceis used for bothstart()andcontinue().mongo:8: a user created insideappdbfails SCRAM with the default (admin) and connects + writes successfully withauthSource: 'appdb'.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
adminwhen not provided).Tests