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

PM-16085 Increase import limitations #5256

Conversation

prograhamming
Copy link
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/jira/software/c/projects/BW/boards/92?selectedIssue=PM-16085

📔 Objective

The ImportCiphersController has a import limit, which has been bumped multiple times over the years. In order to support larger customers either migrating from a different vendor or existing customers with large backups, we need to adjust the value again.

This time instead of just bumping the hard-coded limit, we want to change this into a configuration value, that the SRE team can adjust if needed (temporary scale the limit and then scale it back down).

The value should be set to 40,000.

📸 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

jrmccannon and others added 15 commits January 9, 2025 14:13
* [PM-14378] Introduce GetCipherPermissionsForOrganization query for Dapper CipherRepository

* [PM-14378] Introduce GetCipherPermissionsForOrganization method for Entity Framework

* [PM-14378] Add integration tests for new repository method

* [PM-14378] Introduce IGetCipherPermissionsForUserQuery CQRS query

* [PM-14378] Introduce SecurityTaskOperationRequirement

* [PM-14378] Introduce SecurityTaskAuthorizationHandler.cs

* [PM-14378] Introduce SecurityTaskOrganizationAuthorizationHandler.cs

* [PM-14378] Register new authorization handlers

* [PM-14378] Formatting

* [PM-14378] Add unit tests for GetCipherPermissionsForUserQuery

* [PM-15378] Cleanup SecurityTaskAuthorizationHandler and add tests

* [PM-14378] Add tests for SecurityTaskOrganizationAuthorizationHandler

* [PM-14378] Formatting

* [PM-14378] Update date in migration file

* [PM-14378] Add missing awaits

* [PM-14378] Bump migration script date

* [PM-14378] Remove Unassigned property from OrganizationCipherPermission as it was making the query too complicated

* [PM-14378] Update sproc to use Union All to improve query performance

* [PM-14378] Bump migration script date
* [PM-14380] Add GetManyByOrganizationIdStatusAsync to SecurityTaskRepository

* [PM-14380] Introduce IGetTasksForOrganizationQuery

* [PM-14380] Add /tasks/organization endpoint

* [PM-14380] Add unit tests

* [PM-14380] Formatting

* [PM-14380] Bump migration script date

* [PM-14380] Bump migration script date
* [deps] DbOps: Update dbup-sqlserver to v6

* Update Migrator.csproj 

Update to 6.0.4

* Update Migrator.csproj

Change back to DBup 6.0.0

* update DbUpLogger.cs methods from the IUpgradeLog interface.

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: rkac-bw <[email protected]>
Co-authored-by: Robert Y <[email protected]>
* Repeating pattern values for BitAutoData attribute

* nullable enabled, added documentation

* execute test method even if no repeating pattern data provided (empty array).

* RepeatingPatternBitAutoDataAttribute unit tests
…rtal (#5165)

* feat(NewDeviceVerification) :
- Added constant to constants in Bit.Core because the cache key format needs to be shared between the Identity Server and the MVC project Admin.
- Updated DeviceValidator class to handle checking cache for user information to allow pass through.
- Updated and Added tests to handle new flow.
- Adding exception flow to admin project. Added tests for new methods in UserService.
…related tests (#5246)

* Fix two-factor authentication revocation logic and update related tests

* Refine test for RevokeNonCompliantOrganizationUserCommand to assert single user revocation
Prevent double encoding, as Handlebars encode strings by default.
* kdf defaults on null map to email hash

* cleanup code. add some randomness as well

* remove null check

* fix test

* move to private method

* remove random options

* tests for random defaults

* SetDefaultKdfHmacKey for old test
…est get query (#5250)

* Added userId check on query

* Added required field to inner select

* PM-16947 - Update to filter inner subquery on user id per discussion with Robert

* Updated to use new query with ROW_NUMBER

* More query optimizations to eliminate returning old requests for a device

* Fixed approval condition to be NULL as 0 means denied.

* Added negation of @ExpirationMinutes

---------

Co-authored-by: Todd Martin <[email protected]>
…s-team' into tools/pm-16085/increase-import-limitations
@prograhamming prograhamming requested review from a team as code owners January 13, 2025 18:44
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 57.26681% with 197 lines in your changes missing coverage. Please review.

Project coverage is 44.08%. Comparing base (cf2389e) to head (cfe83cb).
Report is 1 commits behind head on tools/pm-16261/move-importCiphersAsync-method-into-tools-team.

Files with missing lines Patch % Lines
...ries/Queries/CipherOrganizationPermissionsQuery.cs 0.00% 49 Missing ⚠️
...tyFramework/Vault/Repositories/CipherRepository.cs 0.00% 36 Missing ⚠️
...ework/Vault/Repositories/SecurityTaskRepository.cs 0.00% 22 Missing ⚠️
src/Admin/Views/Users/Edit.cshtml 0.00% 19 Missing ⚠️
.../SecurityTasks/SecurityTaskAuthorizationHandler.cs 85.33% 5 Missing and 6 partials ⚠️
src/Admin/Controllers/UsersController.cs 0.00% 10 Missing ⚠️
...ture.Dapper/Vault/Repositories/CipherRepository.cs 0.00% 9 Missing ⚠️
...rc/Api/Vault/Controllers/SecurityTaskController.cs 0.00% 8 Missing ⚠️
...apper/Vault/Repositories/SecurityTaskRepository.cs 0.00% 8 Missing ⚠️
src/Admin/Models/UserEditModel.cs 0.00% 4 Missing ⚠️
... and 8 more
Additional details and impacted files
@@                                      Coverage Diff                                       @@
##           tools/pm-16261/move-importCiphersAsync-method-into-tools-team    #5256   +/-   ##
==============================================================================================
  Coverage                                                          44.07%   44.08%           
==============================================================================================
  Files                                                               1478     1478           
  Lines                                                              68325    68330    +5     
  Branches                                                            6173     6173           
==============================================================================================
+ Hits                                                               30116    30120    +4     
- Misses                                                             36897    36898    +1     
  Partials                                                            1312     1312           

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

Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailse603d6d5-f93f-47a8-9ca2-b82b066d583d

Great job, no security vulnerabilities found in this Pull Request

@djsmith85 djsmith85 changed the base branch from main to tools/pm-16261/move-importCiphersAsync-method-into-tools-team January 14, 2025 10:40
@djsmith85 djsmith85 requested review from a team as code owners January 14, 2025 10:40
@djsmith85 djsmith85 changed the base branch from tools/pm-16261/move-importCiphersAsync-method-into-tools-team to main January 14, 2025 10:40
@djsmith85 djsmith85 removed request for a team January 14, 2025 10:40
@djsmith85 djsmith85 removed request for a team, r-tome and ike-kottlowski January 14, 2025 10:40
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

I assume this is stacked on top of #5245.

Please rebase this PR based on tools/pm-16261/move-importCiphersAsync-method-into-tools-team and then change the merge target from main into the above branch.

Then it'll only show the differences between them for review. Once PM-16261 is merged into main, you can update this PR with main and then it should have no unreviewed changes.

@prograhamming prograhamming changed the base branch from main to tools/pm-16261/move-importCiphersAsync-method-into-tools-team January 15, 2025 17:40
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.