Skip to content

[PM-40248] feat: Enforce Send access controls via Send Controls policy - #2906

Open
matt-livefront wants to merge 3 commits into
mainfrom
matt/PM-40248-send-controls-access-controls
Open

[PM-40248] feat: Enforce Send access controls via Send Controls policy#2906
matt-livefront wants to merge 3 commits into
mainfrom
matt/PM-40248-send-controls-access-controls

Conversation

@matt-livefront

@matt-livefront matt-livefront commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-40248

📔 Objective

Enforce the Send Controls policy's access control option (whoCanAccess) on iOS, behind the pm-31885-send-controls feature flag. When an organization requires a specific access option, the Send add/edit "Who can view" menu shows the enforced value and is disabled, and the required option is forced:

  • Password protected (whoCanAccess = 1) → access type forced to Anyone with a password, and a password is required to save.
  • Email verification (whoCanAccess = 2) → access type forced to Specific people, and at least one recipient email is required to save.
  • Any (0) / flag off / no applying policy → no change to existing behavior.

📸 Screenshots

Password protected Email verification Email domain validation
Screenshot 2026-07-23 at 11 11 01 AM Screenshot 2026-07-23 at 11 10 37 AM Screenshot 2026-07-23 at 11 09 00 AM

@matt-livefront
matt-livefront requested a review from a team as a code owner July 23, 2026 16:51
@matt-livefront matt-livefront added ai-review Request a Claude code review t:feature labels Jul 23, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Jul 23, 2026
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the enforcement of the Send Controls policy's access-control option (whoCanAccess) behind the pm-31885-send-controls flag, including the two follow-up commits that extract earliestRevisionDate into a [Policy] extension and localize the allowed-domain list formatting. The access type resolution (.max() over whoCanAccess, mapping 1 → password / 2 → email verification) is gated by the feature flag, the "Who can view" menu is forced and disabled when enforced, and save validation requires a password or at least one allowed-domain recipient email as appropriate. The recipientEmails refactor to a default single empty row is safe: removeRecipientEmail never empties the array and normalizedRecipientEmails filters empty entries before submission, so dropping the append in accessTypeChanged does not regress the email row. Test coverage is thorough across SendPolicyOptions, SendAccessType, [Policy], PolicyService, the alert extension, state, processor, and view layers.

Code Review Details
  • 🎨 : Property named earliestRevisionDate returns a Policy, not a date
    • BitwardenShared/Core/Vault/Models/Domain/Policy.swift:59

Notes considered and cleared during review:

  • Domain matching is exact (a policy allowing acme.com rejects user@mail.acme.com) and case-insensitive; consistent with the tests and appears intentional.
  • allowedDomains is sourced from the earliest-revision policy that enforces email verification rather than intersecting across multiple orgs. The code comment documents this as deliberate, and the server remains the authoritative enforcement point.
  • Forcing state.accessType in loadData for edit mode intentionally overrides the send's stored access type, matching the PR description.

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.37%. Comparing base (702231d) to head (c47e0a2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2906      +/-   ##
==========================================
- Coverage   79.38%   79.37%   -0.01%     
==========================================
  Files        1172     1172              
  Lines       74875    74950      +75     
==========================================
+ Hits        59438    59492      +54     
- Misses      15437    15458      +21     

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

Comment thread BitwardenShared/Core/Tools/Models/Domain/SendPolicyOptions.swift Outdated
Comment thread BitwardenShared/UI/Platform/Application/Extensions/Alert+Extensions.swift Outdated
Base automatically changed from matt/PM-40139-send-controls-policy-consolidation to main August 3, 2026 21:28
@matt-livefront
matt-livefront force-pushed the matt/PM-40248-send-controls-access-controls branch from 922747c to c47e0a2 Compare August 3, 2026 21:39
@matt-livefront
matt-livefront requested a review from fedemkr August 3, 2026 21:41
extension [Policy] {
/// The policy with the earliest revision date, or `nil` if the list is empty.
///
var earliestRevisionDate: Policy? {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 SUGGESTED: Property name reads as a Date but returns a Policy.

Details and fix

At the call sites this reads as if a date is being returned: policies.earliestRevisionDate?.organizationId (PolicyService.swift:515) and policies.earliestRevisionDate assigned to domainsPolicy (SendPolicyOptions.swift:45). Naming it after the returned type keeps the read accurate:

Suggested change
var earliestRevisionDate: Policy? {
var policyWithEarliestRevisionDate: Policy? {

The original private helper it replaced (policyWithEarliestRevisionDate(from:)) carried this clarity; not required, just easier to read at the call site.

@fedemkr fedemkr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, the only ⛏️ is what Claude is suggesting for the name of the property.

title: Localizations.invalidEmailAddresses,
message: Localizations.onlyIncludeTheFollowingDomainsDescriptionLong(
allowedDomains.joined(separator: ", "),
allowedDomains.formatted(.list(type: .and, width: .narrow)),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants