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-5157] Update duo metadata #4071

Closed
wants to merge 10 commits into from

Conversation

ike-kottlowski
Copy link
Contributor

@ike-kottlowski ike-kottlowski commented May 9, 2024

Type of change

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

Objective

Rename Duo properties to match Duo version 4 updates.

Side Quest: Added entity Framework migration for Providers.GatewayType.

Code changes

  • Various files have been changed adding ClientId and ClientSecret to duo responses and requests.
    • v2 changes will be removed once the database is in a stable state to handle Duo SDK v4 params.
  • Added Entity Framework migrations for Provider.GatewayType

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

@ike-kottlowski ike-kottlowski marked this pull request as ready for review May 9, 2024 16:39
@ike-kottlowski ike-kottlowski requested review from a team as code owners May 9, 2024 16:39
@ike-kottlowski ike-kottlowski requested review from rr-bw and amorask-bitwarden and removed request for rr-bw May 9, 2024 16:39
Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 2.63158% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 38.63%. Comparing base (fd173e8) to head (b374b8c).
Report is 1 commits behind head on main.

❗ Current head b374b8c differs from pull request most recent head 4ffe19c. Consider uploading reports for the commit 4ffe19c to get more accurate results

Files Patch % Lines
...ls/Response/TwoFactor/TwoFactorDuoResponseModel.cs 0.00% 18 Missing ⚠️
.../Api/Auth/Models/Request/TwoFactorRequestModels.cs 0.00% 11 Missing ⚠️
.../Core/Auth/Identity/TemporaryDuoWebV4SDKService.cs 25.00% 2 Missing and 1 partial ⚠️
...rc/Identity/IdentityServer/BaseRequestValidator.cs 0.00% 2 Missing and 1 partial ⚠️
src/Api/Auth/Controllers/TwoFactorController.cs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4071      +/-   ##
==========================================
- Coverage   38.66%   38.63%   -0.04%     
==========================================
  Files        1209     1209              
  Lines       58561    58577      +16     
  Branches     5594     5595       +1     
==========================================
- Hits        22643    22629      -14     
- Misses      34863    34896      +33     
+ Partials     1055     1052       -3     

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

Copy link
Contributor

github-actions bot commented May 9, 2024

Logo
Checkmarx One – Scan Summary & Detailsdac3a522-8b65-4bf2-9176-b3d63a50dbf4

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Privacy_Violation /src/Api/Controllers/DevicesController.cs: 155 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 650 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 703 Attack Vector
LOW Log_Forging /src/Api/Controllers/DevicesController.cs: 146 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 615 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 678 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 607
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 132
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProvidersController.cs: 141
MEDIUM CSRF /src/Api/SecretsManager/Controllers/AccessPoliciesController.cs: 229
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 320
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 163
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 163
MEDIUM CSRF /src/Api/Billing/Controllers/ProviderClientsController.cs: 30
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 190
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 333
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 333
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 669
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 645
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 891
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 173
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 711
MEDIUM CSRF /src/Api/Vault/Controllers/FoldersController.cs: 45
MEDIUM CSRF /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 51
MEDIUM CSRF /src/Api/Controllers/UsersController.cs: 22
MEDIUM CSRF /src/Api/Controllers/DevicesController.cs: 70
MEDIUM CSRF /src/Api/Controllers/DevicesController.cs: 57
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 69
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 49
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 92
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 49
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 142
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 148
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 78
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 61
MEDIUM CSRF /bitwarden_license/src/Sso/Controllers/AccountController.cs: 163
MEDIUM CSRF /bitwarden_license/src/Sso/Controllers/AccountController.cs: 96
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/UsersController.cs: 50
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 161
MEDIUM CSRF /src/Api/Auth/Controllers/EmergencyAccessController.cs: 159
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 98
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 88
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 722
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 187
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 898
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 867
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 50
MEDIUM CSRF /src/Identity/Controllers/AccountsController.cs: 72
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 570
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 175
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 246
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 144
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 411
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 375
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 286
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 222
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 572
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 361
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 570
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 748
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 246
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 411
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 375
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 286
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 188
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 277
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1120
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 774
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 159
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 303
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 931
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/Controllers/SettingsController.cs: 36
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 407
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1100
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 432
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 308
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProvidersController.cs: 243
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 81
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 118
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 230
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 331
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 323
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 530
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 627
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 86
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 218
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 627
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 300
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 318
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1066
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1066
MEDIUM CSRF /src/Api/AdminConsole/Controllers/ProviderOrganizationsController.cs: 53
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 260
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 174
MEDIUM CSRF /src/Api/Auth/Controllers/TwoFactorController.cs: 403
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 926
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 287
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 193
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 786
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1043
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1043
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 59
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 127
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 519
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1017
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1017
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 222
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 156
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 187
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 196
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 811
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 312
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 301
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 449
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 545
MEDIUM CSRF /src/Api/Public/Controllers/CollectionsController.cs: 64
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 825
MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 221
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 50
MEDIUM CSRF /src/Api/Tools/Controllers/ImportCiphersController.cs: 66
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 111
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 125
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 962
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/ProviderOrganizationsController.cs: 40
MEDIUM CSRF /src/Api/Auth/Controllers/WebAuthnController.cs: 130
MEDIUM CSRF /src/Api/Auth/Controllers/WebAuthnController.cs: 101
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 679
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1120
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 93
MEDIUM CSRF

More results are available on AST platform

rkac-bw
rkac-bw previously approved these changes May 14, 2024
Copy link
Contributor

@rkac-bw rkac-bw left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

Nice work! Some questions below (mostly around testing and ensuring that I understand what's going on)!

@@ -217,8 +226,16 @@ public async Task<TwoFactorDuoResponseModel> PutDuo([FromBody] UpdateTwoFactorDu

try
{
var duoApi = new DuoApi(model.IntegrationKey, model.SecretKey, model.Host);
await duoApi.JSONApiCall("GET", "/auth/v2/check");
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ was the removal of await duoApi.JSONApiCall("GET", "/auth/v2/check"); intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

❓ : Do we have tests for this controller which could cover these endpoints and these new scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing the DuoApi proved to be tricky and not worth the trouble due to the removal of DuoApi in a later release.

@ike-kottlowski
Copy link
Contributor Author

closing this PR to match the correct branch name and PR.

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.

None yet

3 participants