server icon indicating copy to clipboard operation
server copied to clipboard
verified publisher icon bitwarden

[SM-1211] Adding API endpoint to send out Access Request for SM to Admins, addi…

Open cd-bitwarden opened this issue 1 year ago • 2 comments

…ng email template

🎟️ Tracking

https://bitwarden.atlassian.net/browse/SM-1211

📔 Objective

Creating API Endpoints for sending access request email to admins for Secrets Manger

⏰ 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

cd-bitwarden avatar Jun 04 '24 14:06 cd-bitwarden

Logo Checkmarx One – Scan Summary & Details – 7581859e-c276-4df6-a4b1-46331016259c

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 110 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/WebAuthnController.cs: 178 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 411 Attack Vector
MEDIUM Privacy_Violation /src/Api/Vault/Controllers/CiphersController.cs: 961 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 846 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 828 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/AccountsController.cs: 548 Attack Vector
MEDIUM Privacy_Violation /src/Api/Auth/Controllers/TwoFactorController.cs: 444 Attack Vector
MEDIUM Privacy_Violation /src/Api/Controllers/DevicesController.cs: 129 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 260 Attack Vector
MEDIUM Privacy_Violation /src/Api/Controllers/DevicesController.cs: 155 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 376 Attack Vector
MEDIUM Privacy_Violation /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 429 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/WebAuthnController.cs: 68 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/WebAuthnController.cs: 85 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/WebAuthnController.cs: 153 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Api/Vault/Controllers/CiphersController.cs: 222 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 188 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 206 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 245 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 254 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 263 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 280 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 289 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 297 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 350 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 369 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 380 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 402 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 403 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 157 Attack Vector
LOW Log_Forging /src/Api/Controllers/DevicesController.cs: 120 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 540 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 820 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/AccountsController.cs: 838 Attack Vector
LOW Log_Forging /src/Api/Vault/Controllers/CiphersController.cs: 953 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 94 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 104 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 122 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 131 Attack Vector
LOW Log_Forging /src/Api/Auth/Controllers/TwoFactorController.cs: 148 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 240 Attack Vector
LOW Log_Forging /src/Api/Controllers/DevicesController.cs: 146 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 341 Attack Vector
LOW Log_Forging /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 404 Attack Vector
LOW Missing_CSP_Header /src/Core/MailTemplates/Handlebars/SecretsManagerAccessRequest.html.hbs: 6 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Public/Controllers/CollectionsController.cs: 87
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 358
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 358
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 143
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 171
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 184
MEDIUM CSRF /bitwarden_license/src/Sso/Controllers/AccountController.cs: 100
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 583
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/Identity/Controllers/AccountsController.cs: 72
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117
LOW Log_Forging /src/Api/Vault/Controllers/CiphersController.cs: 583

github-actions[bot] avatar Jun 04 '24 15:06 github-actions[bot]

Codecov Report

Attention: Patch coverage is 70.17544% with 17 lines in your changes missing coverage. Please review.

Project coverage is 41.60%. Comparing base (53ca95d) to head (548249a). Report is 28 commits behind head on main.

Files Patch % Lines
.../Services/Implementations/HandlebarsMailService.cs 0.00% 12 Missing :warning:
...odels/Mail/RequestSecretsManagerAccessViewModel.cs 0.00% 3 Missing :warning:
...anager/Commands/Requests/RequestSMAccessCommand.cs 92.85% 0 Missing and 1 partial :warning:
...re/Services/NoopImplementations/NoopMailService.cs 0.00% 1 Missing :warning:
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4155      +/-   ##
==========================================
+ Coverage   41.40%   41.60%   +0.19%     
==========================================
  Files        1267     1272       +5     
  Lines       60032    60144     +112     
  Branches     5498     5513      +15     
==========================================
+ Hits        24857    25020     +163     
+ Misses      34030    33964      -66     
- Partials     1145     1160      +15

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 04 '24 17:06 codecov[bot]

@Thomas-Avery @mzieniukbw Can I please get your review on the small changes requested by QA?

  1. Better error message if the user sends over whitespace to the request access api endpoint
  2. If user does not have a name, use their email address to identify them in the email to their org admin (This fixes the 500 error QA was getting)
  3. Words were not word wrapping properly in the email template

cd-bitwarden avatar Jul 02 '24 19:07 cd-bitwarden