[PM-33940] Enforce per-secret access checks in SecretVersionsController

[mandreko-bitwarden]

🎟️ Tracking

PM-33940

📔 Objective

All five secret-version endpoints previously short-circuited the per-secret access check whenever the caller was a ServiceAccount or Organization client, falling back to only the coarse org-level AccessSecretsManager gate. That let any service account in the org read, restore, or delete version history for secrets it had no access policy on, including write impact via the restore endpoint.

The fix removes the bypass branches from GetVersionsBySecretIdAsync, GetByIdAsync, GetManyByIdsAsync, RestoreVersionAsync, and BulkDeleteAsync. Every endpoint now runs AccessToSecretAsync / AccessToSecretsAsync against the caller's AccessClientType, matching the pattern used by SecretsController.GetAsync. RestoreVersionAsync keeps a ServiceAccount vs User split only for snapshot attribution (EditorServiceAccountId vs EditorOrganizationUserId), after the write check has passed.

Why this approach over the alternative

SecretAuthorizationHandler.CanUpdateSecretAsync would also have worked, but SecretsController.GetAsync already calls AccessToSecretAsync directly. Matching that pattern keeps the controller readable and the diff minimal. Both paths land in the same BuildSecretAccessQuery.

Tests

• Fixed two pre-existing BulkDelete tests that mocked GetByIdAsync / AccessToSecretAsync while the controller calls GetManyByIdsAsync / AccessToSecretsAsync. They were passing for the wrong reason.
• Added service-account regression tests for all five endpoints, plus a mixed-batch bulk-read case where the caller has access to one of two secrets in the batch.
• All 20 tests in SecretVersionsControllerTests pass.

Not addressed in this PR

CurrentContext.AccessSecretsManager returns true for any service account in the same org. It's used very broadly across the codebase, and changing it would have a large blast radius. The controller-layer per-secret check is the correct place to enforce, and matches how every other Secrets Manager endpoint already works.

📸 Screenshots

N/A, no UI changes.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR removes an authorization bypass in SecretVersionsController where ServiceAccount and Organization clients skipped the per-secret access check and relied only on the coarse org-level AccessSecretsManager gate. I traced all five endpoints through AccessClientHelper.ToAccessClient and SecretRepository.BuildSecretAccessQuery, confirmed the new flow matches the canonical SecretsController.GetAsync pattern, and verified the RestoreVersionAsync snapshot attribution correctly sets exactly one of the two nullable editor FK fields after the write check passes. Test changes are appropriate — two pre-existing tests that mocked the wrong repository methods are fixed, and regression coverage is added for all five endpoints including a mixed-batch read case.

Code Review Details

No findings. Notes from verification:

• The Organization client path now resolves to (false, false) via the default branch of BuildSecretAccessQuery, identical to every other Secrets Manager read endpoint (SecretsController.GetAsync). This is intentional alignment with the established pattern, not a regression — the prior bypass was the anomaly.
• AccessToSecretsAsync returns one access row per requested secret, so secrets with no policy yield (false, false) and correctly trip the Any(!Read) / Any(!Write) denial in the batch endpoints.
• The currentValue != version.Value snapshot guard and ServiceAccount editor-id attribution are preserved from the original logic.
• Integration tests do not exercise the service-account/organization client paths, so removing those branches does not break existing integration coverage.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[codecov]

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.28%. Comparing base (10169e6) to head (90b2311).

Files with missing lines	Patch %	Lines
...etsManager/Controllers/SecretVersionsController.cs	78.57%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7823      +/-   ##
==========================================
- Coverage   65.67%   61.28%   -4.39%     
==========================================
  Files        2195     2195              
  Lines       97542    97511      -31     
  Branches     8807     8801       -6     
==========================================
- Hits        64061    59762    -4299     
- Misses      31260    35625    +4365     
+ Partials     2221     2124      -97     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.