Backmerge Commcare 2.61.1

[avazirna]

Product Description

Technical Summary

Safety Assurance

Safety story

Automated test coverage

QA Plan

Special deploy instructions

• This PR can be deployed after merge with no further considerations.

Rollback instructions

• This PR can be reverted after deploy with no further considerations.

Review

• The set of people pinged as reviewers is appropriate for the level of risk of the change.

Duplicate PR

Automatically duplicate this PR as defined in contributing.md.

Summary by CodeRabbit

• Bug Fixes
  • Strengthened authentication validation to immediately reject invalid credentials and tokens with clear error messages, improving system reliability and preventing invalid authentication attempts.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The PR adds input validation to the AuthInfo class constructors. Runtime checks in ProvidedAuth() ensure non-empty username and password, while TokenAuth() validates non-empty token values, throwing IllegalArgumentException for violations.

Changes

Cohort / File(s)	Change Summary
Input Validation in AuthInfo src/main/java/org/commcare/core/network/AuthInfo.java	Added StringUtils import. Introduced non-empty validation for username and password in ProvidedAuth constructor, and non-empty token validation in TokenAuth constructor, each throwing IllegalArgumentException with descriptive error messages.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Straightforward validation additions following a consistent pattern
• Single file modification with homogeneous changes
• No complex logic or conditional branching introduced

Possibly related PRs

• Adds argument validations for auth classes #1511 — Directly modifies the same constructors in AuthInfo with identical input validation and StringUtils import changes.

Suggested reviewers

• conroy-ricketts
• OrangeAndGreen

Poem

🐰 A hop, a check, a guard so true,
Empty strings won't slip on through!
With StringUtils and early throws,
Our AuthInfo safely grows. ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title 'Backmerge Commcare 2.61.1' does not clearly describe the actual code changes being made, which involve adding input validation to authentication constructors in AuthInfo.java.	Update the title to reflect the main technical change, such as 'Add input validation to AuthInfo authentication constructors' or similar, to better communicate the actual changeset to reviewers.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch commcare_2.61

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/main/java/org/commcare/core/network/AuthInfo.java (1)

27-32: New ProvidedAuth validation changes constructor behavior; confirm call sites and consider reuse

Enforcing non-empty username/password via IllegalArgumentException is reasonable, but it is an observable behavior change: any existing caller that passed null/"" (even as a sentinel) will now fail at construction time. Please:

• Double-check call sites to ensure they never intentionally pass empty/nullable credentials.
• Add/extend tests to cover these exceptional paths so regressions are caught early.
  If this style of validation expands, consider a small shared helper to avoid repeating the same isEmpty/exception pattern in multiple auth constructors.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 926cab9 and c2e1774.

📒 Files selected for processing (1)

• src/main/java/org/commcare/core/network/AuthInfo.java (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/main/java/org/commcare/core/network/AuthInfo.java (1)

src/main/java/org/commcare/cases/util/StringUtils.java (1)

• StringUtils (13-166)

⏰ 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). (2)

• GitHub Check: task-list-completed
• GitHub Check: task-list-completed

🔇 Additional comments (2)

src/main/java/org/commcare/core/network/AuthInfo.java (2)

3-4: Using shared StringUtils helper is a good choice

Centralizing null/empty checks on StringUtils.isEmpty keeps the constructors concise and avoids re-implementing the same logic.

---

46-47: TokenAuth null/empty guarding is sound but also a behavior change

Rejecting null/empty tokens up front is a good invariant for TokenAuth, but like ProvidedAuth, this will now throw where existing code may previously have constructed a “blank token” instance. Please verify:

• No current callers use null/"" to represent “no token yet” (they should use NoAuth/CurrentAuth instead).
• Tests cover the invalid-token case and any upstream validation paths.

[avazirna]

duplicate this PR 1d3c02c 1d3c02c