[cristim]

Summary

All authentication failure paths in service.go now return the same "Check your email address and password and try again" message, resolving the test failure that was keeping base CI red.

• Extracts a genericLoginError constant so the text is defined once in service.go (closes issue fix(auth): login error message inconsistent -> base CI red (TestLogin_WithMFA_NoSecret) #993).
• Five return sites in getUserAndValidateStatus and verifyPasswordAndMFA that previously returned the old "invalid email or password" string are updated.
• The MFA-missing path (line 240) which already used the correct text is updated to reference the constant.
• service_test.go, service_lockout_test.go, and handler_auth_test.go updated to assert the current message; the TestLogin_OWASPEnumerationInvariant regression guard continues to enforce uniformity across all six failure modes.

Verification

• go test ./internal/auth/ - all tests pass including TestLogin_WithMFA_NoSecret/wrong_password_returns_same_generic_error (previously failing) and TestLogin_OWASPEnumerationInvariant (all six subcases).
• go build ./... - clean.

Closes #993

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 33 minutes and 11 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e3ea2749-43ba-42ca-93f7-1cae927cdf22

📥 Commits

Reviewing files that changed from the base of the PR and between 276404a and 2918d47.

📒 Files selected for processing (4)

• internal/api/handler_auth_test.go
• internal/auth/service.go
• internal/auth/service_lockout_test.go
• internal/auth/service_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/993-login-error-message

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 1107 --repo LeanerCloud/CUDly

echo "=== FILES CHANGED ==="
gh pr diff 1107 --repo LeanerCloud/CUDly

Length of output: 9783

---

---

@cristim: The change is clean, well-structured, and correctly achieves the anti-enumeration goal. One minor style nit worth addressing:

fmt.Errorf without format verbs → errors.New

All six updated return sites use fmt.Errorf(genericLoginError), but since genericLoginError contains no % format directives, the idiomatic and linter-clean choice is errors.New:

// service.go — all six return sites
return nil, errors.New(genericLoginError)   // getUserAndValidateStatus (×3)
return errors.New(genericLoginError)        // verifyPasswordAndMFA (×3)

staticcheck (SA1006) and some go vet configurations will flag fmt.Errorf calls with a constant string that has no verbs as unnecessary; errors.New is slightly cheaper (no format-string parsing) and communicates intent more clearly.

Everything else looks good:

• ✅ genericLoginError correctly unexported — implementation detail, not part of the package API.
• ✅ handler_auth_test.go correctly hardcodes the string (can't reference the unexported constant cross-package).
• ✅ TestLogin_OWASPEnumerationInvariant with its wantMsg local constant provides a solid regression guard across all six failure paths.
• ✅ Doc comment on the constant references both #416 and #993 — good traceability.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

[cristim]

@coderabbitai review

---

Generated by Claude Code

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr view 1107 --repo LeanerCloud/CUDly
echo "=== FILES CHANGED ==="
gh pr diff 1107 --repo LeanerCloud/CUDly

Length of output: 9850

---

---

@cristim: The diff is unchanged from the previous pass — no new commits yet. The only open finding remains:

fmt.Errorf without format verbs → errors.New (all six sites in service.go)

// current
return nil, fmt.Errorf(genericLoginError)
// preferred
return nil, errors.New(genericLoginError)

staticcheck SA1006 flags fmt.Errorf with a plain constant/string that contains no % verbs. errors.New is the idiomatic replacement — no format-string parsing overhead and clearer intent. The six sites are at the three returns in getUserAndValidateStatus and the three in verifyPasswordAndMFA.

Everything else is still ✅ — constant scoping, test assertions, OWASP invariant guard all look correct. Just the errors.New swap left.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T07:04:28.235Z
Learning: In LeanerCloud/CUDly, PR `#907` (`feat(auth): group-membership-only authorization, remove roles, require >=1 group`) landed on `feat/multicloud-web-frontend` and removed `Session.Role` and `User.Role` entirely. Any `Principal` struct must use `GroupIDs []string` (or similar group-based field) instead of `Role string`. The `*auth.User` type no longer exposes `GetRole()`, `GetID()`, or `GetEmail()` accessors via interface assertion — using those in a type assertion causes a silent always-false fail-closed regression at runtime. The group-membership model also requires >=1 group for a valid user session; zero-groups users must be rejected at the authentication layer. Follow-up design issue is `#1010`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `normalizeEngineName` uses `==` (not `strings.Contains`) for oracle and sqlserver checks. Edition-qualified tokens (oracle-se2, oracle-ee, sqlserver-se, sqlserver-web, sqlserver-ex, etc.) must pass through as valid RDS ProductDescription values. Using Contains was self-contradictory: the error message told callers to supply the exact edition tokens. Fixed in PR `#1085` commit 681fd9e81. `TestNormalizeEngineName_EditionTokensPassThrough` covers 10 edition variants.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly GCP service clients (computeengine, cloudsql, memorystore, cloudstorage), `GetRecommendations` filters out non-ACTIVE recommendations using the typed `recommenderpb.RecommendationStateInfo_ACTIVE` enum (CLAIMED/SUCCEEDED/FAILED/DISMISSED are skipped). State is accessed nil-safely via the proto getter. Fixed in PR `#1047` commit 95a390f (H-1 finding). Regression tests: `TestGetRecommendations_FiltersNonActiveStates` (5-rec mock, only ACTIVE survives) and `TestGetRecommendations_ActiveRecIncluded`. Existing mock tests in all four services had StateInfo added to satisfy the new filter.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `findOfferingID` validates AZConfig with a switch that has a `default: error` case covering non-empty invalid values (e.g. "typo-az"), not just an empty-string check. A bad non-empty AZConfig previously fell through to `multiAZ==false` in `paginateRDSOfferings` (same mis-buy as the old default). Fixed in PR `#1085` commit 681fd9e81. Regression test added.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly GCP service converters (`convertGCPRecommendation` in memorystore/client.go and cloudstorage/client.go), `rec.Term` must be derived from `params.Term` with a `"1yr"` default — not hardcoded to `"1yr"`. Without this, 3-year callers always emit 1-year commitments even though the downstream `termYears` derivation from `rec.Term` is correct. Fixed in PR `#1047` commit c6280c390 (F2 for memorystore, F4 for cloudstorage). Regression tests: `TestConvertGCPRecommendation_PropagatesTermFromParams` and `TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams`.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.