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-8107] Remove Duo v2 from server #4934

Merged
merged 34 commits into from
Nov 18, 2024

Conversation

ike-kottlowski
Copy link
Contributor

@ike-kottlowski ike-kottlowski commented Oct 23, 2024

🎟️ Tracking

PM-8107

Blocked By: #4894

📔 Objective

We are moving away from Duo Version 2 artifacts in our code. This aims to removes the last visages of Duo Version 2 from models, objects, and token providers.

A notable change is the Duo two-factor token, and organization token) have been renamed to better align with verbiage from Duo: name change to "Universal" instead of "web".

📸 Screenshots

Enable Duo / Disable - Show data validation and successful save (11/14/24 update)

msedge_MZYLgryruJ.mp4

Enabled Duo - Correct input validation -> Bad config error (Duo.Client.DoHealthCheck() fails) (11/14/24 update)

msedge_caLHcLAn8o.mp4

Login with Duo (11/14/24 update)

msedge_OaHc8O6bDM.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

This comment was marked as off-topic.

@ike-kottlowski ike-kottlowski changed the title Auth/pm 8107/remove duo metadata v2 [PM_8107] Remove Duo v2 from server Oct 23, 2024
@ike-kottlowski ike-kottlowski marked this pull request as ready for review October 23, 2024 22:58
@ike-kottlowski ike-kottlowski requested a review from a team as a code owner October 23, 2024 22:58
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 85.10638% with 28 lines in your changes missing coverage. Please review.

Project coverage is 43.06%. Comparing base (6f7cdcf) to head (51a426a).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...dentity/TokenProviders/DuoUniversalTokenService.cs 64.40% 19 Missing and 2 partials ⚠️
...Providers/OrganizationDuoUniversalTokenProvider.cs 93.61% 2 Missing and 1 partial ⚠️
.../Identity/TokenProviders/YubicoOtpTokenProvider.cs 0.00% 1 Missing and 1 partial ⚠️
.../Api/Auth/Models/Request/TwoFactorRequestModels.cs 90.00% 0 Missing and 1 partial ⚠️
...entity/TokenProviders/DuoUniversalTokenProvider.cs 98.03% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4934      +/-   ##
==========================================
+ Coverage   42.68%   43.06%   +0.38%     
==========================================
  Files        1406     1404       -2     
  Lines       65090    64713     -377     
  Branches     5969     5925      -44     
==========================================
+ Hits        27782    27869      +87     
+ Misses      36063    35610     -453     
+ Partials     1245     1234      -11     

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


🚨 Try these New Features:

@ike-kottlowski ike-kottlowski added the hold Hold this PR or item until later; DO NOT MERGE label Oct 24, 2024
@ike-kottlowski ike-kottlowski removed the hold Hold this PR or item until later; DO NOT MERGE label Oct 24, 2024
@rr-bw rr-bw requested review from JaredSnider-Bitwarden and removed request for rr-bw October 29, 2024 18:52
@ike-kottlowski ike-kottlowski changed the title [PM_8107] Remove Duo v2 from server [PM-8107] Remove Duo v2 from server Oct 30, 2024
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.

Really nice start on these changes! A bit more testing would be great!

src/Core/Auth/Services/IDuoUniversalConfigSerivce.cs Outdated Show resolved Hide resolved
src/Core/Auth/Identity/TokenProviders/DuoTokenProvider.cs Outdated Show resolved Hide resolved
@ike-kottlowski
Copy link
Contributor Author

I've updated the description with current screen recordings.

@JaredSnider-Bitwarden

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.

Really nice work. Thanks for all your efforts adding to our test coverage!

/// OrganizationDuo and Duo TwoFactorProviderTypes both use the same flows so both of those Token Providers will
/// have this class injected to utilize these methods
/// </summary>
public interface IDuoUniversalTokenService
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking Suggestion: we should generally keep interfaces and implementations in separate files for separation of concerns.

@ike-kottlowski ike-kottlowski merged commit ab5d473 into main Nov 18, 2024
52 checks passed
@ike-kottlowski ike-kottlowski deleted the auth/pm-8107/remove-duo-metadata-v2 branch November 18, 2024 23:58
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