[PM-14439] Add PolicyRequirements for enforcement logic #5336

eliykat · 2025-01-29T03:07:08Z

🎟️ Tracking

https://bitwarden.atlassian.net/jira/dashboards/10012

📔 Objective

Background

Enterprise policies are organization-wide settings that limit app behavior for members. For example, if the Disable Send policy is enabled, members of that organization cannot use the Send feature.

Policies are complex because they're cross-domain and cross-team, affecting any area of the application. They can also have different enforcement logic. For example:

most policies exempt Owners and Admins from their effect, but some don't
the SSO Policy sometimes exempts Owners, but this depends on an IGlobalSettings override
most policies are only enforced if the user is in an Accepted state, but some need to be checked before then
different organizations can have conflicting policies which must be reconciled

Today, this is mostly handled by PolicyService. However, this has to know about every type of policy, which is not very maintainable or extendible. It also has a poor interface:

AnyPoliciesApplicableToUserAsync returns true if any policy of a given type is Enabled - which is handy for very simple boolean policies, but only covers a subset of policies.
GetPoliciesApplicableToUserAsync returns all policies of a given type that should be enforced against a user, but then leaves the caller to evaluate and reconcile the Policy objects (including deserializing the Data json blob).

Additionally, many devs don't know about this, and access policies directly using the repository. This bypasses the business logic entirely and leads to repeated bugs.

Proposed solution

We need to stop dealing with raw Policy objects in our business logic. This PR introduces the idea of a "policy requirement", which is the result of filtering and combining policies to create a single object. This object represents business requirements rather than data. In other words, it's a bridge between the policy domain and the caller's domain. Callers no longer have to know about the underlying policies.
A policy requirement is defined for each policy type. It can be anything - whatever is most convenient for representing a policy type's business requirements.
Policy requirements define their own factory function, which takes (at a minimum) all policies that could potentially affect the user. This function defines the policy type's specific rules, e.g. who it applies to.
Policy requirements are registered with the PolicyRequirementQuery.
Callers can call the PolicyRequirementQuery to get the requirement they want to check.

This PR implements a draft version of this for feedback.

TODO

enable nullable
review requirements to make sure their business logic is correct
update all calling code to use the new query
use feature flags
SQL migration script
EF implementation
(in a later PR) consider adding a response model for each requirement and including it in sync data (so the clients don't have to reimplement this logic)

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

…efactor

eliykat · 2025-01-29T03:14:25Z

src/Sql/dbo/Stored Procedures/PolicyDetails_ReadByUserId.sql

Point of interest: this sproc only returns policies where they're enabled and the organization's plan supports them. This is a fundamental requirement of the policy domain so it's not left up to the policy requirements to decide. (Of course if this changed in the future we could lift it up.)

eliykat · 2025-01-29T03:20:26Z

src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/PolicyRequirementQuery.cs

There is an intentional split of responsibilities here: all business logic is in the policy requirements, which are written in a functional style. The query is agnostic about what policies are being handled - its only job is to connect policy requirements to dependencies.

eliykat · 2025-01-29T03:21:52Z

src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/SendRequirement.cs

Point of interest: we have a separate policy type for "Disable Send" and "Send Options". In practice we always need to check both, so we can encapsulate them in the same requirement.

eliykat · 2025-01-29T03:22:39Z

...minConsole/OrganizationFeatures/Policies/PolicyRequirements/SingleOrganizationRequirement.cs

Point of interest: an example of a more complex requirement that can encapsulate additional logic (beyond enabled/disabled) that is currently in the caller (in this case, OrganizationService).

eliykat · 2025-01-29T03:23:08Z

src/Core/Tools/Services/Implementations/SendService.cs

Simple example usage by an outside caller.

eliykat · 2025-01-29T03:26:48Z

src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/PolicyRequirementQuery.cs

+        _policyRequirements.Add(SendPolicyRequirement.Create);
+        _policyRequirements.Add(up
+            => SsoPolicyRequirement.Create(up, globalSettings.Sso));


Example of how you can just register the factory function directly for simple requirements (which will be most of them), or inject additional values if needed (settings, feature flags, etc).

Just an idea but maybe we could use Reflection here to find all classes that inherit IPolicyRequirement and automatically register them. Maybe it adds some unnecessary complexity though.

github-actions · 2025-01-29T04:15:02Z

Checkmarx One – Scan Summary & Details – 0c734754-7fb5-43a9-9a6b-de64550906e5

Great job, no security vulnerabilities found in this Pull Request

r-tome

This is looking very good! Please add some xmldoc to the interfaces

r-tome · 2025-01-29T15:03:27Z

src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/SendPolicyRequirement.cs

+
+public class SendPolicyRequirement : IPolicyRequirement
+{
+    public bool DisableSend { get; init; }


Suggested change

public bool DisableSend { get; init; }

public bool DisableSend { get; private init; }

r-tome · 2025-01-29T15:03:44Z

...re/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/PolicyRequirementHelpers.cs

This is great!

r-tome · 2025-01-29T15:05:26Z

...re/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/PolicyRequirementHelpers.cs

+
+    public static IEnumerable<OrganizationUserPolicyDetails> ExcludeOwnersAndAdmins(
+        this IEnumerable<OrganizationUserPolicyDetails> userPolicyDetails) =>
+            userPolicyDetails.Where(x => x.OrganizationUserType != OrganizationUserType.Owner);


Suggested change

userPolicyDetails.Where(x => x.OrganizationUserType != OrganizationUserType.Owner);

userPolicyDetails.Where(x => x.OrganizationUserType is not OrganizationUserType.Owner and not OrganizationUserType.Admin);

r-tome · 2025-01-29T15:09:49Z

src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/PolicyRequirementQuery.cs

+        _policyRequirements.Add(SendPolicyRequirement.Create);
+        _policyRequirements.Add(up
+            => SsoPolicyRequirement.Create(up, globalSettings.Sso));


Just an idea but maybe we could use Reflection here to find all classes that inherit IPolicyRequirement and automatically register them. Maybe it adds some unnecessary complexity though.

eliykat added 10 commits January 27, 2025 16:41

Add requirements

8701e20

Add draft sql query

d0bcb2b

Simple query without generics

010d6b8

Build out query

9fa1096

Use delegate, wire up

b3977d4

Merge remote-tracking branch 'origin/main' into policy-requirements-r…

a6d4680

…efactor

Tweak naming, use extension methods

983eb70

Wrap dict in separate class

6f5df04

Remove duplicate comment

205dde2

fix linting and style

e335e30

eliykat commented Jan 29, 2025

View reviewed changes

eliykat changed the title ~~Policy requirements refactor~~ [Server] Add PolicyRequirements for enforcement logic Jan 29, 2025

eliykat commented Jan 29, 2025

View reviewed changes

eliykat changed the title ~~[Server] Add PolicyRequirements for enforcement logic~~ [PM-14439] Add PolicyRequirements for enforcement logic Jan 29, 2025

r-tome reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-14439] Add PolicyRequirements for enforcement logic #5336

[PM-14439] Add PolicyRequirements for enforcement logic #5336

eliykat commented Jan 29, 2025 •

edited

Loading

eliykat Jan 29, 2025 •

edited

Loading

eliykat Jan 29, 2025

eliykat Jan 29, 2025

eliykat Jan 29, 2025

eliykat Jan 29, 2025 •

edited

Loading

eliykat Jan 29, 2025 •

edited

Loading

r-tome Jan 29, 2025

github-actions bot commented Jan 29, 2025 •

edited

Loading

r-tome left a comment

r-tome Jan 29, 2025

r-tome Jan 29, 2025

r-tome Jan 29, 2025

r-tome Jan 29, 2025

	public bool DisableSend { get; init; }
	public bool DisableSend { get; private init; }

	userPolicyDetails.Where(x => x.OrganizationUserType != OrganizationUserType.Owner);
	userPolicyDetails.Where(x => x.OrganizationUserType is not OrganizationUserType.Owner and not OrganizationUserType.Admin);

[PM-14439] Add PolicyRequirements for enforcement logic #5336

Are you sure you want to change the base?

[PM-14439] Add PolicyRequirements for enforcement logic #5336

Conversation

eliykat commented Jan 29, 2025 • edited Loading

🎟️ Tracking

📔 Objective

Background

Proposed solution

TODO

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

eliykat Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliykat Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

eliykat Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2025 • edited Loading

Great job, no security vulnerabilities found in this Pull Request

r-tome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliykat commented Jan 29, 2025 •

edited

Loading

eliykat Jan 29, 2025 •

edited

Loading

eliykat Jan 29, 2025 •

edited

Loading

eliykat Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading