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

Drop LimitCollectionCreationDeletion from the database #4810

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

addisonbeck
Copy link
Contributor

@addisonbeck addisonbeck commented Sep 26, 2024

🎟️ Tracking

🧵 Jira Ticket: PM-10863

📚 Stacked PRs

  1. server: Split LimitCollectionCreationDeletion into two database columns #4709
  2. server: Split Organization.LimitCollectionCreationDeletion into two separate business rules #4730
  3. clients: Split Organization.LimitCollectionCreationDeletion into two separate business rules clients#11223
  4. server: Turn on LimitCollectionCreationDeletionSplit for self host #4808
  5. server: [PM-14821] [PM-14822] Remove LimitCollectionCreationDeletionSplit feature flag #4809
  6. clients: [PM-14821] Remove LimitCollectionCreationDeletionSplit feature flag clients#11258
  7. server: Drop LimitCollectionCreationDeletion from the database #4810
    ⬆️ YOU ARE HERE

📔 Objective

🤳 The Bigger Picture

There is an option in the Admin Console that removes collection creation and
deletion controls from all organization members that are not in the Owner and
Admin roles. This PR stack splits this conjoined option into two separate
controls: one to block create operations and one to block delete
operations. This is a part of an effort to make these settings better align
with what customers expect it to do.

👉 This Pull Request 👍

This is the final pull request in this stack. This commit includes:

  1. Removal of the LimitCollectionCreationDeletion database column from all
    database providers.
  2. Removal of the use of LimitCollectionCreationDeletion from all MSSSQL
    views, stored procedures, etc.

📸 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

Copy link
Contributor

github-actions bot commented Sep 26, 2024

Logo
Checkmarx One – Scan Summary & Details84b91410-bbe3-4ba1-858b-9c99ef775569

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Privacy_Violation /src/Core/AdminConsole/OrganizationAuth/UpdateOrganizationAuthRequestCommand.cs: 84 Attack Vector
MEDIUM Privacy_Violation /src/Core/NotificationHub/NotificationHubPushNotificationService.cs: 195 Attack Vector
LOW Log_Forging /src/Api/Tools/Controllers/SendsController.cs: 299 Attack Vector
LOW Log_Forging /src/Api/Tools/Controllers/SendsController.cs: 193 Attack Vector
LOW Log_Forging /src/Api/Tools/Controllers/SendsController.cs: 157 Attack Vector
LOW Log_Forging /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 37 Attack Vector
LOW Log_Forging /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 37 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM CSRF /src/Billing/Controllers/StripeController.cs: 164
MEDIUM CSRF /src/Billing/Controllers/RecoveryController.cs: 38
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 220
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 371
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 220
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 80
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 121
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 470
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 173
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 533
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 533
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 252
MEDIUM Privacy_Violation /src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs: 28
LOW Log_Forging /src/Billing/Controllers/RecoveryController.cs: 38
LOW Log_Forging /src/Billing/Controllers/RecoveryController.cs: 38
LOW Log_Forging /src/Api/Vault/Controllers/CiphersController.cs: 173
LOW Log_Forging /src/Identity/Controllers/AccountsController.cs: 144
LOW Log_Forging /src/Identity/Controllers/AccountsController.cs: 144
LOW Log_Forging /src/Identity/Controllers/AccountsController.cs: 144
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 927
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 164
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 164
LOW Log_Forging /src/Identity/Controllers/AccountsController.cs: 76
LOW Log_Forging /src/Identity/Controllers/AccountsController.cs: 76
LOW Log_Forging /src/Identity/Controllers/AccountsController.cs: 76
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 381
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 261

@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from 04160e4 to b37502a Compare September 26, 2024 18:34
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from d761964 to fed7aeb Compare September 26, 2024 18:41
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from b37502a to ee3d388 Compare September 26, 2024 19:23
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from fed7aeb to 799789f Compare September 26, 2024 19:24
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from ee3d388 to f835f2f Compare September 26, 2024 20:31
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from 799789f to 2f93927 Compare September 26, 2024 20:32
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from f835f2f to 23cec42 Compare September 26, 2024 20:36
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from 2f93927 to b746178 Compare September 26, 2024 20:36
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from 23cec42 to e68688d Compare September 26, 2024 21:23
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from b746178 to 3f3df37 Compare September 26, 2024 21:24
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch 2 times, most recently from 802ac23 to a610591 Compare September 26, 2024 22:31
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from 3f3df37 to 9cf4516 Compare September 26, 2024 22:34
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from a610591 to 8f0193c Compare September 27, 2024 14:33
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from 9cf4516 to 888efa6 Compare September 27, 2024 14:33
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.75%. Comparing base (fb72e82) to head (8f3879a).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4810      +/-   ##
==========================================
- Coverage   43.75%   43.75%   -0.01%     
==========================================
  Files        1476     1476              
  Lines       68146    68144       -2     
  Branches     6172     6172              
==========================================
- Hits        29816    29815       -1     
+ Misses      37032    37031       -1     
  Partials     1298     1298              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch 3 times, most recently from 9b3558e to 709f140 Compare September 28, 2024 18:51
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from 8f0193c to c735cd4 Compare September 30, 2024 20:16
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from e40f956 to 8437f5f Compare September 30, 2024 20:16
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch from c735cd4 to e6d2417 Compare October 4, 2024 19:59
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from 8437f5f to cf147b4 Compare October 4, 2024 20:00
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/remove-feature-toggle branch 9 times, most recently from 994ddb5 to 2e27524 Compare November 25, 2024 16:13
Base automatically changed from ac/addison/pm-10863/remove-feature-toggle to main December 6, 2024 10:46
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch 10 times, most recently from 0142f8e to 37dfd68 Compare January 9, 2025 15:01
@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863/drop-old-columns branch from 37dfd68 to 8f3879a Compare January 9, 2025 15:30
@addisonbeck addisonbeck marked this pull request as ready for review January 9, 2025 15:50
@addisonbeck addisonbeck requested review from a team as code owners January 9, 2025 15:50
@addisonbeck addisonbeck requested a review from JimmyVo16 January 9, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant