Adding authenticator enrollment and verification to authentication client #695

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

@@ -494,6 +522,22 @@ public Task<PushedAuthorizationRequestResponse> PushedAuthorizationRequestAsync(
                cancellationToken: cancellationToken
            );
        }
+
+        /// <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

@@ -494,6 +522,22 @@ public Task<PushedAuthorizationRequestResponse> PushedAuthorizationRequestAsync(
                cancellationToken: cancellationToken
            );
        }
+
+        /// <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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding authenticator enrollment and verification to authentication client #695

Adding authenticator enrollment and verification to authentication client #695

czf commented Dec 29, 2023 •

edited

frederikprijck left a comment

frederikprijck Jan 10, 2024

czf Jan 11, 2024

frederikprijck commented Jan 10, 2024

czf commented Jan 11, 2024

frederikprijck left a comment

frederikprijck Jan 12, 2024

czf Jan 16, 2024

frederikprijck left a comment •

edited

frederikprijck Apr 3, 2024

czf Apr 6, 2024

frederikprijck Apr 3, 2024

czf Apr 6, 2024

frederikprijck Apr 3, 2024

czf Apr 6, 2024

czf left a comment

czf Apr 6, 2024

czf Apr 6, 2024

czf Apr 6, 2024

	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>(

Adding authenticator enrollment and verification to authentication client #695

Are you sure you want to change the base?

Adding authenticator enrollment and verification to authentication client #695

Conversation

czf commented Dec 29, 2023 • edited

Changes

References

Testing

Checklist

frederikprijck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck commented Jan 10, 2024

czf commented Jan 11, 2024

frederikprijck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czf commented Dec 29, 2023 •

edited

frederikprijck left a comment •

edited