Make strongBoxUseTimeProbe a constructor val for consistency

[wksantiago]

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when checking StrongBox authentication-key support.
  • Added a safe fallback for environments where a custom StrongBox availability check is not provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3e341a1c-1f99-4c17-bd5a-54d6105dfc9e

📥 Commits

Reviewing files that changed from the base of the PR and between bc05b8a and 6832262.

📒 Files selected for processing (1)

• app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt

🚧 Files skipped from review as they are similar to previous changes (1)

• app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Walkthrough

AndroidKeystoreStorage accepts an optional StrongBox use-time probe. Authentication-key validation uses the injected probe when provided and the production probe otherwise.

Changes

StrongBox probe handling

Layer / File(s)	Summary
StrongBox probe selection and validation app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt	The constructor adds an optional test probe. Authentication-key validation uses it when provided or falls back to canEncryptWithStrongBoxProbe.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 68322

This localized constructor consistency change has no actionable merge-blocking risk remaining and is merge-ready after normal checks and review.

Suggested reviewers: kwsantiago

Poem

A rabbit checks the StrongBox gate,
An injected probe helps tests wait.
When no probe joins the run,
The built-in probe gets it done.
Keys hop safely through the state.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: moving strongBoxUseTimeProbe into the constructor as a val for consistency.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch strongbox-probe-constructor-val

---

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.}

[kwsantiago]

First, a correction that changes what this PR is. I asked for this on the grounds that #506 left the probe as an internal var that app-module code could reassign. That was wrong. The var existed only on an intermediate branch commit (3ca1972); the merged commit 3b84f1c already carried a constructor parameter and a val, so you had fixed it before merge in response to the original review. I reported from the diff I had read rather than checking what actually landed, and I should not have raised it.

So this is not hardening. Both forms are immutable, and no app-module code can switch the downgrade off in either. Nothing about the security property changes here, and it would be worth not letting the merge message imply otherwise.

What it does change is where the default is resolved:

	main	this PR
property type	() -> Boolean, non-null	(() -> Boolean)?, nullable
default resolved	once, at construction	at each call site
ctor param vs property	same name, shadowed	not shadowed

That is a genuine readability win. The shadowing on main is the kind of thing that reads fine when you wrote it and confuses the next person, and the comment explaining why a member-function default cannot be expressed inline is worth having wherever the seam lives.

Against that: main resolves the default once and guarantees non-null at the type level, so the call site cannot forget the fallback. This version moves that obligation to getOrCreateKeyWithAlias, where (strongBoxUseTimeProbe ?: ::canEncryptWithStrongBoxProbe)() is currently the only consumer but would need repeating if another appeared. A second call site that forgot the elvis would silently skip the probe, which is the one way this shape could bite later.

Both are defensible and I do not have a strong preference. If you keep it, the single call site is worth a brief note that the fallback belongs at every use.

Verified: local gate green (unit tests, lintDebug, RNG hygiene guard), CI green, base contains fa70329, and StrongBoxAuthKeyFallbackTest still injects through the constructor so the downgrade path stays exercised. Pixel 9a, Android 17: 291 tests, 0 failures, 5 skipped, matching main, with authKeyDowngradesToTeeWhenStrongBoxProbeFails passing rather than skipping, so the seam is still genuinely driven through the constructor.

No blockers.

[wksantiago]

Thanks for the correction — agreed on all of it, and no need to apologize for the original report.

You're right that this isn't hardening. Both forms are immutable and no app-module code can turn the downgrade off in either; the only real change is where the default resolves and dropping the ctor-param/member shadowing. I'll keep the squash message framed as readability/consistency so nothing implies a security property changed.

On the one way this shape could bite — a second call site forgetting the elvis and silently skipping the probe — I've added a brief note at the sole use site (6832262) spelling out that the fallback belongs at every use of strongBoxUseTimeProbe. That's the cheap guard against the failure mode you flagged.

Thanks for the verification pass.

[kwsantiago]

Checked 6832262 against bc05b8a, the commit I ran the device suite on: three added lines, comment only, no functional change. So the 291 tests / 0 failures / 5 skipped from that run still applies, including authKeyDowngradesToTeeWhenStrongBoxProbeFails passing rather than skipping. Re-ran the local gate on this head anyway since a -Werror build is cheap: unit tests, lintDebug and the RNG hygiene guard all green, and CI is green.

The note lands in the right place and says the right thing. The failure mode it guards is quiet rather than loud, a future call site omitting the elvis would skip the use-time check for auth-gated keys and nothing would fail, so having it stated at the use site rather than only on the property is what makes it likely to be read by whoever adds the second consumer.

Agreed on the squash framing. Readability and consistency is accurate; no security property changes in either direction here.

No blockers.