[EC-787] Create a method in PolicyService to check if a policy applies to a user by r-tome · Pull Request #2537 · bitwarden/server

r-tome · 2023-01-04T11:23:11Z

Type of change

- [ ] Bug fix
- [ ] New feature development
- [X] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Whilst working on EC-758: Add environment var to enforce SSO for all users it was identified that the logic to check if a policy applies to a user is duplicated in:

Stored procedure [dbo].[PolicyApplicableToUser] which is used by methods PolicyRepository.GetManyByTypeApplicableToUserIdAsync and PolicyRepository.GetCountByTypeApplicableToUserIdAsync
BaseRequestValidator has the same logic to check for the SSO policy

The goal is to lift the business logic from the stored procedure [dbo].[PolicyApplicableToUser] into a method (PolicyService.GetPoliciesApplicableToUser(OrganizationUser orgUser, PolicyType policyType, OrganizationUserType minUserType, OrganizationUserStatusType minStatus)) which will retrieve the user’s organization policies from the database and then filter the results in memory according to our business rules.

Code changes

src/Core/Models/Data/Organizations/OrganizationUsers/OrganizationUserPolicyDetails.cs: Model output for the new Stored Procedure [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails]
src/Core/Repositories/IOrganizationUserRepository.cs: Added method GetByUserIdWithPolicyDetailsAsync(Guid userId) to call Stored Procedure [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails]
src/Core/Services/IPolicyService.cs: Added method GetPoliciesApplicableToUserAsync that will call IOrganizationUserRepository.GetByUserIdWithPolicyDetailsAsync and apply business rules to filter results
src/Core/Services/Implementations/CipherService.cs* Switched from using IPolicyRepository. GetCountByTypeApplicableToUserIdAsync to IPolicyService.GetPoliciesApplicableToUserAsync
src/Core/Services/Implementations/OrganizationService.cs: Switched from using IPolicyRepository. GetCountByTypeApplicableToUserIdAsync to IPolicyService.GetPoliciesApplicableToUserAsync
src/Core/Services/Implementations/PolicyService.cs: Added method GetPoliciesApplicableToUserAsync that will call IOrganizationUserRepository.GetByUserIdWithPolicyDetailsAsync and apply business rules to filter results
src/Core/Services/Implementations/SendService.cs: Switched from using IPolicyRepository. GetCountByTypeApplicableToUserIdAsync to IPolicyService.GetPoliciesApplicableToUserAsync
src/Core/Services/Implementations/UserService.cs: Switched from using IPolicyRepository. GetCountByTypeApplicableToUserIdAsync to IPolicyService.GetPoliciesApplicableToUserAsync
src/Identity/IdentityServer/BaseRequestValidator.cs: Deleted own implementation to use IPolicyService.GetPoliciesApplicableToUserAsync
src/Identity/IdentityServer/CustomTokenRequestValidator.cs: Added base dependency IPolicyService
src/Identity/IdentityServer/ResourceOwnerPasswordValidator.cs: Added base dependency IPolicyService
src/Infrastructure.Dapper/Repositories/OrganizationUserRepository.cs: Added method GetByUserIdWithPolicyDetailsAsync(Guid userId) to call Stored Procedure [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails]
src/Infrastructure.EntityFramework/Repositories/OrganizationUserRepository.cs: EF query implementation of Stored Procedure [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails]
src/Sql/Sql.sqlproj: Added new stored procedure file reference for [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails]
src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByUserIdWithPolicyDetails.sql: Added new stored procedure for [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails]
test/Core.Test/Services/PolicyServiceTests.cs: Unit testing the business rules being applied
test/Core.Test/Services/SendServiceTests.cs: Updated existing unit tests to use IPolicyService
test/Infrastructure.EFIntegration.Test/Repositories/EqualityComparers/OrganizationUserPolicyDetailsCompare.cs: Created comparer to be used on EF integration tests
test/Infrastructure.EFIntegration.Test/Repositories/OrganizationUserRepositoryTests.cs: Testing the multiple implementations of the new query
test/Infrastructure.EFIntegration.Test/Repositories/PolicyRepositoryTests.cs: Fixed issue that was preventing the test from working
util/Migrator/DbScripts/2022-12-26_00_OrganizationUserReadByUserIdWithPolicyDetails.sql: Script to add new stored procedure [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails]

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

…licyDetails

…licyDetailsAsync

…icable to a specific user

…ter the obtained policy data

…ableToUserIdAsync to IPolicyService.GetPoliciesApplicableToUserAsync

…ge of IPolicyService.GetPoliciesApplicableToUserAsync

…serAsync

…o-check-if-a-policy-applies-to-a-user

…ithPolicyDetailsAsync

…o-check-if-a-policy-applies-to-a-user

…erIdAsync and GetCountByTypeApplicableToUserIdAsync as obsolete

eliykat

I like it! Thank you for cleaning up my past sins (when I wrote that SQL query 😁 )

src/Core/Models/Data/Organizations/OrganizationUsers/OrganizationUserPolicyDetails.cs

src/Core/Repositories/IPolicyRepository.cs

src/Core/Services/Implementations/CipherService.cs

src/Core/Services/Implementations/PolicyService.cs

src/Core/Services/Implementations/SendService.cs

util/Migrator/DbScripts/2022-12-26_00_OrganizationUserReadByUserIdWithPolicyDetails.sql

eliykat · 2023-01-25T04:36:40Z

util/Migrator/DbScripts/2022-12-26_00_OrganizationUserReadByUserIdWithPolicyDetails.sql

+LEFT JOIN
+    (SELECT
+        PU.UserId,
+        PO.OrganizationId
+    FROM
+        [dbo].[ProviderUserView] PU
+    INNER JOIN
+        [ProviderOrganizationView] PO ON PO.[ProviderId] = PU.[ProviderId]) PUPO
+    ON PUPO.UserId = OU.UserId
+    AND PUPO.OrganizationId = P.OrganizationId


I don't like this nested query/join but I don't know if we can avoid it. Any ideas?

I rewrote the query, let me know what you think.

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

...re.EFIntegration.Test/Repositories/EqualityComparers/OrganizationUserPolicyDetailsCompare.cs

eliykat · 2023-01-25T04:43:06Z

Also please resolve conflicts (my fault, I'll make sure to re-review promptly)

…o-check-if-a-policy-applies-to-a-user

…dWithPolicyDetails

…ableToUser, Policy_ReadByTypeApplicableToUser and function PolicyApplicableToUser

src/Core/Services/Implementations/PolicyService.cs

…o-check-if-a-policy-applies-to-a-user # Conflicts: # src/Sql/Sql.sqlproj

…turn Permissions json

src/Sql/dbo_future/Functions/PolicyApplicableToUser.sql

src/Sql/Sql.sqlproj

src/Core/Services/Implementations/PolicyService.cs

eliykat · 2023-03-22T05:12:02Z

test/Core.Test/Services/PolicyServiceTests.cs

    }

+    [Theory, BitAutoData]
+    public async Task AnyPoliciesApplicableToUserAsync_WithRequireSsoTypeFilter_WithDefaultUserTypeFilter_WithDefaultOrganizationUserStatusFilter_ReturnsFalse(Guid userId, SutProvider<PolicyService> sutProvider)


suggestion only: these test names are too focused on inputs and outputs and do not describe the actual behaviour being tested. What's the general idea here? "returns false if policy does not apply to user"? "RequireSSO policy does not apply to Owners"? I know we have to jam it into a method name so it won't look exactly like that (I prefer Jest for this reason 😁 ) but you get the idea.

This is non-blocking, a suggestion only.

src/Core/Services/Implementations/SendService.cs

eliykat

This looks great, and good job picking it back up again - I know it's hard to load it all back into memory 😁 a few final changes/questions, then let's be done with it!

eliykat · 2023-03-22T05:41:53Z

src/Core/Services/Implementations/PolicyService.cs

+        var organizationUserPolicyDetails = await _organizationUserRepository.GetByUserIdWithPolicyDetailsAsync(userId);
+        var excludedUserTypes = GetUserTypesExcludedFromPolicy(policyType);
+        return organizationUserPolicyDetails.Where(o =>
+            o.PolicyType == policyType &&


I didn't notice this before (sorry!) but we don't need to fetch every policy type from the database just to filter them here.

The other checks here are fine because they're business logic that is more complex and likely to change in the future. But just getting the requested policy type seems like it should be done at the sproc level to minimize the amount of data being fetched from the db.

You're right, good point. I've added a parameter for PolicyType in the sproc.

eliykat · 2023-03-22T05:51:18Z

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

+LEFT JOIN
+    (SELECT
+        PU.UserId,
+        PO.OrganizationId
+    FROM
+        [dbo].[ProviderUserView] PU
+    INNER JOIN
+        [ProviderOrganizationView] PO ON PO.[ProviderId] = PU.[ProviderId]) PUPO
+    ON PUPO.UserId = OU.UserId
+    AND PUPO.OrganizationId = P.OrganizationId


Is there any benefit to this nested SELECT query, or can we just move the JOINS out into the main query?

eliykat · 2023-03-22T06:04:40Z

src/Core/Services/Implementations/PolicyService.cs

+            o.PolicyEnabled &&
+            !excludedUserTypes.Contains(o.OrganizationUserType) &&
+            o.OrganizationUserStatus >= minStatus &&
+            !o.IsProvider);


Can you please confirm with Product that a ProviderUser for an organization is never subject to that organization's policies (e.g. not even RequireSSO or password complexity). They apply to owners so we may want to extend it there. Or Providers might be responsible for managing their own security.

At the moment Providers are not subject to any policies - if this is going to change, we can do it in a separate PR just so we can merge this in the meantime. It might require client changes as well.

…tByUserIdWithPolicyDetailsAsync

…dded parameter for PolicyType

sonarqubecloud · 2023-03-29T16:46:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

…o-check-if-a-policy-applies-to-a-user # Conflicts: # src/Core/Services/IPolicyService.cs # src/Core/Services/Implementations/PolicyService.cs # src/Identity/IdentityServer/BaseRequestValidator.cs # src/Identity/IdentityServer/CustomTokenRequestValidator.cs # src/Identity/IdentityServer/ResourceOwnerPasswordValidator.cs # test/Core.Test/Services/PolicyServiceTests.cs # test/Core.Test/Tools/Services/SendServiceTests.cs

util/Migrator/DbScripts/2023-03-10_00_OrganizationUserReadByUserIdWithPolicyDetails.sql

…erIdWithPolicyDetails.sql Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>

…o-check-if-a-policy-applies-to-a-user

…o-check-if-a-policy-applies-to-a-user # Conflicts: # src/Sql/Sql.sqlproj

…o-check-if-a-policy-applies-to-a-user # Conflicts: # src/Identity/IdentityServer/BaseRequestValidator.cs

r-tome and others added 14 commits December 29, 2022 16:03

[EC-787] Add new stored procedure OrganizationUser_ReadByUserIdWithPo…

64dd55e

…licyDetails

[EC-787] Add new method IOrganizationUserRepository.GetByUserIdWithPo…

db967b7

…licyDetailsAsync

[EC-787] Add OrganizationUserPolicyDetails to represent policies appl…

50f791e

…icable to a specific user

[EC-787] Add method IPolicyService.GetPoliciesApplicableToUser to fil…

7db2a0b

…ter the obtained policy data

[EC-787] Returning PolicyData on stored procedures

23579a5

[EC-787] Changed GetPoliciesApplicableToUserAsync to return ICollection

859a1fc

[EC-787] Switched all usings of IPolicyRepository.GetManyByTypeApplic…

f696a26

…ableToUserIdAsync to IPolicyService.GetPoliciesApplicableToUserAsync

[EC-787] Removed policy logic from BaseRequestValidator and added usa…

08fc74d

…ge of IPolicyService.GetPoliciesApplicableToUserAsync

[EC-787] Added unit tests for IPolicyService.GetPoliciesApplicableToU…

e883910

…serAsync

Merge branch 'master' into EC-787-create-a-method-in-policy-service-t…

4a43e8b

…o-check-if-a-policy-applies-to-a-user

[EC-787] Added unit tests for OrganizationUserRepository.GetByUserIdW…

cc74604

…ithPolicyDetailsAsync

[EC-787] Changed integration test to check for single result

8dd69f2

Merge branch 'master' into EC-787-create-a-method-in-policy-service-t…

925fed5

…o-check-if-a-policy-applies-to-a-user

[EC-787] Marked IPolicyRepository methods GetManyByTypeApplicableToUs…

4cdd102

…erIdAsync and GetCountByTypeApplicableToUserIdAsync as obsolete

r-tome requested review from a team and eliykat January 4, 2023 15:05

r-tome marked this pull request as ready for review January 4, 2023 15:06

eliykat requested changes Jan 25, 2023

View reviewed changes

r-tome and others added 6 commits January 27, 2023 15:19

Merge branch 'master' into EC-787-create-a-method-in-policy-service-t…

949dcca

…o-check-if-a-policy-applies-to-a-user

[EC-787] Returning OrganizationUserId on OrganizationUser_ReadByUserI…

3fa2936

…dWithPolicyDetails

[EC-787] Remove deprecated stored procedures Policy_CountByTypeApplic…

f13ebbd

…ableToUser, Policy_ReadByTypeApplicableToUser and function PolicyApplicableToUser

[EC-787] Added method IPolicyService.AnyPoliciesApplicableToUserAsync

f16a037

[EC-787] Removed 'OrganizationUserType' parameter from queries

f7324d8

[EC-787] Formatted OrganizationUserPolicyDetailsCompare

380dbf7

eliykat mentioned this pull request Mar 10, 2023

[AC-1070] Enforce master password policy on login #2714

Merged

eliykat reviewed Mar 10, 2023

View reviewed changes

src/Core/Services/Implementations/PolicyService.cs Show resolved Hide resolved

r-tome added 3 commits March 10, 2023 11:00

[EC-787] Renamed SQL migration files

7ef274e

Merge branch 'master' into EC-787-create-a-method-in-policy-service-t…

3695a98

…o-check-if-a-policy-applies-to-a-user # Conflicts: # src/Sql/Sql.sqlproj

[EC-787] Changed OrganizationUser_ReadByUserIdWithPolicyDetails to re…

fe09f9a

…turn Permissions json

r-tome requested a review from a team as a code owner March 10, 2023 15:28

r-tome requested a review from eliykat March 10, 2023 17:17

eliykat reviewed Mar 22, 2023

View reviewed changes

eliykat requested changes Mar 22, 2023

View reviewed changes

eliykat reviewed Mar 22, 2023

View reviewed changes

r-tome added 4 commits March 29, 2023 11:21

[EC-787] Updated dates on dbo_future files

f27137e

[EC-787] Remove dbo_future files from sql proj

205b396

[EC-787] Added parameter PolicyType to IOrganizationUserRepository.Ge…

ac296d8

…tByUserIdWithPolicyDetailsAsync

[EC-787] Rewrote OrganizationUser_ReadByUserIdWithPolicyDetails and a…

33a7eeb

…dded parameter for PolicyType

r-tome requested a review from eliykat March 30, 2023 09:54

r-tome requested review from a team as code owners April 18, 2023 15:42

eliykat requested changes Apr 19, 2023

View reviewed changes

util/Migrator/DbScripts/2023-03-10_00_OrganizationUserReadByUserIdWithPolicyDetails.sql Outdated Show resolved Hide resolved

r-tome and others added 2 commits April 19, 2023 08:47

Update util/Migrator/DbScripts/2023-03-10_00_OrganizationUserReadByUs…

fca21ea

…erIdWithPolicyDetails.sql Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>

Merge branch 'master' into EC-787-create-a-method-in-policy-service-t…

bb61a83

…o-check-if-a-policy-applies-to-a-user

eliykat previously approved these changes Apr 19, 2023

View reviewed changes

r-tome added the needs-qa label Apr 19, 2023

Merge branch 'master' into EC-787-create-a-method-in-policy-service-t…

5e6296f

…o-check-if-a-policy-applies-to-a-user # Conflicts: # src/Sql/Sql.sqlproj

r-tome dismissed eliykat’s stale review via 5e6296f May 4, 2023 15:32

eliykat previously approved these changes May 4, 2023

View reviewed changes

r-tome removed the needs-qa label May 10, 2023

Merge branch 'master' into EC-787-create-a-method-in-policy-service-t…

e67d81e

…o-check-if-a-policy-applies-to-a-user # Conflicts: # src/Identity/IdentityServer/BaseRequestValidator.cs

r-tome dismissed eliykat’s stale review via e67d81e May 11, 2023 09:16

r-tome requested a review from eliykat May 11, 2023 09:17

eliykat approved these changes May 12, 2023

View reviewed changes

r-tome merged commit 8d3fe12 into master May 12, 2023

r-tome deleted the EC-787-create-a-method-in-policy-service-to-check-if-a-policy-applies-to-a-user branch May 12, 2023 07:22

cksapp mentioned this pull request Jun 5, 2023

Custom Role User with Admin Permissions Unable to create Send #3000

Closed

1 task

Conversation

r-tome commented Jan 4, 2023 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Objective

Code changes

Before you submit

Uh oh!

eliykat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliykat Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

r-tome Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliykat commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliykat Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eliykat left a comment

Choose a reason for hiding this comment

Uh oh!

eliykat Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

r-tome Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

eliykat Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

eliykat Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 29, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

r-tome commented Jan 4, 2023 •

edited by atlassian bot

Loading

eliykat commented Jan 25, 2023 •

edited

Loading

eliykat Mar 22, 2023 •

edited

Loading

eliykat Mar 22, 2023 •

edited

Loading