-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
New Issues
Fixed Issues
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
closing this PR to match the correct branch name and PR. |
Type of change
Objective
Rename Duo properties to match Duo version 4 updates.
⭐ Side Quest: Added entity Framework migration for
Providers.GatewayType
.Code changes
ClientId
andClientSecret
to duo responses and requests.Provider.GatewayType
Before you submit
dotnet format --verify-no-changes
) (required)