Skip to content
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

[AC-108] Enterprise policies are not removed when downgraded to teams #3014

Conversation

r-tome
Copy link
Contributor

@r-tome r-tome commented Jun 13, 2023

Type of change

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

Objective

If an organization has UsePolicies = false then any of its members should not be affected by any previously configured policy.

Code changes

Part of the solution was implemented on #2917 to select organization features for te selected plan. I've built further on that by setting the features for the Free and Families plans.

  • src/Admin/Views/Organizations/Edit.cshtml: Setting trial = true when clicking trial buttons
  • src/Admin/Views/Shared/_OrganizationFormScripts.cshtml: Using new parameter trial to set value for SalesAssistedTrialStarted. Added plans 'free' and 'families' to switch to toggle features
  • src/Infrastructure.EntityFramework/Repositories/OrganizationUserRepository.cs: Filtering by organizations that have UsePolicies = true
  • src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByUserIdWithPolicyDetails.sql: Filtering by organizations that have UsePolicies = true
  • util/Migrator/DbScripts/2023-06-13_00_OrgUserPolicyDetailsAddFilterOrgUsePolicies.sql: SQL migration file

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

@r-tome r-tome requested a review from a team June 13, 2023 15:47
@r-tome r-tome marked this pull request as ready for review June 13, 2023 15:47
shane-melton and others added 23 commits June 29, 2023 10:40
…SubscriptionItem (#3037)

* [AC-1423] Add AddonProduct and BitwardenProduct properties to BillingSubscriptionItem

- Add a helper method to determine the appropriate addon type based on the subscription items StripeId

* [AC-1423] Add helper to StaticStore.cs to find a Plan by StripePlanId

* [AC-1423] Use the helper method to set SubscriptionInfo.BitwardenProduct
* Adding the Secret manager to the Plan List

* Adding the unit test for the StaticStoreTests class

* Fix whitespace formatting

* Fix whitespace formatting

* Price update

* Resolving the PR comments

* Resolving PR comments

* Fixing the whitespace

* only password manager plans are return for now

* format whitespace

* Resolve the test issue

* Fixing the failing test

* Refactoring the Plan separation

* add a unit test for SingleOrDefault

* Fix the whitespace format

* Separate the PM and SM plans

* Fixing the whitespace

* Remove unnecessary directive

* Fix imports ordering

* Fix imports ordering

* Resolve imports ordering

* Fixing imports ordering

* Fix response model, add MaxProjects

* Fix filename

* Fix format

* Fix: seat price should match annual/monthly

* Fix service account annual pricing

* Changes for secret manager signup and upgradeplan

* Changes for secrets manager signup and upgrade

* refactoring the code

* Format whitespace

* remove unnecessary using directive

* Resolve the PR comment on Subscription creation

* Resolve PR comment

* Add password manager to the error message

* Add UseSecretsManager to the event log

* Resolve PR comment on plan validation

* Resolving pr comments for service account count

* Resolving pr comments for service account count

* Resolve the pr comments

* Remove the store procedure that is no-longer needed

* Rename a property properly

* Resolving the PR comment

* Resolve PR comments

* Resolving PR comments

* Resolving the Pr comments

* Resolving some PR comments

* Resolving the PR comments

* Resolving the build identity build

* Add additional Validation

* Resolve the Lint issues

* remove unnecessary using directive

* Remove the white spaces

* Adding unit test for the stripe payment

* Remove the incomplete test

* Fixing the failing test

* Fix the failing test

* Fix the fail test on organization service

* Fix the failing unit test

* Fix the whitespace format

* Fix the failing test

* Fix the whitespace format

* resolve pr comments

* Fix the lint message

* Resolve the PR comments

* resolve pr comments

* Resolve pr comments

* Resolve the pr comments

* remove unused code

* Added for sm validation test

* Fix the whitespace format issues

---------

Co-authored-by: Thomas Rittson <[email protected]>
Co-authored-by: Thomas Rittson <[email protected]>
* change the stripeseat id

* change service accountId to align with new product

* make all the Id name for consistent
…anizationSubscriptionResponseModel. Use sp_refreshview instead of sp_refreshsqlmodule in the migration script.
…3036)

* Create UpgradeSecretsManagerSubscription command

---------

Co-authored-by: Thomas Rittson <[email protected]>
* This is a pure lift & shift with no refactors

* Only register subscription commands in Api

---------

Co-authored-by: cyprain-okeke <[email protected]>
* Fix SM parameters not being passed to Stripe

* Fix flaky test

* Fix error message
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

A few notes, I'll review the rest once you've merged in master to clean up the diff.

…when-downgraded-to-teams

# Conflicts:
#	src/Infrastructure.EntityFramework/Repositories/OrganizationUserRepository.cs
#	src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByUserIdWithPolicyDetails.sql
#	util/Migrator/DbScripts/2023-07-18_00_OrganizationUserReadByUserIdWithPolicyDetails.sql
@r-tome
Copy link
Contributor Author

r-tome commented Aug 7, 2023

@eliykat I experimented with using IApplicationCacheService to get the OrganizationAbilities to the determine if a given OrganizationId has UsePolicies enabled. This is what the query method on PolicyService looked like:

private async Task<IEnumerable<OrganizationUserPolicyDetails>> QueryOrganizationUserPolicyDetailsAsync(Guid userId, PolicyType? policyType, OrganizationUserStatusType minStatus = OrganizationUserStatusType.Accepted)
    {
        // Check if the cached policies are available
        if (_cachedOrganizationUserPolicyDetails == null)
        {
            // Cached policies not available, retrieve from the repository
            _cachedOrganizationUserPolicyDetails = await _organizationUserRepository.GetByUserIdWithPolicyDetailsAsync(userId);
        }

        var excludedUserTypes = GetUserTypesExcludedFromPolicy(policyType);
        var orgAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync();
        return _cachedOrganizationUserPolicyDetails.Where(o =>
            (!orgAbilities.ContainsKey(o.OrganizationId) || orgAbilities[o.OrganizationId].Enabled && orgAbilities[o.OrganizationId].UsePolicies) &&
            (policyType == null || o.PolicyType == policyType) &&
            o.PolicyEnabled &&
            !excludedUserTypes.Contains(o.OrganizationUserType) &&
            o.OrganizationUserStatus >= minStatus &&
            !o.IsProvider);
    }

This worked perfectly until I also ran the Admin Panel alongside it and changed the UsePolicies value but then the cached value was not updated (even though we do update the organization abilities). Could this be an issue on my development environment?

@bitwarden-bot
Copy link

bitwarden-bot commented Aug 7, 2023

Logo
Checkmarx One – Scan Summary & Details8e7dbf3d-ab9d-492c-a9de-d24f82756df5

New Issues

Severity Issue Source File / Package Checkmarx Insight
HIGH Reflected_XSS_All_Clients /src/Admin/Views/Shared/_OrganizationFormScripts.cshtml: 133 Attack Vector

…-enterprise-polices-are-not-removed-when-downgraded-to-teams

# Conflicts:
#	src/Admin/Views/Organizations/Edit.cshtml
#	src/Admin/Views/Shared/_OrganizationFormScripts.cshtml
@r-tome r-tome changed the base branch from master to ac/ac-1577/prefill-bitwarden-portal-values August 7, 2023 15:08
@r-tome r-tome requested a review from eliykat August 7, 2023 15:08
@eliykat
Copy link
Member

eliykat commented Aug 8, 2023

If the org abilities cache is in memory only, it wouldn't be shared between the projects. Did you find out whether it's in memory in prod, or is it hosted on a service like redis etc? (Or otherwise shared between the projects somehow?)

@r-tome r-tome marked this pull request as draft August 9, 2023 14:56
@r-tome
Copy link
Contributor Author

r-tome commented Aug 9, 2023

@eliykat I've created another PR with the changes above implemented and tested it by using a service bus resource on Azure. It worked great!

Base automatically changed from ac/ac-1577/prefill-bitwarden-portal-values to master August 15, 2023 23:03
@eliykat eliykat removed their request for review August 20, 2023 22:04
@eliykat
Copy link
Member

eliykat commented Aug 20, 2023

@r-tome just a reminder to close this one if it's no longer needed.

@r-tome r-tome closed this Aug 21, 2023
@r-tome r-tome deleted the AC-108-enterprise-polices-are-not-removed-when-downgraded-to-teams branch August 21, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants