-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[PM-40514] Add custom attribute to check Organization abilities, use for risk insights endpoints #8240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-40514] Add custom attribute to check Organization abilities, use for risk insights endpoints #8240
Changes from all commits
b613d6e
19088f0
098dd03
6ee73de
57bb73e
c3db516
d7952a9
a2496e8
2ff8c73
6796de1
f1e2c35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,61 @@ | |||||||||||
| ο»Ώusing System.Reflection; | |||||||||||
| using Bit.Api.AdminConsole.Authorization; | |||||||||||
| using Bit.Core.AdminConsole.AbilitiesCache; | |||||||||||
| using Bit.Core.Exceptions; | |||||||||||
| using Bit.Core.Models.Data.Organizations; | |||||||||||
| using Microsoft.AspNetCore.Mvc.Filters; | |||||||||||
|
|
|||||||||||
| namespace Bit.Core.Utilities; | |||||||||||
|
|
|||||||||||
| // <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> | |||||||||||
|
Comment on lines
+10
to
+13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π¨ SUGGESTED: The class summary uses Details and fixLines 10 and 13 open/close with /// <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. |
|||||||||||
| public class RequireOrganizationAbilityAttribute : Attribute, IAsyncActionFilter | |||||||||||
| { | |||||||||||
| private readonly PropertyInfo _ability; | |||||||||||
|
|
|||||||||||
| /// <summary> | |||||||||||
| /// Initializes a new instance of the <see cref="RequireOrganizationAbilityAttribute"/> class with the specified ability key. | |||||||||||
| /// </summary> | |||||||||||
| /// <param name="abilityKey">The name of the organization ability to require. Should be a valid boolean property on the <see cref="OrganizationAbility"/> class.</param> | |||||||||||
| // </summary> | |||||||||||
| public RequireOrganizationAbilityAttribute(string abilityKey) | |||||||||||
| { | |||||||||||
| if (string.IsNullOrWhiteSpace(abilityKey) || !typeof(OrganizationAbility).GetProperties().Any(p => p.Name == abilityKey && p.PropertyType == typeof(bool))) | |||||||||||
| { | |||||||||||
| throw new ArgumentException("Ability key must be a valid boolean property on the OrganizationAbility class.", nameof(abilityKey)); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| _ability = typeof(OrganizationAbility).GetProperty(abilityKey)!; | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next) | |||||||||||
| { | |||||||||||
| await OnActionExecutingAsync(context); | |||||||||||
| await next(); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| private async Task OnActionExecutingAsync(ActionExecutingContext context) | |||||||||||
| { | |||||||||||
| var orgId = context.HttpContext.GetOrganizationId(); | |||||||||||
| if (orgId == Guid.Empty) | |||||||||||
| { | |||||||||||
| throw new Exception("Route parameter 'orgId' or 'organizationId' is missing or invalid."); | |||||||||||
| } | |||||||||||
|
Comment on lines
+42
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Details and fix
Before this PR, if (orgId == Guid.Empty)
{
throw new BadRequestException("Route parameter 'orgId' or 'organizationId' is missing or invalid.");
} |
|||||||||||
|
|
|||||||||||
| 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."); | |||||||||||
| } | |||||||||||
|
Comment on lines
+47
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Details and fix
For a caller who is not a member of the target org:
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. Note that Options:
|
|||||||||||
| } | |||||||||||
| } | |||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
β»οΈ DEBT: New file lives in
src/Api/Utilitiesbut declaresnamespace Bit.Core.Utilities.Details and fix
Every other file in
src/Api/Utilitiesusesnamespace Bit.Api.Utilities. This looks like a carry-over fromsrc/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 hasusing Bit.Core.Utilities;will not resolve it.OrganizationReportsControlleralready hasusing Bit.Api.Utilities;;ReportsControllerandtest/Api.Test/Utilities/RequireOrganizationAbilityAttributeTests.cswould need the using swapped.