Skip to content

[PM-8220] New Device Verification #5084

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

Merged
merged 12 commits into from
Dec 12, 2024

Conversation

ike-kottlowski
Copy link
Contributor

@ike-kottlowski ike-kottlowski commented Nov 25, 2024

🎟️ Tracking

PM-8220

📔 Objective

Continue QoL code improvements on BaseRequestValidator.

Users will be required to verify logins on unknown devices via email OTP.

Extras: SSO requirement will inform user before they supply two factor authentication.

📸 Screenshots

New Device Verification Required 400 response

firefox_B3x1TyDfJd.mp4

Early SSO Required failure

firefox_V0CnPmZ79t.mp4

SSO Successful

firefox_6PtVtLuiSi.mp4

SSO with Two factor successful

firefox_q2xmSbQa3b.mp4

Two Factor Request with a goof

firefox_O1gA7shwaR.mp4

Successful New Device Verification

firefox_y2EMqRVzXg.mp4

⏰ 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

…r to follow logic for new device verification.

Updated DeviceValidator to handle new device verification, behind a feature flag. Also move interface to new file.

Updated CustomRequestValidator to take the requesting device so we only query the database once. We still query twice but only in legacy.
Copy link
Contributor

github-actions bot commented Nov 25, 2024

Logo
Checkmarx One – Scan Summary & Detailsde369ee7-308c-47fc-a033-067a002fe113

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Billing/Controllers/PayPalController.cs: 66 Attack Vector
LOW Missing_CSP_Header /src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs: 5 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 68
MEDIUM CSRF /src/Api/Controllers/LicensesController.cs: 44
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 106
MEDIUM CSRF /src/Billing/Controllers/RecoveryController.cs: 38
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Billing/Controllers/StripeController.cs: 164
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 238
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
MEDIUM CSRF /src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 372
MEDIUM CSRF /src/Api/AdminConsole/Controllers/PoliciesController.cs: 89
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 46
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 65
LOW Unsafe_Use_Of_Target_blank /src/Core/MailTemplates/Handlebars/OrganizationDomainUnclaimed.html.hbs: 20

@ike-kottlowski ike-kottlowski marked this pull request as ready for review December 3, 2024 03:31
@ike-kottlowski ike-kottlowski requested a review from a team as a code owner December 3, 2024 03:31
@ike-kottlowski ike-kottlowski requested a review from rr-bw December 3, 2024 03:31
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 77.18447% with 47 lines in your changes missing coverage. Please review.

Project coverage is 42.90%. Comparing base (94761a8) to head (dbac184).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
...tyServer/RequestValidators/BaseRequestValidator.cs 82.85% 10 Missing and 2 partials ⚠️
...Server/RequestValidators/WebAuthnGrantValidator.cs 0.00% 10 Missing ⚠️
...equestValidators/ResourceOwnerPasswordValidator.cs 47.05% 9 Missing ⚠️
...r/RequestValidators/CustomTokenRequestValidator.cs 11.11% 8 Missing ⚠️
...dentityServer/RequestValidators/DeviceValidator.cs 91.48% 5 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5084      +/-   ##
==========================================
- Coverage   42.92%   42.90%   -0.02%     
==========================================
  Files        1433     1440       +7     
  Lines       65672    65948     +276     
  Branches     6022     6052      +30     
==========================================
+ Hits        28191    28298     +107     
- Misses      36215    36380     +165     
- Partials     1266     1270       +4     

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

@rr-bw rr-bw requested review from JaredSnider-Bitwarden and removed request for rr-bw December 3, 2024 22:17
@ike-kottlowski ike-kottlowski marked this pull request as draft December 4, 2024 18:14
…de all device related processes for request validation.
… a single entry point for BseRequestValidator; Refactored tests for BaseRequestValidator to match changes; Updated Tests for DeviceValidator.
@ike-kottlowski ike-kottlowski marked this pull request as ready for review December 11, 2024 09:25
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.

Excellent work. Thank you for all the video tests - appreciate the thoroughness.

@ike-kottlowski ike-kottlowski merged commit 867fa84 into main Dec 12, 2024
52 checks passed
@ike-kottlowski ike-kottlowski deleted the auth/pm-8220/send-400-for-new-device branch December 12, 2024 17:08
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.

2 participants