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

Adding authenticator enrollment and verification to authentication client #695

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

czf
Copy link

@czf czf commented Dec 29, 2023

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

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

@frederikprijck frederikprijck left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

removed this

@frederikprijck
Copy link
Member

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
Copy link
Author

czf commented Jan 11, 2024

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

Copy link
Member

@frederikprijck frederikprijck left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/Auth0.AuthenticationApi/AuthenticationApiClient.cs Outdated Show resolved Hide resolved
src/Auth0.AuthenticationApi/IAuthenticationApiClient.cs Outdated Show resolved Hide resolved
public string AuthenticatorType { get; set; }

[JsonProperty("oob_channel")]
public string OobChannel { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

src/Auth0.AuthenticationApi/Models/MfaOobTokenRequest.cs Outdated Show resolved Hide resolved
Copy link
Member

@frederikprijck frederikprijck left a comment

Choose a reason for hiding this comment

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

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.

@@ -494,6 +522,22 @@ public Task<PushedAuthorizationRequestResponse> PushedAuthorizationRequestAsync(
cancellationToken: cancellationToken
);
}

/// <inheritdoc/>
public Task<AssociateMgaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

made change

throw new ArgumentNullException(nameof(request));
}

return connection.SendAsync<AssociateNewAuthenticatorResponse>(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

made change

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

Choose a reason for hiding this comment

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

This is added through ApplyClientAuthentication

Suggested change
{ "client_secret", request.ClientSecret },

Copy link
Author

Choose a reason for hiding this comment

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

made change

Copy link
Author

@czf czf left a comment

Choose a reason for hiding this comment

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

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.

@@ -494,6 +522,22 @@ public Task<PushedAuthorizationRequestResponse> PushedAuthorizationRequestAsync(
cancellationToken: cancellationToken
);
}

/// <inheritdoc/>
public Task<AssociateMgaAuthenticatorResponse> AssociateMfaAuthenticatorAsync(AssociateMfaAuthenticatorRequest request, CancellationToken cancellationToken = default)
Copy link
Author

Choose a reason for hiding this comment

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

made change

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

Choose a reason for hiding this comment

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

made change

throw new ArgumentNullException(nameof(request));
}

return connection.SendAsync<AssociateNewAuthenticatorResponse>(
Copy link
Author

Choose a reason for hiding this comment

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

made change

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.

Implement mfa/associate endpoint
2 participants