Adding authenticator enrollment and verification to authentication client by czf · Pull Request #695 · auth0/auth0.net

czf · 2023-12-29T00:24:07Z

Changes

Adding support for [/mfa/associate](The authentication client doesn't support mfa/associate) to authentication client
Adding support for /oauth/token verification with OOB to authentication client

Added new methods to authentication client to support the API endpoints
Added request/response classes

References

resolves #694

Testing

This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not

Checklist

I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors

…ient

frederikprijck

Can we please stick to solving the issue at hand, and avoid introducing any unneccesary new-way of doing things (e.g. validationErrors, Required).

It's not that I have a problem with that approach, but I do not believe we should just add it for this one specific endpoint. Neither am I saying we should add it everywhere. But for the scope of this PR, we do not need it.

frederikprijck · 2024-01-10T10:25:06Z

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

+            {
+                throw new ArgumentNullException(nameof(request));
+            }
+            if (!request.IsValid(out List<string> validationErrors))


I am not sure I understand why we are suddenly starting to do this without any context.

removed this

frederikprijck · 2024-01-10T10:34:19Z

Can we be consistent with naming across this PR and #698 ? E.g. #698 uses DeleteMfaAuthenticatorRequest, this PR uses AssociateNewAuthenticatorRequest, while I think AssociateMfaAuthenticatorRequest would be more consistent.

czf · 2024-01-11T19:49:08Z

Can we be consistent with naming across this PR and #698 ? E.g. #698 uses DeleteMfaAuthenticatorRequest, this PR uses AssociateNewAuthenticatorRequest, while I think AssociateMfaAuthenticatorRequest would be more consistent.

renamed to AssociateMfaAuthenticatorRequest

frederikprijck

Added some additional suggestions to make it more consistent with what we have now.

This PR does need to be properly tested, so will be a bit slow before this will be merged.

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

src/Auth0.AuthenticationApi/IAuthenticationApiClient.cs

src/Auth0.AuthenticationApi/Models/AssociateNewAuthenticatorResponse.cs

frederikprijck · 2024-01-12T10:22:38Z

src/Auth0.AuthenticationApi/Models/AssociateNewAuthenticatorResponse.cs

+        public string AuthenticatorType { get; set; }
+
+        [JsonProperty("oob_channel")]
+        public string OobChannel { get; set; }


This is not as mentioned in the docs, as the docs mention oob_channels for the response. I think the code is correct, but the docs is not. I will need to verify this with the corresponding team.

yeah I ran into it when first trying to test with postman.

src/Auth0.AuthenticationApi/Models/AssociateMfaAuthenticatorRequest.cs

src/Auth0.AuthenticationApi/Models/MfaOobTokenRequest.cs

Co-authored-by: Frederik Prijck <frederik.prijck@gmail.com>

frederikprijck

Added a few review comments, but they do concern me. They show this PR has not been tested as it does not compile.

We will need to find time to validate all of these changes, which we currently can not prioritise.

frederikprijck · 2024-04-03T07:01:37Z

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

        }
+
+        /// <inheritdoc/>
+        public Task<AssociateMgaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)


Suggested change

public Task<AssociateMgaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)

public Task<AssociateMfaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)

made change

frederikprijck · 2024-04-03T07:02:13Z

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

+                throw new ArgumentNullException(nameof(request));
+            }
+
+            return connection.SendAsync<AssociateNewAuthenticatorResponse>(


Suggested change

return connection.SendAsync<AssociateNewAuthenticatorResponse>(

return connection.SendAsync<AssociateMfaAuthenticatorResponse>(

made change

frederikprijck · 2024-04-03T07:02:45Z

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

+            {
+                { "grant_type", "http://auth0.com/oauth/grant-type/mfa-oob" },
+                { "client_id", request.ClientId },
+                { "client_secret", request.ClientSecret },


This is added through ApplyClientAuthentication

Suggested change

{ "client_secret", request.ClientSecret },

made change

czf

Added a few review comments, but they do concern me. They show this PR has not been tested as it does not compile.

We will need to find time to validate all of these changes, which we currently can not prioritise.

Sorry about that, we moved to just using http client directly since this wasn't moving and we needed more than just these calls added. I only used the web interface and didn't re run the integration tests. I've run the tests again and they work.

Anyway there's no rush on this as again we are just making the calls using http client.

czf · 2024-04-06T00:28:43Z

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

        }
+
+        /// <inheritdoc/>
+        public Task<AssociateMgaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)


made change

czf · 2024-04-06T00:28:53Z

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

+            {
+                { "grant_type", "http://auth0.com/oauth/grant-type/mfa-oob" },
+                { "client_id", request.ClientId },
+                { "client_secret", request.ClientSecret },


made change

czf · 2024-04-06T00:30:10Z

src/Auth0.AuthenticationApi/AuthenticationApiClient.cs

+                throw new ArgumentNullException(nameof(request));
+            }
+
+            return connection.SendAsync<AssociateNewAuthenticatorResponse>(


made change

kailash-b · 2025-04-02T05:57:18Z

Changes are merged as part of #798

Adding authenticator enrollment and verification to authentication cl…

39ef913

…ient

czf requested a review from a team as a code owner December 29, 2023 00:24

czf had a problem deploying to external December 29, 2023 00:24 — with GitHub Actions Failure

frederikprijck requested changes Jan 10, 2024

View reviewed changes

czf and others added 2 commits January 11, 2024 11:46

requested changes from PR

841b2b2

Merge branch 'master' into mfa

9e9011c

czf requested a review from frederikprijck January 11, 2024 19:49

frederikprijck requested changes Jan 12, 2024

View reviewed changes

Apply suggestions from code review

0a09a7c

Co-authored-by: Frederik Prijck <frederik.prijck@gmail.com>

czf force-pushed the mfa branch from 19593e6 to 0a09a7c Compare January 16, 2024 22:03

czf requested a review from frederikprijck January 16, 2024 22:05

czf requested a deployment to external January 28, 2024 00:24 — with GitHub Actions Waiting

frederikprijck requested changes Apr 3, 2024

View reviewed changes

czf had a problem deploying to external April 6, 2024 00:23 — with GitHub Actions Failure

Missed cleanup

8e3d9db

czf commented Apr 6, 2024

View reviewed changes

kailash-b mentioned this pull request Mar 28, 2025

Adds support to Enroll / Verify / List / Challenge / Delete Multi Factor Authenticators #798

Merged

6 tasks

kailash-b closed this Apr 2, 2025

	public Task<AssociateMgaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)
	public Task<AssociateMfaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)

	return connection.SendAsync<AssociateNewAuthenticatorResponse>(
	return connection.SendAsync<AssociateMfaAuthenticatorResponse>(

Conversation

czf commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

References

Testing

Checklist

Uh oh!

frederikprijck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederikprijck commented Jan 10, 2024

Uh oh!

czf commented Jan 11, 2024

Uh oh!

frederikprijck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frederikprijck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailash-b commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

czf commented Dec 29, 2023 •

edited

Loading

frederikprijck left a comment •

edited

Loading