[PM-40514] Add custom attribute to check Organization abilities, use for risk insights endpoints - #8240
[PM-40514] Add custom attribute to check Organization abilities, use for risk insights endpoints #8240lastbestdev wants to merge 11 commits into
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR introduces Code Review DetailsNew finding this round
Still open from earlier rounds (existing threads — not re-posted inline)
Minor observations (no action required) The same logical condition now returns 400 from the attribute path and 404 from |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
lastbestdev
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8240 +/- ##
==========================================
+ Coverage 63.21% 63.22% +0.01%
==========================================
Files 2410 2411 +1
Lines 104450 104478 +28
Branches 9458 9462 +4
==========================================
+ Hits 66026 66060 +34
+ Misses 36165 36161 -4
+ Partials 2259 2257 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var orgAbilityCacheService = context.HttpContext.RequestServices.GetRequiredService<IOrganizationAbilityCacheService>(); | ||
|
|
||
| var orgAbility = await orgAbilityCacheService.GetOrganizationAbilityAsync(orgId); | ||
| if (orgAbility == null) | ||
| { | ||
| throw new BadRequestException("The user's organization does not have access to this feature in their plan."); | ||
| } | ||
|
|
||
| var hasAbility = (bool)_ability.GetValue(orgAbility)!; | ||
| if (!hasAbility) | ||
| { | ||
| throw new BadRequestException("The user's organization does not have access to this feature in their plan."); | ||
| } |
There was a problem hiding this comment.
Details and fix
[Authorize("Application")] only requires an authenticated user — it does not establish membership in {orgId}. Because this is an action filter, the ability lookup happens before the action body calls _currentContext.AccessReports(orgId).
For a caller who is not a member of the target org:
| Target org state | Before this PR | After this PR |
|---|---|---|
UseRiskInsights = true |
404 | 404 |
UseRiskInsights = false / not in cache |
404 | 400 "The user's organization does not have access to this feature in their plan." |
So any authenticated user holding an org GUID can now distinguish "org has Risk Insights" from "org does not / does not exist" on e.g. GET reports/organizations/{organizationId}/latest. Previously AuthorizeAsync ran AccessReports first and returned 404 uniformly.
Note that ReportsController.AuthorizeAsync (line 230) does this correctly — permission check first, and it throws NotFoundException for the ability failure — so the two halves of this feature now behave differently for the same condition.
Options:
- Have the attribute resolve org access (e.g.
ICurrentContext) before the ability lookup and throwNotFoundExceptionwhen the caller has no relationship to the org, or - Throw
NotFoundExceptionhere so the response is indistinguishable, matchingReportsController.AuthorizeAsync.
| if (orgId == Guid.Empty) | ||
| { | ||
| throw new Exception("Route parameter 'orgId' or 'organizationId' is missing or invalid."); | ||
| } |
There was a problem hiding this comment.
Details and fix
GetOrganizationId() parses 00000000-0000-0000-0000-000000000000 successfully, so GET reports/organizations/00000000-0000-0000-0000-000000000000/latest reaches this branch. A bare Exception falls into the else arm of ExceptionHandlerFilterAttribute, which logs LogError(0, exception, "Unhandled exception") and returns 500.
Before this PR, EnsureValidIds handled the same input with BadRequestException("OrganizationId is required.") → 400. This is client-triggerable, so it will also add noise to error monitoring.
if (orgId == Guid.Empty)
{
throw new BadRequestException("Route parameter 'orgId' or 'organizationId' is missing or invalid.");
}
| // <summary> | ||
| /// Specifies that the class or method that this attribute is applied to requires the specified organization ability | ||
| /// to be enabled. If the organization ability is not enabled, a <see cref="FeatureUnavailableException"/> is thrown | ||
| // </summary> |
There was a problem hiding this comment.
🎨 SUGGESTED: The class summary uses // instead of ///, so it never renders, and it names the wrong exception.
Details and fix
Lines 10 and 13 open/close with //, which means the XML doc for the type is dropped entirely. The text also says FeatureUnavailableException (copied from RequireFeatureAttribute) while this attribute throws BadRequestException. There is also a stray // </summary> at line 22 inside the constructor docs.
/// <summary>
/// Specifies that the class or method that this attribute is applied to requires the specified organization ability
/// to be enabled. If the organization ability is not enabled, a <see cref="BadRequestException"/> is thrown.
/// </summary>Worth fixing on a new shared utility, since consumers will rely on the documented exception type.
| [RequireFeature(FeatureFlagKeys.AccessIntelligenceNewArchitecture)] | ||
| [HttpGet("{organizationId}/{reportId}/file/renew")] | ||
| public async Task<OrganizationReportFileResponseModel> RenewFileUploadUrlAsync( |
There was a problem hiding this comment.
Five actions never got the require org ability attribute: RenewFileUploadUrlAsync (line 298) plus the data/summary (508/526) and data/application (543/562) pairs, two of them writes. Each just needs [RequireOrganizationAbility(nameof(OrganizationAbility.UseRiskInsights))] above the method. Worth pairing it with a reflection test that fails if an endpoint here is missing the attribute, since the comment at OrganizationReportsControllerTests.cs:227 currently says it's on "each action".
[RequireFeature(FeatureFlagKeys.AccessIntelligenceNewArchitecture)]
[HttpGet("{organizationId}/{reportId}/file/renew")]
[RequireOrganizationAbility(nameof(OrganizationAbility.UseRiskInsights))] // + add this
public async Task<OrganizationReportFileResponseModel> RenewFileUploadUrlAsync(
| using Bit.Core.Models.Data.Organizations; | ||
| using Microsoft.AspNetCore.Mvc.Filters; | ||
|
|
||
| namespace Bit.Core.Utilities; |
There was a problem hiding this comment.
♻️ DEBT: New file lives in src/Api/Utilities but declares namespace Bit.Core.Utilities.
Details and fix
Every other file in src/Api/Utilities uses namespace Bit.Api.Utilities. This looks like a carry-over from src/Core/Utilities/RequireFeatureAttribute.cs, which this class is modeled on.
The type cannot actually live in Core — it depends on Bit.Api.AdminConsole.Authorization.HttpContextExtensions.GetOrganizationId() — so the namespace advertises availability from the Core assembly that does not exist, and Core-layer code that has using Bit.Core.Utilities; will not resolve it.
namespace Bit.Api.Utilities;OrganizationReportsController already has using Bit.Api.Utilities;; ReportsController and test/Api.Test/Utilities/RequireOrganizationAbilityAttributeTests.cs would need the using swapped.
|
Closing this PR and reopening a fix without the custom attribute. Due to the complexity of trying to patch together existing authorization checks on Access Intelligence endpoints with the custom attribute, I'll defer creating it until another day. |
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40514
📔 Objective
Adds a reusable custom attribute for checking the presence of an Organization ability before allowing an API request to process and return a result.
The original ticket is for patching up access to v1 Access Intelligence endpoints (aka Risk Insights), 5 of which were not checking the newly added Organization ability for feature access. The new attribute is applied to all Access Intelligence endpoints to gate access.
📸 Screenshots
N/A