Move BMC User Account Unit tests to go mock server#841
Conversation
📝 WalkthroughWalkthroughAccount/auth test logic was moved from in-memory helpers into the Go mock HTTP server. The mock server gained configurable Basic Auth, unavailable mode, collection POST/DELETE handling with hooks, an embedded account store with auth APIs, and mock account data was updated/expanded for tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant MockServer
participant AccountStore
Controller->>MockServer: BasicAuth request (e.g., GetAccountService / change password)
MockServer->>AccountStore: Authenticate / lookup account / apply change
AccountStore-->>MockServer: success / failure
MockServer-->>Controller: HTTP 200 / 401/403 / 503
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/bmcuser_controller_test.go (1)
295-339:⚠️ Potential issue | 🟡 MinorMove account reset to
AfterEachto ensure consistent test isolation.
ResetAccounts()is only called in this spec, while the other specs in thisDescribeblock (e.g., "should create a BMC user and secret", "should just create additional BMC users", "should rotate password ...") also create accounts on the shared mock server. Because the mock is process-global and Ginkgo can be run with--randomize-all, accounts created by earlier specs persist into later ones, which can mask regressions or cause flakes. Per theResetAccounts()docstring ("Call this inAfterEach"), move the reset to anAfterEachblock to ensure every spec in the suite starts with a clean account state.(Note:
GetAccountNames()is thread-safe—it callsloadResource()which holdsRLockand returns a copy, so there is no concurrent map access risk.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/bmcuser_controller_test.go` around lines 295 - 339, The test calls mockServers[0].ResetAccounts() only inside the "should delete BMC user and secret on User deletion" It block, which leaves state from other specs; remove that call from the spec and add an AfterEach in the Describe block that invokes mockServers[0].ResetAccounts() (or iterates all mockServers and calls ResetAccounts() on each) so every spec starts with a clean mock server state; ensure the AfterEach references the existing mockServers slice and ResetAccounts() helper.
🧹 Nitpick comments (2)
internal/controller/suite_test.go (1)
192-196: LGTM!
server.WithAuth()is consistently applied to both mock-server construction paths, andBMCReconcilernow carriesBMCOptionsmatching the other reconcilers — required for the newBasicAuthprobe inbmcConnectionTestto be exercised.Optional: the identical
bmc.Options{...}literal is repeated across seven reconciler setups. Extracting it to a sharedvar defaultBMCOptions = bmc.Options{...}would reduce duplication and make future option changes single-touch.Also applies to: 354-354, 366-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/suite_test.go` around lines 192 - 196, Multiple reconciler setups repeat the same bmc.Options literal; extract that repeated BMCOptions block into a single shared variable (e.g., defaultBMCOptions of type bmc.Options) and replace each inline bmc.Options{...} (used when constructing BMCReconciler instances and test setups exercising bmcConnectionTest) with that variable, ensuring server.WithAuth() usage remains unchanged and BMCReconciler's BMCOptions field is assigned from defaultBMCOptions.bmc/mock/server/server.go (1)
733-754:handleChangePasswordupdates only the auth store, not the resource override.
s.accounts[username] = req.NewPasswordat Line 751 keeps Basic Auth working, but the account resource'sPasswordfield ins.overridesis unchanged. A subsequentGET /redfish/v1/AccountService/Accounts/<id>will still return the original/seed password, which can confuse future tests that assert on the resource body. Also, an emptyNewPasswordor missingUserNameis silently treated as success (returns 204) — consider returning 400 to match the strict ChangePassword contract.♻️ Optional fix
if username, ok := account["UserName"].(string); ok && username != "" && req.NewPassword != "" { s.accounts[username] = req.NewPassword + account["Password"] = req.NewPassword + s.overrides[filePath] = account + } else { + http.Error(w, "missing UserName or NewPassword", http.StatusBadRequest) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/server.go` around lines 733 - 754, The handleChangePassword handler currently updates only the auth store (s.accounts) but not the account resource override, and it treats missing/empty NewPassword or UserName as success; update the function so after loading the account (using loadResourceLocked and the resolved filePath) you extract the username (from account["UserName"]) and validate req.NewPassword is non-empty and username is present, returning http.StatusBadRequest on invalid input, then set s.accounts[username] = req.NewPassword and also update the account resource in s.overrides (e.g., find/modify s.overrides[filePath] or the map entry returned by loadResourceLocked to set its "Password" field to req.NewPassword) while holding the existing mutex to keep state consistent, and finally return 204 only on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bmc/mock/server/server.go`:
- Around line 423-451: The POST handler currently in handleCollectionPost
computes new IDs using len(base.Members)+1 which can collide after non-trailing
deletes and the empty-collection branch omits setting Id/@odata.id, storing the
new resource, and calling onCreate hooks; fix by computing newID as (max
existing numeric ID)+1 by parsing each Member.OdataID (use strconv.Atoi on the
last path segment) to find the highest ID, then set newID = strconv.Itoa(max+1);
unify the two branches so you always inject update["Id"] and
update["@odata.id"], set s.overrides[newMemberPath] = update, set the Location
header when appropriate, and dispatch matching s.onCreate hooks (the code around
s.overrides[urlPath], s.overrides[newMemberPath], update injection, and the for
loop over s.onCreate); add "strconv" to the imports.
---
Outside diff comments:
In `@internal/controller/bmcuser_controller_test.go`:
- Around line 295-339: The test calls mockServers[0].ResetAccounts() only inside
the "should delete BMC user and secret on User deletion" It block, which leaves
state from other specs; remove that call from the spec and add an AfterEach in
the Describe block that invokes mockServers[0].ResetAccounts() (or iterates all
mockServers and calls ResetAccounts() on each) so every spec starts with a clean
mock server state; ensure the AfterEach references the existing mockServers
slice and ResetAccounts() helper.
---
Nitpick comments:
In `@bmc/mock/server/server.go`:
- Around line 733-754: The handleChangePassword handler currently updates only
the auth store (s.accounts) but not the account resource override, and it treats
missing/empty NewPassword or UserName as success; update the function so after
loading the account (using loadResourceLocked and the resolved filePath) you
extract the username (from account["UserName"]) and validate req.NewPassword is
non-empty and username is present, returning http.StatusBadRequest on invalid
input, then set s.accounts[username] = req.NewPassword and also update the
account resource in s.overrides (e.g., find/modify s.overrides[filePath] or the
map entry returned by loadResourceLocked to set its "Password" field to
req.NewPassword) while holding the existing mutex to keep state consistent, and
finally return 204 only on success.
In `@internal/controller/suite_test.go`:
- Around line 192-196: Multiple reconciler setups repeat the same bmc.Options
literal; extract that repeated BMCOptions block into a single shared variable
(e.g., defaultBMCOptions of type bmc.Options) and replace each inline
bmc.Options{...} (used when constructing BMCReconciler instances and test setups
exercising bmcConnectionTest) with that variable, ensuring server.WithAuth()
usage remains unchanged and BMCReconciler's BMCOptions field is assigned from
defaultBMCOptions.
🪄 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: 8595b610-1ffb-47bc-8cc0-0ff9e5fbd68d
📒 Files selected for processing (10)
bmc/mock/server/data/AccountService/Accounts/1/index.jsonbmc/mock/server/data/AccountService/Accounts/2/index.jsonbmc/mock/server/data/AccountService/Accounts/index.jsonbmc/mock/server/server.gobmc/redfish_local.gointernal/controller/biossettings_controller_test.gointernal/controller/bmc_controller_test.gointernal/controller/bmcuser_controller.gointernal/controller/bmcuser_controller_test.gointernal/controller/suite_test.go
nagadeesh-nagaraja
force-pushed
the
accountmgm
branch
from
d2b4ab6 to
a8d9fbd
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bmc/mock/server/data/AccountService/Accounts/2/index.json (1)
1-28:⚠️ Potential issue | 🟡 MinorNote: this fixture is also the template for newly-created accounts.
In
bmc/mock/server/server.go, the/AccountService/AccountsonCreatehook readsdata/AccountService/Accounts/2/index.jsonand seeds missing fields on every newly POSTed account (withActions/@odata.id/PasswordExpirationrewritten per-account). Future edits here will silently affect every new account created in tests, not just account 2. Worth a brief comment in this JSON or alongside the hook so the coupling is discoverable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/data/AccountService/Accounts/2/index.json` around lines 1 - 28, Add a short, explicit comment that this JSON (data/AccountService/Accounts/2/index.json) is used as the template for newly-created accounts and that edits will affect every new account; place the comment at the top of that JSON and also adjacent to the onCreate hook in server.go (the onCreate handler that seeds missing fields and rewrites Actions/@odata.id/PasswordExpiration). The comment should list which fields are rewritten per-account (Actions, `@odata.id`, PasswordExpiration) and advise maintainers to update the template and hook together when changing defaults.
🧹 Nitpick comments (4)
bmc/mock/server/server.go (3)
1241-1247: Mock-only auth — plain string compare is fine, just noting.
Authenticateuses==rather than a constant-time compare. That's appropriate for a mock; if anyone repurposes this server outside of unit tests, switch tocrypto/subtle.ConstantTimeCompareto avoid leaking credential length/contents through timing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/server.go` around lines 1241 - 1247, The Authenticate method on MockServer currently compares passwords with plain == which can leak via timing; update MockServer.Authenticate to use crypto/subtle.ConstantTimeCompare for comparing the stored password and the provided password (convert both to []byte and ensure lengths are handled) while keeping the same RLock/RUnlock and accounts lookup logic so behavior stays identical for mocks but is safe if reused outside tests.
737-769:handleChangePasswordignoresOldPassword— document or enforce.The Redfish
ManagerAccount.ChangePasswordaction contract (and gofish'sAccount.ChangePassword(new, old)) sends bothNewPasswordandOldPassword. The handler only validatesNewPassword, so a test with a wrong/missing old password still succeeds, which can mask real-world failure modes during credential rotation. Either drop a comment that this is intentional for the mock, or also parse and (optionally) checkOldPasswordagainsts.accounts[username]for parity with real BMCs.🔧 Optional: validate the old password
var req struct { + OldPassword string `json:"OldPassword"` NewPassword string `json:"NewPassword"` } if err := json.Unmarshal(body, &req); err != nil { http.Error(w, "invalid body", http.StatusBadRequest) return } if req.NewPassword == "" { http.Error(w, "NewPassword must not be empty", http.StatusBadRequest) return } @@ username, ok := account["UserName"].(string) if !ok || username == "" { http.Error(w, "account has no UserName", http.StatusBadRequest) return } + if stored, ok := s.accounts[username]; ok && req.OldPassword != "" && stored != req.OldPassword { + http.Error(w, "Forbidden", http.StatusForbidden) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/server.go` around lines 737 - 769, handleChangePassword currently ignores OldPassword; update it to unmarshal and require OldPassword and compare it to the stored password before changing: extend the req struct in handleChangePassword to include OldPassword, after resolving username fetch the current password from s.accounts[username] (or account["Password"]) and if OldPassword is missing or does not match return http.StatusBadRequest (or http.StatusUnauthorized) with a clear message, only then update s.accounts[username], account["Password"], and s.overrides[filePath]; alternatively, if you prefer the mock to accept any old password, add a short comment in handleChangePassword noting that OldPassword is intentionally ignored for the mock rather than enforcing parity with real BMCs.
205-238: Template seeding can leak account 2's password into newly created accounts.The
onCreatehook copies any field missing from the POST body out ofdata/AccountService/Accounts/2/index.json(includingPassword: "bar"). If a future test POSTs an account without aPassword, the new entry inherits"bar"ands.accounts[username] = "bar"— silently making the account log-in-able with the template's password. The controller path under test does always provide a password, so this is currently latent, but a small explicit guard would prevent surprises:🔧 Optional: skip Password from template seeding
for k, v := range tmpl { + if k == "Password" { + continue + } if _, exists := data[k]; !exists { data[k] = deepCopyAny(v) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/server.go` around lines 205 - 238, The template-seeding currently copies all fields (including "Password") from the template into the new account and then writes s.accounts[username]=password, which can leak the template password; modify the onCreate handler (the map entry for "/AccountService/Accounts" in MockServer) to skip copying the "Password" key when iterating tmpl -> data (use the same deepCopyAny loop but continue if k == "Password"), and also guard the authentication store update so s.accounts[username] is only set when the resulting password is non-empty and originated from the request (i.e., check password != "" before assigning) to avoid inheriting the template password.internal/controller/bmcuser_controller_test.go (1)
295-339: Consider movingResetAccounts()toBeforeEach/AfterEachfor test isolation.
ResetAccounts()is invoked only in this single spec, while other specs (should create a BMC user and secret,should just create additional BMC users,should rotate password if rotationPeriod is set) rely on the controller's finalizer-drivenDeleteAccountcalls to clean up mock state.EnsureCleanStatedoes not listBMCUserList, so if a spec finishes before the BMC user finalizer completes (or the rotation test leaks rotated secrets, as the inline comment already acknowledges), mock-server account state can leak between specs and skew assertions likeGetAccountNames/Members@odata.count. CentralizingmockServers[0].ResetAccounts()inAfterEachwould make this deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bmc/mock/server/data/AccountService/Accounts/2/index.json`:
- Around line 1-28: Add a short, explicit comment that this JSON
(data/AccountService/Accounts/2/index.json) is used as the template for
newly-created accounts and that edits will affect every new account; place the
comment at the top of that JSON and also adjacent to the onCreate hook in
server.go (the onCreate handler that seeds missing fields and rewrites
Actions/@odata.id/PasswordExpiration). The comment should list which fields are
rewritten per-account (Actions, `@odata.id`, PasswordExpiration) and advise
maintainers to update the template and hook together when changing defaults.
---
Nitpick comments:
In `@bmc/mock/server/server.go`:
- Around line 1241-1247: The Authenticate method on MockServer currently
compares passwords with plain == which can leak via timing; update
MockServer.Authenticate to use crypto/subtle.ConstantTimeCompare for comparing
the stored password and the provided password (convert both to []byte and ensure
lengths are handled) while keeping the same RLock/RUnlock and accounts lookup
logic so behavior stays identical for mocks but is safe if reused outside tests.
- Around line 737-769: handleChangePassword currently ignores OldPassword;
update it to unmarshal and require OldPassword and compare it to the stored
password before changing: extend the req struct in handleChangePassword to
include OldPassword, after resolving username fetch the current password from
s.accounts[username] (or account["Password"]) and if OldPassword is missing or
does not match return http.StatusBadRequest (or http.StatusUnauthorized) with a
clear message, only then update s.accounts[username], account["Password"], and
s.overrides[filePath]; alternatively, if you prefer the mock to accept any old
password, add a short comment in handleChangePassword noting that OldPassword is
intentionally ignored for the mock rather than enforcing parity with real BMCs.
- Around line 205-238: The template-seeding currently copies all fields
(including "Password") from the template into the new account and then writes
s.accounts[username]=password, which can leak the template password; modify the
onCreate handler (the map entry for "/AccountService/Accounts" in MockServer) to
skip copying the "Password" key when iterating tmpl -> data (use the same
deepCopyAny loop but continue if k == "Password"), and also guard the
authentication store update so s.accounts[username] is only set when the
resulting password is non-empty and originated from the request (i.e., check
password != "" before assigning) to avoid inheriting the template password.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f36298f0-1a66-48e2-b9b7-a4da2b9a8b96
📒 Files selected for processing (10)
bmc/mock/server/data/AccountService/Accounts/1/index.jsonbmc/mock/server/data/AccountService/Accounts/2/index.jsonbmc/mock/server/data/AccountService/Accounts/index.jsonbmc/mock/server/server.gobmc/redfish_local.gointernal/controller/biossettings_controller_test.gointernal/controller/bmc_controller_test.gointernal/controller/bmcuser_controller.gointernal/controller/bmcuser_controller_test.gointernal/controller/suite_test.go
✅ Files skipped from review due to trivial changes (2)
- bmc/mock/server/data/AccountService/Accounts/1/index.json
- bmc/mock/server/data/AccountService/Accounts/index.json
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/biossettings_controller_test.go
3 participants
Fixes #840
Summary by CodeRabbit
New Features
Chores
Data