Skip to content

[PM-37168] Fix missing expiration check.#7760

Merged
JimmyVo16 merged 1 commit into
mainfrom
ac/pm-37168/fix-missing-expiration-check
Jun 3, 2026
Merged

[PM-37168] Fix missing expiration check.#7760
JimmyVo16 merged 1 commit into
mainfrom
ac/pm-37168/fix-missing-expiration-check

Conversation

@JimmyVo16
Copy link
Copy Markdown
Contributor

@JimmyVo16 JimmyVo16 commented Jun 3, 2026

🎟️ Tracking

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

📔 Objective

I added fixes to the identified locations and added test coverage for them.
It's a simple change, and I think the automated tests are sufficient.

@JimmyVo16 JimmyVo16 added the ai-review Request a Claude code review label Jun 3, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds a missing token expiration check to two delete-recovery flows (OrganizationsController.PostDeleteRecoverToken and ProviderService.DeleteAsync) by including !data.Valid in the guard alongside the existing IsValid(entity) check. Since ExpiringTokenable.Valid is defined as !IsExpired && TokenIsValid(), this correctly closes a VULN where expired tokens with a matching entity Id would have been accepted. Both code paths receive a focused new test that constructs a tokenable with a matching Id but a 2-hour-past expiration and asserts BadRequestException is thrown and the downstream delete is never invoked. A repo-wide search confirms these were the only two TryUnprotect(...) || !data.IsValid(...) sites missing the expiration guard.

Code Review Details

No findings.

Note on existing bot threads: the github-code-quality "useless assignment to expiredTokenData" comments on ProviderServiceTests.cs:1339 and OrganizationsControllerTests.cs:355 are false positives — the variable is reassigned via the out parameter on TryUnprotect and must be declared for the NSubstitute .Returns(true) setup pattern. The author has already clarified this in both threads.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 3, 2026

{
// Arrange
// Token has a matching provider Id but an expiration date two hours in the past.
var expiredTokenData = new ProviderDeleteTokenable(provider, -2);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

expiredTokenData is being used in an out statement a few lines below. Looks like the compiler just didn't pick it up.

sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);

// Token has a matching organization Id but an expiration date two hours in the past.
var expiredTokenData = new OrgDeleteTokenable(organization, -2);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

expiredTokenData is being used in an out statement a few lines below. Looks like the compiler just didn't pick it up.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 3, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.84%. Comparing base (0cab8e2) to head (10dc45a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...cial.Core/AdminConsole/Services/ProviderService.cs 0.00% 0 Missing and 1 partial ⚠️
...dminConsole/Controllers/OrganizationsController.cs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7760   +/-   ##
=======================================
  Coverage   60.84%   60.84%           
=======================================
  Files        2167     2167           
  Lines       96089    96089           
  Branches     8652     8652           
=======================================
+ Hits        58464    58469    +5     
+ Misses      35539    35532    -7     
- Partials     2086     2088    +2     

☔ View full report in Codecov by Sentry.
📢 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.

@JimmyVo16 JimmyVo16 self-assigned this Jun 3, 2026
@JimmyVo16 JimmyVo16 marked this pull request as ready for review June 3, 2026 18:46
@JimmyVo16 JimmyVo16 requested a review from a team as a code owner June 3, 2026 18:46
@JimmyVo16 JimmyVo16 requested a review from jrmccannon June 3, 2026 18:46
@JimmyVo16 JimmyVo16 merged commit d1bee5f into main Jun 3, 2026
70 of 73 checks passed
@JimmyVo16 JimmyVo16 deleted the ac/pm-37168/fix-missing-expiration-check branch June 3, 2026 20:15
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants