[PM-39077] feat: Add extend trial option to the Admin portal

[cyprain-okeke]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-39077

📔 Objective

Lets Sales and Support extend an organization's trial from the Admin portal instead of editing the subscription in Stripe.

• Adds a Trial section to the Admin portal organization Edit page showing the current trial end whenever the subscription is trialing, with a form to extend it by 1 to 30 days when eligible or the reason it cannot be extended otherwise. The section is only rendered for users holding the new permission.
• Adds OrganizationTrialController.ExtendAsync, gated behind the pm-35092-auth-sales-assisted-trials feature flag, a new Org_ExtendTrial permission (granted to Owner, Admin, Billing, and Sales roles), anti-forgery, and cloud-only.
• Adds ExtendOrganizationTrialCommand, which moves the Stripe trial end with no proration and then syncs the organization's expiration date so a subsequent Edit save cannot overwrite it before the webhook lands. A failure of that local sync is logged and does not fail the command, since Stripe has already moved the trial end and the subscription webhook re-syncs the date; failing would invite a retry that extends the trial twice.
• Centralizes eligibility in TrialExtensionPolicy: the subscription must be trialing, have no schedule attached, and have fewer than 30 days remaining (measured against the Stripe test clock when one is attached). Extensions are capped at 1 to 30 days.
• Writes an audit log on every extension with the acting admin identity, organization, day count, and before/after trial end dates for FedRAMP traceability. The log is written immediately after the Stripe update succeeds so a failed expiration sync cannot lose the record. Refused attempts are logged too: policy rejections (invalid day count, not trialing, 30 or more days remaining, no subscription) at Warning with the actor, organization, requested days and reason; Stripe conflicts and unexpected errors at Error.

Tests cover the policy, the command (including Stripe and database failure paths), the controller outcomes and audit log, the Edit page eligibility state, the form model validation, the role mapping, and the controller's security attributes.

📸 Screenshots

Trial section on the organization Edit page for a Sales user when the trial can be extended:

Success toast after extending by 7 days:

Trial section when the trial cannot be extended because 30 or more days remain:

Recorded local QA pass (Admin portal and Stripe-side checks):

pm39077-qa-testing-full.mp4

🤖 AI-assisted review

Standard local review (code-review-local) run on the branch diff against origin/main: APPROVE, no inline findings. The reviewer noted one low-confidence concern about the audit log being written after the database sync; addressed by moving the log to immediately after the Stripe update, with a test.

The GitHub Claude review then flagged the related retry risk: a failed expiration sync returned Unhandled, and the "please try again" message would have led to a second Stripe extension. Addressed in a160f70 by isolating the sync failure (Error log, command still reports success), with the test updated to pin that behaviour.

A five-aspect local review (code quality, tests, silent failures, comments, type design) then ran over the branch. Its critical and important findings are addressed in 8cbb85a: a single eligibility definition shared by the Edit page and the command, Error logs with actor and organization on failed attempts, a richer sync-failure log, tests for subscriptions without a Stripe test clock (the production shape, previously untested), and one stale comment. Remaining suggestions were judged optional polish and left for reviewer input.

4b3b4c7 pre-empts the likely reviewer asks: the command resolves the subscription through OrganizationSubscriptionHelpers.TryGetSubscriptionAsync and returns a Conflict on resource_missing instead of the generic error; the Edit page exposes a single nullable ExtendableTrialEnd instead of a bool plus date; two comments trimmed.

7f08625 addresses the human review round: the Edit page reads the trial through a new IGetOrganizationTrialQuery in Core so the Admin controller no longer handles Stripe subscription objects, the trial end is shown regardless of eligibility with explicit TrialEnd, CanExtendTrial, and TrialExtensionBlockedReason properties, and the now-unused TrialExtensionPolicy.IsEligible is removed.

0914539 addresses the second round: the three flattened trial properties on the Edit model are replaced by a single embedded OrganizationTrial, which also retires the out-of-date property comment.

929473e follows a recorded local QA pass (Admin portal flag on/off, plus Stripe-side checks through the Stripe CLI: trial end delta, no proration, live not-trialing race, subscription schedule, webhook re-sync, test clock; 53 checks passed). The one gap it surfaced was that policy rejections produced a toast but no log line, so a refused attempt left no audit trail. The controller now logs them at Warning, with two tests pinning the actor, organization, days and reason.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed at 651ea180, which adds the origin/main merge and the OrganizationEditModel.Trial doc-comment clarification on top of the previously reviewed head. I re-walked the whole change rather than only the delta: TrialExtensionPolicy, GetOrganizationTrialQuery, ExtendOrganizationTrialCommand, OrganizationTrial, the embedded model property and Trial section in Edit.cshtml, OrganizationTrialController, the Org_ExtendTrial wiring, the DI registration, and the twenty-odd tests. No finding reached the bar for an inline comment, and the build failure that blocked the previous round is no longer present.

Code Review Details

Previous blocker cleared

The last review requested changes solely because every .NET job was red on 66feca72. At 651ea180 no check is failing: Lint (the build workflow's, which compiles the solution) passes at 2m59s, and all three Build MSSQL migrator utility targets plus Nginx/MsSql/Attachments images pass. Run tests, Analyze (csharp), and the remaining image builds are still pending, which is unknown rather than failing, so it is not a basis for blocking. Worth a glance at the test run before merge.

Verified and deliberately not raised

• Permission enum insertion is safe. Org_ExtendTrial is inserted mid-enum in Permissions.cs, shifting the ordinal of every member after it. AccessControlService.UserHasPermission resolves the role from a ClaimTypes.Role name claim and looks the permission up by membership in RolePermissionMapping; the Permission enum is never persisted to a database, cookie, or configuration value, so no stored ordinal goes stale.
• Role grants match the description — owner, admin, billing, sales, and deliberately not cs; RolePermissionMappingTests pins it.
• Self-hosted exposure closed on both sides. AccessControlService.UserHasPermission returns true unconditionally when self-hosted, but OrganizationsController.Edit (GET) and OrganizationTrialController both carry [SelfHosted(NotSelfHostedOnly = true)], so neither the Trial section nor the POST is reachable there.
• POST surface is fully gated: [Authorize] + [ValidateAntiForgeryToken] + [RequirePermission(Permission.Org_ExtendTrial)] + feature flag, with OrganizationTrialControllerAttributeTests pinning them.
• The new Trial property does not disturb the Edit POST. OrganizationTrial is a positional record with no parameterless constructor, but Trial is a nested complex property and the form posts no Trial.* fields, so ComplexObjectModelBinder never attempts to create it and ModelState stays valid. The POST path redirects and never re-renders the model.
• The extend form is top-level, not nested inside _OrganizationForm.cshtml; the asp-* tag helpers emit the antiforgery token for method="post", and asp-action="Extend" resolves because SuppressAsyncSuffixInActionNames is left at its default.
• TrialEnd! dereferences are provably safe: GetRemainingDays is only reached from GetIneligibilityReason after the not { Status: Trialing, TrialEnd: not null } arm, and the command dereferences only after that call returns null.
• Legacy Bit.Core.Services.IFeatureService in the new controller does not break the build. BWA0002 is listed in WarningsNotAsErrors in Directory.Build.props despite repo-wide TreatWarningsAsErrors, and the sibling Admin Billing controllers use the same legacy interface. Worth folding into the SDK migration later, but not this PR's job.
• Actor logging matches existing precedent — User?.Identity?.Name ?? "unknown" is the same shape already used in OrganizationPlanMigrationCohortsController.cs:141.
• Stripe failure isolation still reads correctly. The audit log is written immediately after UpdateSubscriptionAsync succeeds, and the UpdateExpirationDateAsync failure is logged without failing the command — SubscriptionUpdatedHandler re-syncs ExpirationDate on the webhook, and reporting failure would invite a retry of a non-idempotent Stripe mutation. UpdateExpirationDateAsync itself only sets ExpirationDate/RevisionDate and refreshes the cache, with no side effects that would make the best-effort treatment wrong.
• Both new Core types are registered via TryAddTransient in AddBillingOperations(), which Admin/Startup.cs calls.
• Below the bar, listed for completeness: the 1/30 bounds remain duplicated in the Edit.cshtml input attributes and in the policy's message strings rather than referencing MinExtensionDays/MaxExtensionDays; GetRemainingDays rounds up, so "30 or more days remain" can fire at 29.2 actual days (pinned intentionally by GetRemainingDays_PartialDay_RoundsUp); and the GetTrialAsync XML comment in OrganizationsController describes only the permission case, not the not-trialing or Stripe-error cases.
• No dependency manifest, Claude configuration, or skill files in the diff.

Open threads

eliykat's remaining comment on the OrganizationEditModel.Trial doc comment is addressed by 651ea180, which now distinguishes "lacks Org_ExtendTrial" from CanExtend (Stripe eligibility). The earlier threads on the Stripe leakage into Admin Console, the always-visible trial end, and the feature-flag reuse are all author-addressed; screenshots are in the description.

[codecov]

Codecov Report

❌ Patch coverage is 92.02128% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.60%. Comparing base (0d291b2) to head (651ea18).

Files with missing lines	Patch %	Lines
...Admin/AdminConsole/Views/Organizations/Edit.cshtml	0.00%	11 Missing ⚠️
...Billing/Controllers/OrganizationTrialController.cs	94.54%	0 Missing and 3 partials ⚠️
...zations/Commands/ExtendOrganizationTrialCommand.cs	98.24%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8506      +/-   ##
==========================================
+ Coverage   66.36%   66.60%   +0.24%     
==========================================
  Files        2578     2584       +6     
  Lines      110631   110814     +183     
  Branches    10059    10076      +17     
==========================================
+ Hits        73418    73808     +390     
+ Misses      34810    34599     -211     
- Partials     2403     2407       +4     

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

[kdenney]

Nice job! I just have one non-blocking question.

[kdenney]

❓ Should we show the trial end whenever there is a trial, regardless of "CanExtendTrial"? This seems like valuable information to have on the screen, even if they cannot extend for whatever reason. Although if we do, you'd have to go back to having both the bool CanExtendTrial and the TrialEnd separately on the model.

[eliykat]

I'm not very familiar with the ticket but this seems like a good suggestion. For what it's worth, I also like the separate, explicit bool and Date values, rather than using the null to implicitly represent the user's permissions.

[cyprain-okeke]

Agreed, done in 7f08625. The Trial section now renders whenever the subscription is trialing and shows the trial end; the extend form appears when eligible, otherwise the reason it is blocked. The model has explicit TrialEnd, CanExtendTrial, and TrialExtensionBlockedReason properties rather than one nullable date. The section is still gated on the feature flag and the Org_ExtendTrial permission so we only hit Stripe for users who can act on it.

[eliykat]

This is fairly low level Billing code: I would prefer that AC doesn't have to deal with Stripe subscription objects or handle options like { Expand = ["test_clock"] } which we don't have context for.

Can this be put behind an interface, e.g. return await trialExtensionQuery.Run(organization)? (Example only, up to you how you want to structure it.)

[cyprain-okeke]

Done in 7f08625.

[eliykat]

I'm not very familiar with the ticket but this seems like a good suggestion. For what it's worth, I also like the separate, explicit bool and Date values, rather than using the null to implicitly represent the user's permissions.

[eliykat]

I think this comment is out of date - it is now populated even if the current user cannot extend.

Also Set during the Edit GET is not a useful comment.

[eliykat]

Please also add screenshots for UI changes.

[eliykat]

Feedback is non-blocking, everything else looks good.

[eliykat]

Still incorrect; the permission is indicated by Trial.CanExtend, not by this object being nullable.

I think it can just say

Suggested change

	/// The organization's Stripe trial; null when the subscription is not trialing or the current user lacks the
	/// permission to extend trials.
	/// The organization's Stripe trial; null when they do not have a trial subscription.